Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: bake gapic-generator-java into the hermetic build docker image #3067
Changes from 13 commits
8de1c01
15b2fc5
0a95554
034edf9
9795e23
89fc562
1f2ea9e
8af8bec
b5e49dd
1649403
34c5355
651af1e
4ef977b
6e20135
a5c18b2
89e16c1
1cf63ea
74b36d3
e582817
82e6b8e
3f0dc7f
b599d6f
ddb4ff9
6d35e7f
d08e739
eea978d
cdb137e
9b50a73
4ac8caf
4a57a7d
1164694
5a4cfdc
a9257fe
ab7a684
ddaeaf1
62b281d
e0de3c9
1bc545a
9022ba0
6f1e200
bd71775
0e90ddc
25f2d7f
902f656
a06cd0b
7194148
41ab2da
1301b0f
c4933ea
6379c0d
16435c4
1b4a8cc
8e0ddcc
81f6748
35ad97e
32e9af3
d6844ce
d776b32
1de293f
1895792
7d805e2
1eec008
935534e
b01717a
5fb1be8
38b53a2
87b7425
e639d2c
7dd0f3e
6a4f446
b524768
01d8fa0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Only copying the jar (as opposed to copying the entire
.m2
folder) allows us for a more precise test. We can use the env varsDOCKER_GAPIC_GENERATOR_LOCATION
andDOCKER_GAPIC_GENERATOR_VERSION
to confirm if the preconfigured generator was usedThere 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.
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.
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.
I ran
docker run --network none
and could get pastgenerate_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:
sdk-platform-java/library_generation/owlbot/synthtool/languages/java.py
Line 127 in 0ef6619
Our script pulls googleapis' proto definitions:
sdk-platform-java/library_generation/utils/utilities.py
Lines 175 to 182 in 0ef6619
I thought there was a commit in synthtool that fetches libraries_bom from our config yaml?
sdk-platform-java/library_generation/owlbot/synthtool/languages/java.py
Lines 280 to 287 in 0ef6619
For the googleapis download, I guess it's expected behavior
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.
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
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.
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.
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.
I think this copy logic might already be covered here?
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.
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
?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.
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.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.
Can we just use the jar as it is without copying? Is it for
gapic-generator-java-wrapper
?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 wrapper assumes the jar to be in CWD
sdk-platform-java/library_generation/gapic-generator-java-wrapper
Line 6 in 58da069
I'm not sure if we can both avoid copying the jar and at the same time not have a
DOCKER_GAPIC_GENERATOR_LOCATION
variable. We need a way to know where the jar is and make it compatible with local development as well.Maybe we can copy the jar directly into the home
.m2
folder? We would still be copying it just like we've been doing so far.