scripts: improve regenerate.sh to use the correct proto compiler version #7064

aranjans · 2024-03-25T08:39:33Z

Fixes #6583

This PR includes the following changes:

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 in protoc version for the generated files.
Now we have a install_protoc.sh script(responsible for installing protobuf on client side in case its not present) which is called by vet.sh and regenerate.sh.

RELEASE NOTES: n/a

codecov · 2024-03-25T08:42:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.70%. Comparing base (adf976b) to head (b3106cc).
Report is 85 commits behind head on master.

❗ Current head b3106cc differs from pull request most recent head ab458c5

Please upload reports for the commit ab458c5 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7064      +/-   ##
==========================================
- Coverage   81.24%   80.70%   -0.54%     
==========================================
  Files         345      346       +1     
  Lines       33941    33802     -139     
==========================================
- Hits        27574    27280     -294     
- Misses       5202     5355     +153     
- Partials     1165     1167       +2

see 33 files with indirect coverage changes

arvindbr8

I would prefer if we didnt track the version of protoc in different places.

What might be nicer is if both regenerate.sh and vet.sh points to the same place to install protoc. Also note ./vet.sh -install basically does the same thing of installing protoc for Github Actions.

I prefer creating a new script which installs protoc based on the ${OS}. And for ./vet -install I could call into the script to install the linux and x86_64 flavor of protoc. And do something similar for regenerate.sh

vet.sh

regenerate.sh

arvindbr8

I've made a few comments on your change. Could you please take a look at it?

protoc_installer.sh

regenerate.sh

protoc_installer.sh

scripts/install_protoc.sh

scripts/regenerate.sh

scripts/vet-proto.sh

arvindbr8 · 2024-05-28T23:49:45Z

Also this still seems to be not resolved? #7064 (comment)

arvindbr8 · 2024-05-29T23:54:22Z

i would prefer if the reporter of a comment to close the conversation as resolved. The Github UI hides the conversation, and we would need to click on each to see if/what the response is.

arvindbr8

Great! Looks better now. LGTM modulo the comments from this pass.

scripts/install_protoc.sh

arvindbr8 · 2024-06-01T00:02:37Z

scripts/vet-proto.sh

+ if [[ "${GITHUB_ACTIONS}" = "true" ]]; then
+ source ./scripts/install_protoc.sh "/home/runner/go"
+ else
+ die "run protoc installer https://github.com/grpc/grpc-go/scripts/install_protoc.sh."


broken link?

also shouldn't we mention how to run the installer from the terminal?

We should also put something in the top level package comment in scripts/install_protoc.sh

also i'm not convinced that only Github Actions should be able to run ./scripts/vet-proto.sh with -install. I would expect that if a user runs this, then we install protoc for them

https://github.com/grpc/grpc-go/scripts/install_protoc.sh. install_protoc.sh script is a new file so this link will be accessed only when it is merged.

This is how links to files in our repo @ master would look like https://github.com/grpc/grpc-go/blob/master/admin/admin.go

Also about this; why not just something like:

source ./scripts/install_protoc.sh

scripts/install_protoc.sh

arvindbr8 · 2024-06-04T06:55:25Z

scripts/vet-proto.sh

+ if [[ "${GITHUB_ACTIONS}" = "true" ]]; then
+ source ./scripts/install_protoc.sh "/home/runner/go"
+ else
+ die "run protoc installer https://github.com/grpc/grpc-go/scripts/install_protoc.sh."


This is how links to files in our repo @ master would look like https://github.com/grpc/grpc-go/blob/master/admin/admin.go

Also about this; why not just something like:

source ./scripts/install_protoc.sh

scripts/install_protoc.sh

arvindbr8 · 2024-06-04T07:42:26Z

scripts/vet-proto.sh

+ if [[ "${GITHUB_ACTIONS}" = "true" ]]; then
+ source ./scripts/install_protoc.sh "/home/runner/go"
+ else
+ die "run protoc installer https://github.com/grpc/grpc-go/blob/master/scripts/install_protoc.sh"
+ fi


nit: This still looks like a link to what to do. But just points to a script without the how to. Have you looked at my other comment that talked about why it might be nice to run install_protoc.sh also in this case?

added few lines on how to use install_protoc.sh in case of manual run.

arvindbr8

LGTM, modulo two nits

arvindbr8 · 2024-06-04T07:43:44Z

scripts/regenerate.sh

