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

chore: bake the grpc plugin in the hermetic library generation image #3045

Merged
merged 7 commits into from
Jul 25, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
12 changes: 11 additions & 1 deletion .cloudbuild/library_generation/library_generation.Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ SHELL [ "/bin/bash", "-c" ]

ARG OWLBOT_CLI_COMMITTISH=ac84fa5c423a0069bbce3d2d869c9730c8fdf550
ARG PROTOC_VERSION=25.3
ARG GRPC_VERSION=1.62.2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we configure this with renovate bot, and group it with other grpc update? e.g. There is an existing PR that updates gRPC to the latest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

{
"customType": "regex",
"fileMatch": [
"^gax-java/dependencies\\.properties$",
"^\\.cloudbuild/library_generation/library_generation\\.Dockerfile$"
],
"matchStrings": [
"version\\.io_grpc=(?<currentValue>.+?)\\n",
"ARG GRPC_VERSION=[\"']?(?<currentValue>.+?)[\"']?\\s+"
],
"depNameTemplate": "io.grpc:grpc-core",
"datasourceTemplate": "maven"
},

Copy link
Collaborator

Choose a reason for hiding this comment

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

This version can be updated to 1.65.1 now since this PR is merged..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

ENV HOME=/home
ENV OS_ARCHITECTURE="linux-x86_64"

# install OS tools
RUN apt-get update && apt-get install -y \
Expand All @@ -32,11 +34,19 @@ COPY library_generation /src
# install protoc
WORKDIR /protoc
RUN source /src/utils/utilities.sh \
&& download_protoc "${PROTOC_VERSION}" "linux-x86_64"
&& download_protoc "${PROTOC_VERSION}" "${OS_ARCHITECTURE}"
# we indicate protoc is available in the container via env vars
ENV DOCKER_PROTOC_LOCATION=/protoc
ENV DOCKER_PROTOC_VERSION="${PROTOC_VERSION}"

# install grpc
WORKDIR /grpc
RUN source /src/utils/utilities.sh \
Copy link
Collaborator

Choose a reason for hiding this comment

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

source /src/utils/utilities.sh is not needed anymore. We should probably split

RUN source /src/utils/utilities.sh \
	&& download_protoc "${PROTOC_VERSION}" "${OS_ARCHITECTURE}"

to

RUN source /src/utils/utilities.sh
RUN download_protoc "${PROTOC_VERSION}" "${OS_ARCHITECTURE}"

Then here we can just do

RUN download_grpc_plugin "${GRPC_VERSION}" "${OS_ARCHITECTURE}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is something I considered initially when working on the Dockerfile. The effects of source are contained in the RUN statement only, because it is translated into /bin/sh -c "source script.sh" (see SO)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool, that's good to know!

&& download_grpc_plugin "${GRPC_VERSION}" "${OS_ARCHITECTURE}"
# similar to protoc, we indicate grpc is available in the container via env vars
ENV DOCKER_GRPC_LOCATION="/grpc/protoc-gen-grpc-java-${GRPC_VERSION}-${OS_ARCHITECTURE}.exe"
ENV DOCKER_GRPC_VERSION="${GRPC_VERSION}"

# use python 3.11 (the base image has several python versions; here we define the default one)
RUN rm $(which python3)
RUN ln -s $(which python3.11) /usr/local/bin/python
Expand Down
4 changes: 3 additions & 1 deletion library_generation/generate_library.sh
Original file line number Diff line number Diff line change
Expand Up @@ -183,13 +183,15 @@ case "${proto_path}" in
;;
esac
# download gapic-generator-java, protobuf and grpc plugin.
# the download_tools function will create the environment variables "protoc_path"
# and "grpc_path", to be used in the protoc calls below.
download_tools "${gapic_generator_version}" "${protoc_version}" "${grpc_version}" "${os_architecture}"
##################### Section 1 #####################
# generate grpc-*/
#####################################################
if [[ ! "${transport}" == "rest" ]]; then
# do not need to generate grpc-* if the transport is `rest`.
"${protoc_path}"/protoc "--plugin=protoc-gen-rpc-plugin=protoc-gen-grpc-java-${grpc_version}-${os_architecture}.exe" \
"${protoc_path}"/protoc "--plugin=protoc-gen-rpc-plugin=${grpc_path}" \
"--rpc-plugin_out=:${temp_destination_path}/java_grpc.jar" \
${proto_files} # Do not quote because this variable should not be treated as one long string.
# unzip java_grpc.jar to grpc-*/src/main/java
Expand Down
43 changes: 43 additions & 0 deletions library_generation/test/generate_library_unit_tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,21 @@ get_grpc_version_failed_with_invalid_generator_version_test() {
assertEquals 1 $((res))
}

