feat: dynamically determine protobuf version in build.gradle #1753

JoeWang1127 · 2023-06-06T20:01:25Z

Fixes #1757.

Steps to verify this PR:

find a self-service client, e.g., google-maps-fleetengine.
checkout this branch, assume the path of repo root is /path/sdkplatform-java.
in googleapis repository, run bazelisk build to build the client library with gapic-generator-java and gax-java replaced with local repository.
For example, bazelisk build //google/maps/fleetengine/v1:google-maps-fleetengine-v1-java --override_repository=gapic_generator_java=/path/sdk-platform-java/ --override_repository=com_google_api_gax_java=/path/sdk-platform-java/gax-java/
in googleapis repository, unzip the generated tar file to googleapis root directory.
For example, tar -xf bazel-bin/google/maps/fleetengine/v1/google-maps-fleetengine-v1-java.tar.gz
find protobuf dependency, com.google.protobuf:protobuf-java in proto-*/build.gradle and make sure the version is the same as protobuf version defined in WORKSPACE in gooleapis.
as an alternative, go to the client library directory (e.g., google-maps-fleetengine-v1-java/), and run ./gradlew build.

suztomo · 2023-06-06T20:35:21Z

Can you add how you tested this in the pull request description?

rules_java_gapic/java_gapic_pkg.bzl

gax-java/dependencies.properties

rules_java_gapic/java_gapic_pkg.bzl

suztomo

Would you check whether the protobuf version line canbe removed?

rules_java_gapic/java_gapic_pkg.bzl

suztomo · 2023-06-07T13:25:05Z

gax-java/dependencies.properties

@@ -23,7 +23,7 @@ version.gax_httpjson=0.114.1-SNAPSHOT
 # Versions for dependencies which actual artifacts differ between Bazel and Gradle.
 # Gradle build depends on prebuilt maven artifacts, while Bazel build depends on Bazel workspaces
 # with the sources.
-version.com_google_protobuf=3.23.2
+version.com_google_protobuf=3.21.12


Where is this used now?

This line can't be removed because

sdk-platform-java/gax-java/repositories.bzl

Line 49 in 1e1ca5d

_protobuf_version = PROPERTIES["version.com_google_protobuf"]

depends on the key to be existed.

Of course the next question is the purpose of that line. Who calls com_google_api_gax_java_repositories()?

com_google_api_gax_java_repositories() is called from googleapis/WORKSPACE and gax-java/WORKSPACE.

The protobuf version can be passed from googleapis when building client libraries. However, when running showcase tests, it will create a cycle in WORKSPACE if this line (version.com_google_protobuf=3.21.12) is removed.

Please see #1753 (comment)

Add source code comment. It's used for showcase testing. Not for the generated Gradle file in googleapis project any more.

rules_java_gapic/java_gapic_pkg.bzl

JoeWang1127 · 2023-06-07T18:16:34Z

gax-java/repository_rules.bzl

+    # the version of protobuf defined in googleapis is higher than protobuf
+    # defined in gax-java/dependencies.properties, use this replacement to
+    # sync the two versions.
+    props_as_map["version.com_google_protobuf"] = PROTOBUF_JAVA_VERSION


Inject a key-value pair here, rather than in rules_java_gapic/java_gapic_pkg.bzl.

By doing this, we can remove protobuf version in gax-java/dependencies.properties

This creates a cycle in gax-java/WORKSPACE:

(18:15:51) ERROR: Failed to load Starlark extension '@com_google_protobuf//:protobuf_version.bzl'. Cycle in the workspace file detected. This indicates that a repository is used prior to being defined. The following chain of repository dependencies lead to the missing definition. - @com_google_protobuf This could either mean you have to add the '@com_google_protobuf' repository with a statement like `http_archive` in your WORKSPACE file (note that transitive dependencies are not added automatically), or move an existing definition earlier in your WORKSPACE file.

Full log: https://github.com/googleapis/sdk-platform-java/actions/runs/5203236910/jobs/9385816289?pr=1753

Interesting.

This reverts commit 22d3d0c.

suztomo · 2023-06-07T20:02:50Z

rules_java_gapic/java_gapic_pkg.bzl


 def _wrapPropertyNamesInBraces(properties):
    wrappedProperties = {}
    for k, v in properties.items():
        wrappedProperties["{{%s}}" % k] = v
    return wrappedProperties

-_PROPERTIES = _wrapPropertyNamesInBraces(PROPERTIES)
+_PROPERTIES = _wrapPropertyNamesInBraces(PROPERTIES | _syncVersionWithGoogleapis())


Suggested change

