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

diegomarquezp · 2024-07-26T13:17:31Z

Fixes googleapis/google-cloud-java#10037 ☕

Overview

This will modify the behavior of library generation at the postprocessing stage. The main change is that generated cloud poms (i.e. google-cloud-${service}/pom.xml) will now have the dependencies proto-google-common-protos and grpc-google-iam-v1 declared as <scope>test</scope> if the libraries belong to a monorepo.

Integration test

It will be failing until we update the test branch. Once this approach is validated, I will proceed with updating the branch.

Approach

For new libraries

Via template in owlbot/teplates/poms/cloud_pom.xml.j2. New libraries will have these two dependencies declared as test scope if they are part of a monorepo.

For existing libraries

The script fix_poms.py will now modify the existing poms by setting the two dependencies as test-scoped, only if they are part of a monorepo.

Confirmation that it works

java-bigquerystorage: no effects since these dependencies are not found in the pom
java-datastore: no relevant changes since the dependencies are not found in the pom
java-firestore: no relevant changes since the one targeted dependency is already declared as test scoped
java-logging: no relevant changes] since the dependencies are not declared in the pom
java-pubsub: no changes. Relevant dependency already declared as test scoped
java-pubsublite: no relevant changes since there are no relevant dependencies declared in the pom
java-spanner: ignored - dependency kept as is
java-storage: ignored - kept as is
google-cloud-java: 91 poms affected by converting dependencies to test-scoped (PR only contains pom changes for simplicity + outdated googleapis_committish)
- We had a special situation with java-dataproc because it references google-iam-v1 via grpc-google-iam-v1. However this is incorrect because what this library really needs is the proto classes. The fix for this was to treat this library as any other library and add proto-google-iam-v1 as a dependency.

blakeli0 · 2024-07-29T16:30:39Z

library_generation/owlbot/templates/poms/cloud_pom.xml.j2

@@ -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>


I think what was asked is grpc-google-common-protos, not proto-google-common-protos.

True, my bad! I corrected it.

blakeli0 · 2024-07-29T16:34:59Z

library_generation/owlbot/templates/poms/cloud_pom.xml.j2

@@ -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'] %}


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?

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.

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.

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).

blakeli0 · 2024-07-29T19:32:29Z

library_generation/owlbot/src/fix_poms.py

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?

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"

blakeli0 · 2024-07-29T19:38:51Z

library_generation/owlbot/src/fix_poms.py

+    # 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"]


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

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",

blakeli0 · 2024-08-23T16:28:15Z

library_generation/test/resources/test-owlbot/java-admanager/ad-manager/pom-golden.xml

This golden file tests the changes in fix_poms.py?

@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)

blakeli0 · 2024-08-23T16:29:39Z

library_generation/owlbot/src/fix_poms.py

@@ -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:


I guess this is more like a one time thing to fix the existing poms? After that, this logic would be a no-op?

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.

I see, to prevent accidental manual changes.

blakeli0

LGTM.
Can you please update the description to reflect the recent changes before merging?
And update the dataproc module in google-cloud-java as a follow up?

blakeli0 · 2024-08-27T21:10:08Z

library_generation/owlbot/src/fix_poms.py

@@ -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:


I see, to prevent accidental manual changes.

…ation

sonarcloud · 2024-08-31T00:57:21Z

Quality Gate passed for 'gapic-generator-java-root'

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

sonarcloud · 2024-08-31T01:00:32Z

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

