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

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

Merged
merged 9 commits into from
Aug 31, 2024
47 changes: 45 additions & 2 deletions library_generation/owlbot/src/fix_poms.py
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

# 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 @@ -15,6 +15,8 @@
import sys
import glob
import json
from xml.etree.ElementTree import ElementTree

from lxml import etree
import os
import re
Expand Down Expand Up @@ -92,7 +94,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],
is_monorepo: bool,
):
tree = etree.parse(filename)
root = tree.getroot()
Expand All @@ -104,6 +109,9 @@ def update_cloud_pom(
if m.find("{http://maven.apache.org/POM/4.0.0}artifactId") is not None
]

if is_monorepo:
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 is more like a one time thing to fix the existing poms? After that, this logic would be a no-op?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this change, if we ever declare this dep as compile-scoped in a monorepo library, then it will be turned back to test-scoped.

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, to prevent accidental manual changes.

_set_test_scoped_deps(dependencies)

try:
grpc_index = _find_dependency_index(
dependencies, "com.google.api.grpc", "grpc-"
Expand Down Expand Up @@ -169,6 +177,39 @@ def update_cloud_pom(
tree.write(filename, pretty_print=True, xml_declaration=True, encoding="utf-8")


def _set_test_scoped_deps(dependencies: list[ElementTree]) -> None:
"""
As of July 2024, we have two dependencies that should be declared as
test-scoped in a monorepo: grpc-google-common-protos and grpc-google-iam-v1.
HW libraries are treated as usual
:param dependencies: List of XML Objects representing a <dependency/>
"""
TEST_SCOPED_DEPENDENCIES = ["grpc-google-common-protos", "grpc-google-iam-v1"]
print(
'converting dependencies "grpc-google-common-protos" and "grpc-google-iam-v1" to test-scoped'
)
for d in dependencies:
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 "


def update_parent_pom(filename: str, modules: List[module.Module]):
tree = etree.parse(filename)
root = tree.getroot()
Expand Down Expand Up @@ -492,7 +533,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, monorepo
)
elif artifact_id not in excluded_poms_list:
print("creating missing cloud pom.xml")
templates.render(
Expand Down
18 changes: 16 additions & 2 deletions library_generation/owlbot/templates/poms/cloud_pom.xml.j2
Original file line number Diff line number Diff line change
Expand Up @@ -66,22 +66,36 @@
</dependency>
<dependency>
<groupId>com.google.api.grpc</groupId>
<artifactId>grpc-google-common-protos</artifactId>
<artifactId>proto-google-iam-v1</artifactId>
</dependency>
{%- if not monorepo %}
<dependency>
<groupId>com.google.api.grpc</groupId>
<artifactId>proto-google-iam-v1</artifactId>
<artifactId>grpc-google-common-protos</artifactId>
</dependency>
<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 monorepo %}
<dependency>
<groupId>com.google.api.grpc</groupId>
<artifactId>grpc-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
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:

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