Introduce CompileCommandLineBuilder and CompileCommandLineTest

RELNOTES: NONE.
PiperOrigin-RevId: 158703123

4 files changed, 220 insertions, 44 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java
index 3224702bb2..72814e4042 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CompileCommandLine.java
@@ -13,7 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.rules.cpp;
 
-import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Predicate;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
@@ -21,6 +20,7 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.config.PerLabelOptions;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
+import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables;
 import com.google.devtools.build.lib.rules.cpp.CppCompileAction.DotdFile;
 import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.util.Preconditions;
@@ -37,17 +37,17 @@ public final class CompileCommandLine {
   private final Artifact sourceFile;
   private final Artifact outputFile;
   private final Label sourceLabel;
-  private final DotdFile dotdFile;
   private final List<String> copts;
   private final Predicate<String> coptsFilter;
   private final Collection<String> features;
   private final FeatureConfiguration featureConfiguration;
-  @VisibleForTesting public final CcToolchainFeatures.Variables variables;
+  private final CcToolchainFeatures.Variables variables;
   private final String actionName;
   private final CppConfiguration cppConfiguration;
   private final CcToolchainProvider cppProvider;
+  private final DotdFile dotdFile;
 
-  public CompileCommandLine(
+  private CompileCommandLine(
       Artifact sourceFile,
       Artifact outputFile,
       Label sourceLabel,
@@ -208,4 +208,98 @@ public final class CompileCommandLine {
   public List<String> getCopts() {
     return copts;
   }
+
+  public Variables getVariables() {
+    return variables;
+  }
+
+  public static Builder builder(
+      Artifact sourceFile,
+      Artifact outputFile,
+      Label sourceLabel,
+      ImmutableList<String> copts,
+      Predicate<String> coptsFilter,
+      ImmutableList<String> features,
+      String actionName,
+      CppConfiguration cppConfiguration,
+      DotdFile dotdFile,
+      CcToolchainProvider cppProvider) {
+    return new Builder(
+        sourceFile,
+        outputFile,
+        sourceLabel,
+        copts,
+        coptsFilter,
+        features,
+        actionName,
+        cppConfiguration,
+        dotdFile,
+        cppProvider);
+  }
+
+  /** A builder for a {@link CompileCommandLine}. */
+  public static final class Builder {
+    private final Artifact sourceFile;
+    private final Artifact outputFile;
+    private final Label sourceLabel;
+    private final ImmutableList<String> copts;
+    private final Predicate<String> coptsFilter;
+    private final Collection<String> features;
+    private FeatureConfiguration featureConfiguration;
+    private CcToolchainFeatures.Variables variables = Variables.EMPTY;
+    private final String actionName;
+    private final CppConfiguration cppConfiguration;
+    @Nullable private final DotdFile dotdFile;
+    private final CcToolchainProvider ccToolchainProvider;
+
+    public CompileCommandLine build() {
+      return new CompileCommandLine(
+          Preconditions.checkNotNull(sourceFile),
+          Preconditions.checkNotNull(outputFile),
+          Preconditions.checkNotNull(sourceLabel),
+          Preconditions.checkNotNull(copts),
+          Preconditions.checkNotNull(coptsFilter),
+          Preconditions.checkNotNull(features),
+          Preconditions.checkNotNull(featureConfiguration),
+          Preconditions.checkNotNull(cppConfiguration),
+          Preconditions.checkNotNull(variables),
+          Preconditions.checkNotNull(actionName),
+          dotdFile,
+          Preconditions.checkNotNull(ccToolchainProvider));
+    }
+
+    private Builder(
+        Artifact sourceFile,
+        Artifact outputFile,
+        Label sourceLabel,
+        ImmutableList<String> copts,
+        Predicate<String> coptsFilter,
+        Collection<String> features,
+        String actionName,
+        CppConfiguration cppConfiguration,
+        DotdFile dotdFile,
+        CcToolchainProvider ccToolchainProvider) {
+      this.sourceFile = sourceFile;
+      this.outputFile = outputFile;
+      this.sourceLabel = sourceLabel;
+      this.copts = copts;
+      this.coptsFilter = coptsFilter;
+      this.features = features;
+      this.actionName = actionName;
+      this.cppConfiguration = cppConfiguration;
+      this.dotdFile = dotdFile;
+      this.ccToolchainProvider = ccToolchainProvider;
+    }
+
+    /** Sets the feature configuration for this compile action. */
+    public Builder setFeatureConfiguration(FeatureConfiguration featureConfiguration) {
+      this.featureConfiguration = featureConfiguration;
+      return this;
+    }
+
+    public Builder setVariables(Variables variables) {
+      this.variables = variables;
+      return this;
+    }
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
index 50a4ef6669..352dc6bf14 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileAction.java
@@ -344,19 +344,20 @@ public class CppCompileAction extends AbstractAction
     this.useHeaderModules = useHeaderModules;
     this.discoversInputs = shouldScanIncludes || cppSemantics.needsDotdInputPruning();
     this.compileCommandLine =
-        new CompileCommandLine(
-            sourceFile,
-            outputFile,
-            sourceLabel,
-            copts,
-            coptsFilter,
-            features,
-            featureConfiguration,
-            cppConfiguration,
-            variables,
-            actionName,
-            dotdFile,
-            cppProvider);
+        CompileCommandLine.builder(
+                sourceFile,
+                outputFile,
+                sourceLabel,
+                copts,
+                coptsFilter,
+                features,
+                actionName,
+                cppConfiguration,
+                dotdFile,
+                cppProvider)
+            .setFeatureConfiguration(featureConfiguration)
+            .setVariables(variables)
+            .build();
     this.actionContext = actionContext;
     this.lipoScannables = lipoScannables;
     this.actionClassId = actionClassId;
@@ -1363,6 +1364,10 @@ public class CppCompileAction extends AbstractAction
     return message.toString();
   }
 
+  public CompileCommandLine getCompileCommandLine() {
+    return compileCommandLine;
+  }
+
   /**
    * A reference to a .d file. There are two modes:
    * <ol>
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
index 596bd3af2a..c86348a100 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionBuilder.java
@@ -29,6 +29,7 @@ import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
+import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Variables;
 import com.google.devtools.build.lib.rules.cpp.CppCompileAction.DotdFile;
 import com.google.devtools.build.lib.rules.cpp.CppCompileAction.SpecialInputsHandler;
 import com.google.devtools.build.lib.util.FileType;
@@ -54,7 +55,7 @@ public class CppCompileActionBuilder {
   private final BuildConfiguration configuration;
   private final List<String> features = new ArrayList<>();
   private CcToolchainFeatures.FeatureConfiguration featureConfiguration;
-  private CcToolchainFeatures.Variables variables;
+  private CcToolchainFeatures.Variables variables = Variables.EMPTY;
   private Artifact sourceFile;
   private final Label sourceLabel;
   private final NestedSetBuilder<Artifact> mandatoryInputsBuilder;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
index c04501d8ca..e87ab7b666 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppModel.java
@@ -62,6 +62,78 @@ import javax.annotation.Nullable;
  * i.e. calling {@link #createCcCompileActions} will throw an Exception if called twice.
  */
 public final class CppModel {
+
+  /** Name of the build variable for the path to the source file being compiled. */
+  public static final String SOURCE_FILE_VARIABLE_NAME = "source_file";
+
+  /** Name of the build variable for the path to the compilation output file. */
+  public static final String OUTPUT_FILE_VARIABLE_NAME = "output_file";
+
+  /**
+   * Name of the build variable for the path to the compilation output file in case of assembly
+   * source.
+   */
+  public static final String OUTPUT_ASSEMBLY_FILE_VARIABLE_NAME = "output_assembly_file";
+
+  /**
+   * Name of the build variable for the path to the compilation output file in case of preprocessed
+   * source.
+   */
+  public static final String OUTPUT_PREPROCESS_FILE_VARIABLE_NAME = "output_preprocess_file";
+
+  /** Name of the build variable for the path to the output file when output is an object file. */
+  public static final String OUTPUT_OBJECT_FILE_VARIABLE_NAME = "output_object_file";
+
+  /** Name of the build variable for the module file name. */
+  public static final String MODULE_NAME_VARIABLE_NAME = "module_name";
+
+  /** Name of the build variable for the module map file name. */
+  public static final String MODULE_MAP_FILE_VARIABLE_NAME = "module_map_file";
+
+  /** Name of the build variable for the dependent module map file name. */
+  public static final String DEPENDENT_MODULE_MAP_FILES_VARIABLE_NAME =
+      "dependent_module_map_files";
+
+  /** Name of the build variable for the collection of module files. */
+  public static final String MODULE_FILES_VARIABLE_NAME = "module_files";
+
+  /**
+   * Name of the build variable for the collection of include paths.
+   *
+   * @see CppCompilationContext#getIncludeDirs().
+   */
+  public static final String INCLUDE_PATHS_VARIABLE_NAME = "include_paths";
+
+  /**
+   * Name of the build variable for the collection of quote include paths.
+   *
+   * @see CppCompilationContext#getIncludeDirs().
+   */
+  public static final String QUOTE_INCLUDE_PATHS_VARIABLE_NAME = "quote_include_paths";
+
+  /**
+   * Name of the build variable for the collection of system include paths.
+   *
+   * @see CppCompilationContext#getIncludeDirs().
+   */
+  public static final String SYSTEM_INCLUDE_PATHS_VARIABLE_NAME = "system_include_paths";
+
+  /** Name of the build variable for the collection of macros defined for preprocessor. */
+  public static final String PREPROCESSOR_DEFINES_VARIABLE_NAME = "preprocessor_defines";
+
+  /** Name of the build variable present when the output is compiled as position independent. */
+  public static final String PIC_VARIABLE_NAME = "pic";
+
+  /** Name of the build variable for the gcov coverage file path. */
+  public static final String GCOV_GCNO_FILE_VARIABLE_NAME = "gcov_gcno_file";
+
+  /** Name of the build variable for the per object debug info file. */
+  public static final String PER_OBJECT_DEBUG_INFO_FILE_VARIABLE_NAME =
+      "per_object_debug_info_file";
+
+  /** Name of the build variable for the LTO indexing bitcode file. */
+  public static final String LTO_INDEXING_BITCODE_FILE_VARIABLE_NAME = "lto_indexing_bitcode_file";
+
   private final CppSemantics semantics;
   private final RuleContext ruleContext;
   private final BuildConfiguration configuration;
@@ -221,7 +293,7 @@ public final class CppModel {
     this.variablesExtensions.addAll(variablesExtensions);
     return this;
   }
-  
+
   /**
    * Sets the link type used for the link actions. Note that only static links are supported at this
    * time.
@@ -235,7 +307,7 @@ public final class CppModel {
     this.neverLink = neverLink;
     return this;
   }
-  
+
   /**
    * Adds an artifact to the inputs of any link actions created by this CppModel.
    */
@@ -281,7 +353,7 @@ public final class CppModel {
     this.linkedArtifactNameSuffix = suffix;
     return this;
   }
-  
+
   /**
    * @returns whether we want to provide header modules for the current target.
    */
@@ -355,7 +427,7 @@ public final class CppModel {
     }
 
     builder.setFeatureConfiguration(featureConfiguration);
-    
+
     return builder;
   }
 
@@ -380,7 +452,7 @@ public final class CppModel {
       Map<String, String> sourceSpecificBuildVariables) {
     CcToolchainFeatures.Variables.Builder buildVariables =
         new CcToolchainFeatures.Variables.Builder();
-    
+
     // TODO(bazel-team): Pull out string constants for all build variables.
 
     CppCompilationContext builderContext = builder.getContext();
@@ -388,8 +460,8 @@ public final class CppModel {
     Artifact outputFile = builder.getOutputFile();
     String realOutputFilePath;
 
-    buildVariables.addStringVariable("source_file", sourceFile.getExecPathString());
-    buildVariables.addStringVariable("output_file", outputFile.getExecPathString());
+    buildVariables.addStringVariable(SOURCE_FILE_VARIABLE_NAME, sourceFile.getExecPathString());
+    buildVariables.addStringVariable(OUTPUT_FILE_VARIABLE_NAME, outputFile.getExecPathString());
 
     if (builder.getTempOutputFile() != null) {
       realOutputFilePath = builder.getTempOutputFile().getPathString();
@@ -398,13 +470,13 @@ public final class CppModel {
     }
 
     if (FileType.contains(outputFile, CppFileTypes.ASSEMBLER, CppFileTypes.PIC_ASSEMBLER)) {
-      buildVariables.addStringVariable("output_assembly_file", realOutputFilePath);
+      buildVariables.addStringVariable(OUTPUT_ASSEMBLY_FILE_VARIABLE_NAME, realOutputFilePath);
     } else if (FileType.contains(outputFile, CppFileTypes.PREPROCESSED_C,
         CppFileTypes.PREPROCESSED_CPP, CppFileTypes.PIC_PREPROCESSED_C,
         CppFileTypes.PIC_PREPROCESSED_CPP)) {
-      buildVariables.addStringVariable("output_preprocess_file", realOutputFilePath);
+      buildVariables.addStringVariable(OUTPUT_PREPROCESS_FILE_VARIABLE_NAME, realOutputFilePath);
     } else {
-      buildVariables.addStringVariable("output_object_file", realOutputFilePath);
+      buildVariables.addStringVariable(OUTPUT_OBJECT_FILE_VARIABLE_NAME, realOutputFilePath);
     }
 
     DotdFile dotdFile =
@@ -418,26 +490,29 @@ public final class CppModel {
     if (featureConfiguration.isEnabled(CppRuleClasses.MODULE_MAPS) && cppModuleMap != null) {
       // If the feature is enabled and cppModuleMap is null, we are about to fail during analysis
       // in any case, but don't crash.
-      buildVariables.addStringVariable("module_name", cppModuleMap.getName());
+      buildVariables.addStringVariable(MODULE_NAME_VARIABLE_NAME, cppModuleMap.getName());
       buildVariables.addStringVariable(
-          "module_map_file", cppModuleMap.getArtifact().getExecPathString());
+          MODULE_MAP_FILE_VARIABLE_NAME, cppModuleMap.getArtifact().getExecPathString());
       StringSequenceBuilder sequence = new StringSequenceBuilder();
       for (Artifact artifact : builderContext.getDirectModuleMaps()) {
         sequence.addValue(artifact.getExecPathString());
       }
-      buildVariables.addCustomBuiltVariable("dependent_module_map_files", sequence);
+      buildVariables.addCustomBuiltVariable(DEPENDENT_MODULE_MAP_FILES_VARIABLE_NAME, sequence);
     }
     if (featureConfiguration.isEnabled(CppRuleClasses.USE_HEADER_MODULES)) {
       // Module inputs will be set later when the action is executed.
-      buildVariables.addStringSequenceVariable("module_files", ImmutableSet.<String>of());
+      buildVariables.addStringSequenceVariable(
+          MODULE_FILES_VARIABLE_NAME, ImmutableSet.<String>of());
     }
     if (featureConfiguration.isEnabled(CppRuleClasses.INCLUDE_PATHS)) {
       buildVariables.addStringSequenceVariable(
-          "include_paths", getSafePathStrings(builderContext.getIncludeDirs()));
+          INCLUDE_PATHS_VARIABLE_NAME, getSafePathStrings(builderContext.getIncludeDirs()));
       buildVariables.addStringSequenceVariable(
-          "quote_include_paths", getSafePathStrings(builderContext.getQuoteIncludeDirs()));
+          QUOTE_INCLUDE_PATHS_VARIABLE_NAME,
+          getSafePathStrings(builderContext.getQuoteIncludeDirs()));
       buildVariables.addStringSequenceVariable(
-          "system_include_paths", getSafePathStrings(builderContext.getSystemIncludeDirs()));
+          SYSTEM_INCLUDE_PATHS_VARIABLE_NAME,
+          getSafePathStrings(builderContext.getSystemIncludeDirs()));
     }
 
     if (featureConfiguration.isEnabled(CppRuleClasses.PREPROCESSOR_DEFINES)) {
@@ -455,14 +530,14 @@ public final class CppModel {
         defines = builderContext.getDefines();
       }
 
-      buildVariables.addStringSequenceVariable("preprocessor_defines", defines);
+      buildVariables.addStringSequenceVariable(PREPROCESSOR_DEFINES_VARIABLE_NAME, defines);
     }
 
     if (usePic) {
       if (!featureConfiguration.isEnabled(CppRuleClasses.PIC)) {
         ruleContext.ruleError("PIC compilation is requested but the toolchain does not support it");
       }
-      buildVariables.addStringVariable("pic", "");
+      buildVariables.addStringVariable(PIC_VARIABLE_NAME, "");
     }
 
     if (ccRelativeName != null) {
@@ -471,16 +546,17 @@ public final class CppModel {
           featureConfiguration, fdoSupport);
     }
     if (gcnoFile != null) {
-      buildVariables.addStringVariable("gcov_gcno_file", gcnoFile.getExecPathString());
+      buildVariables.addStringVariable(GCOV_GCNO_FILE_VARIABLE_NAME, gcnoFile.getExecPathString());
     }
 
     if (dwoFile != null) {
-      buildVariables.addStringVariable("per_object_debug_info_file", dwoFile.getExecPathString());
+      buildVariables.addStringVariable(
+          PER_OBJECT_DEBUG_INFO_FILE_VARIABLE_NAME, dwoFile.getExecPathString());
     }
 
     if (ltoIndexingFile != null) {
       buildVariables.addStringVariable(
-          "lto_indexing_bitcode_file", ltoIndexingFile.getExecPathString());
+          LTO_INDEXING_BITCODE_FILE_VARIABLE_NAME, ltoIndexingFile.getExecPathString());
     }
 
     buildVariables.addAllStringVariables(ccToolchain.getBuildVariables());
@@ -490,7 +566,7 @@ public final class CppModel {
     for (VariablesExtension extension : variablesExtensions) {
       extension.addVariables(buildVariables);
     }
-    
+
     CcToolchainFeatures.Variables variables = buildVariables.build();
     builder.setVariables(variables);
   }
@@ -500,7 +576,7 @@ public final class CppModel {
     return CppFileTypes.headerDiscoveryRequired(sourceArtifact)
         && !featureConfiguration.isEnabled(CppRuleClasses.PARSE_SHOWINCLUDES);
   }
-  
+
   /**
    * Constructs the C++ compiler actions. It generally creates one action for every specified source
    * file. It takes into account LIPO, fake-ness, coverage, and PIC, in addition to using the
@@ -628,7 +704,7 @@ public final class CppModel {
       return;
     }
     String outputName = semantics.getEffectiveSourcePath(module).getPathString();
-    
+
     // TODO(djasper): Make this less hacky after refactoring how the PIC/noPIC actions are created.
     boolean pic = module.getFilename().contains(".pic.");
 
@@ -1165,7 +1241,7 @@ public final class CppModel {
       sonameLinkopts = ImmutableList.of("-Wl,-soname=" +
           SolibSymlinkAction.getDynamicLibrarySoname(soImpl.getRootRelativePath(), false));
     }
-    
+
     // Should we also link in any libraries that this library depends on?
     // That is required on some systems...
     CppLinkActionBuilder linkActionBuilder =