Remove the CppConfiguration member from CppLinkAction. This will make

CppLinkAction more suitable for serialization.

PiperOrigin-RevId: 186598828

3 files changed, 110 insertions, 147 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
index f1336f7fb5..abb041cefc 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkAction.java
@@ -106,7 +106,6 @@ public final class CppLinkAction extends AbstractAction
   private static final String FAKE_LINK_GUID = "da36f819-5a15-43a9-8a45-e01b60e10c8b";
   
   @Nullable private final String mnemonic;
-  private final CppConfiguration cppConfiguration;
   private final LibraryToLink outputLibrary;
   private final Artifact linkOutput;
   private final LibraryToLink interfaceOutputLibrary;
@@ -157,7 +156,6 @@ public final class CppLinkAction extends AbstractAction
       String mnemonic,
       Iterable<Artifact> inputs,
       ImmutableList<Artifact> outputs,
-      CppConfiguration cppConfiguration,
       LibraryToLink outputLibrary,
       Artifact linkOutput,
       LibraryToLink interfaceOutputLibrary,
@@ -177,7 +175,6 @@ public final class CppLinkAction extends AbstractAction
       this.mnemonic = mnemonic;
     }
     this.mandatoryInputs = inputs;
-    this.cppConfiguration = cppConfiguration;
     this.outputLibrary = outputLibrary;
     this.linkOutput = linkOutput;
     this.interfaceOutputLibrary = interfaceOutputLibrary;
@@ -194,10 +191,6 @@ public final class CppLinkAction extends AbstractAction
     this.targetCpu = toolchain.getTargetCpu();
   }
 
-  private CppConfiguration getCppConfiguration() {
-    return cppConfiguration;
-  }
-
   @VisibleForTesting
   public String getTargetCpu() {
     return targetCpu;
@@ -278,7 +271,7 @@ public final class CppLinkAction extends AbstractAction
     }
     return result.build();
   }
-  
+
   @Override
   public List<String> getArguments() {
     return linkCommandLine.arguments();
@@ -478,9 +471,7 @@ public final class CppLinkAction extends AbstractAction
     message.append(getProgressMessage());
     message.append('\n');
     message.append("  Command: ");
-    message.append(
-        ShellEscaper.escapeString(
-            getCppConfiguration().getToolPathFragment(Tool.LD).getPathString()));
+    message.append(ShellEscaper.escapeString(linkCommandLine.getLinkerPathString()));
     message.append('\n');
     // Outputting one argument per line makes it easier to diff the results.
     for (String argument : ShellEscaper.escapeAll(linkCommandLine.arguments())) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
index c95cd590c6..6e652378ae 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java
@@ -18,6 +18,7 @@ import static java.nio.charset.StandardCharsets.ISO_8859_1;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
+import com.google.common.base.Predicates;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -728,6 +729,66 @@ public class CppLinkActionBuilder {
     }
   }
 
