Clean up code for review

Author: olafurpg

build.sbt

@@ -167,13 +167,19 @@ lazy val javacPlugin = project
             // referenced from META-INF/services/com.sun.source.util.Plugin
             "com.sourcegraph.semanticdb_javac.SemanticdbPlugin" ->
               "com.sourcegraph.semanticdb_javac.SemanticdbPlugin",
+            // Don't rename PrintJavaVersion because we load it via FQN to
+            // detect the Java of a JVM installation.
             "com.sourcegraph.semanticdb_javac.PrintJavaVersion" ->
               "com.sourcegraph.semanticdb_javac.PrintJavaVersion",
+            // Don't rename InjectSemanticdbOptions because we load it via FQN to
+            // process a list of Java compiler options.
             "com.sourcegraph.semanticdb_javac.InjectSemanticdbOptions" ->
               "com.sourcegraph.semanticdb_javac.InjectSemanticdbOptions",
             "com.google.**" -> "com.sourcegraph.shaded.com.google.@1",
-            // Need to shade the semanticdb-javac compiler plugin in order to be
-            // able to index the plugin code itself.
+            // Shade everything else in the semanticdb-javac compiler plugin in
+            // order to be able to index the plugin code itself. Without this step,
+            // we can't add the plugin to the classpath while compiling the source
+            // code of the plugin itself because it results in cryptic compile errors.
             "com.sourcegraph.**" -> "com.sourcegraph.shaded.com.sourcegraph.@1",
             "google.**" -> "com.sourcegraph.shaded.google.@1",
             "org.relaxng.**" -> "com.sourcegraph.shaded.relaxng.@1"

scip-java/src/main/scala/com/sourcegraph/scip_java/Embedded.scala

@@ -112,7 +112,12 @@ object Embedded {
     }
   }
 
