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.

Sign up for GitHub

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

Jump to bottom

Improve regenerate.sh to use the correct proto compiler version #6583

Open
easwars opened this issue Aug 25, 2023 · 9 comments · May be fixed by #7064
Open

Improve regenerate.sh to use the correct proto compiler version #6583

easwars opened this issue Aug 25, 2023 · 9 comments · May be fixed by #7064
Assignees
@aranjans
Labels
P2 Status: Help Wanted Type: Feature New features or improvements in behavior

Comments

@easwars
Copy link
Contributor

easwars commented Aug 25, 2023

Currently the regenerate script installs protoc-gen-go and protoc-gen-go-grpc in a workdir, but uses the system installed protoc which means that running the regenerate.sh script could result in all generated files being changed with a diff which would look something like:

@@ -20,7 +20,7 @@
 // Code generated by protoc-gen-go. DO NOT EDIT.
 // versions:
 //     protoc-gen-go v1.31.0
-//     protoc        v4.22.0
+//     protoc        v3.21.2
 // source: grpc/lb/v1/load_balancer.proto
@avinilcodes
Copy link

I would like to contribute, can you please assign it to me

@dfawley
Copy link
Member

dfawley commented Sep 26, 2023

@avinilcodes - Thanks!

Note that most people on our team use both linux and macOS, so the script should be able to handle this properly on both systems, or at least fall back to the old behavior gracefully.

@Clement-Jean
Copy link
Contributor

Quick question: why not using the protoc binary from the fresh pull of protobuf repository? Wouldn't that solve the problem?

We could then use Make, CMake or Bazel to generate the binary and replace all the protoc refs to the bin folder of protobuf.

@dfawley
Copy link
Member

dfawley commented Oct 3, 2023

We don't want to always use the latest version because that would require updates to our generated code frequently. We have a release document that includes updating other dependencies that should update this one as well. But we'd like it to be a static version for now.

@arvindbr8
Copy link
Member

@avinilcodes -- friendly ping

@github-actions
Copy link

This issue is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@github-actions github-actions bot added the stale label Oct 22, 2023
@dfawley dfawley removed the stale label Oct 30, 2023
@dfawley dfawley reopened this Oct 30, 2023
@dfawley dfawley added Type: Feature New features or improvements in behavior and removed Status: Requires Reporter Clarification Type: Bug labels Oct 30, 2023
@Aditya-Sood
Copy link
Contributor

hi @dfawley, can I pick this up?

@dfawley
Copy link
Member

dfawley commented Nov 7, 2023

Similar to the other issue, @Aditya-Sood - LMK if you still want this.

@Aditya-Sood
Copy link
Contributor

thank you Doug, as suggested I will close the other issue and then pick this up

@dfawley dfawley mentioned this issue Dec 12, 2023
Closed
@aranjans aranjans linked a pull request Mar 25, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aranjans aranjans

Labels
P2 Status: Help Wanted Type: Feature New features or improvements in behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

scripts: improve regenerate.sh to use the correct proto compiler version aranjans/grpc-go
8 participants
@easwars @ginayeh @Aditya-Sood @arvindbr8 @Clement-Jean @avinilcodes @dfawley @aranjans