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

Remove explicit dependency on protobuf #221

Merged
merged 1 commit into from
May 14, 2024

Conversation

fmeum
Copy link
Contributor

@fmeum fmeum commented May 14, 2024

With --incompatible_enable_proto_toolchain_resolution, the root module is supposed to provide a proto_lang_toolchain for Java, which injects the runtime. Hardcoded dependencies on the protobuf module would negate the benefits of supplying a toolchain with precompiled protoc and runtime.

Since the only used symbol from the protobuf runtime was an exception class, it has been replaced with a check for its class name. If more protobuf runtime symbols should be needed in the future, they should be obtained from a current_java_proto_runtime target that first looks for a proto_lang_toolchain for Java and only then falls back to the hardcoded reference.

The WORKSPACE deps macro still brings in com_google_protobuf as this may be the only source of the repo for users.

With `--incompatible_enable_proto_toolchain_resolution`, the root module is supposed to provide a `proto_lang_toolchain` for Java, which injects the runtime. Hardcoded dependencies on the `protobuf` module would negate the benefits of supplying a toolchain with precompiled `protoc` and runtime.

Since the only used symbol from the protobuf runtime was an exception class, it has been replaced with a check for its class name. If more protobuf runtime symbols should be needed in the future, they should be obtained from a `current_java_proto_runtime` target that first looks for a `proto_lang_toolchain` for Java and only then falls back to the hardcoded reference.
@fmeum fmeum marked this pull request as ready for review May 14, 2024 09:49
@fmeum
Copy link
Contributor Author

fmeum commented May 14, 2024

CC @alexeagle @comius

I needed to make this change to finally remove any dependency on @com_google_protobuf from a ruleset using stardoc to build its documentation. This greatly speeds up GitHub Actions CI times.

Copy link
Contributor

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

Nice fix! Very interesting that users were forced to compile protobuf_java just to get a type-only symbol 😓

Copy link
Collaborator

@tetromino tetromino left a comment

Choose a reason for hiding this comment

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

Nice!

(I tried to do the same change in the legacy WORKSPACE setup - but it looks like there we still need com_google_protobuf as a transitive dep.)

@tetromino tetromino merged commit 3baa5d1 into bazelbuild:master May 14, 2024
16 checks passed
@fmeum fmeum deleted the remove-protobuf-dep branch May 15, 2024 06:57
@fmeum
Copy link
Contributor Author

fmeum commented May 15, 2024

Could you create a release with this change? Then modules in the BCR could start using it without patches.

@alexeagle
Copy link
Contributor

+1 on request for release, I'm running into the need to patch this out as well

@alexeagle
Copy link
Contributor

@tetromino ping again on a release, the last one is 309 days ago

@tetromino
Copy link
Collaborator

@alexeagle - I want to get #231, #232, and #216 in before cutting the release.

@alexeagle
Copy link
Contributor

LMK if you're interested in the techniques we use for OSS rulesets so release instructions are just "push a tag". Makes it such a non-event that you could release both before and after landing some things 😜

@comius
Copy link
Contributor

comius commented Jun 18, 2024

LMK if you're interested in the techniques we use for OSS rulesets so release instructions are just "push a tag". Makes it such a non-event that you could release both before and after landing some things 😜

I agree it’s cool. Me and @meteorcloudy spent a couple of hours each, fixing it on Monday for a minor rules_proto release. But I really think it’s cool when it works. ;)

@meteorcloudy
Copy link
Member

BTW, the issue we hit for rules_proto release is bazel-contrib/.github#16, where XDG_CACHE_HOME is now also respected by Bazel

@tetromino
Copy link
Collaborator

@alexeagle - stardoc-0.7.0 has been released and is now in the central registry. Sorry for the wait!

LMK if you're interested in the techniques we use for OSS rulesets so release instructions are just "push a tag".

Sure - do you have a good example in mind that Stardoc can follow?

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.

5 participants