-
Notifications
You must be signed in to change notification settings - Fork 98
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
Improve backwards compatibility #255
Conversation
Context: bazel-contrib/toolchains_llvm#389 |
a87cc29
to
9400a67
Compare
* Gate `module_ctx.extension_metadata(reproducible = True)` usage behind a `bazel_features` check for compatibility with Bazel 6. * Use the "well-known" name `com_google_protobuf` instead of `protobuf` with WORKSPACE to avoid missing deps and duplication of protobuf targets.
bazel_dep(name = "bazel_skylib", version = "1.7.1") | ||
bazel_dep(name = "platforms", version = "0.0.10") | ||
bazel_dep(name = "protobuf", version = "27.0") | ||
bazel_dep(name = "protobuf", version = "27.0", repo_name = "com_google_protobuf") |
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.
This is going backwards. We'd like to stop calling com_google_protobuf and use Protobuf only. In case of bzlmod, this should be possible. WORKSPACE files might be more problematic. It'd probably be better to just remove Protobuf dependency. It seems that keeping it creates more problems than needed.
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.
Yes, removing it would be better. Is that possible though?
I don't see much of a downside to using com_google_protobuf
here for as long as the module supports WORKSPACE. The name is entirely internal to this module with Bzlmod and quite breaking when changed with WORKSPACE.
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.
Removing it wouldn't break too many people. There are 2 levels. 1 to remove cc_proto_library. I only found 1 project on GitHub using it. 2 is to also remove cc_proto toolchain_type, without introducing an alias back to Protobuf (which I can't introduce until protobuf 29.0 is released)
I think everything in this PR has now been merger. |
module_ctx.extension_metadata(reproducible = True)
usage behind abazel_features
check for compatibility with Bazel 6.com_google_protobuf
instead ofprotobuf
with WORKSPACE to avoid missing deps and duplication of protobuf targets.