Consolidate JavaCompileAction command line flag handling

Construct the command line flags in a single step. This is a follow-up to
commit 58a5a7d5d765979eb06e7515a6935ea5fe34fb3b.

--
PiperOrigin-RevId: 150341925
MOS_MIGRATED_REVID=150341925

2 files changed, 132 insertions, 256 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java
index d3fcf21442..a874203aac 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompilationHelper.java
@@ -173,7 +173,7 @@ public final class JavaCompilationHelper {
     JavaCompileAction.Builder builder = createJavaCompileActionBuilder(semantics);
     builder.setClasspathEntries(attributes.getCompileTimeClassPath());
     builder.addResources(attributes.getResources());
-    builder.addResourceJars(attributes.getResourceJars());
+    builder.setResourceJars(attributes.getResourceJars());
     builder.addClasspathResources(attributes.getClassPathResources());
     builder.setBootclasspathEntries(getBootclasspathOrDefault());
     builder.setExtdirInputs(getExtdirInputs());
diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java
index e575ef9be6..dc8a328ac1 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java
@@ -18,6 +18,7 @@ import static com.google.common.collect.Iterables.getOnlyElement;
 import static com.google.devtools.build.lib.packages.Aspect.INJECTING_RULE_KIND_PARAMETER_KEY;
 import static com.google.devtools.build.lib.util.Preconditions.checkNotNull;
 import static java.nio.charset.StandardCharsets.ISO_8859_1;
+import static java.util.Objects.requireNonNull;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Joiner;
@@ -86,7 +87,7 @@ public final class JavaCompileAction extends AbstractAction {
   private static final ResourceSet LOCAL_RESOURCES =
       ResourceSet.createWithRamCpuIo(750 /*MB*/, 0.5 /*CPU*/, 0.0 /*IO*/);
 
-  /** A {@link clientEnvironmentVariables} entry that sets the UTF-8 charset. */
+  /** Environment variable that sets the UTF-8 charset. */
   static final ImmutableMap<String, String> UTF8_ENVIRONMENT =
       ImmutableMap.of("LC_CTYPE", "en_US.UTF-8");
 
@@ -470,187 +471,6 @@ public final class JavaCompileAction extends AbstractAction {
         .setExtension(JavaCompileInfo.javaCompileInfo, info.build());
   }
 
-  /**
-   * Collect common command line arguments together in a single ArgvFragment.
-   *
-   * @param classDirectory the directory in which generated classfiles are placed relative to the
-   *     exec root
-   * @param sourceGenDirectory the directory where source files generated by annotation processors
-   *     should be stored.
-   * @param tempDirectory a directory in which the library builder can store temporary files
-   *     relative to the exec root
-   * @param outputJar output jar
-   * @param compressJar if true compress the output jar
-   * @param outputDepsProto the proto file capturing dependency information
-   * @param processorPath the classpath files where javac should search for annotation processors
-   * @param processorPathDirs the classpath dirs where javac should search for annotation processors
-   * @param processorNames the classes that javac should use as annotation processors
-   * @param messages the message files for translation
-   * @param resources the set of resources to put into the jar
-   * @param classpathResources the set of classpath resources to put into the jar
-   * @param sourceJars the set of jars containing additional source files to compile
-   * @param sourceFiles the set of explicit Java source files to compile
-   * @param javacOpts the compiler options to pass to javac
-   */
-  private static CustomMultiArgv commonJavaBuilderArgs(
-      final JavaSemantics semantics,
-      final PathFragment classDirectory,
-      final PathFragment sourceGenDirectory,
-      final PathFragment tempDirectory,
-      final Artifact outputJar,
-      final Artifact gensrcOutputJar,
-      final Artifact manifestProto,
-      final boolean compressJar,
-      final Artifact outputDepsProto,
-      final List<Artifact> processorPath,
-      final Set<PathFragment> processorPathDirs,
-      final List<String> processorNames,
-      final Collection<Artifact> messages,
-      final Map<PathFragment, Artifact> resources,
-      final NestedSet<Artifact> resourceJars,
-      final Collection<Artifact> classpathResources,
-      final Collection<Artifact> sourceJars,
-      final Collection<Artifact> sourceFiles,
-      final Collection<Artifact> extdirInputs,
-      final Collection<Artifact> bootclasspathEntries,
-      final List<String> javacOpts,
-      final String ruleKind,
-      final Label targetLabel,
-      final String pathSeparator) {
-    return new CustomMultiArgv() {
-      @Override
-      public Iterable<String> argv() {
-        checkNotNull(classDirectory);
-        checkNotNull(tempDirectory);
-        CustomCommandLine.Builder result = CustomCommandLine.builder();
-
-        result.add("--classdir").addPath(classDirectory);
-        result.add("--tempdir").addPath(tempDirectory);
-        if (outputJar != null) {
-          result.addExecPath("--output", outputJar);
-        }
-        if (sourceGenDirectory != null) {
-          result.add("--sourcegendir").addPath(sourceGenDirectory);
-        }
-        if (gensrcOutputJar != null) {
-          result.addExecPath("--generated_sources_output", gensrcOutputJar);
-        }
-        if (manifestProto != null) {
-          result.addExecPath("--output_manifest_proto", manifestProto);
-        }
-        if (compressJar) {
-          result.add("--compress_jar");
-        }
-        if (outputDepsProto != null) {
-          result.addExecPath("--output_deps_proto", outputDepsProto);
-        }
-        if (!extdirInputs.isEmpty()) {
-          result.addJoinExecPaths("--extdir", pathSeparator, extdirInputs);
-        }
-        if (!bootclasspathEntries.isEmpty()) {
-          result.addJoinExecPaths("--bootclasspath", pathSeparator, bootclasspathEntries);
-        }
-        if (!processorPath.isEmpty() || !processorPathDirs.isEmpty()) {
-          ImmutableList.Builder<String> execPathStrings = ImmutableList.<String>builder();
-          execPathStrings.addAll(Artifact.toExecPaths(processorPath));
-          for (PathFragment processorPathDir : processorPathDirs) {
-            execPathStrings.add(processorPathDir.toString());
-          }
-          result.addJoinStrings("--processorpath", pathSeparator, execPathStrings.build());
-        }
-        if (!processorNames.isEmpty()) {
-          result.add("--processors", processorNames);
-        }
-        if (!messages.isEmpty()) {
-          result.add("--messages");
-          for (Artifact message : messages) {
-            addAsResourcePrefixedExecPath(
-                semantics.getDefaultJavaResourcePath(message.getRootRelativePath()),
-                message,
-                result);
-          }
-        }
-        if (!resources.isEmpty()) {
-          result.add("--resources");
-          for (Map.Entry<PathFragment, Artifact> resource : resources.entrySet()) {
-            addAsResourcePrefixedExecPath(resource.getKey(), resource.getValue(), result);
-          }
-        }
-        if (!resourceJars.isEmpty()) {
-          result.addExecPaths("--resource_jars", resourceJars);
-        }
-        if (!classpathResources.isEmpty()) {
-          result.addExecPaths("--classpath_resources", classpathResources);
-        }
-        if (!sourceJars.isEmpty()) {
-          result.addExecPaths("--source_jars", sourceJars);
-        }
-        if (!sourceFiles.isEmpty()) {
-          result.addExecPaths("--sources", sourceFiles);
-        }
-        if (!javacOpts.isEmpty()) {
-          result.add("--javacopts", javacOpts);
-        }
-        if (ruleKind != null) {
-          result.add("--rule_kind");
-          result.add(ruleKind);
-        }
-        if (targetLabel != null) {
-          result.add("--target_label");
-          if (targetLabel.getPackageIdentifier().getRepository().isDefault()
-              || targetLabel.getPackageIdentifier().getRepository().isMain()) {
-            result.add(targetLabel.toString());
-          } else {
-            // @-prefixed strings will be assumed to be filenames and expanded by
-            // {@link JavaLibraryBuildRequest}, so add an extra &at; to escape it.
-            result.add("@" + targetLabel);
-          }
-        }
-        return result.build().arguments();
-      }
-    };
-  }
-
-  /**
-   * Creates an instance.
-   * @param commonJavaBuilderArgs common flag values consumed by JavaBuilder
-   * @param configuration the build configuration, which provides the default options and the path
-   *     to the compiler, etc.
-   * @param classpath the complete classpath, the directory in which generated classfiles are placed
-   */
-  private static CustomCommandLine.Builder javaCompileCommandLine(
-      CustomMultiArgv commonJavaBuilderArgs,
-      final BuildConfiguration configuration,
-      final NestedSet<Artifact> classpath,
-      final NestedSet<Artifact> directJars,
-      BuildConfiguration.StrictDepsMode strictJavaDeps,
-      Collection<Artifact> compileTimeDependencyArtifacts) {
-    CustomCommandLine.Builder result = CustomCommandLine.builder();
-
-    result.add(commonJavaBuilderArgs);
-    if (!classpath.isEmpty()) {
-      result.addJoinExecPaths("--classpath", configuration.getHostPathSeparator(), classpath);
-    }
-
-    // strict_java_deps controls whether the mapping from jars to targets is
-    // written out and whether we try to minimize the compile-time classpath.
-    if (strictJavaDeps != BuildConfiguration.StrictDepsMode.OFF) {
-      result.add("--strict_java_deps");
-      result.add(strictJavaDeps.toString());
-      result.add(new JarsToTargetsArgv(classpath, directJars));
-
-      if (configuration.getFragment(JavaConfiguration.class).getReduceJavaClasspath()
-          == JavaClasspathMode.JAVABUILDER) {
-        result.add("--reduce_classpath");
-
-        if (!compileTimeDependencyArtifacts.isEmpty()) {
-          result.addExecPaths("--deps_artifacts", compileTimeDependencyArtifacts);
-        }
-      }
-    }
-    return result;
-  }
-
   private static void addAsResourcePrefixedExecPath(PathFragment resourcePath,
       Artifact artifact, CustomCommandLine.Builder builder) {
     PathFragment execPath = artifact.getExecPath();
@@ -805,7 +625,7 @@ public final class JavaCompileAction extends AbstractAction {
     private final Collection<Artifact> sourceFiles = new ArrayList<>();
     private final Collection<Artifact> sourceJars = new ArrayList<>();
     private final Map<PathFragment, Artifact> resources = new LinkedHashMap<>();
-    private final NestedSetBuilder<Artifact> resourceJars = NestedSetBuilder.stableOrder();
+    private NestedSet<Artifact> resourceJars = NestedSetBuilder.emptySet(Order.NAIVE_LINK_ORDER);
     private final Collection<Artifact> classpathResources = new ArrayList<>();
     private final Collection<Artifact> translations = new LinkedHashSet<>();
     private BuildConfiguration.StrictDepsMode strictJavaDeps =
@@ -860,36 +680,6 @@ public final class JavaCompileAction extends AbstractAction {
           ruleContext.getConfiguration(), semantics);
     }
 
-    /**
-     * May add extra command line options to the Java compile command line.
-     */
-    private static void buildJavaCommandLine(
-        Collection<Artifact> outputs,
-        BuildConfiguration configuration,
-        CustomCommandLine.Builder result,
-        Label targetLabel) {
-      Artifact metadata = null;
-      for (Artifact artifact : outputs) {
-        if (artifact.getExecPathString().endsWith(".em")) {
-          metadata = artifact;
-          break;
-        }
-      }
-
-      if (metadata == null) {
-        return;
-      }
-
-      result.add("--post_processor");
-      result.addExecPath(JACOCO_INSTRUMENTATION_PROCESSOR, metadata);
-      result.addPath(
-          configuration
-              .getCoverageMetadataDirectory(targetLabel.getPackageIdentifier().getRepository())
-              .getExecPath());
-      result.add("-*Test");
-      result.add("-*TestCase");
-    }
-
     public JavaCompileAction build() {
       // TODO(bazel-team): all the params should be calculated before getting here, and the various
       // aggregation code below should go away.
@@ -918,8 +708,6 @@ public final class JavaCompileAction extends AbstractAction {
             configuration.getBinDirectory(targetLabel.getPackageIdentifier().getRepository()));
       }
 
-      NestedSet<Artifact> resourceJars = this.resourceJars.build();
-
       // ImmutableIterable is safe to use here because we know that none of the components of
       // the Iterable.concat() will change. Without ImmutableIterable, AbstractAction will
       // waste memory by making a preventive copy of the iterable.
@@ -952,44 +740,7 @@ public final class JavaCompileAction extends AbstractAction {
       }
       ImmutableList<Artifact> outputs = outputsBuilder.build();
 
-      CustomMultiArgv commonJavaBuilderArgs =
-          commonJavaBuilderArgs(
-              semantics,
-              classDirectory,
-              sourceGenDirectory,
-              tempDirectory,
-              outputJar,
-              gensrcOutputJar,
-              manifestProtoOutput,
-              compressJar,
-              outputDepsProto,
-              processorPath,
-              processorPathDirs,
-              processorNames,
-              translations,
-              resources,
-              resourceJars,
-              classpathResources,
-              sourceJars,
-              sourceFiles,
-              extdirInputs,
-              bootclasspathEntries,
-              internedJcopts,
-              ruleKind,
-              targetLabel,
-              pathSeparator);
-
-      CustomCommandLine.Builder paramFileContentsBuilder = javaCompileCommandLine(
-          commonJavaBuilderArgs,
-          configuration,
-          classpathEntries,
-          directJars,
-          strictJavaDeps,
-          compileTimeDependencyArtifacts
-      );
-      buildJavaCommandLine(
-          outputs, configuration, paramFileContentsBuilder, targetLabel);
-      CommandLine paramFileContents = paramFileContentsBuilder.build();
+      CustomCommandLine paramFileContents = buildParamFileContents(internedJcopts);
       Action parameterFileWriteAction = new ParameterFileWriteAction(owner, paramFile,
           paramFileContents, ParameterFile.ParameterFileType.UNQUOTED, ISO_8859_1);
       analysisEnvironment.registerAction(parameterFileWriteAction);
@@ -1043,6 +794,131 @@ public final class JavaCompileAction extends AbstractAction {
           resources.size() + classpathResources.size() + translations.size());
     }
 
+    private CustomCommandLine buildParamFileContents(Collection<String> javacOpts) {
+      checkNotNull(classDirectory, "classDirectory should not be null");
+      checkNotNull(tempDirectory, "tempDirectory should not be null");
+
+      final String pathSeparator = configuration.getHostPathSeparator();
+
+      CustomCommandLine.Builder result = CustomCommandLine.builder();
+
+      result.add("--classdir").addPath(classDirectory);
+      result.add("--tempdir").addPath(tempDirectory);
+      if (outputJar != null) {
+        result.addExecPath("--output", outputJar);
+      }
+      if (sourceGenDirectory != null) {
+        result.add("--sourcegendir").addPath(sourceGenDirectory);
+      }
+      if (gensrcOutputJar != null) {
+        result.addExecPath("--generated_sources_output", gensrcOutputJar);
+      }
+      if (manifestProtoOutput != null) {
+        result.addExecPath("--output_manifest_proto", manifestProtoOutput);
+      }
+      if (compressJar) {
+        result.add("--compress_jar");
+      }
+      if (outputDepsProto != null) {
+        result.addExecPath("--output_deps_proto", outputDepsProto);
+      }
+      if (!extdirInputs.isEmpty()) {
+        result.addJoinExecPaths("--extdir", pathSeparator, extdirInputs);
+      }
+      if (!bootclasspathEntries.isEmpty()) {
+        result.addJoinExecPaths(
+            "--bootclasspath", pathSeparator, bootclasspathEntries);
+      }
+      if (!processorPath.isEmpty() || !processorPathDirs.isEmpty()) {
+        ImmutableList.Builder<String> execPathStrings = ImmutableList.<String>builder();
+        execPathStrings.addAll(Artifact.toExecPaths(processorPath));
+        for (PathFragment processorPathDir : processorPathDirs) {
+          execPathStrings.add(processorPathDir.toString());
+        }
+        result.addJoinStrings(
+            "--processorpath", pathSeparator, execPathStrings.build());
+      }
+      if (!processorNames.isEmpty()) {
+        result.add("--processors", processorNames);
+      }
+      if (!translations.isEmpty()) {
+        result.add("--messages");
+        for (Artifact message : translations) {
+          addAsResourcePrefixedExecPath(
+              semantics.getDefaultJavaResourcePath(message.getRootRelativePath()), message, result);
+        }
+      }
+      if (!resources.isEmpty()) {
+        result.add("--resources");
+        for (Map.Entry<PathFragment, Artifact> resource : resources.entrySet()) {
+          addAsResourcePrefixedExecPath(resource.getKey(), resource.getValue(), result);
+        }
+      }
+      if (!resourceJars.isEmpty()) {
+        result.addExecPaths("--resource_jars", resourceJars);
+      }
+      if (!classpathResources.isEmpty()) {
+        result.addExecPaths("--classpath_resources", classpathResources);
+      }
+      if (!sourceJars.isEmpty()) {
+        result.addExecPaths("--source_jars", sourceJars);
+      }
+      if (!sourceFiles.isEmpty()) {
+        result.addExecPaths("--sources", sourceFiles);
+      }
+      if (!javacOpts.isEmpty()) {
+        result.add("--javacopts", javacOpts);
+      }
+      if (ruleKind != null) {
+        result.add("--rule_kind");
+        result.add(ruleKind);
+      }
+      if (targetLabel != null) {
+        result.add("--target_label");
+        if (targetLabel.getPackageIdentifier().getRepository().isDefault()
+            || targetLabel.getPackageIdentifier().getRepository().isMain()) {
+          result.add(targetLabel.toString());
+        } else {
+          // @-prefixed strings will be assumed to be filenames and expanded by
+          // {@link JavaLibraryBuildRequest}, so add an extra &at; to escape it.
+          result.add("@" + targetLabel);
+        }
+      }
+
+      if (!classpathEntries.isEmpty()) {
+        result.addJoinExecPaths(
+            "--classpath", pathSeparator, classpathEntries);
+      }
+
+      // strict_java_deps controls whether the mapping from jars to targets is
+      // written out and whether we try to minimize the compile-time classpath.
+      if (strictJavaDeps != BuildConfiguration.StrictDepsMode.OFF) {
+        result.add("--strict_java_deps");
+        result.add(strictJavaDeps.toString());
+        result.add(new JarsToTargetsArgv(classpathEntries, directJars));
+
+        if (configuration.getFragment(JavaConfiguration.class).getReduceJavaClasspath()
+            == JavaClasspathMode.JAVABUILDER) {
+          result.add("--reduce_classpath");
+
+          if (!compileTimeDependencyArtifacts.isEmpty()) {
+            result.addExecPaths("--deps_artifacts", compileTimeDependencyArtifacts);
+          }
+        }
+      }
+      if (metadata != null) {
+        result.add("--post_processor");
+        result.addExecPath(JACOCO_INSTRUMENTATION_PROCESSOR, metadata);
+        result.addPath(
+            configuration
+                .getCoverageMetadataDirectory(targetLabel.getPackageIdentifier().getRepository())
+                .getExecPath());
+        result.add("-*Test");
+        result.add("-*TestCase");
+      }
+      return result.build();
+    }
+
     public Builder setParameterFile(Artifact paramFile) {
       this.paramFile = paramFile;
       return this;
@@ -1108,8 +984,8 @@ public final class JavaCompileAction extends AbstractAction {
       return this;
     }
 
-    public Builder addResourceJars(NestedSet<Artifact> resourceJars) {
-      this.resourceJars.addTransitive(resourceJars);
+    public Builder setResourceJars(NestedSet<Artifact> resourceJars) {
+      this.resourceJars = requireNonNull(resourceJars);
       return this;
     }