Refactor CMake build configuration

[benbovy]

I've reworked a bit the CMake build scripts and changed some default build options to make it more friendly when used with package managers like conda-forge, so we can use the latter to install the s2geometry, abseil-cpp and gtest packages instead of building those dependencies from source (I did it successfully on my machine). Also, no patch will be required if we later want to provide a s2geography package (conda-forge generally requires shipping external dependencies as separate packages).

I'm not familiar with how R compiled extensions are usually built, but I added new options so that hopefully it should still be possible to build s2geography like it has been so far.

More specifically:

• I added the S2GEOGRAPHY_FETCH_S2GEOMETRY=OFF option to download and include s2geometry in this project. By default CMake look for the s2 library installed in one of the default search paths (or using either S2_ROOT or S2_DIR variable).
• Likewise, I added the S2GEOGRAPHY_FETCH_GTEST=OFF option for the google test library.
• I set the default standard to C++17. It seems that many related libraries supports it already, at least on conda-forge (Abseil overhaul, again conda-forge/abseil-cpp-feedstock#45 (comment)). It can be overridden anyway.

TODO?

• Remove the S2GEOGRAPHY_ prefix for all options? I find it a bit verbose but I don't have strong opinions on this.
• Update CI and install dependencies using conda-forge? This would probably be easier for running the tests and/or the examples on all supported platforms.
• Remove the abseil dependency? It seems to be used only for absl::make_unique... Could we replace it by std::make_unique and require C++14 as the oldest compatible standard? Or is there anything that forces us to stick with C++11?

[codecov-commenter]

Codecov Report

Base: 14.36% // Head: 14.36% // No change to project coverage 👍

Coverage data is based on head (a1a8270) compared to base (a2dff00).
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff           @@
##           master       #3   +/-   ##
=======================================
  Coverage   14.36%   14.36%           
=======================================
  Files          14       14           
  Lines        1490     1490           
  Branches       29       29           
=======================================
  Hits          214      214           
  Misses       1270     1270           
  Partials        6        6           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[benbovy]

@paleolimbot @brendan-ward I'd be glad to read your thoughts on this.

[paleolimbot]

Just a few thoughts! You're welcome to merge this and run, although something about these changes breaks my local build (the first error that pops up is that the include directory for OpenSSL isn't picked up). I had to add include_directories("${OPENSSL_ROOT_DIR}/include") to get the build to pass but I don't know if that only works because I've specified that in my CMakeUserPresets.

[paleolimbot]

This is pretty short...could you just inline this into CMakeLists.txt? (CMake builds are already pretty difficult to reason about and sometimes it helps to have it all in one place?)

[benbovy]

This is indeed pretty short, but it is common practice to have this in a separate FIND<xxx>.cmake module. This is mentioned in CMake's documentation and this is used in libraries like S2geometry, GDAL, Xtensor, Pybind11, etc.

[paleolimbot]

