Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[imgui-sfml] Force imgui-sfml to be a static library #10840

Merged
merged 2 commits into from
May 15, 2020

Conversation

JackBoosY
Copy link
Contributor

Force imgui-sfml to be a static library to avoid the issue of loading global variables in imgui twice.

Related: #9660.

@JackBoosY JackBoosY added the info:internal This PR or Issue was filed by the vcpkg team. label Apr 15, 2020
@VincentZalzal
Copy link

I was one of those affected by issue #9660.

I tried this PR and it indeed fixes the crash.

There is one new small annoyance though. On Windows (at least), I now get link errors unless I add opengl32.lib to my project, which was not required when this was a dynamic library. I'm not sure if this is to be expected or if there is a way to automatically link to opengl32.lib too using vcpkg. I tested with VS2019 in Debug x64 configuration.

Sample of link errors:

1>ImGui-SFML.lib(imgui-SFML.cpp.obj) : error LNK2019: unresolved external symbol __imp_glBindTexture referenced in function "void __cdecl `anonymous namespace'::RenderDrawLists(struct ImDrawData *)" (?RenderDrawLists@?A0x823403ac@@YAXPEAUImDrawData@@@Z)
1>ImGui-SFML.lib(imgui-SFML.cpp.obj) : error LNK2019: unresolved external symbol __imp_glBlendFunc referenced in function "void __cdecl `anonymous namespace'::RenderDrawLists(struct ImDrawData *)" (?RenderDrawLists@?A0x823403ac@@YAXPEAUImDrawData@@@Z)
1>ImGui-SFML.lib(imgui-SFML.cpp.obj) : error LNK2019: unresolved external symbol __imp_glColorPointer referenced in function "void __cdecl `anonymous namespace'::RenderDrawLists(struct ImDrawData *)" (?RenderDrawLists@?A0x823403ac@@YAXPEAUImDrawData@@@Z)

@JackBoosY
Copy link
Contributor Author

@VincentZalzal The issue that the system library is not automatically added after integration using the msvc project(sln/vcxproj) is currently unresolved. So you need to add them manually now.

@JackBoosY JackBoosY marked this pull request as ready for review April 16, 2020 02:43
@VincentZalzal
Copy link

@JackBoosY That's ok, it's still a lot better to be able to use vcpkg to handle all the rest!

@JackBoosY JackBoosY requested review from LilyWangL and removed request for PhoebeHui April 16, 2020 06:28
@JackBoosY
Copy link
Contributor Author

@Neumann-A At present, x64-linux is used to generate static libraries by default. If there is a dynamic library in lib, it must be a port bug.

@Neumann-A
Copy link
Contributor

there is x64-osx-dynamic in the community triplets so somebody adding x64-linux-dynamic is not that far away. Also having vcpkg_check_linkage(ONLY_DYNAMIC_LIBRARY) in x64-linux won't trigger an error since it is perfectly allowed by vcpkg since VCPKG_CRT_LINKAGE is dynamic in x64-linux.

- if (NOT BUILD_SHARED_LIBS)
- set(SFML_STATIC_LIBRARIES ON)
- endif()
find_package(SFML COMPONENTS graphics system window)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The consuming package should not need to care about whether SFML is static or dynamic -- we should introduce a vcpkg-cmake-wrapper in the sfml package that resolves this issue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ras0219-msft
Here, SFML_STATIC_LIBRARIES is forced to be BUILD_SHARED_LIBS, which causes sfml to be associated with imgui-sfml build type. So I think it needs to be removed here.

Since vcpkg_check_linkage changes the value of VCPKG_LIBRARY_LINKAGE, if we set this value in vcpkg-cmake-wrapper, we cannot use VCPKG_LIBRARY_LINKAGE to determine what VCPKG_LIBRARY_LINKAGE should be set to.

@JackBoosY
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JackBoosY
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@JackBoosY
Copy link
Contributor Author

@strega-nil Please review this PR.

Thanks.

@strega-nil
Copy link
Contributor

Cool, thanks @JackBoosY :)

@strega-nil strega-nil merged commit 6319183 into microsoft:master May 15, 2020
@cenit
Copy link
Contributor

cenit commented May 16, 2020

This has been merged broken despite @Neumann-A review... :(

@JackBoosY JackBoosY deleted the dev/jack/9660 branch May 18, 2020 02:14
@JackBoosY
Copy link
Contributor Author

@Neumann-A @cenit I will continue to fix this issue.

@strega-nil
Copy link
Contributor

oh shoot, I'm sorry -.-

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants