-
Notifications
You must be signed in to change notification settings - Fork 52
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 protoc into the hermetic build docker image #2707
Changes from 46 commits
0b65f15
a6bb064
d4f27e2
19a0b04
bc693b5
f335cbb
aab50ef
f97cdca
d68c14a
eca0d92
6aa8533
53c5dea
cc4309b
7a4b6b8
4838d5b
1b01713
c149461
4ee1692
e77bacc
9120c94
3d18426
427c564
7b79763
240742a
8498f2c
f893e5c
2faf45d
7ce9548
8474452
b58e990
00cb2a7
b27a57e
6c6432a
eca8c62
0041c6b
7ec0f4c
4399bfe
ef105e7
b1d3fc3
1df1aa1
054458d
00d9ecc
1a5faec
9e25542
5a7d525
3cde6f5
acf26fa
1ef4006
358be32
17de6db
0fd0d21
e3d6e50
315feb9
291f911
5fe0a97
eb48f45
706d095
868a1bf
5d9beb5
6d76843
8a57737
550f162
340622c
5e71efa
c4b7f72
df0d23c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,24 +15,36 @@ | |
# build from the root of this repo: | ||
FROM gcr.io/cloud-devrel-public-resources/python | ||
|
||
SHELL [ "/bin/bash", "-c" ] | ||
|
||
ARG SYNTHTOOL_COMMITTISH=63cc541da2c45fcfca2136c43e638da1fbae174d | ||
ARG OWLBOT_CLI_COMMITTISH=ac84fa5c423a0069bbce3d2d869c9730c8fdf550 | ||
ARG PROTOC_VERSION=25.3 | ||
ENV HOME=/home | ||
|
||
# install OS tools | ||
RUN apt-get update && apt-get install -y \ | ||
unzip openjdk-17-jdk rsync maven jq \ | ||
&& apt-get clean | ||
|
||
# copy source code | ||
COPY library_generation /src | ||
|
||
# install protobuf | ||
diegomarquezp marked this conversation as resolved.
Show resolved
Hide resolved
|
||
WORKDIR /protoc | ||
RUN ls /src | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this just for troubleshooting purposes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed. I removed it |
||
RUN source /src/utils/utilities.sh \ | ||
&& download_protoc "${PROTOC_VERSION}" "linux-x86_64" | ||
# we indicate protoc is available in the container via env vars | ||
ENV DOCKER_PROTOC_LOCATION=/protoc | ||
ENV DOCKER_PROTOC_VERSION="${PROTOC_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 | ||
RUN ln -s $(which python3.11) /usr/local/bin/python3 | ||
RUN python -m pip install --upgrade pip | ||
|
||
# copy source code | ||
COPY library_generation /src | ||
|
||
# install scripts as a python package | ||
WORKDIR /src | ||
RUN python -m pip install -r requirements.txt | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -134,6 +134,18 @@ download_protoc_failed_with_invalid_arch_test() { | |||||||||||||||||
assertEquals 1 $((res)) | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
download_protoc_succeed_with_baked_protoc() { | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to declare this method at the end of the file in order this method to run. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added it and fixed the corresponding unit test |
||||||||||||||||||
# this mimics a docker container scenario. | ||||||||||||||||||
# if the specified version matches the docker env var, then it will be just | ||||||||||||||||||
# copied from the docker protoc location (also specified in an env var). | ||||||||||||||||||
export DOCKER_PROTOC_LOCATION=$(mktmp -d) | ||||||||||||||||||
mkdir "${DOCKER_PROTOC_LOCATION}/protoc-99.99" | ||||||||||||||||||
export DOCKER_PROTOC_VERSION="99.99" | ||||||||||||||||||
download_protoc "99.99" "linux-x86_64" | ||||||||||||||||||
assertFileOrDirectoryExists "protoc-99.99" | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you explain more why a non-existent version, 99.99, of protoc exists? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I expanded the explanation in the test: sdk-platform-java/library_generation/test/generate_library_unit_tests.sh Lines 138 to 145 in 358be32
|
||||||||||||||||||
rm -rf "protoc-99.99" | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
download_grpc_plugin_succeed_with_valid_version_linux_test() { | ||||||||||||||||||
download_grpc_plugin "1.55.1" "linux-x86_64" | ||||||||||||||||||
assertFileOrDirectoryExists "protoc-gen-grpc-java-1.55.1-linux-x86_64.exe" | ||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -157,7 +157,20 @@ download_generator_artifact() { | |
download_protoc() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general, I feel like we may not need any changes in
But I think this requires us to reuse the proto location specified in Dockerfile. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left |
||
local protoc_version=$1 | ||
local os_architecture=$2 | ||
if [ ! -d "protoc-${protoc_version}" ]; then | ||
|
||
protoc_path="${output_folder}/protoc-${protoc_version}/bin" | ||
if [[ -f "${protoc_path}/protoc" ]]; then | ||
return | ||
fi | ||
|
||
if [[ -n "${DOCKER_PROTOC_VERSION}" ]] \ | ||
&& [[ "${DOCKER_PROTOC_VERSION}" == "${protoc_version}" ]]; then | ||
# if the specified protoc_version matches the one baked in the docker | ||
# container, we just copy it into the output folder | ||
cp -r "${DOCKER_PROTOC_LOCATION}"/* "${output_folder}" | ||
fi | ||
|
||
if [ ! -d "${protoc_path}" ]; then | ||
# pull proto files and protoc from protobuf repository as maven central | ||
# doesn't have proto files | ||
download_from \ | ||
|
@@ -169,7 +182,6 @@ download_protoc() { | |
rm "protoc-${protoc_version}.zip" | ||
fi | ||
|
||
protoc_path="${output_folder}/protoc-${protoc_version}/bin" | ||
} | ||
|
||
download_grpc_plugin() { | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -28,6 +28,17 @@ | |||||||||||||||||
"depNameTemplate": "com.google.protobuf:protobuf-java", | ||||||||||||||||||
"datasourceTemplate": "maven" | ||||||||||||||||||
}, | ||||||||||||||||||
{ | ||||||||||||||||||
"customType": "regex", | ||||||||||||||||||
"fileMatch": [ | ||||||||||||||||||
"^\\.cloudbuild/library_generation/library_generation\\.Dockerfile$" | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you verify this setting will work? Do we need to specify There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @JoeWang1127 I verified it worked locally. I added the results to the PR description. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually we probably don't want to let renovate bot to automatically update protoc. Not before we can update the whole repo(java-comon-protos, java-iam, test files in gapic-generator-java) along side the protoc update. Because the new protoc version may not work with the current protos or the newly generated code may not work with the generator. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. Both sdk-platform-java/renovate.json Lines 154 to 161 in eb48f45
I guess we can keep these regexes and enable them afterwards? |
||||||||||||||||||
], | ||||||||||||||||||
"matchStrings": [ | ||||||||||||||||||
"ARG PROTOC_VERSION==(?<currentValue>.+?)\\n" | ||||||||||||||||||
], | ||||||||||||||||||
"depNameTemplate": "com.google.protobuf:protobuf-java", | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The version of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the catch. I corrected it to use the github releases |
||||||||||||||||||
"datasourceTemplate": "maven" | ||||||||||||||||||
}, | ||||||||||||||||||
{ | ||||||||||||||||||
"customType": "regex", | ||||||||||||||||||
"fileMatch": [ | ||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything requires us to change the default shell to bash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
nvm
installation script assumes the environment isbash
based. Usingsh
makes the following line to have no effects.sdk-platform-java/.cloudbuild/library_generation/library_generation.Dockerfile
Line 63 in 5e71efa