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

Use built-in CMake support when CMake >= 3.28 #25

Closed
wants to merge 1 commit into from

Conversation

jcar87
Copy link

@jcar87 jcar87 commented Nov 17, 2023

CMake 3.28 enables support for module handling using dependency scanning (when using a supported compiler and generator): https://cmake.org/cmake/help/latest/release/3.28.html

Note that for dependency scanning to be enabled automatically, the policy level needs to be 3.28 or higher. Wonder if it's worth setting that from here or not (or at least an error/warning if it isn't).

@jcar87
Copy link
Author

jcar87 commented Nov 17, 2023

Tested locally on Linux with Ninja and Clang++ 17, and on Windows with VS2022, using the latest CMake 3.28 release candidate. May be worth having dedicated testing in the CI plan for this, may need to some pointers as to what's the best way to proceed (we would need to have CMake 3.28 installed and set up, and so on...)

Copy link
Owner

@vitaut vitaut left a 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!

Comment on lines +206 to +210
if(CMAKE_VERSION VERSION_LESS 3.28)
target_sources(${name} PRIVATE ${sources})
else()
target_sources(${name} PUBLIC FILE_SET fmt_module TYPE CXX_MODULES FILES ${sources})
endif()
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's better to restructure the code to check CMake version in one place instead of three, something like:

if (CMAKE_VERSION VERSION_GREATER_EQUAL 3.28)
    target_sources(${name} PUBLIC FILE_SET fmt_module TYPE CXX_MODULES FILES ${sources})
    return()
endif ()

earlier in this function

Also note spaces before opening parentheses in statement-like constructs like if.

@vitaut
Copy link
Owner

vitaut commented Nov 23, 2023

May be worth having dedicated testing in the CI plan for this, may need to some pointers as to what's the best way to proceed (we would need to have CMake 3.28 installed and set up, and so on...)

It would be good to have a CI config for this. Here's an example how to install necessary packages:
https://github.com/fmtlib/fmt/blob/dd6f657a79104101a2e4ea6ba90f69e0dc114822/.github/workflows/linux.yml#L18

@vitaut
Copy link
Owner

vitaut commented Apr 18, 2024

Closing for now but let me know if you plan to address the remaining comment(s) and I'll reopen.

@vitaut vitaut closed this Apr 18, 2024
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 this pull request may close these issues.

2 participants