-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
[sfml][imgui-sfml] Push SFML_STATIC_LIBRARIES setting into sfml
#11800
[sfml][imgui-sfml] Push SFML_STATIC_LIBRARIES setting into sfml
#11800
Conversation
FILE(READ ${CURRENT_PACKAGES_DIR}/share/sfml/SFMLConfig.cmake SFML_CONFIG) | ||
if(VCPKG_LIBRARY_LINKAGE STREQUAL "static") | ||
FILE(READ ${CURRENT_PACKAGES_DIR}/share/sfml/SFMLConfig.cmake SFML_CONFIG) | ||
FILE(WRITE ${CURRENT_PACKAGES_DIR}/share/sfml/SFMLConfig.cmake "set(SFML_STATIC_LIBRARIES true)\ninclude(CMakeFindDependencyMacro)\nfind_dependency(Freetype)\n${SFML_CONFIG}") | ||
else() | ||
FILE(WRITE ${CURRENT_PACKAGES_DIR}/share/sfml/SFMLConfig.cmake "set(SFML_STATIC_LIBRARIES false)\n${SFML_CONFIG}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get why you need this change. I also don't get why imgui-sfml
required SFML_STATIC_LIBRARIES
since SFML seems to already set SFML_STATIC_LIBRARIES
if required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is potentially an issue in the reverse case -- if sfml
is dynamic and imgui-sfml
is static. This change ensures that SFML_STATIC_LIBRARIES
is always set exactly according to how sfml was built, regardless of whether the outer project sets the variable or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better make that a FATAL_ERROR
instead of silently accepting it because it means a consumer needs to be patched instead and might be assuming something wrongly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it's better to be optimistic here:
- Patching every single consumer is not realistic
- The vast majority of cases will work without patching
- The cases that don't work will have build failures, not silent misbehavior
Finally, it's essentially impossible for consumers to do the right thing; setting SFML_STATIC_LIBRARIES
requires knowledge of whether SFML was built statically or not. Therefore, it's actually somewhat impossible to patch consumers correctly.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
PR #10840 addresses the core problem of #9660, however it misplaces the truth of whether
sfml
was built statically. This PR causesSFMLConfig.cmake
to always override the definition ofSFML_STATIC_LIBRARIES
according to the original build configuration.As an additional drive-by fix, I've added a dependency on
opengl
to fix #10840 (comment).CC: @Neumann-A @JackBoosY @cenit
What does your PR fix?
Fixes residual issues with [imgui-sfml] Force imgui-sfml to be a static library #10840
Which triplets are supported/not supported? Have you updated the CI baseline?
No changes
Does your PR follow the maintainer guide?
Yes