google / s2geometry

Computational geometry and spatial indexing on the sphere
http://s2geometry.io/
Apache License 2.0
2.29k stars 302 forks source link

Add modern cmake config targets to s2geometry #339

Closed jherico closed 7 months ago

jherico commented 7 months ago

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 commented 7 months ago

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

jherico commented 7 months ago

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.

jmr commented 7 months ago

Thanks for the explanations, and sorry for the delay!