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.

Sign up for GitHub

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

Jump to bottom

chore: generate poms with grpc dependencies as test scoped #3072

Merged
merged 9 commits into from
Aug 31, 2024
Merged

chore: generate poms with grpc dependencies as test scoped #3072

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
eb912d5
fix: make grpc deps `common-protos` and `iam-v1` test-scoped
diegomarquezp Jul 23, 2024
daf93fb
add java-dataproc to ignored modules
diegomarquezp Jul 25, 2024
0c4be79
fix new library template
diegomarquezp Jul 26, 2024
1fabb63
modify golden file
diegomarquezp Jul 26, 2024
fdd9d27
lint
diegomarquezp Jul 26, 2024
c915614
correct pom template
diegomarquezp Jul 30, 2024
18591bd
apply this change only on monorepos
diegomarquezp Aug 23, 2024
06e2f56
update templates to decide via monorepo variable
diegomarquezp Aug 23, 2024
176fa8c
Merge remote-tracking branch 'origin/main' into conditional-pom-gener…
diegomarquezp Aug 31, 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
46 changes: 44 additions & 2 deletions library_generation/owlbot/src/fix_poms.py
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please remind me when fix_poms.py is called? IIRC, both on new client library generation and as part of the post-processing?

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, it's part of running our owlbot postprocessor

sdk-platform-java/library_generation/owlbot/bin/entrypoint.sh

Lines 61 to 64 in 6913db5

# write or restore pom.xml files
echo "Generating missing pom.xml..."
python3 "${scripts_root}/owlbot/src/fix_poms.py" "${versions_file}" "${is_monorepo}"
echo "...done"

Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,10 @@ def _is_cloud_client(existing_modules: List[module.Module]) -> bool:


def update_cloud_pom(
filename: str, proto_modules: List[module.Module], grpc_modules: List[module.Module]
filename: str,
proto_modules: List[module.Module],
grpc_modules: List[module.Module],
repo_metadata: dict,
):
tree = etree.parse(filename)
root = tree.getroot()
Expand All @@ -104,6 +107,43 @@ def update_cloud_pom(
if m.find("{http://maven.apache.org/POM/4.0.0}artifactId") is not None
]

# as of July 2024, we have two dependencies that should be declared as
# test-scoped: grpc-google-common-protos and grpc-google-iam-v1. Only in
# java-storage, java-spanner and java-dataproc we keep them as they are
TEST_SCOPED_DEPENDENCIES = ["grpc-google-common-protos", "grpc-google-iam-v1"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand why we need this part. Is the template change not enough?

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 template is rendered only on new libraries, otherwise the function update_cloud_pom() is called.

sdk-platform-java/library_generation/owlbot/src/fix_poms.py

Lines 532 to 541 in fdd9d27

if os.path.isfile(f"{artifact_id}/pom.xml"):
print("updating modules in cloud pom.xml")
if artifact_id not in excluded_poms_list:
update_cloud_pom(
f"{artifact_id}/pom.xml", proto_modules, grpc_modules, repo_metadata
)
elif artifact_id not in excluded_poms_list:
print("creating missing cloud pom.xml")
templates.render(
template_name="cloud_pom.xml.j2",

print(
'converting old dependencies "grpc-google-common-protos" and "grpc-google-iam-v1" to test-scoped'
)
for d in dependencies:
if repo_metadata["repo_short"] in [
"java-spanner",
"java-storage",
"java-dataproc",
]:
print(
f"skipping test-scoped-dependency fix for special case repo: {repo_metadata['repo_short']}"
)
continue
artifact_query = "{http://maven.apache.org/POM/4.0.0}artifactId"
scope_query = "{http://maven.apache.org/POM/4.0.0}scope"
current_scope = d.find(scope_query)
artifact_id_elem = d.find(artifact_query)
if artifact_id_elem is None:
continue
artifact_id = artifact_id_elem.text
is_test_scoped = (
current_scope.text == "test" if current_scope is not None else False
)
if artifact_id in TEST_SCOPED_DEPENDENCIES and not is_test_scoped:
new_scope = etree.Element(scope_query)
new_scope.text = "test"
if current_scope is not None:
d.replace(current_scope, new_scope)
else:
d.append(new_scope)
new_scope.tail = "\n "
new_scope.getprevious().tail = "\n "

try:
grpc_index = _find_dependency_index(
dependencies, "com.google.api.grpc", "grpc-"
Expand Down Expand Up @@ -492,7 +532,9 @@ def main(versions_file, monorepo):
if os.path.isfile(f"{artifact_id}/pom.xml"):
print("updating modules in cloud pom.xml")
if artifact_id not in excluded_poms_list:
update_cloud_pom(f"{artifact_id}/pom.xml", proto_modules, grpc_modules)
update_cloud_pom(
f"{artifact_id}/pom.xml", proto_modules, grpc_modules, repo_metadata
)
elif artifact_id not in excluded_poms_list:
print("creating missing cloud pom.xml")
templates.render(
Expand Down
16 changes: 16 additions & 0 deletions library_generation/owlbot/templates/poms/cloud_pom.xml.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,12 @@
<groupId>com.google.protobuf</groupId>
<artifactId>protobuf-java</artifactId>
</dependency>
{%- if repo in ['java-storage', 'java-spanner', 'java-dataproc'] %}
<dependency>
<groupId>com.google.api.grpc</groupId>
<artifactId>proto-google-common-protos</artifactId>
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 what was asked is grpc-google-common-protos, not proto-google-common-protos.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, my bad! I corrected it.

</dependency>
{%- endif %}
{% for module in proto_modules %}
<dependency>
<groupId>{{module.group_id}}</groupId>
Expand Down Expand Up @@ -72,16 +74,30 @@
<groupId>com.google.api.grpc</groupId>
<artifactId>proto-google-iam-v1</artifactId>
</dependency>
{%- if repo in ['java-storage', 'java-spanner', 'java-dataproc'] %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know I said hardcoding is not too bad if there is only a few, but I noticed that we already have excluded_dependencies that is used in pom generation, how much effort would it be to introduce a test_dependencies as well?

Copy link
Contributor Author

@diegomarquezp diegomarquezp Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

excluded_dependencies prevents items from the existing_modules list to be added into the final required_dependencies list. required_dependencies is then split into proto_modules and grpc_modules and passed into update_cloud_bom.

Each gRPC module (from required_dependencies -> grpc_modules) passed into update_cloud_bom is added as a <scope>test</scope> dependency if not already existing. It does nothing if it's already existing.

Excluding the dependencies via .repo-metadata.json would not cause the dependencies to be removed since this logic is only to prevent new dependencies from being added to the pom.

Edit: I misunderstood your point. I'll address it in the next comment of this thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a test_dependencies config key in repo_metadata would make sense. We can intersect the contents of test_dependencies and required_dependencies to convert existing poms' dependencies to test-scoped.

I think the effort is small when it comes to fix_poms.py (the hardcoded array is now obtained from repo-metadata).

The other part would be to have a mini-script to create a PR for all libraries in google-cloud-java except java-dataproc.

I guess we can keep adding entries to .repo-metadata since we already have java-postprocessing-specific ones.

Conclusion: small effort.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We decided to rely on the monorepo variable instead, since the only case where it would not compile is for java-dataproc, but it was because it needed the transitive dependency proto-google-iam-v1, so the correct approach is to also set its targeted dependencies as test-scoped and additionally include proto-google-iam-v1 (imported here).

<dependency>
<groupId>com.google.api.grpc</groupId>
<artifactId>grpc-google-iam-v1</artifactId>
</dependency>
{%- endif %}
<dependency>
<groupId>org.threeten</groupId>
<artifactId>threetenbp</artifactId>
</dependency>

<!-- Test dependencies -->
{%- if repo not in ['java-storage', 'java-spanner','java-dataproc'] %}
<dependency>
<groupId>com.google.api.grpc</groupId>
<artifactId>proto-google-common-protos</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.google.api.grpc</groupId>
<artifactId>grpc-google-iam-v1</artifactId>
<scope>test</scope>
</dependency>
{%- endif %}
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
Expand Down
2 changes: 2 additions & 0 deletions library_generation/test/resources/test-owlbot/java-admanager/ad-manager/pom-golden.xml
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

This golden file tests the changes in fix_poms.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@blakeli0 yes, this is the unit test that deals with this golden:

sdk-platform-java/library_generation/test/owlbot/fix_poms_unit_tests.py

Lines 21 to 36 in 06e2f56

resources_dir = os.path.join(script_dir, "..", "resources", "test-owlbot")
class FixPomsTest(unittest.TestCase):
def test_update_poms_group_id_does_not_start_with_google_correctly(self):
ad_manager_resource = os.path.join(resources_dir, "java-admanager")
versions_file = os.path.join(ad_manager_resource, "versions.txt")
os.chdir(ad_manager_resource)
sub_dirs = ["ad-manager", "ad-manager-bom", "proto-ad-manager-v1", "."]
for sub_dir in sub_dirs:
self.__copy__golden(ad_manager_resource, sub_dir)
main(versions_file, "true")
for sub_dir in sub_dirs:
self.assertFalse(compare_pom_in_subdir(ad_manager_resource, sub_dir))
for sub_dir in sub_dirs:
self.__remove_file_in_subdir(ad_manager_resource, sub_dir)

Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@
<dependency>
<groupId>com.google.api.grpc</groupId>
<artifactId>grpc-google-common-protos</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>com.google.api.grpc</groupId>
Expand All @@ -72,6 +73,7 @@
<dependency>
<groupId>com.google.api.grpc</groupId>
<artifactId>grpc-google-iam-v1</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.threeten</groupId>
Expand Down
Loading