@@ -19,12 +19,16 @@ WORKDIR=$(mktemp -d)

 function finish {
 rm -rf "$WORKDIR"
+ # Revert back the PATH to client's original value
+ export PATH=$ORIGINAL_PATH
+ echo "Restored PATH variable to original value."


nit: remove this echo as well. The user doesnt have to know that the state changed and was reverted.

aranjans force-pushed the aranjans_6583 branch 2 times, most recently from 8bb8747 to af6bc49 Compare March 25, 2024 17:33

dfawley added the Type: Meta Github repo, process, etc label Mar 25, 2024

dfawley added this to the 1.64 Release milestone Mar 25, 2024

dfawley requested a review from arvindbr8 March 25, 2024 17:37

aranjans marked this pull request as ready for review March 25, 2024 17:37

aranjans force-pushed the aranjans_6583 branch 2 times, most recently from e404049 to a582a54 Compare March 26, 2024 06:59

arvindbr8 requested changes Mar 26, 2024

View reviewed changes

vet.sh Outdated Show resolved Hide resolved

regenerate.sh Show resolved Hide resolved

regenerate.sh Outdated Show resolved Hide resolved

arvindbr8 assigned aranjans Mar 26, 2024

aranjans force-pushed the aranjans_6583 branch 12 times, most recently from 20c21cb to a880e4d Compare March 27, 2024 10:00

dfawley assigned arvindbr8 and unassigned aranjans Mar 27, 2024

arvindbr8 requested changes Mar 29, 2024

View reviewed changes

arvindbr8 assigned aranjans and unassigned arvindbr8 Mar 29, 2024

aranjans force-pushed the aranjans_6583 branch 2 times, most recently from 2cddfeb to cc78601 Compare April 1, 2024 06:40

aranjans removed their assignment May 28, 2024

aranjans force-pushed the aranjans_6583 branch from 13f5110 to 64983a1 Compare May 28, 2024 08:24

arvindbr8 requested changes May 28, 2024

View reviewed changes

arvindbr8 assigned aranjans and unassigned arvindbr8 May 28, 2024

arvindbr8 added the Status: Requires Reporter Clarification label May 29, 2024

aranjans force-pushed the aranjans_6583 branch 2 times, most recently from 2baac2d to 32610ba Compare May 30, 2024 07:45

Responded to Arvind's comment

77ac334

aranjans force-pushed the aranjans_6583 branch from 32610ba to 77ac334 Compare May 30, 2024 07:48

aranjans removed the Status: Requires Reporter Clarification label May 30, 2024

arvindbr8 requested changes Jun 1, 2024

View reviewed changes

arvindbr8 added the Status: Requires Reporter Clarification label Jun 1, 2024

aranjans removed the Status: Requires Reporter Clarification label Jun 3, 2024

aranjans force-pushed the aranjans_6583 branch 3 times, most recently from 5313711 to 6f7efac Compare June 4, 2024 06:39

arvindbr8 requested changes Jun 4, 2024

View reviewed changes

final fix

193e22c

aranjans force-pushed the aranjans_6583 branch from 6f7efac to 193e22c Compare June 4, 2024 07:18

fix

8142941

aranjans force-pushed the aranjans_6583 branch from b5bb81f to 8142941 Compare June 4, 2024 07:35

arvindbr8 reviewed Jun 4, 2024

View reviewed changes

arvindbr8 approved these changes Jun 4, 2024

View reviewed changes

arvindbr8 reviewed Jun 4, 2024

View reviewed changes

fix

ab458c5

purnesh42H assigned purnesh42H and unassigned aranjans Jun 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scripts: improve regenerate.sh to use the correct proto compiler version #7064

scripts: improve regenerate.sh to use the correct proto compiler version #7064

aranjans commented Mar 25, 2024 •

edited

codecov bot commented Mar 25, 2024 •

edited

arvindbr8 left a comment

arvindbr8 left a comment

arvindbr8 commented May 28, 2024 •

edited

arvindbr8 commented May 29, 2024

arvindbr8 left a comment

arvindbr8 Jun 1, 2024

arvindbr8 Jun 1, 2024

arvindbr8 Jun 1, 2024

aranjans Jun 3, 2024

arvindbr8 Jun 4, 2024

arvindbr8 Jun 4, 2024

arvindbr8 Jun 4, 2024

aranjans Jun 4, 2024

arvindbr8 left a comment •

edited

arvindbr8 Jun 4, 2024

aranjans Jun 4, 2024

scripts: improve regenerate.sh to use the correct proto compiler version #7064

Are you sure you want to change the base?

scripts: improve regenerate.sh to use the correct proto compiler version #7064

Conversation

aranjans commented Mar 25, 2024 • edited

codecov bot commented Mar 25, 2024 • edited

Codecov Report

arvindbr8 left a comment

Choose a reason for hiding this comment

arvindbr8 left a comment

Choose a reason for hiding this comment

arvindbr8 commented May 28, 2024 • edited

arvindbr8 commented May 29, 2024

arvindbr8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvindbr8 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aranjans commented Mar 25, 2024 •

edited

codecov bot commented Mar 25, 2024 •

edited

arvindbr8 commented May 28, 2024 •

edited

arvindbr8 left a comment •

edited