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

[s2geometry] Added the latest s2geometry commit as a version in BCR. #3751

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

vtsao-openai
Copy link
Contributor

@vtsao-openai vtsao-openai commented Feb 7, 2025

No description provided.

Copy link

google-cla bot commented Feb 7, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@bazel-io
Copy link
Member

bazel-io commented Feb 7, 2025

Hello @bazelbuild/bcr-maintainers, modules without existing maintainers (s2geometry) have been updated in this PR.
Please review the changes. You can view a diff against the previous version in the "Generate module diff" check.

@vtsao-openai
Copy link
Contributor Author

@bazel-io skip_check unstable_url

@bazel-io bazel-io added the skip-url-stability-check Skip the URL stability check for the PR label Feb 7, 2025
@vtsao-openai vtsao-openai marked this pull request as ready for review February 7, 2025 22:20
@@ -10,6 +10,7 @@
"github:google/s2geometry"
],
"versions": [
"0.0.0-20250201-7c96080",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it start with 0.10.0, otherwise this will be considered an older version and will lose during dependency resolution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I wasn't sure how to name this. I couldn't really find any other examples where there are official release versions, but we want to do a pre-release one.

I renamed it to 0.11.1-* since that's the latest release version.

Copy link
Member

@Wyverald Wyverald Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that 0.11.1-foo is older than 0.11.1, since the -foo part means that it's a pre-release.

We often use the .bcr.<N> suffix for a release that only happens on BCR -- but that's usually for a patched release (so 1.0.bcr.1 is usually based on 1.0 but with extra patches). I don't remember any BCR submission that's just flat out based on a git commit with no associated version whatsoever, though. If you know 0.12.0 is going to be their next release, I'd say something like 0.12.0-bcr.<date> is the best bet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, got it. I asked them for a new release here: google/s2geometry#408

but I don't want to be blocked on it if possible so I renamed the version to 0.12.0-bcr.20250201-7c96080 like you suggested. Guess it should be okay if 0.12.0 is never an actual version (like if they skip it or something).

- macos
- macos_arm64
bazel:
- 7.x
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also test with Bazel 8.x?

- "--cxxopt=-std=c++17"
- "--host_cxxopt=-std=c++17"
test_targets:
- "@s2geometry//:*"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually we don't specify the whole test suite for the module in the BCR presubmit.yml file. And looks like there are some test failures.

@meteorcloudy meteorcloudy added the presubmit-auto-run Presubmit jobs will be triggered for new changes automatically without reviewer's approval label Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
presubmit-auto-run Presubmit jobs will be triggered for new changes automatically without reviewer's approval skip-url-stability-check Skip the URL stability check for the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants