-
Notifications
You must be signed in to change notification settings - Fork 53
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: transfer java-related synthtool code into the library_generation image #3011
Conversation
Here is the summary of possible violations 😱 There is a possible violation for not having product prefix.
There is a format violation for a region tag.
The end of the violation section. All the stuff below is FYI purposes only. Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
RUN python3 -m pip install -r requirements.in | ||
# install java-only synthtool scripts as a python package | ||
WORKDIR /src/owlbot/synthtool | ||
RUN python -m pip install -r requirements.txt |
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 combine this requirements.txt with library_generation/requirements.txt
?
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 renamed library_generation/requirementss.txt
to requirementss.in
(base dependencies) and created a hashed requirements.txt
. This is in addition of combining the dependency files of owlbot/synthool
|
||
# This default should be switched to "main" once we've migrated | ||
# the majority of our repositories: | ||
return os.getenv("DEFAULT_BRANCH", "master") |
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.
Should we change the default value to main
?
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.
True, but I just realized this is used to compute a template value not used in our templates
t = templates.TemplateGroup(self._template_root / directory, self.excludes) | ||
|
||
if "repository" in kwargs["metadata"] and "repo" in kwargs["metadata"]: | ||
kwargs["metadata"]["repo"]["default_branch"] = _get_default_branch_name( |
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.
Do you know which template uses this key?
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.
No template uses it, removing.
PS: I saw this comment after reading #3011 (comment)
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.
Do you know how this file and its functions are used?
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.
From the PR description, the functions are used to render the README in java.py
metadata["samples"] = samples.all_samples(["samples/**/src/main/java/**/*.java"]) |
There is a link to the part of the README template where this is used.
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.
Same comment here, do you know how this file and its functions are used?
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.
From the PR description, the functions are used to render the README in java.py
sdk-platform-java/library_generation/owlbot/synthtool/languages/java.py
Lines 289 to 291 in 635d92a
metadata["snippets"] = snippets.all_snippets( | |
["samples/**/src/main/java/**/*.java", "samples/**/pom.xml"] | |
) |
) | ||
|
||
# skip README generation on Kokoro (autosynth) | ||
if os.environ.get("KOKORO_ROOT") is not None: |
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 we can remove this line since we won't run tests/build in Kokoro.
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.
Good point. Removed
) | ||
|
||
|
||
def latest_maven_version(group_id: str, artifact_id: str) -> Optional[str]: |
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 we don't need this function once #3007 is merged. We can keep it for now.
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.
Now #3007 is merged, could you create a follow up PR to remove this function?
cloud_prefix = "cloud-" if cloud_api else "" | ||
package_name = package_pattern.format(service=service, version=version) | ||
fix_proto_headers( | ||
library / f"proto-google-{cloud_prefix}{service}-{version}{suffix}" |
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.
nit: could we use a variable for f"proto-google-{cloud_prefix}{service}-{version}{suffix}"
?
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.
Yes, Done.
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.
Could you search owlbot.py
in google-cloud-java and handwritten libraries to see whether there are unused functions and remove them from this file?
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.
Thanks, there was one function dont_overwrite
that wasn't used.
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.
Done
nox==2024.4.15 | ||
requests==2.32.3 | ||
setuptools==65.5.1 | ||
jinja2==3.1.4 |
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.
this package appears twice.
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.
Removed
library_generation/requirements.in
Outdated
setuptools==65.5.1 | ||
jinja2==3.1.4 | ||
deprecation==2.1.0 | ||
protobuf==3.20.2 |
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.
Did you find which functions use deprecation
and protobuf
package?
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 didn't look at the functions but tried running the generation without these. It worked, so I'm removing them. Thanks for catching this.
Could you fix the error in unit tests and lint? |
I had to pin a transitive dependency to a specific version (added a comment in requirements.in) |
Could you enrich the comment about the error and why pinning this dependency can fix the issue. |
Quality Gate passed for 'gapic-generator-java-root'Issues Measures |
Quality Gate passed for 'java_showcase_integration_tests'Issues Measures |
This PR moves
java.py
and the minimum required dependencies from synthtool.It contains the changes of @JoeWang1127's googleapis/synthtool@696c4bf
Details of code transfer
Some files have been simplified and others simply verbatim-copied. Here is a detailed list of changes (the paths are relative to
library_generation/owlbot
:synthtool/__init__.py
: No changes made. This is the module initialization configuration to allow statements such asimport synthtool as s
combined withs.copy
,s.replace
, etc. Without this, we would have to useimport synthtool.transforms as s
.synthtool/_tracked_paths.py
: No changes made. It allows to use relative paths internally when working with library files.synthtool/gcp/common.py
: Simplified to use java-only functions. This file originally had a set of functions to support postprocessing of languages written in multiple languages. The most important function iscommon_templates
, which renders the templates for the library (e.g. workflow files, kokoro files). Note thatcommon_templates
was modified in order to require and only allow specifying the path to the templates via theSYNTHTOOL_TEMPLATES
env var, as opposed to its original support of 3 separate ways, including cloning synthtool and reading the templates from there, because this is now an internal detail of how thelibrary_generation
image will work.synthtool/gcp/samples.py
: No changes made. This is a helper to obtain path and metadata about the generated samples of a library. It is then used when rendering the README.synthtool/gcp/snippets.py
: No changes made. Similar tosamples.py
in the way it's used to render a library's READMEsynthool/languages/java.py
: Small modifications around the fact that we dropped several files (e.g. usegcp.common.CommonTemplates
instead ofgcp.CommonTemplates
to save us from an extragcp/__init__.py
).synthtool/sources/templates.py
: No changes made. Internally used bycommon_templates
. Contains the underlying usage of jinja2 to render the templates.synthtool/transforms.py
: No changes made. Contains a few functions that are commonly used byowlbot.py
files (example)Changes in Dockerfile
We will not clone
synthtool
anymore. We will instead install it as a separate package whose source code is within sdk-platform-java.