chore(internal/librariangen): remove the legacy protoc/gRPC plugins #13372
Conversation
Fixes googleapis/librarian#2796 Note that we *may* need to reinstate this in the Dockerfile in order to support go-genproto, but we shouldn't need to reinstate the logic in the Bazel parser etc. We deliberately spot cases where we *would* have used the old plugin, in order to fail hard instead of just omitting the gRPC output. This change affects which version of protoc is used for proto-only paths, but running "librarian update-image" using a locally-build container demonstrates that this doesn't affect the generated code.
Summary of ChangesHello @jskeet, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant cleanup by removing all traces of the legacy Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes the legacy protoc/gRPC plugins, which simplifies the codebase and tooling. The changes are well-executed, removing dead code from the Dockerfile, Go source files, and associated tests. A key improvement is the introduction of a hard failure when the legacy plugin is detected in a BUILD.bazel file, preventing silent misconfigurations. The tests have been updated to reflect this new behavior. My review includes a couple of minor suggestions to improve an error message and strengthen a test case.
| } | ||
| c.hasLegacyGRPC = strings.Contains(goProtoLibraryBlock, "@io_bazel_rules_go//proto:go_grpc") | ||
| if strings.Contains(goProtoLibraryBlock, "@io_bazel_rules_go//proto:go_grpc") { | ||
| return nil, fmt.Errorf("librariangen: BUILD.bazel requires legacy gRPC plugin: %s", fp) |
There was a problem hiding this comment.
The error message requires legacy gRPC plugin could be misinterpreted as a missing dependency. To improve clarity for users who encounter this error, consider rephrasing it to state that the legacy plugin is no longer supported.
| return nil, fmt.Errorf("librariangen: BUILD.bazel requires legacy gRPC plugin: %s", fp) | |
| return nil, fmt.Errorf("librariangen: BUILD.bazel uses legacy gRPC plugin (@io_bazel_rules_go//proto:go_grpc) which is no longer supported: %s", fp) |
| _, err := Parse(tmpDir) | ||
| if err == nil { | ||
| t.Error("Parse() succeeded; want error") | ||
| } |
There was a problem hiding this comment.
To make this test more robust, it's a good practice to assert not just that an error occurred, but that the correct error occurred. You can check if the error message contains a specific substring related to the legacy plugin. Also, consider using t.Fatal when an error is expected but not found, as it stops the test execution immediately, which is appropriate here.
| _, err := Parse(tmpDir) | |
| if err == nil { | |
| t.Error("Parse() succeeded; want error") | |
| } | |
| _, err := Parse(tmpDir) | |
| if err == nil { | |
| t.Fatal("Parse() succeeded; want error") | |
| } | |
| if !strings.Contains(err.Error(), "legacy gRPC plugin") { | |
| t.Errorf("Parse() returned unexpected error: got %v, want substring 'legacy gRPC plugin'", err) | |
| } |
|
Just noticed there were two failed generations with this code - checking... |
jskeet
added
the
do not merge
|
Okay, there are two libraries affected:
@quartzmo Do you think we can migrate https://github.com/googleapis/googleapis/blob/master/google/longrunning/BUILD.bazel? Is there a reason it hasn't already been changed? |
|
Googlers: I've created cl/833760584 to try to change the BUILD.bazel files here. |
|
CL submitted. Once that has gone through, I should be able to run my check again. If there are no changes at that point, I'll remove the do-not-merge label. |
1 participant
Fixes googleapis/librarian#2796
Note that we may need to reinstate this in the Dockerfile in order to support go-genproto, but we shouldn't need to reinstate the logic in the Bazel parser etc.
We deliberately spot cases where we would have used the old plugin, in order to fail hard instead of just omitting the gRPC output.
This change affects which version of protoc is used for proto-only paths, but running "librarian update-image" using a locally-build container demonstrates that this doesn't affect the generated code.