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

feat: bake gapic-generator-java into the hermetic build docker image #3067

Merged
merged 72 commits into from
Sep 3, 2024

Conversation

diegomarquezp
Copy link
Contributor

@diegomarquezp diegomarquezp commented Jul 24, 2024

Makes the gapic-generator-java jar a prepared binary in the Docker image whose location is now assumed by the scripts. Developers need now to prepare this well-know location (specified in library_generation/DEVELOPMENT.md.

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Jul 24, 2024

RUN mvn install -DskipTests -Dclirr.skip -Dcheckstyle.skip
RUN ls "/root/.m2/repository/com/google/api/gapic-generator-java/"
RUN cp "/root/.m2/repository/com/google/api/gapic-generator-java/${DOCKER_GAPIC_GENERATOR_VERSION}/gapic-generator-java-${DOCKER_GAPIC_GENERATOR_VERSION}.jar" .
Copy link
Contributor Author

@diegomarquezp diegomarquezp Jul 31, 2024

Choose a reason for hiding this comment

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

Only copying the jar (as opposed to copying the entire .m2 folder) allows us for a more precise test. We can use the env vars DOCKER_GAPIC_GENERATOR_LOCATION and DOCKER_GAPIC_GENERATOR_VERSION to confirm if the preconfigured generator was used

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this jar have all the info we need? Can you test running the image without internet? Sometimes the dependencies of the jar are still downloaded.

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 ran docker run --network none and could get past generate_library.sh (i.e. no jars needed), but I found two things we may want to follow up with:

This function from the transferred synthtool fetches data from Maven:

def latest_maven_version(group_id: str, artifact_id: str) -> Optional[str]:

Our script pulls googleapis' proto definitions:

if not (
os.path.exists(f"{output_folder}/google")
and os.path.exists(f"{output_folder}/grafeas")
):
print("downloading googleapis")
sh_util(
f"download_googleapis_files_and_folders {output_folder} {googleapis_commitish}"
)

I thought there was a commit in synthtool that fetches libraries_bom from our config yaml?

metadata["latest_version"] = latest_maven_version(
group_id=group_id, artifact_id=artifact_id
)
metadata["latest_bom_version"] = latest_maven_version(
group_id="com.google.cloud",
artifact_id="libraries-bom",
)

For the googleapis download, I guess it's expected behavior

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 did a temporary bypass (not committed) to see if the generation is successful. It is except for the mvn fmt:format part that needs to fetch data when processing a library. The data includes fetching the parent pom of the target library. Nothing about sdk-platform is fetched.

https://paste.googleplex.com/5092731556986880?raw

Copy link
Collaborator

@blakeli0 blakeli0 Aug 7, 2024

Choose a reason for hiding this comment

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

Sounds good! Yes we should definitely follow up but I don't think they should block this PR, since we already have all the jars in the docker image.

@diegomarquezp diegomarquezp marked this pull request as ready for review July 31, 2024 21:44
@diegomarquezp diegomarquezp requested a review from blakeli0 July 31, 2024 21:44

RUN mvn install -DskipTests -Dclirr.skip -Dcheckstyle.skip
RUN ls "/root/.m2/repository/com/google/api/gapic-generator-java/"
RUN cp "/root/.m2/repository/com/google/api/gapic-generator-java/${DOCKER_GAPIC_GENERATOR_VERSION}/gapic-generator-java-${DOCKER_GAPIC_GENERATOR_VERSION}.jar" .
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this jar have all the info we need? Can you test running the image without internet? Sometimes the dependencies of the jar are still downloaded.

# container, we copy the generator jar into the output folder so it can be
# picked up by gapic-generator-java-wrapper
>&2 echo "Using gapic-generator-java version baked into the container: ${DOCKER_GAPIC_GENERATOR_VERSION}"
cp "${DOCKER_GAPIC_GENERATOR_LOCATION}" "${output_folder}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this copy logic might already be covered here?

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 decided to not follow the .m2 folder structure in favor of easier testing (https://github.com/googleapis/sdk-platform-java/pull/3067/files#r1698944411). I think it's ok to treat this baked-in jar as a special case outside of .m2?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. Yes it's definitely OK to put it into a different location, as long as we don't use maven to retrieve it. Let's see if we can get rid of gapic_generator_version so that we don't need this whole block.

library_generation/model/generation_config.py Outdated Show resolved Hide resolved
library_generation/utils/utilities.sh Show resolved Hide resolved
@diegomarquezp diegomarquezp requested a review from blakeli0 August 7, 2024 19:38
@JoeWang1127
Copy link
Collaborator

@JoeWang1127 I modified the integration test to lock-in a specific generator jar downloaded via mvn. PTAL when you have a chance :)

Thanks, could you change the pr body since this is no longer a pending issue.

this jar is expected to be there. Soon enough, more binaries such as the gRPC
and protoc plugins will also be stored here. Developers must make sure
this folder is properly configured before running the scripts locally.
Note that this relies on the `HOME` en var which is always
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: one sentence per line.

Copy link
Contributor Author

@diegomarquezp diegomarquezp Aug 31, 2024

Choose a reason for hiding this comment

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

I found this article that confirms this is a good practice. I reformatted the file.

# Note that the destination is a well-known location that will be assumed at runtime
# We hard-code the location string to avoid making it configurable (via ARG) as
# well as to avoid it making it overridable at runtime (via ENV).
COPY --from=ggj-build "/sdk-platform-java/gapic-generator-java.jar" "${HOME}/.library_generation/gapic-generator-java.jar"
Copy link
Collaborator

Choose a reason for hiding this comment

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

A future improvement: only include necessary files in the final image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we want to get rid of the source files (owlbot cli, sdk-platform, etc) by building in this stage then only copying the binaries. This is part of the image cleanup task.

Copy link

sonarqubecloud bot commented Sep 3, 2024

@@ -247,6 +294,8 @@ def __run_entry_point_in_docker_container(
f"{repo_location}:/workspace/repo",
"-v",
f"{config_location}:/workspace/config",
"-v",
f"{config_dir}/{WELL_KNOWN_GENERATOR_JAR_FILENAME}:/home/.library_generation/{WELL_KNOWN_GENERATOR_JAR_FILENAME}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this would override the SNAPSHOT jar built in the previous step docker build?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly. This is just to ensure that the outcome of the test doesn't depend on changes to the generator code.

Copy link

sonarqubecloud bot commented Sep 3, 2024

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@diegomarquezp diegomarquezp merged commit a372e82 into main Sep 3, 2024
46 of 47 checks passed
@diegomarquezp diegomarquezp deleted the bake-ggj-docker-image-2 branch September 3, 2024 23:13
ldetmer added a commit that referenced this pull request Sep 9, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>2.45.0</summary>

##
[2.45.0](v2.44.0...v2.45.0)
(2024-09-09)


### Features

* add Batcher#close(timeout) and Batcher#cancelOutstanding
([#3141](#3141))
([b5a92e4](b5a92e4))
* add full RetrySettings sample code to Settings classes
([#3056](#3056))
([8fe3a2d](8fe3a2d))
* add toString to futures returned by operations
([#3140](#3140))
([afecb8c](afecb8c))
* bake gapic-generator-java into the hermetic build docker image
([#3067](#3067))
([a372e82](a372e82))


### Bug Fixes

* **gax:** prevent truncation/overflow when converting time values
([#3095](#3095))
([699074e](699074e))


### Dependencies

* add opentelemetry exporter-metrics and shared-resoucemapping to shared
dependencies
([#3078](#3078))
([fc8d80d](fc8d80d))
* update dependency certifi to v2024.8.30
([#3150](#3150))
([c18b705](c18b705))
* update dependency com.google.api-client:google-api-client-bom to
v2.7.0
([#3151](#3151))
([5f43e43](5f43e43))
* update dependency com.google.errorprone:error_prone_annotations to
v2.31.0
([#3153](#3153))
([3071509](3071509))
* update dependency com.google.errorprone:error_prone_annotations to
v2.31.0
([#3154](#3154))
([335ee63](335ee63))
* update dependency com.google.guava:guava to v33.3.0-jre
([#3119](#3119))
([41174b0](41174b0))
* update dependency dev.cel:cel to v0.7.1
([#3155](#3155))
([b1ddd16](b1ddd16))
* update dependency filelock to v3.16.0
([#3175](#3175))
([6681113](6681113))
* update dependency idna to v3.8
([#3156](#3156))
([82f5326](82f5326))
* update dependency io.netty:netty-tcnative-boringssl-static to
v2.0.66.final
([#3148](#3148))
([a7efaa8](a7efaa8))
* update dependency net.bytebuddy:byte-buddy to v1.15.1
([#3115](#3115))
([0e06c5f](0e06c5f))
* update dependency org.apache.commons:commons-lang3 to v3.17.0
([#3157](#3157))
([8d3b9fd](8d3b9fd))
* update dependency org.checkerframework:checker-qual to v3.47.0
([#3166](#3166))
([365674d](365674d))
* update dependency org.yaml:snakeyaml to v2.3
([#3158](#3158))
([e67ea9a](e67ea9a))
* update dependency platformdirs to v4.3.2
([#3176](#3176))
([4f2f9e0](4f2f9e0))
* update dependency virtualenv to v20.26.4
([#3177](#3177))
([080e078](080e078))
* update google api dependencies
([#3118](#3118))
([67342ea](67342ea))
* update google auth library dependencies to v1.25.0
([#3168](#3168))
([715884a](715884a))
* update google http client dependencies to v1.45.0
([#3159](#3159))
([a3fe612](a3fe612))
* update googleapis/java-cloud-bom digest to 6626f91
([#3147](#3147))
([658e40e](658e40e))
* update junit5 monorepo to v5.11.0
([#3111](#3111))
([6bf84c8](6bf84c8))
* update netty dependencies to v4.1.113.final
([#3165](#3165))
([9b5957d](9b5957d))
* update opentelemetry-java monorepo to v1.42.0
([#3172](#3172))
([413c44e](413c44e))


### Documentation

* Update DEVELOPMENT.md
([#3126](#3126))
([92bdf4e](92bdf4e))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: ldetmer <1771267+ldetmer@users.noreply.github.com>
ldetmer pushed a commit that referenced this pull request Sep 17, 2024
…3067)

Makes the gapic-generator-java jar a prepared binary in the Docker image
whose location is now assumed by the scripts. Developers need now to
prepare this well-know location (specified in
`library_generation/DEVELOPMENT.md`.

---------

Co-authored-by: Blake Li <blakeli@google.com>
Co-authored-by: Joe Wang <106995533+JoeWang1127@users.noreply.github.com>
ldetmer added a commit that referenced this pull request Sep 17, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>2.45.0</summary>

##
[2.45.0](v2.44.0...v2.45.0)
(2024-09-09)


### Features

* add Batcher#close(timeout) and Batcher#cancelOutstanding
([#3141](#3141))
([b5a92e4](b5a92e4))
* add full RetrySettings sample code to Settings classes
([#3056](#3056))
([8fe3a2d](8fe3a2d))
* add toString to futures returned by operations
([#3140](#3140))
([afecb8c](afecb8c))
* bake gapic-generator-java into the hermetic build docker image
([#3067](#3067))
([a372e82](a372e82))


### Bug Fixes

* **gax:** prevent truncation/overflow when converting time values
([#3095](#3095))
([699074e](699074e))


### Dependencies

* add opentelemetry exporter-metrics and shared-resoucemapping to shared
dependencies
([#3078](#3078))
([fc8d80d](fc8d80d))
* update dependency certifi to v2024.8.30
([#3150](#3150))
([c18b705](c18b705))
* update dependency com.google.api-client:google-api-client-bom to
v2.7.0
([#3151](#3151))
([5f43e43](5f43e43))
* update dependency com.google.errorprone:error_prone_annotations to
v2.31.0
([#3153](#3153))
([3071509](3071509))
* update dependency com.google.errorprone:error_prone_annotations to
v2.31.0
([#3154](#3154))
([335ee63](335ee63))
* update dependency com.google.guava:guava to v33.3.0-jre
([#3119](#3119))
([41174b0](41174b0))
* update dependency dev.cel:cel to v0.7.1
([#3155](#3155))
([b1ddd16](b1ddd16))
* update dependency filelock to v3.16.0
([#3175](#3175))
([6681113](6681113))
* update dependency idna to v3.8
([#3156](#3156))
([82f5326](82f5326))
* update dependency io.netty:netty-tcnative-boringssl-static to
v2.0.66.final
([#3148](#3148))
([a7efaa8](a7efaa8))
* update dependency net.bytebuddy:byte-buddy to v1.15.1
([#3115](#3115))
([0e06c5f](0e06c5f))
* update dependency org.apache.commons:commons-lang3 to v3.17.0
([#3157](#3157))
([8d3b9fd](8d3b9fd))
* update dependency org.checkerframework:checker-qual to v3.47.0
([#3166](#3166))
([365674d](365674d))
* update dependency org.yaml:snakeyaml to v2.3
([#3158](#3158))
([e67ea9a](e67ea9a))
* update dependency platformdirs to v4.3.2
([#3176](#3176))
([4f2f9e0](4f2f9e0))
* update dependency virtualenv to v20.26.4
([#3177](#3177))
([080e078](080e078))
* update google api dependencies
([#3118](#3118))
([67342ea](67342ea))
* update google auth library dependencies to v1.25.0
([#3168](#3168))
([715884a](715884a))
* update google http client dependencies to v1.45.0
([#3159](#3159))
([a3fe612](a3fe612))
* update googleapis/java-cloud-bom digest to 6626f91
([#3147](#3147))
([658e40e](658e40e))
* update junit5 monorepo to v5.11.0
([#3111](#3111))
([6bf84c8](6bf84c8))
* update netty dependencies to v4.1.113.final
([#3165](#3165))
([9b5957d](9b5957d))
* update opentelemetry-java monorepo to v1.42.0
([#3172](#3172))
([413c44e](413c44e))


### Documentation

* Update DEVELOPMENT.md
([#3126](#3126))
([92bdf4e](92bdf4e))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: ldetmer <1771267+ldetmer@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants