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

Add modern cmake config targets to s2geometry #339

Merged
merged 5 commits into from
Jan 26, 2024

Conversation

jherico
Copy link
Contributor

@jherico jherico commented Jan 10, 2024

The current vcpkg version of s2geometry has to patch the CMakeLists.txt and generate a Config.cmake.in file in order to create a modern CMake target file that can then be found by other packages via find_package.

Because this is a vcpkg addition, they mandate that this be placed in the unofficial namespace, in order to prevent any potential conflict with an eventual upstream config that might be introduced. This PR introduces the upstream config so that it can be used by anyone who builds the package and so that the vcpkg version no longer requires any patching at all.

The changes here are essentially duplicating what the vcpkg patch does, except that they drop the unofficial prefix to everything, since the config would now be coming from the primary source.

@jmr
Copy link
Member

jmr commented Jan 10, 2024

I will have to do some reading about modern CMake. I just based this on an example about ten years ago.

Copy link
Member

@jmr jmr left a comment

Choose a reason for hiding this comment

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

Does this change anything for people using it without vcpkg?

@jherico
Copy link
Contributor Author

jherico commented Jan 10, 2024

Does this change anything for people using it without vcpkg?

If you're asking if it will break existing downstream consumers, then the answer is no. Downstream projects, regardless of how they're building, and whether or not they use the S2_USE_SYSTEM_INCLUDES option, should be unaffected by this change.

Strictly speaking it does add to the build artifacts generated and installed (regardless of whether you're building from vcpkg or not), because it will create a few files in <INSTALL_PREFIX>/share/s2, in particular s2Config.cmake which contains the information required for downstream CMake builds to be to use s2 via find_package in Config mode as demonstrated here...

find_package(s2 CONFIG REQUIRED)
target_link_library(my_project PUBLIC s2::s2)

This is the standard mechanism for packaging modern libraries built with CMake for other projects that are also using CMake. Additionally the exported target s2::s2 isn't simply a variable but a top-level CMake target type that contains properties, so the target_link_library doesn't just properly set the linker flags for the downstream project, but include directories and a variety of other possible settings. It also has the advantage of incorporating information about the distinct files required for the build type, so downstream consumers don't have to worry about matching release vs debug builds.

This makes lives easier for downstream consumers, with the caveat that package builders need to do a little more work to generate the config, but as you can see from this PR it's a very small amount of work.

For further reading I'd suggest this section of the CMake online documents, or a recent edition of this book.

I do kind of wish CMake would offer a simpler mode for developers that provided a prescribed file layout that can be customized, but that makes writing a CMake file for a project like this one more trivial, the sort of "convention over configuration" approach Maven managed to do for Java, and which ultimately has become the norm for more modern languages like Rust, Go, Python, etc... maybe someday.

Fix formatting to be consistent with existing formatting.

Mostly, this is aligning after "(" if possible without a line wrap and moving ")" to be on a line with other content.  [")" handling does not seem to be consistent with the CMake docs, but is consistent with the rest of the file.]
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
@jherico jherico requested a review from jmr January 26, 2024 20:41
@jmr
Copy link
Member

jmr commented Jan 26, 2024

Thanks for the explanations, and sorry for the delay!

@jmr jmr merged commit 0f6bd97 into google:master Jan 26, 2024
1 check passed
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