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

* feat: preserve some values when regenerating BUILD.bazel

* pr feedback

* use bazel version v2.2.0

* added comment to the generated BUILD.bazel

* pr feedback

* fix: accept buildozer path as a parameter

* pr feedback

.circleci/config.yml

@@ -342,7 +342,7 @@ jobs:
     working_directory: /tmp/
     environment:
       TEST_REPORTS_DIR: /tmp/workspace/bazel/reports/gapic-generator
-      BAZEL_VERSION: 2.0.0
+      BAZEL_VERSION: 3.3.0
       PYTHON_VERSION: 3.5.2
     machine: true
     steps:

WORKSPACE

@@ -27,3 +27,8 @@ switched_rules_by_language(
 )
 
 com_google_protoc_java_resource_names_plugin_repositories(omit_com_google_protobuf = True)
+
+### Dependencies for buildozer
+load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")
+go_rules_dependencies()
+go_register_toolchains()

repositories.bzl

@@ -60,9 +60,7 @@ def com_google_api_codegen_repositories():
     _maybe(
         http_archive,
         name = "bazel_skylib",
-        sha256 = "bbccf674aa441c266df9894182d80de104cabd19be98be002f6d478aaa31574d",
-        strip_prefix = "bazel-skylib-2169ae1c374aab4a09aa90e65efe1a3aad4e279b",
-        urls = ["https://github.com/bazelbuild/bazel-skylib/archive/2169ae1c374aab4a09aa90e65efe1a3aad4e279b.tar.gz"],
+        urls = ["https://github.com/bazelbuild/bazel-skylib/releases/download/0.9.0/bazel_skylib-0.9.0.tar.gz"],
     )
 
     _maybe(
@@ -100,6 +98,25 @@ def com_google_api_codegen_repositories():
         actual = "@error_prone_annotations_maven//jar",
     )
 
+    ## Dependencies for buildozer
+    _maybe(
+        http_archive,
+        name = "io_bazel_rules_go",
+        urls = [
+            "https://github.com/bazelbuild/rules_go/archive/v0.23.3.zip",
+        ],
+        strip_prefix = "rules_go-0.23.3",
+    )
+
+    _maybe(
+        http_archive,
+        name = "com_github_bazelbuild_buildtools",
+        strip_prefix = "buildtools-3.2.1",
+        urls = [
+            "https://github.com/bazelbuild/buildtools/archive/3.2.1.tar.gz",
+        ],
+    )
+
 def _maybe(repo_rule, name, strip_repo_prefix = "", **kwargs):
     if not name.startswith(strip_repo_prefix):
         return

rules_gapic/bazel/BUILD.bazel