_PROPERTIES = _wrapPropertyNamesInBraces(PROPERTIES | _syncVersionWithGoogleapis())

# Before this replacement, there is a problem (e.g., b/284292352) when ...

PROPERTIES["version.com_google_protobuf"] = PROTOBUF_JAVA_VERSION

_PROPERTIES = _wrapPropertyNamesInBraces(PROPERTIES)

This actually won't work as it will violate mutability rule.

ERROR: Traceback (most recent call last): File "/private/var/tmp/_bazel_joewa/b6b64da05090260654456c80e641ce7c/external/gapic_generator_java/rules_java_gapic/java_gapic_pkg.bzl", line 28, column 43, in <toplevel> PROPERTIES["version.com_google_protobuf"] = PROTOBUF_JAVA_VERSION Error: trying to mutate a frozen dict value

Nice learning. Then create a copy or operate on _PROPERTIES. The latter needs to tweak the key with braces.

JoeWang1127 · 2023-06-08T01:27:52Z

rules_java_gapic/java_gapic_pkg.bzl

+# the version of protobuf defined in googleapis is higher than protobuf
+# defined in gax-java/dependencies.properties, use this replacement to
+# sync the two versions.
+SYNCED_PROPERTIES = PROPERTIES | {"version.com_google_protobuf": PROTOBUF_JAVA_VERSION}


@suztomo is this the idea of "making a copy"?

This is better than what I was thinking with "dict.copy()".

suztomo · 2023-06-08T02:08:57Z

rules_java_gapic/java_gapic_pkg.bzl


 def _wrapPropertyNamesInBraces(properties):
    wrappedProperties = {}
    for k, v in properties.items():
        wrappedProperties["{{%s}}" % k] = v
    return wrappedProperties

-_PROPERTIES = _wrapPropertyNamesInBraces(PROPERTIES)
+# Before this replacement, there is a problem (e.g., b/284292352) when


Suggested change

# Before this replacement, there is a problem (e.g., b/284292352) when

# Before this replacement, there was a problem (e.g., b/284292352) when

blakeli0 · 2023-06-08T14:30:39Z

gax-java/dependencies.properties

@@ -23,6 +23,8 @@ version.gax_httpjson=0.114.1-SNAPSHOT
 # Versions for dependencies which actual artifacts differ between Bazel and Gradle.
 # Gradle build depends on prebuilt maven artifacts, while Bazel build depends on Bazel workspaces
 # with the sources.
+# The protobuf version is only used by showcase tests, not used by gradle files
+# in generated, self-service clients (from googleapis project).
 version.com_google_protobuf=3.23.2


Is this used by showcase? I think the protobuf runtime version in Showcase module extends from gapic-generator-java-parent-pom

Correct me if I'm wrong.

Looking at the ci error log, showcase golden tests eventually invoke bazelisk run //showcase:verify_gapic.

bazelisk run //showcase:verify_gapic will fail if version.com_google_protobuf=3.23.2 is removed from dependencies.properties.

I see, I didn't notice that we are making sure showcase modules are compilable right after generation, they are still verified with the generated gradle files though.

blakeli0 · 2023-06-08T16:01:10Z

gax-java/dependencies.properties

+# The protobuf version is only used by showcase tests, not used by gradle files
+# in generated, self-service clients (from googleapis project).


Suggested change

# The protobuf version is only used by showcase tests, not used by gradle files

# in generated, self-service clients (from googleapis project).

# The protobuf version is only used for generating gradle files for showcase module,

# not for self-service clients (from googleapis project).

sonarqubecloud · 2023-06-08T17:37:12Z

