-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix: always use an interface lib for flatsample #8897
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
base: master
Are you sure you want to change the base?
Conversation
|
I don't know about the MSVC case but i added the second case for old cmake versions here #8173 i think we break cmake older than 3.20.0 (https://github.com/Kitware/CMake/releases/tag/v3.20.0 released 2021) I guess this will still break older systems like debian bullseye https://packages.debian.org/bullseye/cmake If we want to do that i would propose to set |
|
We definitely can't do that -- what specifically breaks in the older versions? Interface libs have been supported since 3.0 |
|
It might be supported since 3.0 but it it started working properly only with 3.20 or something. To reproduce -> But it looks like it is already broken right now (using the type |
| add_library(flatsample INTERFACE) | ||
|
|
||
| # Since flatsample has no sources, we have to explicitly set the linker lang. | ||
| set_target_properties(flatsample PROPERTIES LINKER_LANGUAGE CXX) |
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.
looking at the issue again i would propose to remove this line (578) as well and then it works with old cmake version, no idea about the MSVC case but i prose to fix that later if that still breaks for someone and we understand what the issues is
|
Yeah the static failure is what was reported in the issue I'm looking to fix. Thanks for the repro steps!! |
Fixes #8889
I'm not sure why the STATIC fallback case exists, so removing it.