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.

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

scripts: minor refactor to scripts #7403

Merged
merged 5 commits into from
Jul 12, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
File renamed without changes.
78 changes: 78 additions & 0 deletions scripts/install-protoc.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
#!/bin/bash
# Copyright 2024 gRPC authors.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
# 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


set -eu -o pipefail

source "$(dirname $0)/common.sh"

# The version of protoc that will be installed.
PROTOC_VERSION="27.1"

# Function to download pre-built binaries for Linux with
# ARCH as $1, OS as $2, and INSTALL_PATH as $3 arguments.
download_binary() {
DOWNLOAD_URL="https://github.com/protocolbuffers/protobuf/releases/download/v${PROTOC_VERSION}/protoc-${PROTOC_VERSION}-$2-$1.zip"
INSTALL_DIR="${3:-${GOBIN:-${GOPATH:-$HOME/go}}}"

# Download and unzip.
# -L follows redirects.
# -O writes output to a file.
curl -LO "${DOWNLOAD_URL}"
unzip "protoc-${PROTOC_VERSION}-$2-$1.zip" -d "${INSTALL_DIR}"
rm "protoc-${PROTOC_VERSION}-$2-$1.zip"
rm "${INSTALL_DIR}/readme.txt"
}


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ###"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if INSTALL_VERSION=$(protoc --version | cut -d' ' -f2 2>/dev/null); then
if [ "$INSTALL_VERSION" = "$PROTOC_VERSION" ]; then
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."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will update this in the next PR

fi
else
echo "Unable to determine installed protoc version. Starting the installation."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

fi
fi

# Detect the architecture
case "$(uname -m)" in
"x86_64") ARCH="x86_64";;
"aarch64") ARCH="aarch_64";;
"arm64") ARCH="aarch_64";;
*) die "Unsupported architecture. Please consider manual installation from \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good callout. Fixed.

https://github.com/protocolbuffers/protobuf/releases/ and add to PATH."
esac

# Detect the Operating System
INSTALL_PATH="${1}"
case "$(uname -s)" in
"Darwin") download_binary $ARCH "osx" "$INSTALL_PATH";;
"Linux") download_binary $ARCH "linux" "$INSTALL_PATH";;
*) die "Unsupported OS. Please consider manual installation from \
https://github.com/protocolbuffers/protobuf/releases/ and add to PATH" ;;
esac
}

main "$@"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a newline at the end of the file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

69 changes: 0 additions & 69 deletions scripts/install_protoc.sh

This file was deleted.

10 changes: 4 additions & 6 deletions scripts/regenerate.sh
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,12 @@ WORKDIR=$(mktemp -d)

function finish {
rm -rf "$WORKDIR"
# Revert back the PATH to client's original value
export PATH=$ORIGINAL_PATH
}
trap finish EXIT

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


export GOBIN="${WORKDIR}"/bin
export PATH="${GOBIN}:${PATH}"
mkdir -p "${GOBIN}"

Expand All @@ -51,7 +50,6 @@ mkdir -p "${WORKDIR}/googleapis/google/rpc"
echo "curl https://raw.githubusercontent.com/googleapis/googleapis/master/google/rpc/code.proto"
curl --silent https://raw.githubusercontent.com/googleapis/googleapis/master/google/rpc/code.proto > "${WORKDIR}/googleapis/google/rpc/code.proto"

source ./scripts/install_protoc.sh $WORKDIR

mkdir -p "${WORKDIR}/out"

Expand Down Expand Up @@ -98,7 +96,7 @@ Mgrpc/testing/empty.proto=google.golang.org/grpc/interop/grpc_testing

for src in ${SOURCES[@]}; do
echo "protoc ${src}"
protoc --go_out=${OPTS}:${WORKDIR}/out --go-grpc_out=${OPTS},use_generic_streams_experimental=true:${WORKDIR}/out \
protoc --go_out=${OPTS}:${WORKDIR}/out --go-grpc_out=${OPTS}:${WORKDIR}/out \
-I"." \
-I"${WORKDIR}/grpc-proto" \
-I"${WORKDIR}/googleapis" \
Expand Down
6 changes: 3 additions & 3 deletions scripts/vet-proto.sh
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ set -ex # Exit on error; debugging enabled.
set -o pipefail # Fail a pipe if any sub-command fails.

# - Source them sweet sweet helpers.
source "$(dirname $0)/vet-common.sh"
source "$(dirname $0)/common.sh"

# - Check to make sure it's safe to modify the user's git repo.
git status --porcelain | fail_on_output
Expand All @@ -21,9 +21,9 @@ trap cleanup EXIT
# go.)
if [[ "$1" = "-install" ]]; then
if [[ "${GITHUB_ACTIONS}" = "true" ]]; then
source ./scripts/install_protoc.sh "/home/runner/go"
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"
die "run protoc installer https://github.com/grpc/grpc-go/blob/master/scripts/install-protoc.sh"
fi
echo SUCCESS
exit 0
Expand Down
2 changes: 1 addition & 1 deletion scripts/vet.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
set -ex # Exit on error; debugging enabled.
set -o pipefail # Fail a pipe if any sub-command fails.

source "$(dirname $0)/vet-common.sh"
source "$(dirname $0)/common.sh"

# Check to make sure it's safe to modify the user's git repo.
git status --porcelain | fail_on_output
Expand Down