[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarqubecloud · 2023-06-08T17:42:40Z

[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarqubecloud · 2023-06-08T17:48:16Z

[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

feat: dynamically determine protobuf version in build.gradle

5aefe55

product-auto-label bot added the size: xs Pull request size is extra small. label Jun 6, 2023

JoeWang1127 linked an issue Jun 6, 2023 that may be closed by this pull request

Dynamically determine protobuf version in build.gradle. #1757

Closed

JoeWang1127 marked this pull request as ready for review June 6, 2023 21:30

JoeWang1127 requested a review from a team as a code owner June 6, 2023 21:30

JoeWang1127 requested review from suztomo and blakeli0 June 6, 2023 21:30

JoeWang1127 added the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 6, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 6, 2023

suztomo reviewed Jun 7, 2023

View reviewed changes

rules_java_gapic/java_gapic_pkg.bzl Outdated Show resolved Hide resolved

add code comment

3176c3c

product-auto-label bot added size: s Pull request size is small. and removed size: xs Pull request size is extra small. labels Jun 7, 2023

merge main

04c513c

suztomo requested changes Jun 7, 2023

View reviewed changes

gax-java/dependencies.properties Outdated Show resolved Hide resolved

rules_java_gapic/java_gapic_pkg.bzl Show resolved Hide resolved

add a method to sync versions from googleapis

d297e27

suztomo requested changes Jun 7, 2023

View reviewed changes

JoeWang1127 added 3 commits June 7, 2023 10:10

add a seperate dict for synced version

09c9895

restore protobuf version in dependencies.properties

982d818

add synced version after reading properties

1370b23

blakeli0 reviewed Jun 7, 2023

View reviewed changes

rules_java_gapic/java_gapic_pkg.bzl Outdated Show resolved Hide resolved

suztomo reviewed Jun 7, 2023

View reviewed changes

rules_java_gapic/java_gapic_pkg.bzl Outdated Show resolved Hide resolved

JoeWang1127 added 3 commits June 7, 2023 13:50

combine two properties

d123a20

remove protobuf version in dependencies.properties

22d3d0c

empty commit

7b2b7f2

JoeWang1127 commented Jun 7, 2023

View reviewed changes

Revert "remove protobuf version in dependencies.properties"

e265b65

This reverts commit 22d3d0c.

suztomo reviewed Jun 7, 2023

View reviewed changes

add comment for protobuf version

64cfb1b

make a copy of properties

530ea55

JoeWang1127 commented Jun 8, 2023

View reviewed changes

suztomo approved these changes Jun 8, 2023

View reviewed changes

change comment

3eaa5aa

blakeli0 reviewed Jun 8, 2023

View reviewed changes

change comments

0ba592e

JoeWang1127 requested a review from blakeli0 June 8, 2023 17:11

blakeli0 approved these changes Jun 8, 2023

View reviewed changes

JoeWang1127 merged commit 6de5110 into main Jun 8, 2023

JoeWang1127 deleted the feat/dynamic-version branch June 8, 2023 18:29

release-please bot mentioned this pull request Jun 8, 2023

chore(main): release 2.22.0 #1761

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: dynamically determine protobuf version in build.gradle #1753

feat: dynamically determine protobuf version in build.gradle #1753

JoeWang1127 commented Jun 6, 2023 •

edited

Loading

suztomo commented Jun 6, 2023

suztomo left a comment

suztomo Jun 7, 2023

JoeWang1127 Jun 7, 2023

suztomo Jun 7, 2023

JoeWang1127 Jun 7, 2023 •

edited

Loading

suztomo Jun 7, 2023

JoeWang1127 Jun 7, 2023

JoeWang1127 Jun 7, 2023

JoeWang1127 Jun 7, 2023

suztomo Jun 7, 2023

suztomo Jun 7, 2023

JoeWang1127 Jun 7, 2023 •

edited

Loading

suztomo Jun 7, 2023 •

edited

Loading

JoeWang1127 Jun 8, 2023

suztomo Jun 8, 2023 •

edited

Loading

suztomo Jun 8, 2023

JoeWang1127 Jun 8, 2023

blakeli0 Jun 8, 2023

JoeWang1127 Jun 8, 2023

blakeli0 Jun 8, 2023

blakeli0 Jun 8, 2023

JoeWang1127 Jun 8, 2023

sonarqubecloud bot commented Jun 8, 2023

sonarqubecloud bot commented Jun 8, 2023

sonarqubecloud bot commented Jun 8, 2023

	# Before this replacement, there is a problem (e.g., b/284292352) when
	# Before this replacement, there was a problem (e.g., b/284292352) when

		# The protobuf version is only used by showcase tests, not used by gradle files
		# in generated, self-service clients (from googleapis project).

feat: dynamically determine protobuf version in build.gradle #1753

feat: dynamically determine protobuf version in build.gradle #1753

Conversation

JoeWang1127 commented Jun 6, 2023 • edited Loading

Steps to verify this PR:

suztomo commented Jun 6, 2023

suztomo left a 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

JoeWang1127 Jun 7, 2023 • 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

JoeWang1127 Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

suztomo Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suztomo Jun 8, 2023 • 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

sonarqubecloud bot commented Jun 8, 2023

sonarqubecloud bot commented Jun 8, 2023

sonarqubecloud bot commented Jun 8, 2023

JoeWang1127 commented Jun 6, 2023 •

edited

Loading

JoeWang1127 Jun 7, 2023 •

edited

Loading

JoeWang1127 Jun 7, 2023 •

edited

Loading

suztomo Jun 7, 2023 •

edited

Loading

suztomo Jun 8, 2023 •

edited

Loading