-
Notifications
You must be signed in to change notification settings - Fork 543
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: use com_google_protobuf in WORKSPACE #2432
Conversation
Hey @rickeylev, this should now also support WORKSPACE mode better in combination with autoloaded rules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update changelog. I've pushed a change for the deps part.
Can you add something under Fixed for what it fixes? I get that the repo name being different might cause issues, but don't entirely understand what broke/why (e.g. our CI is green). Is it that the broader WORKSPACE ecosystem has settled more on com_google_protobuf as the name? Or this only shows up in Bazel 9, or something?
Does this also fix #2429 ? |
Yes, I think so. Added to the description. |
Will add to Fixed. The reason for @com_google_protobuf is much simpler. Bazel 6 and 7 have the name hardcoded in the default values of flags, like --protocol_compiler. To have continuity of WORKSPACE mode with Bazel 8, we need to keep the same name. Once we're able to drop the WORKSPACE mode, Bzlmod doesn't care how protobuf is called (the name is correctly resolved in bzlmod mode even when coming from a flag). Things are even worse. Bazel 6 and 7 call @rules_cc//cc:defs.bzl from @bazel_tools. So they trigger the need for @protobuf even if you don't do anything. (Bazel 8 doesn't use defs.bzl). I also can't remove cc_proto_library from @rules_cc//cc:defs.bzl, because some major repositories like grpc depends on it. |
b215c76
to
bcbd4e0
Compare
The only way to support both workspace and bzlmod mode, is to call protobuf com_google_protobuf. Upgrade rules_cc to 0.0.16 and rules_java to 8.3.1. Those are minimal versions that are also calling protobuf again com_google_protobuf.
bcbd4e0
to
1e43ae6
Compare
re: docs building: Thanks for noticing that. We just need our docs to build for (ubuntu + bzlmod + current bazel) (they shouldn't have been building for other configs). Having it work for macs is only needed for local development. I've push some changes to modify the target_compatible_with settings so docs skip unsupported cases. //sphinxdocs looks OK, too, now. This should remove the need for changing the CI configs. I've pushed a change to that effect; lets see what CI says. |
Update on docs building: rules_java 8.5.1 will contain a fix, so a part of this PR could be undone by another upgrade (IMO it's better to build docs only on one platform) |
The only way to support both workspace and bzlmod mode, is to call protobuf com_google_protobuf. This is because old Bazel's encode it in default values of
--protoco_compiler
flag, and so new Bazel 8 needs to do the same.For bzlmod, upgrade rules_cc to 0.0.16 and rules_java (dev dep) to 8.3.1. Those are minimal versions that are also calling protobuf again com_google_protobuf.
For workspace, upgrade rules_cc to 0.1.0. This is an incompatible version that doesn't call Protobuf. rules_python users may use it. In case they need cc_proto_library in
@rules_cc//cc/defs.bzl
, they can overwrite the version to 0.0.16 in WORKSPACE (or use protobuf_deps that already does that).Disable docs generation targets on WORKSPACE CI setups. They are broken by rules_java upgrade.
Upgrades dependencies:
Fixes #2429