scripts: minor refactor to scripts #7403

arvindbr8 · 2024-07-10T21:58:46Z

Fixing parts of #7400.

Changes:

install_protoc.sh
- rename to install-protoc.sh ( following the rest of the file names under scripts/ )
- In order to easily find the start of the program, put the main program in a function called main as the bottom-most function.
- Move all checks from download_binary() to main()
- Modify top-level comment to talk about usage of the script
- Remove INSTALL_PATH=${1:+"$1"}
regenerate.sh
- exec and not source install_protoc.sh as the very first thing in the script
- export GOBIN
- does not save and restore $PATH anymore
vet_common.sh
- rename to common.sh

This PR does not make functional changes to any scripts -- which will be made in an upcoming PR

RELEASE NOTES: none

codecov · 2024-07-10T22:04:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.51%. Comparing base (e54f441) to head (c103912).
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7403      +/-   ##
==========================================
+ Coverage   81.46%   81.51%   +0.05%     
==========================================
  Files         348      348              
  Lines       26752    26752              
==========================================
+ Hits        21793    21807      +14     
+ Misses       3771     3764       -7     
+ Partials     1188     1181       -7

see 17 files with indirect coverage changes

dfawley

Pretty much all LGTM, just some very minor nits. Thanks for picking this up.

dfawley · 2024-07-10T22:24:06Z

scripts/install-protoc.sh

+# This script ensures the installation of protobuf on client machine.
+# In case of manual run of this script, make sure you pass the args
+# expected at
+# https://github.com/grpc/grpc-go/blob/master/scripts/install-protoc.sh#L60


This feels very prone to move around. In fact, line 60 of this file is something to detect your architecture. Maybe we can do something better?

I've updated the top-level comment to talk more about usages. PTAL

dfawley · 2024-07-10T22:25:50Z

scripts/install-protoc.sh

+
+main() {
+  # Check if protoc is already available.
+  if command -v protoc &> /dev/null; then


So if protoc is in my path somewhere, but I wanted to download to a specific path, it still won't do it?

Perhaps the PR description should say what this change is attempting to fix, specifically, instead of just "parts of ###"?

Yes that's actually really confusing. i will fix that behavior in the PR that's to follow.

Perhaps the PR description should say what this change is attempting to fix, specifically, instead of just "parts of ###"?

err.. sorry about that. Initially I had only imagined having a very trivial change in this PR. I've updated the description. PTAL

dfawley · 2024-07-10T22:27:22Z

scripts/install-protoc.sh

+        echo "protoc version $PROTOC_VERSION is already installed."
+        return
+      else
+        die "Existing protoc version ($INSTALL_VERSION) differs. Kindly make sure you have $PROTOC_VERSION installed."


The entire job of this script is to install it for me. It's funny for it to say "please install it". :)

Will update this in the next PR

dfawley · 2024-07-10T22:27:43Z

scripts/install-protoc.sh

+    else
+      echo "Unable to determine installed protoc version. Starting the installation."


dfawley · 2024-07-10T22:28:22Z

scripts/install-protoc.sh

+    "x86_64") ARCH="x86_64";;
+    "aarch64") ARCH="aarch_64";;
+    "arm64") ARCH="aarch_64";;
+  *) die "Unsupported architecture. Please consider manual installation from \


Shouldn't these lines line up? (and below)

Yes, good callout. Fixed.

dfawley · 2024-07-10T22:28:56Z

scripts/install-protoc.sh

+  esac
+}
+
+main "$@"


Please add a newline at the end of the file.

dfawley · 2024-07-10T22:29:32Z

scripts/regenerate.sh

 }
 trap finish EXIT

-GOBIN="${WORKDIR}"/bin
-ORIGINAL_PATH=$PATH
+source ./scripts/install-protoc.sh "${WORKDIR}"


Why source? Generally you don't want to source anything unless it's explicitly supposed to add stuff to your environment.

arvindbr8

Addressed most of your comments in the latest commit. For a few, i will take care of it in the next PR. PTAL

arvindbr8 · 2024-07-11T20:00:14Z

scripts/install-protoc.sh

