Address review comments

Author: olafurpg

WORKSPACE

@@ -16,13 +16,13 @@ http_archive(
 ##########
 # Protobuf
 ##########
+# To update this version, copy-paste instructions from https://github.com/bazelbuild/rules_proto/releases
 http_archive(
     name = "rules_proto",
-    sha256 = "66bfdf8782796239d3875d37e7de19b1d94301e8972b3cbd2446b332429b4df1",
-    strip_prefix = "rules_proto-4.0.0",
+    sha256 = "e017528fd1c91c5a33f15493e3a398181a9e821a804eb7ff5acdd1d2d6c2b18d",
+    strip_prefix = "rules_proto-4.0.0-3.20.0",
     urls = [
-        "https://mirror.bazel.build/github.com/bazelbuild/rules_proto/archive/refs/tags/4.0.0.tar.gz",
-        "https://github.com/bazelbuild/rules_proto/archive/refs/tags/4.0.0.tar.gz",
+        "https://github.com/bazelbuild/rules_proto/archive/refs/tags/4.0.0-3.20.0.tar.gz",
     ],
 )
 load("@rules_proto//proto:repositories.bzl", "rules_proto_dependencies", "rules_proto_toolchains")

examples/bazel-example/WORKSPACE

@@ -27,13 +27,13 @@ local_repository(
 ##########
 # Protobuf
 ##########
+# To update this version, copy-paste instructions from https://github.com/bazelbuild/rules_proto/releases
 http_archive(
     name = "rules_proto",
-    sha256 = "66bfdf8782796239d3875d37e7de19b1d94301e8972b3cbd2446b332429b4df1",
-    strip_prefix = "rules_proto-4.0.0",
+    sha256 = "e017528fd1c91c5a33f15493e3a398181a9e821a804eb7ff5acdd1d2d6c2b18d",
+    strip_prefix = "rules_proto-4.0.0-3.20.0",
     urls = [
-        "https://mirror.bazel.build/github.com/bazelbuild/rules_proto/archive/refs/tags/4.0.0.tar.gz",
-        "https://github.com/bazelbuild/rules_proto/archive/refs/tags/4.0.0.tar.gz",
+        "https://github.com/bazelbuild/rules_proto/archive/refs/tags/4.0.0-3.20.0.tar.gz",
     ],
 )
 load("@rules_proto//proto:repositories.bzl", "rules_proto_dependencies", "rules_proto_toolchains")

examples/bazel-example/bazel-bazel-example

@@ -15,7 +15,6 @@
 
 public class BazelBuildTool {
 
-
   public static int runAndReturnExitCode(String[] args) throws IOException {
     Optional<BazelOptions> maybeOptions = BazelOptions.parse(args);
     if (!maybeOptions.isPresent()) {
@@ -57,8 +56,7 @@ public void error(Throwable e) {
     return 0;
   }
 
-  public static List<MavenPackage> mavenPackages(BazelOptions options)
-      throws IOException {
+  public static List<MavenPackage> mavenPackages(BazelOptions options) throws IOException {
     ArrayList<MavenPackage> result = new ArrayList<>();
     if (!options.isQueryMavenImports) {
       return result;
@@ -89,7 +87,7 @@ public static List<MavenPackage> mavenPackages(BazelOptions options)
               String[] parts = tag.substring("maven_coordinates=".length()).split(":");
               if (parts.length == 3) {
                 // The jar part is populated via `withJar()` below.
-                basePackage = new MavenPackage(/* jar = */null, parts[0], parts[1], parts[2]);
+                basePackage = new MavenPackage(/* jar = */ null, parts[0], parts[1], parts[2]);
               }
             }
           }
@@ -114,10 +112,7 @@ public static Bazelbuild.QueryResult runBazelQuery(BazelOptions options, String
       throws IOException {
     List<String> command = Arrays.asList(options.bazelBinary, "query", query, "--output=proto");
     System.out.println("running: " + String.join(" ", command));
-    Process bazelQuery =
-        new ProcessBuilder(command)
-            .directory(options.sourceroot.toFile())
-            .start();
+    Process bazelQuery = new ProcessBuilder(command).directory(options.sourceroot.toFile()).start();
     byte[] bytes = InputStreamBytes.readAll(bazelQuery.getInputStream());
     if (bazelQuery.isAlive()) {
       throw new RuntimeException(new String(InputStreamBytes.readAll(bazelQuery.getErrorStream())));

lsif-semanticdb/src/main/java/com/sourcegraph/lsif_semanticdb/BazelBuildTool.java

@@ -20,17 +20,18 @@ public static void printHelp() {
     System.out.println();
     System.out.println("Command-line tool to generate LSIF for Java targets in a Bazel build.");
     System.out.println(
-            "The idiomatic way to run this tool is to compile it from source via Bazel\n"
-                    + "and invoke it through `bazel run ...`.");
+        "The idiomatic way to run this tool is to compile it from source via Bazel\n"
+            + "and invoke it through `bazel run ...`.");
     System.out.println();
     System.out.println("OPTIONS:");
     System.out.println(
-            "  --sourceroot <path> the absolute path to the root directory of the Bazel codebase");
+        "  --sourceroot <path> the absolute path to the root directory of the Bazel codebase");
     System.out.println(
-            "                      TIP: use --sourceroot $PWD to pass the current working directory");
+        "                      TIP: use --sourceroot $PWD to pass the current working directory");
     System.out.println("  --output <path> the absolute path to the file that should be generated");
     System.out.println("  --parallel whether to process files in parallel");
-    System.out.println("  --no-query-maven-imports whether to skip the `bazel query @maven//...` phase.");
+    System.out.println(
+        "  --no-query-maven-imports whether to skip the `bazel query @maven//...` phase.");
   }
 
   public static Optional<BazelOptions> parse(String[] args) throws IOException {
@@ -93,18 +94,20 @@ public static Optional<BazelOptions> parse(String[] args) throws IOException {
 
     if (options.sourceroot == null) {
       if (args.length == 0) {
-        errors.add("missing required flag --sourceroot <path>. To fix this problem, pass in the `--sourceroot` flag like this: bazel run @lsif_java//lsif-semanticdb:bazel -- --sourceroot $PWD");
+        errors.add(
+            "missing required flag --sourceroot <path>. To fix this problem, pass in the `--sourceroot` flag like this: bazel run @lsif_java//lsif-semanticdb:bazel -- --sourceroot $PWD");
       } else {
-      errors.add("missing required flag --sourceroot <path>");
+        errors.add("missing required flag --sourceroot <path>");
       }
     } else if (!options.sourceroot.isAbsolute()) {
       // result.sourceroot must be an absolute path because `System.getProperty("user.dir")` is a
       // temporary directory that's generated by Bazel.
       errors.add(
-          String.format("relative path --sourceroot '%s'. To fix this problem, pass in an an absolute path.", options.sourceroot));
+          String.format(
+              "relative path --sourceroot '%s'. To fix this problem, pass in an an absolute path.",
+              options.sourceroot));
     } else if (!Files.isDirectory(options.sourceroot)) {
-      errors.add(
-              String.format("not a directory --sourceroot '%s'", options.sourceroot));
+      errors.add(String.format("not a directory --sourceroot '%s'", options.sourceroot));
     }
 
     if (options.output == null) {

lsif-semanticdb/src/main/java/com/sourcegraph/lsif_semanticdb/BazelOptions.java

@@ -23,7 +23,7 @@ public class SemanticdbJavacOptions {
   public boolean includeText = false;
   public boolean verboseEnabled = false;
   public final ArrayList<String> errors;
-  public boolean isErrorReported = false;
+  public boolean alreadyReportedErrors = false;
   public UriScheme uriScheme = UriScheme.DEFAULT;
 
   public static String stubClassName = "META-INF-semanticdb-stub";
@@ -78,7 +78,7 @@ public static SemanticdbJavacOptions parse(String[] args, Context ctx) {
     if (result.targetroot == null && !useJavacClassesDir) {
       result.errors.add(missingRequiredDirectoryOption("targetroot"));
     }
-    if (result.uriScheme != UriScheme.BAZEL && result.sourceroot == null) {
+    if (!isSourcerootDefined(result)) {
       // When using -build-tool:bazel, the sourceroot is automatically inferred from the first
       // compilation unit.
       // See `SemanticdbTaskListener.inferBazelSourceroot()` for the method that infers the
@@ -88,6 +88,13 @@ public static SemanticdbJavacOptions parse(String[] args, Context ctx) {
     return result;
   }
 
+  private static boolean isSourcerootDefined(SemanticdbJavacOptions options) {
+    if (options.uriScheme == UriScheme.BAZEL) {
+      return true; // The sourceroot is automatically inferred for Bazel.
+    }
+    return options.sourceroot != null;
+  }
+
   private static Path getJavacClassesDir(SemanticdbJavacOptions result, Context ctx) {
     // I'm not aware of a better way to get the class output directory from javac
     Path outputDir = null;

semanticdb-javac/src/main/java/com/sourcegraph/semanticdb_javac/SemanticdbJavacOptions.java

@@ -5,7 +5,6 @@
 import com.sun.source.util.TaskListener;
 import com.sun.source.util.Trees;
 import com.sun.tools.javac.model.JavacTypes;
-import com.sun.tools.javac.util.Options;
 
 import javax.tools.Diagnostic;
 import javax.tools.JavaFileObject;
@@ -48,8 +47,8 @@ public void started(TaskEvent e) {}
   public void finished(TaskEvent e) {
     if (e.getKind() != TaskEvent.Kind.ANALYZE) return;
     if (!options.errors.isEmpty()) {
-      if (!options.isErrorReported) {
-        options.isErrorReported = true;
+      if (!options.alreadyReportedErrors) {
+        options.alreadyReportedErrors = true;
         for (String error : options.errors) {
           trees.printMessage(
               Diagnostic.Kind.ERROR,
@@ -129,7 +128,8 @@ public static Path absolutePathFromUri(SemanticdbJavacOptions options, JavaFileO
     }
   }
 
-  // Infers the `-sourceroot:` flag from the provided file
+  // Infers the `-sourceroot:` flag from the provided file.
+  // FIXME: add unit tests https://github.com/sourcegraph/lsif-java/issues/444
   private void inferBazelSourceroot(JavaFileObject file) {
     if (options.uriScheme != UriScheme.BAZEL || options.sourceroot != null) {
       return;
@@ -141,12 +141,14 @@ private void inferBazelSourceroot(JavaFileObject file) {
     // uriPath is the sandbox/temporary file path, for example
     // /private/var/tmp/com/example/Hello.java
     //
-    // We infer sourceroot by iterating the names of both files in reverse order and stop at the
-    // first entry where  the two paths are different. For the  example above, we compare
-    // "Hello.java", then "example",  then "com",  and when we reach  "repo" !+ "tmp" then we guess
-    // that "/home/repo" is the sourceroot. This  logic is brittle  and it would be nice to use more
-    // dedicated APIs, but Bazel actively makes an effort to  sandbox compilation  and hide access
-    // to the original workspace, which is why we resort to  solutions like this.
+    // We infer sourceroot by iterating the names of both files in reverse order
+    // and stop at the first entry where  the two paths are different. For the
+    // example above, we compare "Hello.java", then "example", then "com", and
+    // when we reach "repo" != "tmp" then we guess that "/home/repo" is the
+    // sourceroot. This logic is brittle  and it would be nice to use more
+    // dedicated APIs, but Bazel actively makes an effort to  sandbox
+    // compilation and hide access to the original workspace, which is why we
+    // resort to solutions like this.
     int relativePathDepth = 0;
     int uriPathDepth = uriPath.getNameCount();
     int absolutePathDepth = absolutePath.getNameCount();