feat: preserve some values when regenerating BUILD.bazel #3237

alexander-fenster · 2020-06-18T16:51:42Z

In this (surprisingly large) PR I'm adding some code to preserve values of some manually edited attributes in the generated BUILD.bazel files.

The initial generation of build files is not changed, but if a file exists, I use buildozer (which is an official tool for manipulating BUILD.bazel files and has a proper syntax parser inside) and save the values we want to preserve. When the file is regenerated, I use buildozer again to set those preserved values.

List of preserved values:

name of all the *_gapic_assembly_pkg rules (which is useful for non-cloud library owners who want to remove cloud from the rule name; also, this is the only user-facing rules so making it possible to change their names make sense;
the following parameters of all the *_gapic_library rules:
- extra_protoc_parameters
- extra_protoc_file_parameters
- package_name
- main_service
- bundle_config
- iam_service
  (the last four make sense for TypeScript; other languages can add their parameters-to-be-preserved to the list)

Now this all looks wonderful, until we realize that buildozer is written in Go and does not have any API other than its CLI, so to make it all bazel friendly, apparently the only way is to include the buildozer binary dependency to the .jar as a resource. This indeed makes .jar non-portable (because it would build buildozer for the current platform only), but since we only use this build file generator as bazel run, it should be fine. If you bazel run on Linux, your .jar will contain a Linux buildozer binary; if you're on Mac, it will be a Mac binary; if you're on Windows, it's very unlikely it will work at all but who cares.

buildozer, which is a Go dependency, is now a data dependency of the java_binary task.

This is my second Java PR in a few years, I have no idea how to write Java code properly, so please forgive me :)

codecov · 2020-06-18T16:51:57Z

Codecov Report

Merging #3237 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #3237      +/-   ##
============================================
- Coverage     87.12%   87.12%   -0.01%     
+ Complexity     6077     6076       -1     
============================================
  Files           494      494              
  Lines         24052    24052              
  Branches       2613     2613              
============================================
- Hits          20956    20955       -1     
  Misses         2234     2234              
- Partials        862      863       +1

