Roll back https://github.com/bazelbuild/bazel/commit/4929ad79865f8c13ef3b33c827040f4a037e4afe

And use params files for turbine actions with transitive classpaths

For actions where direct deps cannot be used, turbine spawns should always use
params files. The transitive classpath may exceed the command line length
limit.

PiperOrigin-RevId: 159473987

3 files changed, 82 insertions, 80 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 104ef4fcb7..0917a472a8 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,14 +14,16 @@
 
 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;
-import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
+import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionOwner;
@@ -29,6 +31,7 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.BaseSpawn;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.ParameterFile;
+import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
 import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnActionContext;
@@ -319,7 +322,7 @@ public class JavaHeaderCompileAction extends SpawnAction {
       ruleContext.registerAction(buildInternal(javaToolchain));
     }
 
-    private ActionAnalysisMetadata[] buildInternal(JavaToolchainProvider javaToolchain) {
+    private Action[] buildInternal(JavaToolchainProvider javaToolchain) {
       checkNotNull(outputDepsProto, "outputDepsProto must not be null");
       checkNotNull(sourceFiles, "sourceFiles must not be null");
       checkNotNull(sourceJars, "sourceJars must not be null");
@@ -340,47 +343,58 @@ 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
-        return new ActionAnalysisMetadata[] {
-          new SpawnAction(
-              ruleContext.getActionOwner(),
-              tools,
-              directInputs,
-              outputs,
-              LOCAL_RESOURCES,
-              directCommandLine,
-              false,
-              JavaCompileAction.UTF8_ENVIRONMENT,
-              /*executionInfo=*/ ImmutableSet.<String>of(),
-              getProgressMessage(),
-              "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) {
+        SpawnAction.Builder builder = new SpawnAction.Builder();
+        NestedSet<Artifact> classpath;
+        if (!directJars.isEmpty() || classpathEntries.isEmpty()) {
+          classpath = directJars;
+        } else {
+          classpath = classpathEntries;
+          // Transitive classpath actions may exceed the command line length limit.
+          builder.alwaysUseParameterFile(ParameterFileType.SHELL_QUOTED);
+        }
+        CustomCommandLine.Builder commandLine =
+            baseCommandLine(CustomCommandLine.builder(), classpath);
+        if (noFallback) {
+          commandLine.add("--nojavac_fallback");
+        }
+        return builder
+            .addTools(tools)
+            .addTransitiveInputs(baseInputs)
+            .addTransitiveInputs(classpath)
+            .addOutputs(outputs)
+            .setCommandLine(commandLine.build())
+            .setJarExecutable(
+                ruleContext.getHostConfiguration().getFragment(Jvm.class).getJavaExecutable(),
+                javaToolchain.getHeaderCompiler(),
+                ImmutableList.<String>builder()
+                    .add("-Xbootclasspath/p:" + javacJar.getExecPath())
+                    .addAll(javaToolchain.getJvmOptions())
+                    .build())
+            .setMnemonic("Turbine")
+            .setProgressMessage(getProgressMessage())
+            .build(ruleContext);
       }
 
       CommandLine transitiveParams = transitiveCommandLine();
@@ -400,16 +414,17 @@ 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.
-        return new ActionAnalysisMetadata[] {
+
+      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 Action[] {
           parameterFileWriteAction,
           new SpawnAction(
               ruleContext.getActionOwner(),
@@ -421,11 +436,23 @@ public class JavaHeaderCompileAction extends SpawnAction {
               false,
               JavaCompileAction.UTF8_ENVIRONMENT,
               /*executionInfo=*/ ImmutableSet.<String>of(),
-              getProgressMessage(),
+              getProgressMessageWithAnnotationProcessors(),
               "JavacTurbine")
         };
       }
-      return new ActionAnalysisMetadata[] {
+
+      // 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 Action[] {
         parameterFileWriteAction,
         new JavaHeaderCompileAction(
             ruleContext.getActionOwner(),
@@ -439,6 +466,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)",
@@ -458,7 +496,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) {
@@ -492,13 +531,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);
       }
@@ -514,28 +554,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;