Closed benbovy closed 1 year ago
Base: 14.36% // Head: 14.36% // No change to project coverage :thumbsup:
Coverage data is based on head (
a1a8270
) compared to base (a2dff00
). Patch has no changes to coverable lines.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
@paleolimbot @brendan-ward I'd be glad to read your thoughts on this.
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:
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.
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).
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)
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})
?
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?
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.
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 (https://github.com/google/s2geometry/pull/161), and v0.10.0 is the latest version...
Ideally this should be fixed upstream, i.e., s2geometry should provide an s2Config.cmake
file.
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).
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 https://github.com/conda-forge/s2geometry-feedstock/pull/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.
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.
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.
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.
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:
-DS2GEOGRAPHY_S2_SOURCE=CONDA
(or AUTO
)-DS2GEOGRAPHY_S2_SOURCE=CONDA
(or AUTO
)-DS2GEOGRAPHY_S2_SOURCE=BUNDLED
-DS2GEOGRAPHY_S2_SOURCE=BREW
-DS2GEOGRAPHY_S2_SOURCE=BUNDLED
(all using C++17 standard)
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)
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.
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
andgtest
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 as2geography
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:
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 eitherS2_ROOT
orS2_DIR
variable).S2GEOGRAPHY_FETCH_GTEST=OFF
option for the google test library.TODO?
S2GEOGRAPHY_
prefix for all options? I find it a bit verbose but I don't have strong opinions on this.absl::make_unique
... Could we replace it bystd::make_unique
and require C++14 as the oldest compatible standard? Or is there anything that forces us to stick with C++11?