@@ -1,7 +1,16 @@
+genrule(
+    name = "buildozer_binary",
+    srcs = ["@com_github_bazelbuild_buildtools//buildozer:buildozer"],
+    outs = ["buildozer.bin"],
+    cmd = "cp $(location @com_github_bazelbuild_buildtools//buildozer:buildozer) \"$@\" && chmod +x \"$@\"",
+)
+
 java_binary(
     name = "build_file_generator",
     srcs = glob(["src/main/java/**/*.java"]),
     resources = glob(["src/main/java/**/*.mustache"]),
+    data = [":buildozer_binary"],
+    args = ["--buildozer=$(location :buildozer_binary)"],
     create_executable = True,
     javacopts = ["-source", "1.8", "-target", "1.8"],
     jvm_flags = ["-Xmx1024m"],
@@ -13,6 +22,7 @@ java_test(
     name = "build_file_generator_test",
     srcs = glob(["src/test/java/**/*.java"]),
     deps = [":build_file_generator"],
-    data = glob(["src/test/data/**/*.*"]),
+    data = glob(["src/test/data/**/*.*"]) + [":buildozer_binary"],
     test_class = "com.google.api.codegen.bazel.BuildFileGeneratorTest",
+    jvm_flags = ["-Dbuildozer=$(location :buildozer_binary)"],
 )

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

@@ -1,7 +1,11 @@
 package com.google.api.codegen.bazel;
 
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.TreeSet;
@@ -40,6 +44,18 @@ class ApiVersionedDir {
 
   private static String CLOUD_AUTH_SCOPE = "https://www.googleapis.com/auth/cloud-platform";
 
+  private static final String[] PRESERVED_PROTO_LIBRARY_STRING_ATTRIBUTES = {
+    // TypeScript:
+    "package_name", "main_service", "bundle_config", "iam_service",
+    // Other languages: add below
+  };
+
+  private static final String[] PRESERVED_PROTO_LIBRARY_LIST_ATTRIBUTES = {
+    // All languages:
+    "extra_protoc_parameters", "extra_protoc_file_parameters",
+    // Other languages: add below
+  };
+
   // A reference to the object representing the parent dir of this versioned API dir.
   // For example: google/example/library.
   private ApiDir parent;
@@ -123,6 +139,13 @@ class ApiVersionedDir {
   //         https://www.googleapis.com/auth/cloud-platform
   private boolean cloudScope;
 
+  // Names of *_gapic_assembly_* rules (since they may be overridden by the user)
+  private final Map<String, String> assemblyPkgRulesNames = new HashMap<>();
+
+  // Attributes of *_gapic_library rules to be overridden
+  private final Map<String, Map<String, String>> overriddenStringAttributes = new HashMap<>();
+  private final Map<String, Map<String, List<String>>> overriddenListAttributes = new HashMap<>();
+
   void setParent(ApiDir parent) {
     this.parent = parent;
   }
@@ -183,6 +206,18 @@ boolean getCloudScope() {
     return cloudScope;
   }
 
+  Map<String, Map<String, String>> getOverriddenStringAttributes() {
+    return overriddenStringAttributes;
+  }
+
+  Map<String, Map<String, List<String>>> getOverriddenListAttributes() {
+    return overriddenListAttributes;
+  }
+
+  Map<String, String> getAssemblyPkgRulesNames() {
+    return assemblyPkgRulesNames;
+  }
+
   void parseYamlFile(String fileName, String fileBody) {
     // It is a gapic yaml
     Matcher m = GAPIC_YAML_TYPE.matcher(fileBody);
@@ -285,6 +320,59 @@ void parseJsonFile(String fileName, String fileBody) {
     }
   }
 
+  void parseBazelBuildFile(Path file) {
+    try {
+      Buildozer buildozer = Buildozer.getInstance();
+
+      // We cannot and we do not want to preserve all the content of the file.
+      // We will let the user edit just the following:
+      // - names of the final targets (*_gapic_assembly_*) because they are user-facing;
+      // - extra protoc plugin parameters for *_gapic_library rules.
+      List<String> allRules = buildozer.execute(file, "print kind name", "*");
+      for (String rule : allRules) {
+        String[] split = rule.split(" ");
+        if (split.length != 2) {
+          // some rules e.g. package() don't have "name" attribute, just skip them
+          continue;
+        }
+        String kind = split[0];
+        String name = split[1];
+        if (kind.contains("_gapic_assembly_")) {
+          if (this.assemblyPkgRulesNames.containsKey(kind)) {
+            // Duplicated rule of the same kind will break our logic for preserving rule name.
+            System.err.println("There are more than one rule of kind " + kind + ".");
+            System.err.println(
+                "Bazel build file generator does not support regenerating BUILD.bazel in this case.");
+            System.err.println(
+                "Please run it with --overwrite option to overwrite the existing BUILD.bazel completely.");
+            throw new RuntimeException("Duplicated rule " + kind);
+          }
+          this.assemblyPkgRulesNames.put(kind, name);
+        } else if (kind.endsWith("_gapic_library")) {
+          this.overriddenStringAttributes.put(name, new HashMap<>());
+          this.overriddenListAttributes.put(name, new HashMap<>());
+          for (String attr : ApiVersionedDir.PRESERVED_PROTO_LIBRARY_STRING_ATTRIBUTES) {
+            String value = buildozer.getAttribute(file, name, attr);
+            if (value != null) {
+              this.overriddenStringAttributes.get(name).put(attr, value);
+            }
+          }
+          for (String attr : ApiVersionedDir.PRESERVED_PROTO_LIBRARY_LIST_ATTRIBUTES) {
+            String value = buildozer.getAttribute(file, name, attr);
+            if (value != null && value.startsWith("[") && value.endsWith("]")) {
+              value = value.substring(1, value.length() - 1);
+              String[] values = value.split(" ");
+              this.overriddenListAttributes.get(name).put(attr, Arrays.asList(values));
+            }
+          }
+        }
+      }
+    } catch (IOException exception) {
+      System.err.println(
+          "Error parsing BUILD.bazel file in " + file.toString() + ": " + exception.toString());
+    }
+  }
+
   void injectFieldsFromTopLevel() {
     if (parent == null || serviceYamlPath != null) {
       return;

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

@@ -25,6 +25,7 @@ interface FileWriter {
   private final BazelBuildFileTemplate rawApiTempl;
   private final Path srcDir;
   private final Path destDir;
+  private final boolean overwrite;
   private boolean writerMode;
   private final FileWriter fileWriter;
 
@@ -34,12 +35,14 @@ interface FileWriter {
       String gapicApiTempl,
       String rootApiTempl,
       String rawApiTempl,
+      boolean overwrite,
       FileWriter fileWriter) {
     this.gapicApiTempl = new BazelBuildFileTemplate(gapicApiTempl);
     this.rootApiTempl = new BazelBuildFileTemplate(rootApiTempl);
     this.rawApiTempl = new BazelBuildFileTemplate(rawApiTempl);
     this.srcDir = srcDir.normalize();
     this.destDir = destDir.normalize();
+    this.overwrite = overwrite;
     this.writerMode = false;
     this.fileWriter =
         (fileWriter != null)
@@ -98,9 +101,10 @@ 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.
+      // if --overwrite is given, we don't care what's in the existing BUILD.bazel file.
+      if (!overwrite) {
+        bp.parseBazelBuildFile(file);
+      }
     } else if (fileName.endsWith(".json")) {
       bp.parseJsonFile(fileName, readFile(file));
     }

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

@@ -15,6 +15,10 @@ class ArgsParser {
   ArgsParser(String[] args) {
     for (String arg : args) {
       String[] argNameVal = arg.split("=");
+      if (argNameVal.length == 1) {
+        parsedArgs.put(argNameVal[0], "true");
+        continue;
+      }
       if (argNameVal.length != 2) {
         System.out.println("WARNING: Ignoring unrecognized argument: " + arg);
         continue;
@@ -29,15 +33,38 @@ class ArgsParser {
               + "The required arguments are: "
               + required;
       System.out.println(msg);
+      ArgsParser.printUsage();
       throw new IllegalArgumentException(msg);
     }
   }
 
+  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"
+            + "\n"
+            + "Command line options:\n"
+            + "  --src=path: location of googleapis directory\n"
+            + "  --dest=path: destination folder, defaults to the value of --src\n"
+            + "  --overwrite: do not preserve any of the manually changed values in the generated BUILD.bazel files\n";
+    System.out.println(helpMessage);
+  }
+
   ApisVisitor createApisVisitor(ApisVisitor.FileWriter fileWriter, String relativePathPrefix)
       throws IOException {
+    if (parsedArgs.get("--help") != null) {
+      ArgsParser.printUsage();
+      throw new IllegalArgumentException();
+    }
+
+    String buildozerPath = parsedArgs.get("--buildozer");
     String gapicApiTemplPath = parsedArgs.get("--gapic_api_templ");
     String rootApiTemplPath = parsedArgs.get("--root_api_templ");
     String rawApiTempl = parsedArgs.get("--raw_api_templ");
+    String overwrite = parsedArgs.get("--overwrite");
+    if (overwrite == null) {
+      overwrite = "false";
+    }
 
     Path srcPath = Paths.get(parsedArgs.get("--src")).normalize();
     Path destPath = srcPath;
@@ -55,6 +82,14 @@ ApisVisitor createApisVisitor(ApisVisitor.FileWriter fileWriter, String relative
       }
     }
 
+    if (buildozerPath == null && !overwrite.equals("true")) {
+      System.err.println("This tool requires Buildozer tool to parse BUILD.bazel files.");
+      System.err.println("Please use --buildozer=/path/to/buildozer to point to Buildozer,");
+      System.err.println("or use --overwrite if you want to rewrite all BUILD.bazel files.");
+      throw new IllegalArgumentException();
+    }
+    Buildozer.setBinaryPath(buildozerPath);
+
     return new ApisVisitor(
         srcPath,
         destPath,
@@ -67,6 +102,7 @@ ApisVisitor createApisVisitor(ApisVisitor.FileWriter fileWriter, String relative
         rawApiTempl == null
             ? readResource("BUILD.bazel.raw_api.mustache")
             : ApisVisitor.readFile(rawApiTempl),
+        overwrite.equals("true"),
         fileWriter);
   }
 

rules_gapic/bazel/src/main/java/com/google/api/codegen/bazel/BUILD.bazel.gapic_api.mustache

@@ -1,4 +1,13 @@
 # This file was automatically generated by BuildFileGenerator
+# https://github.com/googleapis/gapic-generator/tree/master/rules_gapic/bazel
+
+# Most of the manual changes to this file will be overwritten.
+# It's **only** allowed to change the following rule attribute values:
+# - names of *_gapic_assembly_* rules
+# - certain parameters of *_gapic_library rules, including but not limited to:
+#    * extra_protoc_parameters
+#    * extra_protoc_file_parameters
+# The complete list of preserved parameters can be found in the source code.
 
 # This is an API workspace, having public visibility by default makes perfect sense.
 package(default_visibility = ["//visibility:public"])