+  /**
+   * Returns the string contents of the scip_java.bzl file on disk.
+   */
   def bazelAspectFile(tmpDir: Path): String = {
+    // We could in theory load the resource straight into a string but it was
+    // easier to copy it to a file and read it from there.
     val tmpFile = copyFile(tmpDir, "scip-java/scip_java.bzl")
     val contents =
       new String(Files.readAllBytes(tmpFile), StandardCharsets.UTF_8)

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/BazelBuildTool.scala

@@ -39,12 +39,11 @@ class BazelBuildTool(index: IndexCommand) extends BuildTool("Bazel", index) {
         // to the provided targetroot directory.
         buildCommand += "--spawn_strategy=local"
 
-        val targetSpecs = ListBuffer.empty[String]
-        if (index.buildCommand.isEmpty) {
-          targetSpecs += "//..."
-        } else {
-          targetSpecs ++= index.buildCommand
-        }
+        val targetSpecs =
+          if (index.buildCommand.isEmpty)
+            List("//...")
+          else
+            index.buildCommand
         buildCommand ++= targetSpecs
 
         buildCommand += "--aspects"
@@ -86,11 +85,17 @@ class BazelBuildTool(index: IndexCommand) extends BuildTool("Bazel", index) {
           .process(buildCommand.toList)
           .call(check = false, stderr = Readlines(sandbox))
         if (commandResult.exitCode != 0) {
-          // Automatically run the sandbox command to help the user debug the problem.
-          index
-            .app
-            .process("bash", "-c", sandbox.commandLines().mkString("\n"))
-            .call(check = false, stdout = os.Inherit, stderr = os.Inherit)
+          if (index.bazelAutorunSandboxCommand) {
+            index
+              .app
+              .info(
+                "Automatically re-running sandbox command to help debug the problem."
+              )
+            index
+              .app
+              .process("bash", "-c", sandbox.commandLines().mkString("\n"))
+              .call(check = false, stdout = os.Inherit, stderr = os.Inherit)
+          }
           index
             .app
             .error(
@@ -105,6 +110,7 @@ class BazelBuildTool(index: IndexCommand) extends BuildTool("Bazel", index) {
             )
           return commandResult.exitCode
         }
+
         // Final step after running the aspect: aggregate all the generated `*.scip` files into a single index.scip file.
         // We have to do this step outside of Bazel because Bazel does not allow actions to generate outputs outside
         // of the bazel-out directory. Ideally, we should be able to implement the aggregation step inside Bazel
@@ -114,10 +120,20 @@ class BazelBuildTool(index: IndexCommand) extends BuildTool("Bazel", index) {
         Files.deleteIfExists(index.finalOutput)
         Files.createDirectories(index.finalOutput.getParent)
         val scipPattern = FileSystems.getDefault.getPathMatcher("glob:**.scip")
-        val binDirectory = Files
-          .readSymbolicLink(index.workingDirectory.resolve("bazel-bin"))
+        val bazelOut = index.workingDirectory.resolve("bazel-out")
+        if (!Files.exists(bazelOut)) {
+          index
+            .app
+            .error(
+              s"doing nothing, the directory $bazelOut does not exist. " +
+                s"The most likely cause for this problem is that there are no Java targets in this Bazel workspace. " +
+                s"Please report an issue to the scip-java issue tracker if the command `bazel query 'kind(java_*, //...)'` returns non-empty output."
+            )
+          return 0
+        }
+        val bazelOutLink = Files.readSymbolicLink(bazelOut)
         Files.walkFileTree(
-          binDirectory,
+          bazelOutLink,
           new SimpleFileVisitor[Path] {
             override def visitFile(
                 file: Path,
@@ -153,11 +169,10 @@ class BazelBuildTool(index: IndexCommand) extends BuildTool("Bazel", index) {
     private var isSandboxCommandPrinting = false
     private val lines = ListBuffer.empty[String]
     def commandLines(): List[String] = lines.toList
-    def command(): List[String] = List("bash", "-c", lines.mkString("\n"))
     override def apply(line: String): Unit = {
       if (
         !isSandboxCommandPrinting && line.startsWith("ERROR:") &&
-        line.endsWith("error executing command ")
+        line.contains("error executing command")
       ) {
         isSandboxCommandPrinting = true
       } else if (isSandboxCommandPrinting && !line.startsWith("  ")) {

scip-java/src/main/scala/com/sourcegraph/scip_java/buildtools/ScipBuildTool.scala

@@ -528,16 +528,13 @@ class ScipBuildTool(index: IndexCommand) extends BuildTool("SCIP", index) {
       arguments += processors.mkString(",")
     }
     arguments ++=
-      config
-        .javacOptions
-        .filterNot(option =>
-          option.startsWith("-Xep") ||
-            option
-              .startsWith("-Xplugin:semanticdb") || option.startsWith("-XD") ||
-            index
-              .scipIgnoredJavacOptionPrefixes
-              .exists(prefix => option.startsWith(prefix))
-        )
+      fixJavacOptions(config.javacOptions).filterNot(option =>
+        option.startsWith("-Xep") || option.startsWith("-Xplugin:semanticdb") ||
+          option.startsWith("-XD") ||
+          index
+            .scipIgnoredJavacOptionPrefixes
+            .exists(prefix => option.startsWith(prefix))
+      )
     if (config.kind == "jdk" && moduleInfos.nonEmpty) {
       moduleInfos.foreach { module =>
         arguments += "--module"
@@ -577,6 +574,16 @@ class ScipBuildTool(index: IndexCommand) extends BuildTool("SCIP", index) {
       Failure(SubprocessException(result))
   }
 
+  private def fixJavacOptions(options: List[String]): List[String] =
+    options match {
+      case "--release" :: _ :: rest =>
+        fixJavacOptions(rest)
+      case option :: rest =>
+        option :: fixJavacOptions(rest)
+      case Nil =>
+        Nil
+    }
+
   private def javacPath(config: Config, tmp: Path): Path = {
     config.javaHome match {
       case Some(home) =>

scip-java/src/main/scala/com/sourcegraph/scip_java/commands/IndexCommand.scala

@@ -75,6 +75,9 @@ case class IndexCommand(
     @Description(
       "If true, overwrites the existing Bazel aspect file (if any)"
     ) bazelOverwriteAspectFile: Boolean = false,
+    @Description(
+      "If true, automatically tries to extract the printed out sandbox command and re-run the command to reveal the underlying problem."
+    ) bazelAutorunSandboxCommand: Boolean = true,
     @Description(
       "Optional. The build command to use to compile all sources. " +
         "Defaults to a build-specific command. For example, the default command for Maven command is 'clean verify -DskipTests'." +

scip-semanticdb/src/main/java/com/sourcegraph/scip_semanticdb/BazelBuildTool.java

@@ -63,9 +63,9 @@ public boolean hasErrors() {
             ScipOutputFormat.TYPED_PROTOBUF,
             options.parallel,
             mavenPackages,
-            "",
-            true,
-            true);
+            /* buildKind */ "",
+            /* emitInverseRelationships */ true,
+            /* allowEmptyIndex */ true);
     ScipSemanticdb.run(scipOptions);
 
     if (!scipOptions.reporter.hasErrors()) {
@@ -109,7 +109,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]);
               }
             }
           }

scip-semanticdb/src/main/java/com/sourcegraph/scip_semanticdb/MessageOnlyException.java

@@ -1,5 +1,10 @@
 package com.sourcegraph.scip_semanticdb;
 
+/**
+ * Exception that doesn't fill out the stack trace, it only prints out the message.
+ *
+ * <p>Use this exception if you want prettier console output without stack trace noise.
+ */
 public class MessageOnlyException extends Throwable {
   public MessageOnlyException(String message) {
     super(message);