scripts: regenerate pbs with caching deps to a fixed tmp folder #7409

arvindbr8 · 2024-07-12T23:31:28Z

@dfawley I went with something that's more along the lines of option no.3 in the issue. Now the deps are installed in a
"well-known" directory which is /tmp/grpc-go-tools. This will ensure not requiring to re-download all deps when running regenerate.

Changes:

install-protoc.sh

Now requires an INSTALL_PATH to be specified
- This is a behavior change. Previously INSTALL_PATH was optional and would put it in your GOBIN/GOPATH. We dont want to be polluting the users GO dir with non-golang binaries.
Now checks if protoc in the required version is present in INSTALL_PATH and not $PATH

regenerate.sh

Installs everything to /tmp/grpc-go-tools instead of mktmp
Only download deps if not present
Other shell linter changes

cc: @aranjans

RELEASE NOTES: none

…orks

codecov · 2024-07-12T23:37:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.47%. Comparing base (ecbb837) to head (4a49832).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7409      +/-   ##
==========================================
- Coverage   81.47%   81.47%   -0.01%     
==========================================
  Files         350      350              
  Lines       26843    26846       +3     
==========================================
+ Hits        21871    21873       +2     
- Misses       3784     3785       +1     
  Partials     1188     1188

see 19 files with indirect coverage changes

dfawley

LGTM, just some nits.

dfawley · 2024-07-16T19:53:15Z

scripts/install-protoc.sh

+  # Detect the hardware platform.
+  ARCH="$(uname -m)"
+  # Detect the Operating System.
+  OS="$(uname -s)"


I'd probably delete these vars and do these things in download_binary instead. Note that you're setting ARCH twice to different things, so that's a little confusing, and you're passing these as positional parameters, so separating their usage from their definition.

Yes you are right. I dont know why we needed a function to be defined like this. I've moved everything into main() now. Looks cleaner this way.

dfawley · 2024-07-16T19:54:46Z

scripts/regenerate.sh


-./scripts/install-protoc.sh "${WORKDIR}"
+"./$(dirname "${0}")/install-protoc.sh" ${WORKDIR}


I think you want to remove ./ at the start? Try executing as /abs/path/to/scripts/regenerate.sh and I think it will fail?

Yea, for whatever reason i've always run executables with a leading ./. Every time I do a shell PR I learn something new.

I also did not want to put the abs/path since that would require that you run the script from the gRPC-go/ dir

for whatever reason i've always run executables with a leading ./.

If you're running one in the current directory, then that's important. If it's in an absolute path, you'd use /usr/bin/ls, e.g. If it's in a relative path, then either ./bin/foo or bin/foo work.

dfawley · 2024-07-16T19:55:43Z

scripts/regenerate.sh

@@ -31,7 +28,7 @@ mkdir -p "${GOBIN}"
 echo "remove existing generated files"


Nit: "Removing existing generated files."

dfawley · 2024-07-16T19:56:31Z

scripts/regenerate.sh

@@ -31,7 +28,7 @@ mkdir -p "${GOBIN}"
 echo "remove existing generated files"
 # grpc_testing_not_regenerate/*.pb.go is not re-generated,
 # see grpc_testing_not_regenerate/README.md for details.
-rm -f $(find . -name '*.pb.go' | grep -v 'grpc_testing_not_regenerate')
+find . -name '*.pb.go' | grep -v 'grpc_testing_not_regenerate' | xargs rm -f || true

 echo "go install google.golang.org/protobuf/cmd/protoc-gen-go"


Maybe "Executing: go install _______" (etc)?

Done. Let me know how you feel about the new echo-s

dfawley · 2024-07-16T19:56:50Z

scripts/regenerate.sh

@@ -40,10 +37,18 @@ echo "go install cmd/protoc-gen-go-grpc"
 (cd cmd/protoc-gen-go-grpc && go install .)

 echo "git clone https://github.com/grpc/grpc-proto"
-git clone --quiet https://github.com/grpc/grpc-proto "${WORKDIR}/grpc-proto"
+if [ -d "${WORKDIR}/grpc-proto" ]; then
+  (cd "${WORKDIR}/grpc-proto" && git pull)


Why parens here (and below)?

so that I dont have to cd - after the command :)

Fair enough

…#7409)

scripts: regenerate pbs with caching deps to a fixed tmp folder

05b03f1

arvindbr8 added the Type: Meta Github repo, process, etc label Jul 12, 2024

arvindbr8 added this to the 1.66 Release milestone Jul 12, 2024

arvindbr8 requested a review from dfawley July 12, 2024 23:31

arvindbr8 assigned dfawley Jul 12, 2024

seems like tar isnt working in GithubActions. Lets see if adding -x w…

75e2749

…orks

arvindbr8 added 2 commits July 12, 2024 16:43

fallback to using unzip.. tar does'nt like zip. who would have guessed

7d4ee55

undo an oopsie change. everything is OKAY ow

969fb3f

dfawley approved these changes Jul 16, 2024

View reviewed changes

dfawley assigned arvindbr8 and unassigned dfawley Jul 16, 2024

arvindbr8 added 2 commits July 16, 2024 15:16

make changes to install-protoc

a278ecc

address dfawley's comments

ddd59fa

arvindbr8 requested a review from dfawley July 16, 2024 22:38

remove ws and double quote line 22 in regenerate.sh

4a49832

arvindbr8 assigned dfawley and unassigned arvindbr8 Jul 16, 2024

dfawley approved these changes Jul 17, 2024

View reviewed changes

dfawley assigned arvindbr8 and unassigned dfawley Jul 17, 2024

arvindbr8 merged commit 64adc81 into grpc:master Jul 17, 2024
13 checks passed

printchard pushed a commit to printchard/grpc-go that referenced this pull request Jul 30, 2024

scripts: regenerate pbs with caching deps to a fixed tmp folder (grpc…

192dc41

…#7409)

printchard pushed a commit to printchard/grpc-go that referenced this pull request Jul 30, 2024

scripts: regenerate pbs with caching deps to a fixed tmp folder (grpc…

60d5d72

…#7409)

tbg mentioned this pull request Nov 27, 2024

DEPS: upgrade grpc to v1.68.0 cockroachdb/cockroach#136278

Draft

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scripts: regenerate pbs with caching deps to a fixed tmp folder #7409

scripts: regenerate pbs with caching deps to a fixed tmp folder #7409

arvindbr8 commented Jul 12, 2024

codecov bot commented Jul 12, 2024 •

edited

Loading

dfawley left a comment

dfawley Jul 16, 2024

arvindbr8 Jul 16, 2024

dfawley Jul 16, 2024

arvindbr8 Jul 16, 2024

dfawley Jul 17, 2024

dfawley Jul 16, 2024

arvindbr8 Jul 16, 2024

dfawley Jul 16, 2024

arvindbr8 Jul 16, 2024

dfawley Jul 16, 2024

arvindbr8 Jul 16, 2024

dfawley Jul 17, 2024


		./scripts/install-protoc.sh "${WORKDIR}"
		"./$(dirname "${0}")/install-protoc.sh" ${WORKDIR}

		@@ -31,7 +28,7 @@ mkdir -p "${GOBIN}"
		echo "remove existing generated files"

scripts: regenerate pbs with caching deps to a fixed tmp folder #7409

scripts: regenerate pbs with caching deps to a fixed tmp folder #7409

Conversation

arvindbr8 commented Jul 12, 2024

codecov bot commented Jul 12, 2024 • edited Loading

Codecov Report

dfawley 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

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

codecov bot commented Jul 12, 2024 •

edited

Loading