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 protoc into the hermetic build docker image #2707

Merged
merged 66 commits into from
May 23, 2024
Merged
Show file tree
Hide file tree
Changes from 46 commits
Commits
Show all changes
66 commits
Select commit Hold shift + click to select a range
0b65f15
chore: bake synthtool into library_generation docker image
diegomarquezp Apr 2, 2024
a6bb064
remove synthtool usage
diegomarquezp Apr 2, 2024
d4f27e2
do not use config file
diegomarquezp Apr 2, 2024
19a0b04
bake synthtool in dockerfile
diegomarquezp Apr 2, 2024
bc693b5
bake copy-code into the image
diegomarquezp Apr 2, 2024
f335cbb
remove unnecessary SHELL statement
diegomarquezp Apr 2, 2024
aab50ef
add info for installing synthtool
diegomarquezp Apr 2, 2024
f97cdca
modify owl-bot usage in postprocess_library
diegomarquezp Apr 2, 2024
d68c14a
Merge remote-tracking branch 'origin/main' into transfer-synthtool-java
diegomarquezp Apr 10, 2024
eca0d92
fix synthtool installation
diegomarquezp Apr 10, 2024
6aa8533
change permissions to script folder
diegomarquezp Apr 10, 2024
53c5dea
assume location of generation_config.yaml
diegomarquezp Apr 10, 2024
cc4309b
fix permissions
diegomarquezp Apr 10, 2024
7a4b6b8
fix path to config yaml
diegomarquezp Apr 10, 2024
4838d5b
use entrypoint for docker image
diegomarquezp Apr 11, 2024
1b01713
format
diegomarquezp Apr 11, 2024
c149461
finish development guide
diegomarquezp Apr 11, 2024
4ee1692
remove olwbot_cli usage
diegomarquezp Apr 11, 2024
e77bacc
add entrypoint for docker image
diegomarquezp Apr 17, 2024
9120c94
manually set HOME var in owlbot
diegomarquezp Apr 18, 2024
3d18426
Merge remote-tracking branch 'origin/main' into transfer-synthtool-java
diegomarquezp Apr 18, 2024
427c564
post-merge cleanup
diegomarquezp Apr 18, 2024
7b79763
fix parameters
diegomarquezp Apr 18, 2024
240742a
fix synthtool sha
diegomarquezp Apr 18, 2024
8498f2c
fix entrypoint
diegomarquezp Apr 19, 2024
f893e5c
Merge remote-tracking branch 'origin/main' into transfer-synthtool-java
diegomarquezp Apr 19, 2024
2faf45d
lint
diegomarquezp Apr 19, 2024
7ce9548
remove unused entrypoint file
diegomarquezp Apr 19, 2024
8474452
remove owlbot CLI from readmne
diegomarquezp Apr 19, 2024
b58e990
restore xtrace
diegomarquezp Apr 19, 2024
00cb2a7
correct comment
diegomarquezp Apr 19, 2024
b27a57e
remove test helper
diegomarquezp Apr 19, 2024
6c6432a
checkout owlbot cli committish
diegomarquezp Apr 19, 2024
eca8c62
remove owlbot config
diegomarquezp Apr 19, 2024
0041c6b
fix golden file
diegomarquezp Apr 19, 2024
7ec0f4c
Update library_generation/DEVELOPMENT.md
diegomarquezp Apr 29, 2024
4399bfe
docker instructions, pwd blurb, unit tests instructions
diegomarquezp Apr 29, 2024
ef105e7
Merge remote-tracking branch 'origin/main' into transfer-synthtool-java
diegomarquezp Apr 29, 2024
b1d3fc3
Merge remote-tracking branch 'origin/transfer-synthtool-java' into tr…
diegomarquezp Apr 29, 2024
1df1aa1
restore generate repo
diegomarquezp Apr 30, 2024
054458d
chore: bake protoc into the library_generation docker image
diegomarquezp Apr 30, 2024
00d9ecc
fix baked protoc transfer
diegomarquezp Apr 30, 2024
1a5faec
add unit test for baked protoc
diegomarquezp Apr 30, 2024
9e25542
Merge remote-tracking branch 'origin/main' into bake-protoc-hermetic-…
diegomarquezp May 1, 2024
5a7d525
restore change from main
diegomarquezp May 1, 2024
3cde6f5
add renovate config for protoc version
diegomarquezp May 1, 2024
acf26fa
add to unit tests list
diegomarquezp May 1, 2024
1ef4006
expand explanation of the test
diegomarquezp May 1, 2024
358be32
fix renovate config
diegomarquezp May 12, 2024
17de6db
fix unit tests
diegomarquezp May 12, 2024
0fd0d21
Merge branch 'main' into bake-protoc-hermetic-build
diegomarquezp May 12, 2024
e3d6e50
remove debug tracing
diegomarquezp May 13, 2024
315feb9
disable PROTOC in renovate
diegomarquezp May 13, 2024
291f911
default to protoc version in docker image if not found
diegomarquezp May 14, 2024
5fe0a97
add message when using baked protoc version
diegomarquezp May 14, 2024
eb48f45
Update .cloudbuild/library_generation/library_generation.Dockerfile
diegomarquezp May 16, 2024
706d095
remove debug dockerfile directive
diegomarquezp May 21, 2024
868a1bf
move docker embedded protoc logic outside of download_protoc
diegomarquezp May 22, 2024
5d9beb5
Merge remote-tracking branch 'origin/main' into bake-protoc-hermetic-…
diegomarquezp May 22, 2024
6d76843
return from function early
diegomarquezp May 22, 2024
8a57737
remove test file from commit history
diegomarquezp May 22, 2024
550f162
fix unit tests
diegomarquezp May 22, 2024
340622c
fix python units
diegomarquezp May 22, 2024
5e71efa
fix IT
diegomarquezp May 22, 2024
c4b7f72
lint
diegomarquezp May 23, 2024
df0d23c
Merge branch 'main' into bake-protoc-hermetic-build
diegomarquezp May 23, 2024
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
18 changes: 15 additions & 3 deletions .cloudbuild/library_generation/library_generation.Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -15,24 +15,36 @@
# build from the root of this repo:
FROM gcr.io/cloud-devrel-public-resources/python