Impacted Files	Coverage Δ	Complexity Δ
.../java/com/google/api/codegen/discovery/Schema.java	`84.34% <0.00%> (-0.51%)`	`42.00% <0.00%> (-1.00%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 720e000...2bc118d. Read the comment docs.

noahdietz

Do you think we should have a flag that disables buildozer preservation? In the event someone wants to wipe out a BUILD.bazel file.

repositories.bzl

rules_gapic/bazel/src/main/java/com/google/api/codegen/bazel/ApiVersionedDir.java

noahdietz · 2020-06-18T17:50:49Z

rules_gapic/bazel/src/main/java/com/google/api/codegen/bazel/ApisVisitor.java

@@ -98,9 +98,7 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO
    } else if (fileName.endsWith(".proto")) {
      bp.parseProtoFile(fileName, readFile(file));
    } else if (fileName.endsWith(".bazel")) {
-      // Consider merging BUILD.bazel files if it becomes necessary (i.e. people will be doing many
-      // valuable manual edits in their BUILD.bazel files). This will complicate the whole logic
-      // so not doing it for now, hoping it will not be required.


One can hope! But alas... 😉

This comment was very helpful to find the actual place to make changes! :)

alexander-fenster · 2020-06-18T22:36:33Z

Updates based on feedback from @noahdietz (thank you!):

--overwrite can be passed to disable the feature;
Upgraded Go rules to v0.23.3 (this also required upgrading Skylib, I'm now using the same version as we use in googleapis)
Misc fixes
--help option is now supported :)

noahdietz · 2020-06-18T23:05:54Z

LGTM, but I'll let @vam-google approve.

Thanks Alex, this is great.

.circleci/config.yml

rules_gapic/bazel/src/main/java/com/google/api/codegen/bazel/ApiVersionedDir.java

vam-google · 2020-06-19T07:43:04Z

rules_gapic/bazel/src/main/java/com/google/api/codegen/bazel/ApiVersionedDir.java

+        String name = split[1];
+        if (kind.contains("_gapic_assembly_")) {
+          this.assemblyPkgRulesNames.put(kind, name);
+        } else if (kind.endsWith("_gapic_library")) {


How will this logic work if there is more than one instance of the same rule type in build file? (i.e. like two java_gapic_library targets in the same BUIDL.bazel file?)

It won't work. Let's just throw an exception in this case and ask the user to use --overwrite, what do you think?

Looks Ok to me.

vam-google · 2020-06-19T22:43:24Z

rules_gapic/bazel/src/main/java/com/google/api/codegen/bazel/Buildozer.java

+    if (!target.startsWith(":")) {
+      target = ":" + target;
+    }
+    return execute(new String[] {command, String.format("%s%s", bazelBuildFile, target)}, null);


Subjective, but String.format("%s%s", bazelBuildFile, target) looks unnecessary, bazelBuildFile + target is looks shorter and cleaner.Using plus for string concatenation is still very idiomatic in java.

After refactor, it's now "%s:%s" which looks a little bit better :)

rules_gapic/bazel/src/main/java/com/google/api/codegen/bazel/Buildozer.java

vam-google · 2020-06-20T00:11:50Z

rules_gapic/bazel/src/main/java/com/google/api/codegen/bazel/ApiVersionedDir.java

@@ -40,6 +44,18 @@

  private static String CLOUD_AUTH_SCOPE = "https://www.googleapis.com/auth/cloud-platform";

+  private static final String[] PRESERVED_PROTO_LIBRARY_STRING_ATTRIBUTES = {


How hard would it be to make this stuff configurable (probably to the extent of passing it from command line)?

I would prefer not to do it now until we have the first user who needs it. That's a kind of YAGNI :)

alexander-fenster · 2020-06-21T05:17:13Z

@vam-google Can you please take a look once more? I resolved most of the comments. Let me know if it looks good.

vam-google

LGTM, thought I still don't like the help part. Also, did you have a chance to actually test it from googleapis repo?

vam-google · 2020-06-22T21:56:46Z

rules_gapic/bazel/src/main/java/com/google/api/codegen/bazel/ArgsParser.java

+  static void printUsage() {
+    String helpMessage =
+        "Usage (when running from googleapis folder):\n"
+            + "  bazel run //:build_gen -- --src=rules_gapic/bazel/src/test/data/googleapis\n"


Now the help became specific to googleapis context... That was kind of the reason why I did nto want to include any helps at all. Notice, the tool in its help speaks about how it must be executed from bazel (the java tool does not have to be bazel specific).

vam-google

LGTM

vam-google · 2020-06-24T00:10:20Z

rules_gapic/bazel/src/test/java/com/google/api/codegen/bazel/BuildFileGeneratorTest.java

-    ArgsParser args = new ArgsParser(new String[] {"--src=" + copiedGoogleapis.toString()});
+    ArgsParser args =
+        new ArgsParser(
+            new String[] {"--buildozer=" + buildozerPath, "--src=" + copiedGoogleapis.toString()});


toString() is called automatically for all objects participating in string cancatenation. Please remove it (here and below)

New generator feature: preserve some values when regenerating BUILD.bazel. More information: googleapis/gapic-generator#3237 PiperOrigin-RevId: 317983262

New generator feature: preserve some values when regenerating BUILD.bazel. More information: googleapis/gapic-generator#3237 PiperOrigin-RevId: 317983262 Source-Author: Google APIs <noreply@google.com> Source-Date: Tue Jun 23 19:03:52 2020 -0700 Source-Repo: googleapis/googleapis Source-Sha: 927ff51dcfbdf092d469083dc3964173851f5575 Source-Link: googleapis/googleapis@927ff51

feat: preserve some values when regenerating BUILD.bazel

45afa4e

alexander-fenster requested review from noahdietz and vam-google June 18, 2020 16:51

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 18, 2020

noahdietz reviewed Jun 18, 2020

View reviewed changes

noahdietz mentioned this pull request Jun 18, 2020

build_gen: don't hard code parts of the assembly_pkg target name #3236

Closed

pr feedback

5977c0b

use bazel version v2.2.0

88252e7

added comment to the generated BUILD.bazel

a30ed2f

alexander-fenster force-pushed the preserve branch from a07063f to a30ed2f Compare June 18, 2020 23:10

vam-google approved these changes Jun 20, 2020

View reviewed changes

pr feedback

97f2411

alexander-fenster force-pushed the preserve branch from 7809945 to 97f2411 Compare June 20, 2020 11:24

vam-google approved these changes Jun 22, 2020

View reviewed changes

fix: accept buildozer path as a parameter

7f6dcf9

alexander-fenster force-pushed the preserve branch from 4037a2e to 7f6dcf9 Compare June 23, 2020 23:58

vam-google approved these changes Jun 24, 2020

View reviewed changes

pr feedback

2bc118d

alexander-fenster merged commit ba34bae into master Jun 24, 2020

alexander-fenster deleted the preserve branch June 24, 2020 00:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: preserve some values when regenerating BUILD.bazel #3237

feat: preserve some values when regenerating BUILD.bazel #3237

alexander-fenster commented Jun 18, 2020 •

edited

Loading

codecov bot commented Jun 18, 2020 •

edited

Loading

noahdietz left a comment

noahdietz Jun 18, 2020

alexander-fenster Jun 18, 2020

alexander-fenster commented Jun 18, 2020

noahdietz commented Jun 18, 2020

vam-google Jun 19, 2020

alexander-fenster Jun 20, 2020

vam-google Jun 22, 2020

vam-google Jun 19, 2020

alexander-fenster Jun 20, 2020

vam-google Jun 20, 2020

alexander-fenster Jun 20, 2020

alexander-fenster commented Jun 21, 2020

vam-google left a comment

vam-google Jun 22, 2020

vam-google left a comment

vam-google Jun 24, 2020

		@@ -40,6 +44,18 @@

		private static String CLOUD_AUTH_SCOPE = "https://www.googleapis.com/auth/cloud-platform";

		private static final String[] PRESERVED_PROTO_LIBRARY_STRING_ATTRIBUTES = {

feat: preserve some values when regenerating BUILD.bazel #3237

feat: preserve some values when regenerating BUILD.bazel #3237

Conversation

alexander-fenster commented Jun 18, 2020 • edited Loading

codecov bot commented Jun 18, 2020 • edited Loading

Codecov Report

noahdietz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-fenster commented Jun 18, 2020

noahdietz commented Jun 18, 2020

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

alexander-fenster commented Jun 21, 2020

vam-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vam-google left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-fenster commented Jun 18, 2020 •

edited

Loading

codecov bot commented Jun 18, 2020 •

edited

Loading