+
+main() {
+  # Check if protoc is already available.
+  if command -v protoc &> /dev/null; then


Yes that's actually really confusing. i will fix that behavior in the PR that's to follow.

Perhaps the PR description should say what this change is attempting to fix, specifically, instead of just "parts of ###"?

err.. sorry about that. Initially I had only imagined having a very trivial change in this PR. I've updated the description. PTAL

arvindbr8 · 2024-07-11T20:00:28Z

scripts/install-protoc.sh

+        echo "protoc version $PROTOC_VERSION is already installed."
+        return
+      else
+        die "Existing protoc version ($INSTALL_VERSION) differs. Kindly make sure you have $PROTOC_VERSION installed."


Will update this in the next PR

arvindbr8 · 2024-07-11T20:01:08Z

scripts/install-protoc.sh

+# This script ensures the installation of protobuf on client machine.
+# In case of manual run of this script, make sure you pass the args
+# expected at
+# https://github.com/grpc/grpc-go/blob/master/scripts/install-protoc.sh#L60


I've updated the top-level comment to talk more about usages. PTAL

arvindbr8 · 2024-07-11T20:01:15Z

scripts/install-protoc.sh

+    else
+      echo "Unable to determine installed protoc version. Starting the installation."


arvindbr8 · 2024-07-11T20:01:52Z

scripts/install-protoc.sh

+    "x86_64") ARCH="x86_64";;
+    "aarch64") ARCH="aarch_64";;
+    "arm64") ARCH="aarch_64";;
+  *) die "Unsupported architecture. Please consider manual installation from \


Yes, good callout. Fixed.

arvindbr8 · 2024-07-11T20:01:57Z

scripts/install-protoc.sh

+  esac
+}
+
+main "$@"


arvindbr8 · 2024-07-11T20:02:01Z

scripts/regenerate.sh

 }
 trap finish EXIT

-GOBIN="${WORKDIR}"/bin
-ORIGINAL_PATH=$PATH
+source ./scripts/install-protoc.sh "${WORKDIR}"


scripts: minor refactor to scripts

2820038

arvindbr8 added the Type: Testing label Jul 10, 2024

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

arvindbr8 requested a review from dfawley July 10, 2024 21:58

arvindbr8 assigned dfawley Jul 10, 2024

add untracked files

b646b4e

fix download URL

dca3554

dfawley reviewed Jul 10, 2024

View reviewed changes

address comments

8cbcc0d

arvindbr8 commented Jul 11, 2024

View reviewed changes

arvindbr8 requested a review from dfawley July 11, 2024 20:02

uncomment debug

c103912

dfawley approved these changes Jul 12, 2024

View reviewed changes

dfawley assigned arvindbr8 and unassigned dfawley Jul 12, 2024

arvindbr8 merged commit c5c0e18 into grpc:master Jul 12, 2024
13 checks passed

arvindbr8 deleted the fix_regenerate_1 branch July 12, 2024 17:32

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

scripts: minor refactor to scripts (grpc#7403)

598830c

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

scripts: minor refactor to scripts (grpc#7403)

aa67bc1

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: minor refactor to scripts #7403

scripts: minor refactor to scripts #7403

arvindbr8 commented Jul 10, 2024 •

edited

Loading

codecov bot commented Jul 10, 2024 •

edited

Loading

dfawley left a comment

dfawley Jul 10, 2024

arvindbr8 Jul 11, 2024

dfawley Jul 10, 2024

arvindbr8 Jul 11, 2024

dfawley Jul 10, 2024

arvindbr8 Jul 11, 2024

dfawley Jul 10, 2024

arvindbr8 Jul 11, 2024

dfawley Jul 10, 2024

arvindbr8 Jul 11, 2024

dfawley Jul 10, 2024

arvindbr8 Jul 11, 2024

dfawley Jul 10, 2024

arvindbr8 Jul 11, 2024

arvindbr8 left a comment

arvindbr8 Jul 11, 2024

arvindbr8 Jul 11, 2024

arvindbr8 Jul 11, 2024

arvindbr8 Jul 11, 2024

arvindbr8 Jul 11, 2024

arvindbr8 Jul 11, 2024

arvindbr8 Jul 11, 2024

		else
		echo "Unable to determine installed protoc version. Starting the installation."

scripts: minor refactor to scripts #7403

scripts: minor refactor to scripts #7403

Conversation

arvindbr8 commented Jul 10, 2024 • edited Loading

codecov bot commented Jul 10, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

arvindbr8 commented Jul 10, 2024 •

edited

Loading

codecov bot commented Jul 10, 2024 •

edited

Loading