Fixes googleapis/google-cloud-java#10037 ☕ ### Overview This will modify the behavior of library generation at the postprocessing stage. The main change is that generated _cloud_ poms (i.e. `google-cloud-${service}/pom.xml`) will now have the dependencies `proto-google-common-protos` and `grpc-google-iam-v1` declared as `<scope>test</scope>` if the libraries belong to a monorepo. ### Integration test It will be failing until we update the test branch. Once this approach is validated, I will proceed with updating the branch. ### Approach #### For new libraries Via template in `owlbot/teplates/poms/cloud_pom.xml.j2`. New libraries will have these two dependencies declared as test scope if they are part of a monorepo. #### For existing libraries The script `fix_poms.py` will now modify the existing poms by setting the two dependencies as test-scoped, only if they are part of a monorepo. ### Confirmation that it works * java-bigquerystorage: no effects since these dependencies are [not found in the pom](https://github.com/search?q=repo%3Agoogleapis%2Fjava-bigquerystorage%20path%3Agoogle-cloud-bigquerystorage%2Fpom.xml%20grpc-google&type=code) * java-datastore: [no relevant changes](https://github.com/googleapis/java-datastore/pull/1530/files) since the dependencies are [not found in the pom](https://github.com/googleapis/java-datastore/blob/main/google-cloud-datastore/pom.xml) * java-firestore: [no relevant changes](googleapis/java-firestore#1764) since the one targeted dependency is [already declared as test scoped](https://github.com/googleapis/java-firestore/blob/main/google-cloud-firestore/pom.xml#L160-L164) * java-logging: [no relevant changes](googleapis/java-logging#1660)] since the dependencies are [not declared in the pom](https://github.com/googleapis/java-logging/blob/main/google-cloud-logging/pom.xml) * java-pubsub: no changes. Relevant dependency [already declared as test scoped](https://github.com/googleapis/java-pubsub/blob/2b15b0a3f43c9ccef663fedf0398375f58fd9183/google-cloud-pubsub/pom.xml#L135-L139) * java-pubsublite: [no relevant changes](googleapis/java-pubsublite#1692) since there are [no relevant dependencies declared in the pom](https://github.com/googleapis/java-pubsublite/blob/main/google-cloud-pubsublite/pom.xml) * java-spanner: ignored - dependency kept as is * java-storage: ignored - kept as is * **google-cloud-java: [91 poms affected](https://github.com/googleapis/google-cloud-java/pull/11031/files) by converting dependencies to test-scoped (PR only contains pom changes for simplicity + outdated googleapis_committish)** * We had a special situation with `java-dataproc` because it references `google-iam-v1` via `grpc-google-iam-v1`. However this is incorrect because what this library really needs is the proto classes. The fix for this was to treat this library as any other library and add `proto-google-iam-v1` as a dependency.

diegomarquezp added 3 commits July 23, 2024 01:25

fix: make grpc deps common-protos and iam-v1 test-scoped

eb912d5

add java-dataproc to ignored modules

daf93fb

fix new library template

0c4be79

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

diegomarquezp added the hermetic-build label Jul 26, 2024

diegomarquezp added 2 commits July 26, 2024 13:40

modify golden file

1fabb63

lint

fdd9d27

diegomarquezp requested a review from blakeli0 July 29, 2024 14:08

blakeli0 reviewed Jul 29, 2024

View reviewed changes

diegomarquezp added 3 commits July 30, 2024 17:32

correct pom template

c915614

apply this change only on monorepos

18591bd

update templates to decide via monorepo variable

06e2f56

blakeli0 reviewed Aug 23, 2024

View reviewed changes

This was referenced Aug 27, 2024

chore: make the owlbot postprocessor check non-required googleapis/java-bigtable#2321

Merged

chore: disable the Owl Bot post-processor googleapis/java-bigtable#2320

Merged

blakeli0 approved these changes Aug 27, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into conditional-pom-gener…

176fa8c

…ation

diegomarquezp added the automerge Merge the pull request once unit tests and other checks pass. label Aug 31, 2024

diegomarquezp mentioned this pull request Aug 31, 2024

chore: fix dependencies in java-dataproc googleapis/google-cloud-java#11109

Merged

diegomarquezp merged commit 0e784a2 into main Aug 31, 2024
34 checks passed

diegomarquezp deleted the conditional-pom-generation branch August 31, 2024 01:04

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Aug 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

diegomarquezp commented Jul 26, 2024 •

edited

Loading

blakeli0 Jul 29, 2024

diegomarquezp Jul 30, 2024

blakeli0 Jul 29, 2024

diegomarquezp Jul 30, 2024 •

edited

Loading

diegomarquezp Jul 30, 2024

diegomarquezp Aug 23, 2024

blakeli0 Jul 29, 2024

diegomarquezp Jul 29, 2024

blakeli0 Jul 29, 2024

diegomarquezp Jul 29, 2024

blakeli0 Aug 23, 2024

diegomarquezp Aug 27, 2024

blakeli0 Aug 23, 2024

diegomarquezp Aug 27, 2024

blakeli0 Aug 27, 2024

blakeli0 left a comment

blakeli0 Aug 27, 2024

sonarcloud bot commented Aug 31, 2024

sonarcloud bot commented Aug 31, 2024

	# 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"

	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",

	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)

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

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

Conversation

diegomarquezp commented Jul 26, 2024 • edited Loading

Overview

Integration test

Approach

For new libraries

For existing libraries

Confirmation that it works

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegomarquezp Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakeli0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Aug 31, 2024

Quality Gate passed for 'gapic-generator-java-root'

sonarcloud bot commented Aug 31, 2024

Quality Gate passed for 'java_showcase_integration_tests'

diegomarquezp commented Jul 26, 2024 •

edited

Loading

diegomarquezp Jul 30, 2024 •

edited

Loading