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

Fix vs2019 & namespace for install targets #349

Merged
merged 1 commit into from
Feb 3, 2024
Merged

Conversation

jherico
Copy link
Contributor

@jherico jherico commented Feb 2, 2024

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 jmr merged commit 05b0694 into google:master Feb 3, 2024
1 check passed
@jmr
Copy link
Member

jmr commented Feb 3, 2024

@jherico
Copy link
Contributor Author

jherico commented Feb 3, 2024

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

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.

None yet

2 participants