Does this usually work? My recollection is that s2 always needs its own custom build of Abseil because of the C++ standard thing but I could be recalling incorrectly (or maybe it just doesn't work with homebrew Abseil). My preference would be to use FetchContent here to make it so that git clone + Open in VSCode works out-of-the-box (with the option to use a shared Abseil in the unlikely event that the user has a custom-built version ready to go).

[benbovy]

Yes it seems to work well (I've only tested this on my laptop, though).

This is also used in S2's CMakeLists.txt, actually.

[benbovy]

With Homebrew it looks like both the abseil and s2geometry formulae are built with the C++17 standard.

S2geometry 0.10 is not yet available with conda-forge (I'll fix that and probably use C++17 there too). No issue with version 0.9 that doesn't depend on Abseil.

[benbovy]

Thanks for your feedback @paleolimbot! I answered most of your inline comments. It looks like we have diverging opinions regarding whether we should by default let cmake fetch and build the dependencies or tell cmake try finding them on the system. I lean towards the latter option for the following reasons:

• It is a common practice that I see in many popular C++ libraries that use CMake as a build tool
• S2Geography's dependencies are available through package managers, e.g., s2geometry can be easily installed with homebrew or conda-forge, although admittedly not widely yet but with the 0.10 stable release and abseil as external dependency this might change (a debian package soon?)
• S2Geography does not require specific builds for its dependencies
• Fetching packages using CMake's FetchContent may have side effects like installing the dependencies, which is not always desired (e.g., when creating packages for S2Geography)

I don't have strong opinion on this, though, as long as we can re-use dependencies and/or tools pre-installed on the system. I agree this can be convenient to automatically download and build them.

I had to add include_directories("${OPENSSL_ROOT_DIR}/include") to get the build to pass but I don't know if that only works because I've specified that in my CMakeUserPresets.

Have you tried removing it in your CMakeUserPresets? Normally it shouldn't be required if openssl is installed in a default location, find_package(OpenSSL REQUIRED) is S2's CMakeLists.txt should find the headers. I didn't had any problem with both s2geometry pre-installed with conda-forge or download and built with cmake.

[paleolimbot]

All good! You should merge this and run...if I get to it I'll make a PR along the lines of "make sure CMake configuration works out-of-the box without a conda environment".

[paleolimbot]

I tested this with a local checkout and its awesome! The only thing I had to add was include_directories(${OPENSSL_INCLUDE_DIR}) since the OpenSSL headers are transitively included by some s2 headers. I imagine it works on linux just fine because these headers are in the system include path (whereas on Mac they aren't).

[paleolimbot]

Maybe one other suggestion...in the "AUTO" bit where you're resolving which s2 to use, do some messaging so that I know which s2 is getting picked up (and maybe what version)? (Again, feel free to merge and run...CMake config is the worst and this is a huge improvement)

[benbovy]

The only thing I had to add was include_directories(${OPENSSL_INCLUDE_DIR})

Ah yes I see. It works fine on my config (macOS) but because I installed openssl in the conda envrionment I use for this project. Did you install it using Homebrew?

Does it work if you set OPENSSL_ROOT_DIR=... (env var or command line) instead of adding include_directories(${OPENSSL_INCLUDE_DIR})?

[paleolimbot]

Setting OPENSSL_ROOT_DIR is required for the fetched build, which passes no problem. Setting OPENSSL_ROOT_DIR with Homebrew S2 doesn't work, possibly because S2 doesn't export that as part of its target_include_directories() ( https://github.com/google/s2geometry/blob/master/CMakeLists.txt#L258 ) or possibly because I'm not specifying the version that system S2 was compiled against (I have version 1.1 and 3 installed, I think). find_package(OpenSSL) will set OPENSSL_INCLUDE_DIR, so it's probably safest to use that?

[benbovy]

I could reproduce it indeed. As a quick fix, I added a "BREW" mode for S2GEOGRAPHY_S2_SOURCE. I could successfully build and run the tests with:

$ brew install s2geometry

$ cmake ../.. \
-DS2GEOGRAPHY_BUILD_TESTS=ON \
-DOPENSSL_ROOT_DIR="/usr/local/opt/openssl@3/" \
-DS2GEOGRAPHY_S2_SOURCE=BREW \
-DCMAKE_CXX_STANDARD=17 \
-DBUILD_SHARED_LIBS=OFF

It's weird, though. The issue with openssl headers not found doesn't happen with s2geometry and openssl installed using conda. It's not the same s2geometry version but I don't see anything that changes between v0.9 and v0.10 regarding how those headers are included in S2's CMake build.

Using BUILD_SHARED_LIBS=ON I get linking errors with absl, which I don't really understand as presumably both absl and s2geometry homebrew package are compiled with C++17.

[benbovy]

some messaging so that I know which s2 is getting picked up

Good point. I added the version info for the "BUNDLED" mode. It would be great for the "SYSTEM" mode too, but there's no (easy?) way to get the version installed prior to v0.10.0 (google/s2geometry#161), and v0.10.0 is the latest version...

Ideally this should be fixed upstream, i.e., s2geometry should provide an s2Config.cmake file.

[paleolimbot]

I wouldn't hold your breath for s2's CMake configuration to improve...some of the patches Arrow developers have contributed to google libraries have taken years to be merged and end up in a release. It was never really designed to be a system library (which is why I think pinning a commit/fetching/building static libs is a totally valid approach).

[benbovy]

I wouldn't hold your breath for s2's CMake configuration to improve... some of the patches Arrow developers have contributed to google libraries have taken years to be merged and end up in a release. It was never really designed to be a system library (which is why I think pinning a commit/fetching/building static libs is a totally valid approach).

Yeah I've seen this too in s2geometry (although it seems a bit faster than years now). I still have to patch the last release in conda-forge/s2geometry-feedstock#9.

The brave conda-forge maintainers are trying hard reusing shared libs as much as possible. Let's see later if I'm brave enough to submit a PR to s2geometry for a s2Config.cmake file and/or add it to the conda package 🙂.

So I think I'm going to merge this PR soon unless you have other suggestions. Once conda packages are available for s2geometry v0.10.0, I'll update S2Geography CI in another PR with the tests run on macOS, Windows, etc.

[paleolimbot]

Somehow I still need to add include_directories(${OPENSSL_INCLUDE_DIR}) to get this to work with a bundled build. Is there some reason it shouldn't be added? It does accurately reflect the include requirements of s2geography because of the transitive include of the bignum headers.

[paleolimbot]

I only make a stink about that because I will need to have my local build work to review your PRs! Feel free to merge anyway and I'll put up a PR fixing my local build later on.

[benbovy]

The last commit should hopefully fix openssl headers not found. Normally CMake should also find the right location of those headers on macOS (homebrew installed openssl) without manually specifying it.

[benbovy]

I updated README since many things have changed in the CMake config. @paleolimbot let me know if you want to review it before I merge this.

I tested locally (MacOS 11.6) those configs for building the tests, with success:

• conda installed dependencies, s2geometry 0.9.0, -DS2GEOGRAPHY_S2_SOURCE=CONDA (or AUTO)
• conda installed dependencies, s2geometry 0.10.0, -DS2GEOGRAPHY_S2_SOURCE=CONDA (or AUTO)
• conda installed absl / openssl, -DS2GEOGRAPHY_S2_SOURCE=BUNDLED
• homebrew installed dependencies, s2geometry 0.10.0, -DS2GEOGRAPHY_S2_SOURCE=BREW
• homebrew installed absl / openssl, -DS2GEOGRAPHY_S2_SOURCE=BUNDLED

(all using C++17 standard)

[jorisvandenbossche]

vcpkg is another way you can have s2geometry installed in a way that CMake will automatically find it (https://github.com/microsoft/vcpkg/tree/master/ports/s2geometry, and it seems they have some patches to ensure it installs a cmake config file)
(not that you need to test / add support for that, to be clear! Just mentioning as another example, and we might want to use that eventually for making python wheels)

[benbovy]

Ok, I'm going to merge this PR and move on with CI / tests.

Many thanks @paleolimbot for your review and feedback! It greatly helped improving the initial modifications made here.

Thanks @jorisvandenbossche for the vcpkg suggestion, looks like a good candidate indeed for later building python wheels.