Clean up turbine action creation

Support disabling javac fallback for actions without a direct
classpath, and only use the 'JavacTurbine' mnemonic for spawns
that require javac-turbine due to annotation processing to make
it easier to collect metrics on that.

Finally, remove --java_header_compilation_direct_classpath now
that it has been productionized and enabled by default.

PiperOrigin-RevId: 158359858

3 files changed, 55 insertions, 64 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java
index 1c9364dc5c..c70d0cc481 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java
@@ -146,7 +146,6 @@ public final class JavaConfiguration extends Fragment {
   private final Label javaLauncherLabel;
   private final boolean useIjars;
   private final boolean useHeaderCompilation;
-  private final boolean headerCompilationDirectClasspath;
   private final boolean headerCompilationDisableJavacFallback;
   private final boolean generateJavaDeps;
   private final boolean strictDepsJavaProtos;
@@ -179,7 +178,6 @@ public final class JavaConfiguration extends Fragment {
     this.javaLauncherLabel = javaOptions.javaLauncher;
     this.useIjars = javaOptions.useIjars;
     this.useHeaderCompilation = javaOptions.headerCompilation;
-    this.headerCompilationDirectClasspath = javaOptions.headerCompilationDirectClasspath;
     this.headerCompilationDisableJavacFallback = javaOptions.headerCompilationDisableJavacFallback;
     this.generateJavaDeps = generateJavaDeps;
     this.javaClasspath = javaOptions.javaClasspath;
@@ -261,11 +259,6 @@ public final class JavaConfiguration extends Fragment {
     return useHeaderCompilation;
   }
 
-  /** Returns true if header compilations should use direct dependencies only. */
-  public boolean headerCompilationDirectClasspath() {
-    return headerCompilationDirectClasspath;
-  }
-
   /**
    * If --java_header_compilation is set, report diagnostics from turbine instead of falling back to
    * javac. Diagnostics will be produced more quickly, but may be less helpful.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java
index 02c072002e..371f0e1610 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java
@@ -14,10 +14,12 @@
 
 package com.google.devtools.build.lib.rules.java;
 
+import static com.google.common.base.Preconditions.checkState;
 import static com.google.devtools.build.lib.util.Preconditions.checkNotNull;
 import static java.nio.charset.StandardCharsets.ISO_8859_1;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -342,41 +344,42 @@ public class JavaHeaderCompileAction extends SpawnAction {
         directJars = NestedSetBuilder.emptySet(Order.NAIVE_LINK_ORDER);
         compileTimeDependencyArtifacts = NestedSetBuilder.emptySet(Order.STABLE_ORDER);
       }
-      boolean useDirectClasspath = useDirectClasspath();
-      boolean disableJavacFallback =
-          ruleContext.getFragment(JavaConfiguration.class).headerCompilationDisableJavacFallback();
-      CommandLine directCommandLine = null;
-      if (useDirectClasspath) {
-        CustomCommandLine.Builder builder =
-            baseCommandLine(getBaseArgs(javaToolchain)).addExecPaths("--classpath", directJars);
-        if (disableJavacFallback) {
-          builder.add("--nojavac_fallback");
-        }
-        directCommandLine = builder.build();
-      }
+
+      // The compilation uses API-generating annotation processors and has to fall back to
+      // javac-turbine.
+      boolean requiresAnnotationProcessing = !processorNames.isEmpty();
+
       Iterable<Artifact> tools = ImmutableList.of(javacJar, javaToolchain.getHeaderCompiler());
       ImmutableList<Artifact> outputs = ImmutableList.of(outputJar, outputDepsProto);
-      NestedSet<Artifact> directInputs =
+      NestedSet<Artifact> baseInputs =
           NestedSetBuilder.<Artifact>stableOrder()
               .addTransitive(javabaseInputs)
               .addAll(bootclasspathEntries)
               .addAll(sourceJars)
               .addAll(sourceFiles)
-              .addTransitive(directJars)
               .addAll(tools)
               .build();
 
-      if (useDirectClasspath && disableJavacFallback) {
-        // use a regular SpawnAction to invoke turbine with direct deps only,
-        // and no fallback to javac-turbine
+      boolean noFallback =
+          ruleContext.getFragment(JavaConfiguration.class).headerCompilationDisableJavacFallback();
+      // The action doesn't require annotation processing and either javac-turbine fallback is
+      // disabled, or the action doesn't distinguish between direct and transitive deps, so
+      // use a plain SpawnAction to invoke turbine.
+      if ((noFallback || directJars.isEmpty()) && !requiresAnnotationProcessing) {
+        NestedSet<Artifact> classpath = !directJars.isEmpty() ? directJars : classpathEntries;
+        CustomCommandLine.Builder commandLine =
+            baseCommandLine(getBaseArgs(javaToolchain), classpath);
+        if (noFallback) {
+          commandLine.add("--nojavac_fallback");
+        }
         return new ActionAnalysisMetadata[] {
           new SpawnAction(
               ruleContext.getActionOwner(),
               tools,
-              directInputs,
+              NestedSetBuilder.fromNestedSet(baseInputs).addTransitive(classpath).build(),
               outputs,
               LOCAL_RESOURCES,
-              directCommandLine,
+              commandLine.build(),
               false,
               JavaCompileAction.UTF8_ENVIRONMENT,
               /*executionInfo=*/ ImmutableSet.<String>of(),
@@ -402,15 +405,16 @@ public class JavaHeaderCompileAction extends SpawnAction {
           getBaseArgs(javaToolchain).addPaths("@%s", paramsFile.getExecPath()).build();
       NestedSet<Artifact> transitiveInputs =
           NestedSetBuilder.<Artifact>stableOrder()
-              .addTransitive(directInputs)
+              .addTransitive(baseInputs)
               .addTransitive(classpathEntries)
               .addTransitive(processorPath)
               .addTransitive(compileTimeDependencyArtifacts)
               .add(paramsFile)
               .build();
-      if (!useDirectClasspath) {
-        // If direct classpaths are disabled (e.g. because the compilation uses API-generating
-        // annotation processors) skip the custom action implementation and just use SpawnAction.
+
+      if (requiresAnnotationProcessing) {
+        // turbine doesn't support API-generating annotation processors, so skip the two-tiered
+        // turbine/javac-turbine action and just use SpawnAction to invoke javac-turbine.
         return new ActionAnalysisMetadata[] {
           parameterFileWriteAction,
           new SpawnAction(
@@ -423,10 +427,22 @@ public class JavaHeaderCompileAction extends SpawnAction {
               false,
               JavaCompileAction.UTF8_ENVIRONMENT,
               /*executionInfo=*/ ImmutableSet.<String>of(),
-              getProgressMessage(),
+              getProgressMessageWithAnnotationProcessors(),
               "JavacTurbine")
         };
       }
+
+      // The action doesn't require annotation processing, javac-turbine fallback is enabled, and
+      // the target distinguishes between direct and transitive deps. Try a two-tiered spawn
+      // the invokes turbine with direct deps, and falls back to javac-turbine on failures to
+      // produce better diagnostics. (At the cost of slower failed actions and a larger
+      // cache footprint.)
+      // TODO(cushon): productionize --nojavac_fallback and remove this path
+      checkState(!directJars.isEmpty());
+      NestedSet<Artifact> directInputs =
+          NestedSetBuilder.fromNestedSet(baseInputs).addTransitive(directJars).build();
+      CustomCommandLine directCommandLine =
+          baseCommandLine(getBaseArgs(javaToolchain), directJars).build();
       return new ActionAnalysisMetadata[] {
         parameterFileWriteAction,
         new JavaHeaderCompileAction(
@@ -441,6 +457,17 @@ public class JavaHeaderCompileAction extends SpawnAction {
       };
     }
 
+    private String getProgressMessageWithAnnotationProcessors() {
+      List<String> shortNames = new ArrayList<>();
+      for (String name : processorNames) {
+        shortNames.add(name.substring(name.lastIndexOf('.') + 1));
+      }
+      return getProgressMessage()
+          + " and running annotation processors ("
+          + Joiner.on(", ").join(shortNames)
+          + ")";
+    }
+
     private String getProgressMessage() {
       return String.format(
           "Compiling Java headers %s (%d files)",
@@ -460,7 +487,8 @@ public class JavaHeaderCompileAction extends SpawnAction {
      * Adds the command line arguments shared by direct classpath and transitive classpath
      * invocations.
      */
-    private CustomCommandLine.Builder baseCommandLine(CustomCommandLine.Builder result) {
+    private CustomCommandLine.Builder baseCommandLine(
+        CustomCommandLine.Builder result, NestedSet<Artifact> classpathEntries) {
       result.addExecPath("--output", outputJar);
 
       if (outputDepsProto != null) {
@@ -494,13 +522,14 @@ public class JavaHeaderCompileAction extends SpawnAction {
           result.add("@" + targetLabel);
         }
       }
+      result.addExecPaths("--classpath", classpathEntries);
       return result;
     }
 
     /** Builds a transitive classpath command line. */
     private CommandLine transitiveCommandLine() {
       CustomCommandLine.Builder result = CustomCommandLine.builder();
-      baseCommandLine(result);
+      baseCommandLine(result, classpathEntries);
       if (!processorNames.isEmpty()) {
         result.add("--processors", processorNames);
       }
@@ -516,28 +545,7 @@ public class JavaHeaderCompileAction extends SpawnAction {
           result.addExecPaths("--deps_artifacts", compileTimeDependencyArtifacts);
         }
       }
-      result.addExecPaths("--classpath", classpathEntries);
       return result.build();
     }
-
-    /** Returns true if the header compilation classpath should only include direct deps. */
-    boolean useDirectClasspath() {
-      if (directJars.isEmpty() && !classpathEntries.isEmpty()) {
-        // the compilation doesn't distinguish direct deps, e.g. because it doesn't support strict
-        // java deps
-        return false;
-      }
-      if (!processorNames.isEmpty()) {
-        // the compilation uses API-generating annotation processors and has to fall back to
-        // javac-turbine, which doesn't support direct classpaths
-        return false;
-      }
-      JavaConfiguration javaConfiguration = ruleContext.getFragment(JavaConfiguration.class);
-      if (!javaConfiguration.headerCompilationDirectClasspath()) {
-        // the experiment is disabled
-        return false;
-      }
-      return true;
-    }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java
index 55f83e5395..f5764e6a18 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaOptions.java
@@ -417,15 +417,6 @@ public class JavaOptions extends FragmentOptions {
   public boolean strictDepsJavaProtos;
 
   @Option(
-    name = "java_header_compilation_direct_classpath",
-    defaultValue = "true",
-    optionUsageRestrictions = OptionUsageRestrictions.UNDOCUMENTED,
-    help = "Experimental option to limit the header compilation classpath to direct deps.",
-    oldName = "experimental_java_header_compilation_direct_classpath"
-  )
-  public boolean headerCompilationDirectClasspath;
-
-  @Option(
     name = "experimental_java_header_compilation_disable_javac_fallback",
     defaultValue = "false",
     optionUsageRestrictions = OptionUsageRestrictions.UNDOCUMENTED,
@@ -463,7 +454,6 @@ public class JavaOptions extends FragmentOptions {
     // incremental build performance is important.
     host.useIjars = useIjars;
     host.headerCompilation = headerCompilation;
-    host.headerCompilationDirectClasspath = headerCompilationDirectClasspath;
     host.headerCompilationDisableJavacFallback = headerCompilationDisableJavacFallback;
 
     host.javaDeps = javaDeps;