google / s2geometry

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

Fix vs2019 & namespace for install targets #349

Closed jherico closed 7 months ago

jherico commented 7 months ago

Three minor changes related to my recent fixes for vcpkg and Windows builds

First, the namespace for the export target should be s2::, not s2. Otherwise the downstream imported target becomes s2s2 instead of s2::s2, which is the actual convention. Sorry, this was a result of my inexperience with the details of building these configs as opposed to consuming them.

Second, I failed to include put the absl dependency into the exported exported CMake config for downstream projects. This is necessary because absl is part of the public interface for s2, and thus will be a dependency of any downstream project. The VCPKG dependency mechanism already ensures that absl will be built and installed if s2geometry is requested, but this change is requires to avoid downstream packages having to explicitly call find_dependency(absl CONFIG) before using the s2 package.

Lastly, the file src/s2/s2lax_polygon_shape.cc uses the "terse" form of static_assert, which is technically a part of the C++17 standard, while the project is marked as C++14 compliant. It appears that almost every compiler, including MSVC 2022, supports the terse form even without a C++17 flag except MSVC 2019. MSVC 2019 generates an error causing S2 to fail to build on that platform, which unfortunately is one of the CI targets for the downstream project I'm working on.

Also, if we could push a v0.11.1 tag for these fixes, that would enable me to fix the vcpkg version without creating new patches there. I'd really appreciate it.

Cheers.

jmr commented 7 months ago

https://github.com/google/s2geometry/releases/tag/v0.11.1

jherico commented 7 months ago

wow, this must be what "Convincing John" feels like