SHELL [ "/bin/bash", "-c" ]
Copy link
Collaborator

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?

Copy link
Contributor Author

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 is bash based. Using sh makes the following line to have no effects.

RUN curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.35.3/install.sh | bash


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
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 just for troubleshooting purposes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Down
12 changes: 12 additions & 0 deletions library_generation/test/generate_library_unit_tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,18 @@ download_protoc_failed_with_invalid_arch_test() {
assertEquals 1 $((res))
}

download_protoc_succeed_with_baked_protoc() {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

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 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"
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

@diegomarquezp diegomarquezp May 12, 2024

Choose a reason for hiding this comment

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

I expanded the explanation in the test:

# This mimics a docker container scenario.
# This test consists of creating an empty /tmp/.../protoc-99.99/bin folder and map
# it to the DOCKER_PROTOC_LOCATION env var (which is treated specially in the
# `download_protoc` function). If `DOCKER_PROTOC_VERSION` matches exactly as
# the version passed to `download_protoc`, then we will not download protoc
# but simply copy it from DOCKER_PROTOC_LOCATION into ${output_folder} (which
# we manually created in this test), so we should expect ${output_folder} to
# contain a folder called protoc-99.99.

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"
Expand Down
16 changes: 14 additions & 2 deletions library_generation/utils/utilities.sh
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,20 @@ download_generator_artifact() {
download_protoc() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In general, I feel like we may not need any changes in download_protoc. Ideally this function can stay as it is and we can add logics before calling it, for example

if [[ "${protoc_version}" != "${DOCKER_PROTOC_VERSION}" ]]; then
  download_protoc "${protoc_version}" "${os_architecture}"
fi

But I think this requires us to reuse the proto location specified in Dockerfile.

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 left download_protoc almost untouched and left the DOCKER_PROTOC_VERSION handling in download_tools (one level up in the call hierarchy).

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 \
Expand All @@ -169,7 +182,6 @@ download_protoc() {
rm "protoc-${protoc_version}.zip"
fi

protoc_path="${output_folder}/protoc-${protoc_version}/bin"
}

download_grpc_plugin() {
Expand Down
11 changes: 11 additions & 0 deletions renovate.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,17 @@
"depNameTemplate": "com.google.protobuf:protobuf-java",
"datasourceTemplate": "maven"
},
{
"customType": "regex",
"fileMatch": [
"^\\.cloudbuild/library_generation/library_generation\\.Dockerfile$"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you verify this setting will work?

Do we need to specify includePaths?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Both protobuf-java and protoc are disabled:

{
"matchPackagePatterns": [
"^com.google.protobuf",
"^protocolbuffers/protobuf"
],
"groupName": "Protobuf dependencies",
"enabled": false
},

I guess we can keep these regexes and enable them afterwards?

],
"matchStrings": [
"ARG PROTOC_VERSION==(?<currentValue>.+?)\\n"
],
"depNameTemplate": "com.google.protobuf:protobuf-java",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The version of com.google.protobuf:protobuf-java is not the same as protoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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": [
Expand Down
Loading