get_grpc_version_succeed_docker_env_var_test() {
local version_with_docker
local version_without_docker
export DOCKER_GRPC_VERSION="9.9.9"
# get_grpc_version should prioritize DOCKER_GRPC_VERSION
version_with_docker=$(get_grpc_version "2.24.0")
assertEquals "${DOCKER_GRPC_VERSION}" "${version_with_docker}"
unset DOCKER_GRPC_VERSION
}

get_protoc_version_succeed_docker_env_var_test() {
local version_with_docker
local version_without_docker
export DOCKER_PROTOC_VERSION="9.9.9"
# get_protoc_version should prioritize DOCKER_PROTOC_VERSION
version_with_docker=$(get_protoc_version "2.24.0")
assertEquals "${DOCKER_PROTOC_VERSION}" "${version_with_docker}"
unset DOCKER_PROTOC_VERSION
Expand Down Expand Up @@ -162,13 +173,43 @@ download_tools_succeed_with_baked_protoc() {

local test_ggj_version="2.40.0"
local test_grpc_version="1.64.0"
# we expect download_tools to decide to use DOCKER_PROTOC_LOCATION because
# the protoc version we want to download is the same as DOCKER_PROTOC_VERSION.
# Note that `protoc_bin_folder` is just the expected formatted value that
# download_tools will format using DOCKER_PROTOC_VERSION (via
# download_protoc).
download_tools "${test_ggj_version}" "99.99" "${test_grpc_version}" "linux-x86_64"
assertEquals "${protoc_bin_folder}" "${protoc_path}"

rm -rdf "${output_folder}"
unset DOCKER_PROTOC_LOCATION
unset DOCKER_PROTOC_VERSION
unset output_folder
unset protoc_path
}

download_tools_succeed_with_baked_grpc() {
# This test has the same structure as
# download_tools_succeed_with_baked_protoc, but meant for the grpc plugin.
local test_dir=$(mktemp -d)
pushd "${test_dir}"
export DOCKER_GRPC_LOCATION=$(mktemp -d)
export DOCKER_GRPC_VERSION="99.99"
export output_folder=$(get_output_folder)
mkdir "${output_folder}"

local test_ggj_version="2.40.0"
local test_protoc_version="1.64.0"
# we expect download_tools to decide to use DOCKER_GRPC_LOCATION because
# the protoc version we want to download is the same as DOCKER_GRPC_VERSION
download_tools "${test_ggj_version}" "${test_protoc_version}" "99.99" "linux-x86_64"
assertEquals "${DOCKER_GRPC_LOCATION}" "${grpc_path}"

rm -rdf "${output_folder}"
unset DOCKER_GRPC_LOCATION
unset DOCKER_GRPC_VERSION
unset output_folder
unset grpc_path
}

download_grpc_plugin_succeed_with_valid_version_linux_test() {
Expand Down Expand Up @@ -293,6 +334,7 @@ test_list=(
extract_folder_name_test
get_grpc_version_succeed_with_valid_generator_version_test
get_grpc_version_failed_with_invalid_generator_version_test
get_grpc_version_succeed_docker_env_var_test
get_protoc_version_succeed_docker_env_var_test
get_protoc_version_succeed_with_valid_generator_version_test
get_protoc_version_failed_with_invalid_generator_version_test
Expand All @@ -307,6 +349,7 @@ test_list=(
download_protoc_failed_with_invalid_version_linux_test
download_protoc_failed_with_invalid_arch_test
download_tools_succeed_with_baked_protoc
download_tools_succeed_with_baked_grpc
download_grpc_plugin_succeed_with_valid_version_linux_test
download_grpc_plugin_succeed_with_valid_version_macos_test
download_grpc_plugin_failed_with_invalid_version_linux_test
Expand Down
45 changes: 39 additions & 6 deletions library_generation/utils/utilities.sh
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,18 @@ download_gapic_generator_pom_parent() {
download_generator_artifact "${gapic_generator_version}" "gapic-generator-java-pom-parent-${gapic_generator_version}.pom" "gapic-generator-java-pom-parent"
}

# This function returns the version of the grpc plugin to generate the libraries. If
# DOCKER_GRPC_VERSION is set, this will be the version. Otherwise, it will be
# computed from the gapic-generator-pom-parent artifact at the specified
# gapic_generator_version.
get_grpc_version() {
local gapic_generator_version=$1
local grpc_version
if [[ -n "${DOCKER_GRPC_VERSION}" ]]; then
>&2 echo "Using grpc version baked into the container: ${DOCKER_GRPC_VERSION}"
echo "${DOCKER_GRPC_VERSION}"
return
fi
pushd "${output_folder}" > /dev/null
# get grpc version from gapic-generator-java-pom-parent/pom.xml
download_gapic_generator_pom_parent "${gapic_generator_version}"
Expand All @@ -113,6 +122,10 @@ get_grpc_version() {
echo "${grpc_version}"
}

# This function returns the version of protoc to generate the libraries. If
# DOCKER_PROTOC_VERSION is set, this will be the version. Otherwise, it will be
# computed from the gapic-generator-pom-parent artifact at the specified
# gapic_generator_version.
get_protoc_version() {
local gapic_generator_version=$1
local protoc_version
Expand All @@ -129,6 +142,16 @@ get_protoc_version() {
echo "${protoc_version}"
}

# Given the versions of the gapic generator, protoc and the protoc-grpc plugin,
# this function will download each one of the tools and create the environment
# variables "protoc_path" and "grpc_path" which are expected upstream. Note that
# if the specified versions of protoc and grpc match DOCKER_PROTOC_VERSION and
# DOCKER_GRPC_VERSION respectively, this function will instead set "protoc_path"
# and "grpc_path" to DOCKER_PROTOC_PATH and DOCKER_GRPC_PATH respectively (no
# download), since the docker image will have downloaded these tools beforehand.
# For the case of gapic-generator-java, no env var will be exported for the
# upstream flow, but instead it will be assigned a default filename that will be
# referenced by the file `library_generation/gapic-generator-java-wrapper`.
download_tools() {
local gapic_generator_version=$1
local protoc_version=$2
Expand All @@ -147,7 +170,15 @@ download_tools() {
export protoc_path=$(download_protoc "${protoc_version}" "${os_architecture}")
fi

download_grpc_plugin "${grpc_version}" "${os_architecture}"
# similar case with grpc
if [[ "${DOCKER_GRPC_VERSION}" == "${grpc_version}" ]]; then
# if the specified protoc_version matches the one baked in the docker
diegomarquezp marked this conversation as resolved.
Show resolved Hide resolved
# container, we just point grpc_path to its location.
export grpc_path="${DOCKER_GRPC_LOCATION}"
else
export grpc_path=$(download_grpc_plugin "${grpc_version}" "${os_architecture}")
fi

popd
}

Expand Down Expand Up @@ -198,13 +229,15 @@ download_protoc() {
download_grpc_plugin() {
local grpc_version=$1
local os_architecture=$2
if [ ! -f "protoc-gen-grpc-java-${grpc_version}-${os_architecture}.exe" ]; then
grpc_filename="protoc-gen-grpc-java-${grpc_version}-${os_architecture}.exe"
if [ ! -f "${grpc_filename}" ]; then
# download protoc-gen-grpc-java plugin from Google maven central mirror.
download_from \
"https://maven-central.storage-download.googleapis.com/maven2/io/grpc/protoc-gen-grpc-java/${grpc_version}/protoc-gen-grpc-java-${grpc_version}-${os_architecture}.exe" \
"protoc-gen-grpc-java-${grpc_version}-${os_architecture}.exe"
chmod +x "protoc-gen-grpc-java-${grpc_version}-${os_architecture}.exe"
"https://maven-central.storage-download.googleapis.com/maven2/io/grpc/protoc-gen-grpc-java/${grpc_version}/${grpc_filename}" \
"${grpc_filename}"
chmod +x "${grpc_filename}"
fi
echo "$(pwd)/${grpc_filename}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a debug log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's the return statement of the function. Many functions in utilities.sh are made this way. The way we use is variable=$(my_function "arg1" "arg2"). The only possible return values for return are integers from 0 to 255 (and consumed via $?) so it's not useful.

}

download_from() {
Expand Down Expand Up @@ -282,7 +315,7 @@ get_proto_path_from_preprocessed_sources() {
pushd "${sources}" > /dev/null
local proto_library=$(find . -maxdepth 1 -type d -name 'proto-*' | sed 's/\.\///')
local found_libraries=$(echo "${proto_library}" | wc -l)
if [ -z ${proto_library} ]; then
if [[ -z ${proto_library} ]]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that the shell unit tests were expecting the binary brackets because proto_library could be expanded to a string with spaces.

+ '[' -z proto-2 proto-1 ']'
/usr/local/google/home/diegomarquezp/google/sdk-platform-java/library_generation/test/../utils/utilities.sh: line 318: [: proto-2: binary operator expected

echo "no proto libraries found in the supplied sources path"
exit 1
elif [ ${found_libraries} -gt 1 ]; then
Expand Down
Loading