Automated g4 rollback of commit 923d7df521f67d031b288180560848bd35e20976.

*** Reason for rollback ***

Breaks dozens of targets in the nightly with Tool Failure errors

*** Original change description ***

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: 159260596

3 files changed, 64 insertions, 55 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 c70d0cc481..1c9364dc5c 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,6 +146,7 @@ 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;
@@ -178,6 +179,7 @@ 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;
@@ -259,6 +261,11 @@ 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 371f0e1610..02c072002e 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,12 +14,10 @@
 
 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;
@@ -344,42 +342,41 @@ public class JavaHeaderCompileAction extends SpawnAction {
         directJars = NestedSetBuilder.emptySet(Order.NAIVE_LINK_ORDER);
         compileTimeDependencyArtifacts = NestedSetBuilder.emptySet(Order.STABLE_ORDER);
       }
-
-      // The compilation uses API-generating annotation processors and has to fall back to
-      // javac-turbine.
-      boolean requiresAnnotationProcessing = !processorNames.isEmpty();
-
+      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();
+      }
       Iterable<Artifact> tools = ImmutableList.of(javacJar, javaToolchain.getHeaderCompiler());
       ImmutableList<Artifact> outputs = ImmutableList.of(outputJar, outputDepsProto);
-      NestedSet<Artifact> baseInputs =
+      NestedSet<Artifact> directInputs =
           NestedSetBuilder.<Artifact>stableOrder()
               .addTransitive(javabaseInputs)
               .addAll(bootclasspathEntries)
               .addAll(sourceJars)
               .addAll(sourceFiles)
+              .addTransitive(directJars)
               .addAll(tools)
               .build();
 
-      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");
-        }
+      if (useDirectClasspath && disableJavacFallback) {
+        // use a regular SpawnAction to invoke turbine with direct deps only,
+        // and no fallback to javac-turbine
         return new ActionAnalysisMetadata[] {
           new SpawnAction(
               ruleContext.getActionOwner(),
               tools,
-              NestedSetBuilder.fromNestedSet(baseInputs).addTransitive(classpath).build(),
+              directInputs,
               outputs,
               LOCAL_RESOURCES,
-              commandLine.build(),
+              directCommandLine,
               false,
               JavaCompileAction.UTF8_ENVIRONMENT,
               /*executionInfo=*/ ImmutableSet.<String>of(),
@@ -405,16 +402,15 @@ public class JavaHeaderCompileAction extends SpawnAction {
           getBaseArgs(javaToolchain).addPaths("@%s", paramsFile.getExecPath()).build();
       NestedSet<Artifact> transitiveInputs =
           NestedSetBuilder.<Artifact>stableOrder()
-              .addTransitive(baseInputs)
+              .addTransitive(directInputs)
               .addTransitive(classpathEntries)
               .addTransitive(processorPath)
               .addTransitive(compileTimeDependencyArtifacts)
               .add(paramsFile)
               .build();
-
-      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.
+      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.
         return new ActionAnalysisMetadata[] {
           parameterFileWriteAction,
           new SpawnAction(
@@ -427,22 +423,10 @@ public class JavaHeaderCompileAction extends SpawnAction {
               false,
               JavaCompileAction.UTF8_ENVIRONMENT,
               /*executionInfo=*/ ImmutableSet.<String>of(),
-              getProgressMessageWithAnnotationProcessors(),
+              getProgressMessage(),
               "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(
@@ -457,17 +441,6 @@ 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)",
@@ -487,8 +460,7 @@ 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, NestedSet<Artifact> classpathEntries) {
+    private CustomCommandLine.Builder baseCommandLine(CustomCommandLine.Builder result) {
       result.addExecPath("--output", outputJar);
 
       if (outputDepsProto != null) {
@@ -522,14 +494,13 @@ 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, classpathEntries);
+      baseCommandLine(result);
       if (!processorNames.isEmpty()) {
         result.add("--processors", processorNames);
       }
@@ -545,7 +516,28 @@ 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 f5764e6a18..55f83e5395 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,6 +417,15 @@ 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,
@@ -454,6 +463,7 @@ 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;