-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fix options for C++20 experimental module in CMake #3134
Conversation
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.
Thanks for the PR.
CMakeLists.txt
Outdated
set(FMT_CAN_MODULE OFF) | ||
if (CMAKE_CXX_STANDARD GREATER 17 AND NOT MSVC) | ||
set(FMT_CAN_MODULE ON) | ||
endif () |
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.
Most compiler versions don't support modules so this should be disabled by default, not enabled by default.
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.
This is just a guard for the dependent option which will hide the option if the new module system is not available. If this is TRUE
, FMT_MODULE
will be the default value which is FALSE
, as in the following code segment. Or otherwise, the option will be hidden completely from the user.
CMakeLists.txt
Outdated
@@ -80,19 +86,13 @@ option(FMT_TEST "Generate the test target." ${FMT_MASTER_PROJECT}) | |||
option(FMT_FUZZ "Generate the fuzz target." OFF) | |||
option(FMT_CUDA_TEST "Generate the cuda-test target." OFF) | |||
option(FMT_OS "Include core requiring OS (Windows/Posix) " ON) | |||
option(FMT_MODULE "Build a module instead of a traditional library." OFF) | |||
cmake_dependent_option(FMT_MODULE "Build a module instead of a traditional library." OFF FMT_CAN_MODULE OFF) |
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 think it should be a dependent option. Users should be able to override detection if needed.
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.
Alright, I've changed this to the simpler method, which is just excluding the MSVC, but still allowing the user to force overriding the FMT_MODULE
.
I think you should replace FMT_CAN_MODULE with FMT_MODULE in test to fix CI. |
Thanks for the hint. I actually found the same issue from the CI logs, but I was hesitating if it's valid to replace the variable. |
Thank you |
This PR, as discussed previously under commit 3def950, is to fix a mistake introduced in #2432 which disabled option for building the module with the new C++20 module system completely (even for compatible compiler).