+  private ImmutableList<String> getToolchainFlags(
+      ImmutableSet<String> features, List<String> linkopts) {
+    if (Staticness.STATIC.equals(linkType.staticness())) {
+      return ImmutableList.of();
+    }
+    boolean fullyStatic = (linkStaticness == LinkStaticness.FULLY_STATIC);
+    boolean mostlyStatic = (linkStaticness == LinkStaticness.MOSTLY_STATIC);
+    boolean sharedLinkopts =
+        linkType == LinkTargetType.DYNAMIC_LIBRARY
+            || linkopts.contains("-shared")
+            || cppConfiguration.hasSharedLinkOption();
+
+    List<String> result = new ArrayList<>();
+
+    /*
+     * For backwards compatibility, linkopts come _after_ inputFiles.
+     * This is needed to allow linkopts to contain libraries and
+     * positional library-related options such as
+     *    -Wl,--begin-group -lfoo -lbar -Wl,--end-group
+     * or
+     *    -Wl,--as-needed -lfoo -Wl,--no-as-needed
+     *
+     * As for the relative order of the three different flavours of linkopts
+     * (global defaults, per-target linkopts, and command-line linkopts),
+     * we have no idea what the right order should be, or if anyone cares.
+     */
+    result.addAll(linkopts);
+    // Extra toolchain link options based on the output's link staticness.
+    if (fullyStatic) {
+      result.addAll(
+          CppHelper.getFullyStaticLinkOptions(
+              cppConfiguration, toolchain, features, sharedLinkopts));
+    } else if (mostlyStatic) {
+      result.addAll(
+          CppHelper.getMostlyStaticLinkOptions(
+              cppConfiguration, toolchain, features, sharedLinkopts));
+    } else {
+      result.addAll(
+          CppHelper.getDynamicLinkOptions(cppConfiguration, toolchain, features, sharedLinkopts));
+    }
+
+    // Extra test-specific link options.
+    if (useTestOnlyFlags) {
+      result.addAll(toolchain.getTestOnlyLinkOptions());
+    }
+
+    result.addAll(toolchain.getLinkOptions());
+
+    // -pie is not compatible with shared and should be
+    // removed when the latter is part of the link command. Should we need to further
+    // distinguish between shared libraries and executables, we could add additional
+    // command line / CROSSTOOL flags that distinguish them. But as long as this is
+    // the only relevant use case we're just special-casing it here.
+    if (linkType == LinkTargetType.DYNAMIC_LIBRARY) {
+      Iterables.removeIf(result, Predicates.equalTo("-pie"));
+    }
+
+    return ImmutableList.copyOf(result);
+  }
+
   /** Builds the Action as configured and returns it. */
   public CppLinkAction build() throws InterruptedException {
     // Executable links do not have library identifiers.
@@ -954,6 +1015,7 @@ public class CppLinkActionBuilder {
     for (VariablesExtension extraVariablesExtension : variablesExtensions) {
       extraVariablesExtension.addVariables(buildVariablesBuilder);
     }
+
     Variables buildVariables = buildVariablesBuilder.build();
 
     Preconditions.checkArgument(
@@ -979,19 +1041,17 @@ public class CppLinkActionBuilder {
     }
 
     LinkCommandLine.Builder linkCommandLineBuilder =
-        new LinkCommandLine.Builder(configuration, ruleContext)
+        new LinkCommandLine.Builder(ruleContext)
             .setLinkerInputs(linkerInputs)
             .setRuntimeInputs(runtimeLinkerInputs)
             .setLinkTargetType(linkType)
             .setLinkStaticness(linkStaticness)
-            .setFeatures(features)
             .setRuntimeSolibDir(linkType.staticness() == Staticness.STATIC ? null : runtimeSolibDir)
             .setNativeDeps(isNativeDeps)
             .setUseTestOnlyFlags(useTestOnlyFlags)
             .setParamFile(paramFile)
-            .setToolchain(toolchain)
-            .setBuildVariables(buildVariables)
-            .setFeatureConfiguration(featureConfiguration);
+            .setFeatureConfiguration(featureConfiguration)
+            .setCrosstoolTopPathFragment(cppConfiguration.getCrosstoolTopPathFragment());
 
     // TODO(b/62693279): Cleanup once internal crosstools specify ifso building correctly.
     if (shouldUseLinkDynamicLibraryTool()) {
@@ -999,18 +1059,32 @@ public class CppLinkActionBuilder {
           toolchain.getLinkDynamicLibraryTool().getExecPathString());
     }
 
+    ImmutableList<String> linkoptsForVariables;
     if (!isLtoIndexing) {
-      linkCommandLineBuilder
-          .setBuildInfoHeaderArtifacts(buildInfoHeaderArtifacts)
-          .setLinkopts(ImmutableList.copyOf(linkopts));
+      linkoptsForVariables = ImmutableList.copyOf(linkopts);
+      linkCommandLineBuilder.setBuildInfoHeaderArtifacts(buildInfoHeaderArtifacts);
+
     } else {
       List<String> opts = new ArrayList<>(linkopts);
       opts.addAll(
           featureConfiguration.getCommandLine("lto-indexing", buildVariables, null /* expander */));
       opts.addAll(cppConfiguration.getLtoIndexOptions());
-      linkCommandLineBuilder.setLinkopts(ImmutableList.copyOf(opts));
+      linkoptsForVariables = ImmutableList.copyOf(opts);
     }
 
+    // For now, silently ignore linkopts if this is a static library
+    linkoptsForVariables =
+        linkType.staticness() == Staticness.STATIC ? ImmutableList.of() : linkoptsForVariables;
+    linkCommandLineBuilder.setLinkopts(linkoptsForVariables);
+
+    Variables patchedVariables =
+        new Variables.Builder(buildVariables)
+            .addStringSequenceVariable(
+                CppLinkActionBuilder.LEGACY_LINK_FLAGS_VARIABLE,
+                getToolchainFlags(features, linkoptsForVariables))
+            .build();
+
+    linkCommandLineBuilder.setBuildVariables(patchedVariables);
     LinkCommandLine linkCommandLine = linkCommandLineBuilder.build();
 
     // Compute the set of inputs - we only need stable order here.
@@ -1158,7 +1232,6 @@ public class CppLinkActionBuilder {
         mnemonic,
         inputsBuilder.deduplicate().build(),
         actionOutputs,
-        cppConfiguration,
         outputLibrary,
         output,
         interfaceOutputLibrary,
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java
index 123c401dc8..3539126e2d 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/LinkCommandLine.java
@@ -16,15 +16,11 @@ package com.google.devtools.build.lib.rules.cpp;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
-import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
 import com.google.devtools.build.lib.actions.CommandLine;
 import com.google.devtools.build.lib.analysis.RuleContext;
-import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.collect.CollectionUtils;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
@@ -46,7 +42,7 @@ import javax.annotation.Nullable;
 public final class LinkCommandLine extends CommandLine {
   private final String actionName;
   private final String forcedToolPath;
-  private final CppConfiguration cppConfiguration;
+  private final PathFragment crosstoolTopPathFragment;
   private final CcToolchainFeatures.Variables variables;
   // The feature config can be null for tests.
   @Nullable private final FeatureConfiguration featureConfiguration;
@@ -56,36 +52,32 @@ public final class LinkCommandLine extends CommandLine {
   private final LinkTargetType linkTargetType;
   private final LinkStaticness linkStaticness;
   private final ImmutableList<String> linkopts;
-  private final ImmutableSet<String> features;
   @Nullable private final PathFragment runtimeSolibDir;
   private final boolean nativeDeps;
   private final boolean useTestOnlyFlags;
-  private final CcToolchainProvider ccProvider;
 
   @Nullable private final Artifact paramFile;
 
   private LinkCommandLine(
       String actionName,
       String forcedToolPath,
-      BuildConfiguration configuration,
+      PathFragment crosstoolTopPathFragment,
       ImmutableList<Artifact> buildInfoHeaderArtifacts,
       Iterable<? extends LinkerInput> linkerInputs,
       Iterable<? extends LinkerInput> runtimeInputs,
       LinkTargetType linkTargetType,
       LinkStaticness linkStaticness,
       ImmutableList<String> linkopts,
-      ImmutableSet<String> features,
       @Nullable PathFragment runtimeSolibDir,
       boolean nativeDeps,
       boolean useTestOnlyFlags,
       @Nullable Artifact paramFile,
       CcToolchainFeatures.Variables variables,
-      @Nullable FeatureConfiguration featureConfiguration,
-      CcToolchainProvider ccProvider) {
+      @Nullable FeatureConfiguration featureConfiguration) {
 
     this.actionName = actionName;
     this.forcedToolPath = forcedToolPath;
-    this.cppConfiguration = configuration.getFragment(CppConfiguration.class);
+    this.crosstoolTopPathFragment = crosstoolTopPathFragment;
     this.variables = variables;
     this.featureConfiguration = featureConfiguration;
     this.buildInfoHeaderArtifacts = Preconditions.checkNotNull(buildInfoHeaderArtifacts);
@@ -93,17 +85,11 @@ public final class LinkCommandLine extends CommandLine {
     this.runtimeInputs = Preconditions.checkNotNull(runtimeInputs);
     this.linkTargetType = Preconditions.checkNotNull(linkTargetType);
     this.linkStaticness = Preconditions.checkNotNull(linkStaticness);
-    // For now, silently ignore linkopts if this is a static library link.
-    this.linkopts =
-        linkTargetType.staticness() == Staticness.STATIC
-            ? ImmutableList.of()
-            : Preconditions.checkNotNull(linkopts);
-    this.features = Preconditions.checkNotNull(features);
+    this.linkopts = linkopts;
     this.runtimeSolibDir = runtimeSolibDir;
     this.nativeDeps = nativeDeps;
     this.useTestOnlyFlags = useTestOnlyFlags;
     this.paramFile = paramFile;
-    this.ccProvider = ccProvider;
   }
 
   @Nullable
@@ -151,6 +137,14 @@ public final class LinkCommandLine extends CommandLine {
     return linkopts;
   }
 
+  /** Returns the path to the linker. */
+  public String getLinkerPathString() {
+    return featureConfiguration
+        .getToolForAction(linkTargetType.getActionName())
+        .getToolPath(crosstoolTopPathFragment)
+        .getPathString();
+  }
+
   /**
    * Returns the location of the C++ runtime solib symlinks. If null, the C++ dynamic runtime
    * libraries either do not exist (because they do not come from the depot) or they are in the
@@ -293,65 +287,6 @@ public final class LinkCommandLine extends CommandLine {
     }
   }
   
-  private ImmutableList<String> getToolchainFlags() {
-    if (Staticness.STATIC.equals(linkTargetType.staticness())) {
-      return ImmutableList.of();
-    }
-    boolean fullyStatic = (linkStaticness == LinkStaticness.FULLY_STATIC);
-    boolean mostlyStatic = (linkStaticness == LinkStaticness.MOSTLY_STATIC);
-    boolean sharedLinkopts =
-        linkTargetType == LinkTargetType.DYNAMIC_LIBRARY
-            || linkopts.contains("-shared")
-            || cppConfiguration.hasSharedLinkOption();
-
-    List<String> toolchainFlags = new ArrayList<>();
-
-    /*
-     * For backwards compatibility, linkopts come _after_ inputFiles.
-     * This is needed to allow linkopts to contain libraries and
-     * positional library-related options such as
-     *    -Wl,--begin-group -lfoo -lbar -Wl,--end-group
-     * or
-     *    -Wl,--as-needed -lfoo -Wl,--no-as-needed
-     *
-     * As for the relative order of the three different flavours of linkopts
-     * (global defaults, per-target linkopts, and command-line linkopts),
-     * we have no idea what the right order should be, or if anyone cares.
-     */
-    toolchainFlags.addAll(linkopts);
-    // Extra toolchain link options based on the output's link staticness.
-    if (fullyStatic) {
-      toolchainFlags.addAll(
-          CppHelper.getFullyStaticLinkOptions(
-              cppConfiguration, ccProvider, features, sharedLinkopts));
-    } else if (mostlyStatic) {
-      toolchainFlags.addAll(
-          CppHelper.getMostlyStaticLinkOptions(
-              cppConfiguration, ccProvider, features, sharedLinkopts));
-    } else {
-      toolchainFlags.addAll(
-          CppHelper.getDynamicLinkOptions(cppConfiguration, ccProvider, features, sharedLinkopts));
-    }
-
-    // Extra test-specific link options.
-    if (useTestOnlyFlags) {
-      toolchainFlags.addAll(ccProvider.getTestOnlyLinkOptions());
-    }
-
-    toolchainFlags.addAll(ccProvider.getLinkOptions());
-
-    // -pie is not compatible with shared and should be
-    // removed when the latter is part of the link command. Should we need to further
-    // distinguish between shared libraries and executables, we could add additional
-    // command line / CROSSTOOL flags that distinguish them. But as long as this is
-    // the only relevant use case we're just special-casing it here.
-    if (linkTargetType == LinkTargetType.DYNAMIC_LIBRARY) {
-      Iterables.removeIf(toolchainFlags, Predicates.equalTo("-pie"));
-    }
-
-    return ImmutableList.copyOf(toolchainFlags);
-  }
-
   /**
    * Returns a raw link command for the given link invocation, including both command and arguments
    * (argv). The version that uses the expander is preferred, but that one can't be used during
@@ -381,17 +316,10 @@ public final class LinkCommandLine extends CommandLine {
       argv.add(
           featureConfiguration
               .getToolForAction(linkTargetType.getActionName())
-              .getToolPath(cppConfiguration.getCrosstoolTopPathFragment())
+              .getToolPath(crosstoolTopPathFragment)
               .getPathString());
     }
-    argv.addAll(
-        featureConfiguration.getCommandLine(
-            actionName,
-            new Variables.Builder(variables)
-                .addStringSequenceVariable(
-                    CppLinkActionBuilder.LEGACY_LINK_FLAGS_VARIABLE, getToolchainFlags())
-                .build(),
-            expander));
+    argv.addAll(featureConfiguration.getCommandLine(actionName, variables, expander));
     return argv;
   }
 
@@ -419,7 +347,6 @@ public final class LinkCommandLine extends CommandLine {
   /** A builder for a {@link LinkCommandLine}. */
   public static final class Builder {
 
-    private final BuildConfiguration configuration;
     private final RuleContext ruleContext;
     private String forcedToolPath;
     private ImmutableList<Artifact> buildInfoHeaderArtifacts = ImmutableList.of();
@@ -428,25 +355,16 @@ public final class LinkCommandLine extends CommandLine {
     @Nullable private LinkTargetType linkTargetType;
     private LinkStaticness linkStaticness = LinkStaticness.FULLY_STATIC;
     private ImmutableList<String> linkopts = ImmutableList.of();
-    private ImmutableSet<String> features = ImmutableSet.of();
     @Nullable private PathFragment runtimeSolibDir;
     private boolean nativeDeps;
     private boolean useTestOnlyFlags;
     @Nullable private Artifact paramFile;
-    private CcToolchainProvider toolchain;
     private Variables variables;
     private FeatureConfiguration featureConfiguration;
-
-    // This interface is needed to support tests that don't create a
-    // ruleContext, in which case the configuration and action owner
-    // cannot be accessed off of the give ruleContext.
-    public Builder(BuildConfiguration configuration, RuleContext ruleContext) {
-      this.configuration = configuration;
-      this.ruleContext = ruleContext;
-    }
+    private PathFragment crosstoolTopPathFragment;
 
     public Builder(RuleContext ruleContext) {
-      this(ruleContext.getConfiguration(), ruleContext);
+      this.ruleContext = ruleContext;
     }
 
     public LinkCommandLine build() {
@@ -457,12 +375,6 @@ public final class LinkCommandLine extends CommandLine {
             "build info headers may only be present on dynamic library or executable links");
       }
 
-      if (toolchain == null) {
-        toolchain =
-            Preconditions.checkNotNull(
-                CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext));
-      }
-
       // The ruleContext can be null for some tests.
       if (ruleContext != null) {
         Preconditions.checkNotNull(featureConfiguration);
@@ -477,30 +389,19 @@ public final class LinkCommandLine extends CommandLine {
       return new LinkCommandLine(
           actionName,
           forcedToolPath,
-          configuration,
+          crosstoolTopPathFragment,
           buildInfoHeaderArtifacts,
           linkerInputs,
           runtimeInputs,
           linkTargetType,
           linkStaticness,
           linkopts,
-          features,
           runtimeSolibDir,
           nativeDeps,
           useTestOnlyFlags,
           paramFile,
           variables,
-          featureConfiguration,
-          toolchain);
-    }
-
-    /**
-     * Sets the toolchain to use for link flags. If this is not called, the toolchain
-     * is retrieved from the rule.
-     */
-    public Builder setToolchain(CcToolchainProvider toolchain) {
-      this.toolchain = toolchain;
-      return this;
+          featureConfiguration);
     }
 
     /** Use given tool path instead of the one from feature configuration */
@@ -575,14 +476,6 @@ public final class LinkCommandLine extends CommandLine {
     }
 
     /**
-     * Sets the features enabled for the rule.
-     */
-    public Builder setFeatures(ImmutableSet<String> features) {
-      this.features = features;
-      return this;
-    }
-
-    /**
      * Whether the resulting library is intended to be used as a native library from another
      * programming language. This influences the rpath. The {@link #build} method throws an
      * exception if this is true for a static link (see {@link LinkTargetType#staticness()}}).
@@ -615,5 +508,11 @@ public final class LinkCommandLine extends CommandLine {
       this.runtimeSolibDir = runtimeSolibDir;
       return this;
     }
+
+    /** Sets the path to the CROSSTOOL, to be used in finding paths to tools (e.g. the linker) */
+    public Builder setCrosstoolTopPathFragment(PathFragment crosstoolTopPathFragment) {
+      this.crosstoolTopPathFragment = crosstoolTopPathFragment;
+      return this;
+    }
   }
 }