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

Hardcoding CMAKE_CXX_STANDARD breaks source-consuming downstream projects using target_compile_features #3189

Closed
ChrisThrasher opened this issue Nov 17, 2022 · 9 comments · Fixed by #3205

Comments

@ChrisThrasher
Copy link
Contributor

ChrisThrasher commented Nov 17, 2022

if (NOT CMAKE_CXX_STANDARD)
set(CMAKE_CXX_STANDARD 11)
endif()

For project consuming the latest release of fmt via add_subdirectory like seen here who also use the modern target_compile_features-based approach to enable a specific language standard like C++17 have their builds broken by the above snippet. The above snippet can result in their requested language standard being overridden by the default language standard used in fmt unless they're careful to set CMAKE_CXX_STANDARD themselves before including fmt.

I see this is likely the case because the library claims to support CMake versions all the way back to 3.1, well before target_compile_features was added. Can the minimum version be raised to use target_compile_features or can we at least add some extra CMake code to us the modern language specification functions when possible?

@vitaut
Copy link
Contributor

vitaut commented Nov 17, 2022

target_compile_features is available in CMake 3.1.3: https://cmake.org/cmake/help/v3.1/command/target_compile_features.html. A PR to bump the minimum required version and replace CMAKE_CXX_STANDARD with something target-based would be welcome.

@ChrisThrasher
Copy link
Contributor Author

The cxx_std_xx properties weren’t added until version 3.8. We’d have to make a more substantial minimum version increase. Is that okay?

@ChrisThrasher
Copy link
Contributor Author

https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html#prop_tgt:CXX_STANDARD

The value 17 for CXX_STANDARD also isn't supported until CMake 3.8.

@vitaut
Copy link
Contributor

vitaut commented Nov 18, 2022

I suggest replacing CMAKE_CXX_STANDARD with FMT_CXX_STANDARD and using this variable to set the targets' CXX_STANDARD property. This way we don't need to bump cmake all the way to 3.8 (c++17 can be supported conditionally).

@ChrisThrasher
Copy link
Contributor Author

ChrisThrasher commented Nov 21, 2022

What platforms are you supporting that are still limited to CMake version 3.1? That's quite old at this point. It may be worth it to let such users stick to the current release so that we can improve the experience for the majority of users who are using ~3.10 or newer.

Here's a convenient table showing what platforms ship what version of CMake:

https://alexreinking.com/blog/how-to-use-cmake-without-the-agonizing-pain-part-1.html

In short, the oldest version still in regular use is CMake 3.16 on Ubuntu 20.04 LTS but if you want to go back to Ubuntu 18.04 LTS which is nearing EOL, that still has CMake 3.10.

@vitaut
Copy link
Contributor

vitaut commented Nov 25, 2022

It's more about the question whether we need to bump the version rather than supporting some specific platform. Anyway, 3.8 is quite old version so it's probably OK to bump it if it makes things significantly simpler.

@ChrisThrasher
Copy link
Contributor Author

https://github.com/ChrisThrasher/fmt/commit/daf0c44be7b4b5768e5892c46af5b347303321d4

Here's what I had in mind. Please correct me if C++11 is not in fact the minimum language standard.

@phprus
Copy link
Contributor

phprus commented Nov 25, 2022

@ChrisThrasher
C++11 is the minimum supported language standard.

@ChrisThrasher
Copy link
Contributor Author

See PR #3205

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants