-
Notifications
You must be signed in to change notification settings - Fork 56
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 55 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 |
---|---|---|
|
@@ -28,6 +28,18 @@ get_grpc_version_failed_with_invalid_generator_version_test() { | |
assertEquals 1 $((res)) | ||
} | ||
|
||
get_protoc_version_succeed_docker_env_var_test() { | ||
local version_with_docker | ||
local version_without_docker | ||
export DOCKER_PROTOC_VERSION="9.9.9" | ||
version_with_docker=$(get_protoc_version "2.24.0") | ||
assertEquals "${DOCKER_PROTOC_VERSION}" "${version_with_docker}" | ||
unset DOCKER_PROTOC_VERSION | ||
version_without_docker=$(get_protoc_version "2.24.0") | ||
assertEquals "23.2" "${version_without_docker}" | ||
rm "gapic-generator-java-pom-parent-2.24.0.pom" | ||
} | ||
|
||
get_protoc_version_succeed_with_valid_generator_version_test() { | ||
local actual_version | ||
actual_version=$(get_protoc_version "2.24.0") | ||
|
@@ -134,6 +146,27 @@ 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. | ||
# 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. | ||
export DOCKER_PROTOC_LOCATION=$(mktemp -d) | ||
export DOCKER_PROTOC_VERSION="99.99" | ||
export output_folder=$(get_output_folder) | ||
mkdir -p "${DOCKER_PROTOC_LOCATION}/protoc-99.99/bin" | ||
download_protoc "99.99" "linux-x86_64" | ||
assertFileOrDirectoryExists "${output_folder}/protoc-99.99" | ||
rm -rf "${output_folder}/protoc-99.99" | ||
unset DOCKER_PROTOC_LOCATION | ||
unset DOCKER_PROTOC_VERSION | ||
rm -rdf $(get_output_folder) | ||
} | ||
|
||
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" | ||
|
@@ -256,6 +289,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_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 | ||
get_gapic_opts_with_rest_test | ||
|
@@ -268,6 +302,7 @@ test_list=( | |
download_protoc_succeed_with_valid_version_macos_test | ||
download_protoc_failed_with_invalid_version_linux_test | ||
download_protoc_failed_with_invalid_arch_test | ||
download_protoc_succeed_with_baked_protoc | ||
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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,197 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
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. Since this is a test resource, could you remove all unused part? 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. My bad, it slipped into my commits! 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. Removed |
||
<project xmlns="http://maven.apache.org/POM/4.0.0" | ||
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd" child.project.url.inherit.append.path="false"> | ||
<modelVersion>4.0.0</modelVersion> | ||
<groupId>com.google.api</groupId> | ||
<artifactId>gapic-generator-java-pom-parent</artifactId> | ||
<version>2.24.0</version><!-- {x-version-update:gapic-generator-java:current} --> | ||
<packaging>pom</packaging> | ||
<name>GAPIC Generator Java POM Parent</name> | ||
<url>https://github.com/googleapis/sdk-platform-java</url> | ||
<description> | ||
The top-level parent for all modules in the repository. | ||
</description> | ||
<parent> | ||
<groupId>com.google.cloud</groupId> | ||
<artifactId>google-cloud-shared-config</artifactId> | ||
<version>1.5.7</version> | ||
<relativePath/> | ||
</parent> | ||
|
||
<properties> | ||
<skipUnitTests>false</skipUnitTests> | ||
<checkstyle.header.file>java.header</checkstyle.header.file> | ||
|
||
<!-- External dependencies, especially gRPC and Protobuf version, should be | ||
consistent across modules in this repository --> | ||
<javax.annotation-api.version>1.3.2</javax.annotation-api.version> | ||
<auto-value.version>1.10.2</auto-value.version> | ||
<grpc.version>1.56.1</grpc.version> | ||
<google.auth.version>1.19.0</google.auth.version> | ||
<google.http-client.version>1.43.3</google.http-client.version> | ||
<gson.version>2.10.1</gson.version> | ||
<guava.version>32.1.2-jre</guava.version> | ||
<!-- On next protobuf upgrade (to 3.23.3 or higher), | ||
remove temporarily j2objc-annotations exclusions from protobuf-java-util dependencies. | ||
For context: https://github.com/googleapis/sdk-platform-java/pull/1791--> | ||
<protobuf.version>3.23.2</protobuf.version> | ||
<maven.compiler.release>8</maven.compiler.release> | ||
</properties> | ||
|
||
<developers> | ||
<developer> | ||
<id>suztomo</id> | ||
<name>Tomo Suzuki</name> | ||
<email>[email protected]</email> | ||
<organization>Google</organization> | ||
<roles> | ||
<role>Developer</role> | ||
</roles> | ||
</developer> | ||
</developers> | ||
<organization> | ||
<name>Google LLC</name> | ||
</organization> | ||
<scm child.scm.connection.inherit.append.path="false" child.scm.developerConnection.inherit.append.path="false" | ||
child.scm.url.inherit.append.path="false"> | ||
<connection>scm:git:[email protected]:googleapis/sdk-platform-java.git</connection> | ||
<developerConnection>scm:git:[email protected]:googleapis/sdk-platform-java.git</developerConnection> | ||
<url>https://github.com/googleapis/sdk-platform-java</url> | ||
<tag>HEAD</tag> | ||
</scm> | ||
<issueManagement> | ||
<url>https://github.com/googleapis/sdk-platform-java/issues</url> | ||
<system>GitHub Issues</system> | ||
</issueManagement> | ||
|
||
<licenses> | ||
<license> | ||
<name>Apache-2.0</name> | ||
<url>https://www.apache.org/licenses/LICENSE-2.0.txt</url> | ||
</license> | ||
</licenses> | ||
|
||
<profiles> | ||
<profile> | ||
<!-- Only run checkstyle plugin on Java 11+ (checkstyle artifact only supports Java 11+) --> | ||
<id>checkstyle-tests</id> | ||
<activation> | ||
<jdk>[11,)</jdk> | ||
</activation> | ||
<build> | ||
<plugins> | ||
<plugin> | ||
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-checkstyle-plugin</artifactId> | ||
<executions> | ||
<execution> | ||
<id>checkstyle</id> | ||
<phase>validate</phase> | ||
<goals> | ||
<goal>check</goal> | ||
</goals> | ||
<configuration> | ||
<headerLocation>${checkstyle.header.file}</headerLocation> | ||
</configuration> | ||
</execution> | ||
</executions> | ||
</plugin> | ||
</plugins> | ||
</build> | ||
</profile> | ||
|
||
<profile> | ||
<id>test-coverage</id> | ||
<activation> | ||
<property> | ||
<name>enableShowcaseTestCoverage</name> | ||
</property> | ||
</activation> | ||
<build> | ||
<pluginManagement> | ||
<plugins> | ||
<plugin> | ||
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-surefire-plugin</artifactId> | ||
<version>3.0.0-M8</version> | ||
<configuration> | ||
<!-- Excludes integration tests and smoke tests when unit tests are run --> | ||
<excludes> | ||
<exclude>**/*SmokeTest.java</exclude> | ||
<exclude>**/IT*.java</exclude> | ||
</excludes> | ||
<reportNameSuffix>sponge_log</reportNameSuffix> | ||
<argLine>${surefire.jacoco.args}</argLine> | ||
<skipTests>${skipUnitTests}</skipTests> | ||
</configuration> | ||
</plugin> | ||
|
||
<plugin> | ||
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-failsafe-plugin</artifactId> | ||
<version>3.0.0</version> | ||
<executions> | ||
<execution> | ||
<goals> | ||
<goal>integration-test</goal> | ||
<goal>verify</goal> | ||
</goals> | ||
</execution> | ||
</executions> | ||
<configuration> | ||
<forkedProcessTimeoutInSeconds>3600</forkedProcessTimeoutInSeconds> | ||
<reportNameSuffix>sponge_log</reportNameSuffix> | ||
<includes> | ||
<include>**/IT*.java</include> | ||
<include>**/*SmokeTest.java</include> | ||
</includes> | ||
<argLine>${failsafe.jacoco.args}</argLine> | ||
</configuration> | ||
</plugin> | ||
</plugins> | ||
</pluginManagement> | ||
<plugins> | ||
<plugin> | ||
<groupId>org.jacoco</groupId> | ||
<artifactId>jacoco-maven-plugin</artifactId> | ||
<version>0.8.8</version> | ||
<executions> | ||
<execution> | ||
<id>unit-test-execution</id> | ||
<goals> | ||
<goal>prepare-agent</goal> | ||
</goals> | ||
<configuration> | ||
<propertyName>surefire.jacoco.args</propertyName> | ||
</configuration> | ||
</execution> | ||
<execution> | ||
<id>integration-test-execution</id> | ||
<phase>pre-integration-test</phase> | ||
<goals> | ||
<goal>prepare-agent</goal> | ||
</goals> | ||
<configuration> | ||
<propertyName>failsafe.jacoco.args</propertyName> | ||
</configuration> | ||
</execution> | ||
</executions> | ||
</plugin> | ||
</plugins> | ||
</build> | ||
</profile> | ||
</profiles> | ||
<repositories> | ||
<repository> | ||
<id>google-maven-central-copy</id> | ||
<name>Google Maven Central copy</name> | ||
<url>https://maven-central.storage-download.googleapis.com/maven2</url> | ||
</repository> | ||
<repository> | ||
<id>maven-central</id> | ||
<name>Maven Central</name> | ||
<url>https://repo1.maven.org/maven2</url> | ||
</repository> | ||
</repositories> | ||
</project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -111,6 +111,10 @@ get_grpc_version() { | |
get_protoc_version() { | ||
local gapic_generator_version=$1 | ||
local protoc_version | ||
if [[ -n "${DOCKER_PROTOC_VERSION}" ]]; then | ||
>&2 echo "Using protoc version baked into the container: ${DOCKER_PROTOC_VERSION}" | ||
echo "${DOCKER_PROTOC_VERSION}" | ||
fi | ||
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 guess we can return this function early? 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.
Removed. |
||
pushd "${output_folder}" > /dev/null | ||
# get protobuf version from gapic-generator-java-pom-parent/pom.xml | ||
download_gapic_generator_pom_parent "${gapic_generator_version}" | ||
|
@@ -157,7 +161,22 @@ 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_dirname="protoc-${protoc_version}" | ||
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 add a comment specifying the the protoc version priority? |
||
protoc_path="${output_folder}/${protoc_dirname}/bin" | ||
if [[ -f "${protoc_path}/protoc" ]]; then | ||
return | ||
fi | ||
|
||
if [[ "${DOCKER_PROTOC_VERSION}" == "${protoc_version}" ]]; then | ||
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 protoc version priority:
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. Added this logic to |
||
# if the specified protoc_version matches the one baked in the docker | ||
# container, we just copy it into the output folder | ||
mkdir -p "${output_folder}/${protoc_dirname}" | ||
cp -r "${DOCKER_PROTOC_LOCATION}/${protoc_dirname}" \ | ||
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. Can we use the protoc 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. Yes. We now explicitly |
||
"${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 +188,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,18 @@ | |||||||||||||||||
"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>.+?)[\"']?\\s+" | ||||||||||||||||||
], | ||||||||||||||||||
"datasourceTemplate": "github-releases", | ||||||||||||||||||
"depNameTemplate": "protocolbuffers/protobuf", | ||||||||||||||||||
"extractVersionTemplate": "^v(?<version>.*)$" | ||||||||||||||||||
}, | ||||||||||||||||||
{ | ||||||||||||||||||
"customType": "regex", | ||||||||||||||||||
"fileMatch": [ | ||||||||||||||||||
|
@@ -141,7 +153,8 @@ | |||||||||||||||||
}, | ||||||||||||||||||
{ | ||||||||||||||||||
"matchPackagePatterns": [ | ||||||||||||||||||
"^com.google.protobuf" | ||||||||||||||||||
"^com.google.protobuf", | ||||||||||||||||||
"^protocolbuffers/protobuf" | ||||||||||||||||||
], | ||||||||||||||||||
"groupName": "Protobuf dependencies", | ||||||||||||||||||
"enabled": false | ||||||||||||||||||
|
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