The link command line API can consume a feature configuration to configure flags and environment variables.

--
MOS_MIGRATED_REVID=122735641

2 files changed, 69 insertions, 25 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 33b1306973..3c88c70546 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
@@ -112,13 +112,14 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo
   /**
    * The name of this action for the purpose of crosstool features/action_configs
    */
-  private static final String ACTION_NAME = "cpp-link";
+  private static final String ACTION_NAME = "c++-link";
   
   private final CppConfiguration cppConfiguration;
   private final LibraryToLink outputLibrary;
   private final LibraryToLink interfaceOutputLibrary;
+  private final Map<String, String> toolchainEnv;
   private final ImmutableSet<String> executionRequirements;
-  
+
   private final LinkCommandLine linkCommandLine;
 
   /** True for cc_fake_binary targets. */
@@ -164,6 +165,7 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo
       boolean isLTOIndexing,
       Iterable<LTOBackendArtifacts> allLTOBackendArtifacts,
       LinkCommandLine linkCommandLine,
+      Map<String, String> toolchainEnv,
       ImmutableSet<String> executionRequirements) {
     super(owner, inputs, outputs);
     this.mandatoryInputs = inputs;
@@ -174,6 +176,7 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo
     this.isLTOIndexing = isLTOIndexing;
     this.allLTOBackendArtifacts = allLTOBackendArtifacts;
     this.linkCommandLine = linkCommandLine;
+    this.toolchainEnv = toolchainEnv;
     this.executionRequirements = executionRequirements;
   }
 
@@ -209,8 +212,12 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo
   }
 
   public ImmutableMap<String, String> getEnvironment() {
+    ImmutableMap.Builder<String, String> result = ImmutableMap.<String, String>builder();
+
+    result.putAll(toolchainEnv);
+
     if (OS.getCurrent() == OS.WINDOWS) {
-      // TODO(bazel-team): Both GCC and clang rely on their execution directories being on
+      // Both GCC and clang rely on their execution directories being on
       // PATH, otherwise they fail to find dependent DLLs (and they fail silently...). On
       // the other hand, Windows documentation says that the directory of the executable
       // is always searched for DLLs first. Not sure what to make of it.
@@ -218,13 +225,15 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo
       // the crosstool file.
       //
       // @see com.google.devtools.build.lib.rules.cpp.CppCompileAction#getEnvironment.
-      return ImmutableMap.of(
+      // TODO(b/28791924): Use the crosstool to provide this value.
+      result.put(
           "PATH",
-          cppConfiguration.getToolPathFragment(CppConfiguration.Tool.GCC).getParentDirectory()
-              .getPathString()
-      );
+          cppConfiguration
+              .getToolPathFragment(CppConfiguration.Tool.GCC)
+              .getParentDirectory()
+              .getPathString());
     }
-    return ImmutableMap.of();
+    return result.build();
   }
 
   /**
@@ -424,7 +433,7 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo
     f.addString(fake ? FAKE_LINK_GUID : LINK_GUID);
     f.addString(getCppConfiguration().getLdExecutable().getPathString());
     f.addStrings(linkCommandLine.arguments());
-    f.addStrings(executionRequirements);
+    f.addStrings(getExecutionInfo().keySet());
 
     // TODO(bazel-team): For correctness, we need to ensure the invariant that all values accessed
     // during the execution phase are also covered by the key. Above, we add the argv to the key,
@@ -540,6 +549,8 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo
     protected final BuildConfiguration configuration;
     private final CppConfiguration cppConfiguration;
     private FeatureConfiguration featureConfiguration;
+    private CcToolchainFeatures.Variables buildVariables =
+        new CcToolchainFeatures.Variables.Builder().build();
 
     // Morally equivalent with {@link Context}, except these are mutable.
     // Keep these in sync with {@link Context}.
@@ -641,6 +652,14 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo
       this.useTestOnlyFlags = linkContext.useTestOnlyFlags;
     }
 
+    /**
+     * Returns the action name for purposes of querying the crosstool.
+     */
+    // TODO(b/28791924): Expand action types to values in Link.LinkTargetType.
+    private String getActionName() {
+      return ACTION_NAME;
+    }
+
     public CppLinkAction.Builder setLinkArtifactFactory(LinkArtifactFactory linkArtifactFactory) {
       this.linkArtifactFactory = linkArtifactFactory;
       return this;
@@ -787,6 +806,7 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo
 
       LinkCommandLine.Builder linkCommandLineBuilder =
           new LinkCommandLine.Builder(configuration, getOwner(), ruleContext)
+              .setActionName(getActionName())
               .setLinkerInputs(linkerInputs)
               .setRuntimeInputs(
                   ImmutableList.copyOf(LinkerInputs.simpleLinkerInputs(runtimeInputs)))
@@ -893,17 +913,26 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo
         analysisEnvironment.registerAction(parameterFileWriteAction);
       }
 
+      
+      // For backwards compatibility, and for tests, 
+      // we permit the link action to be instantiated without a feature configuration.  
+      // In this case, an empty feature configuration is used.
+      if (featureConfiguration == null) {
+        this.featureConfiguration = new FeatureConfiguration();
+      }
+
+      Map<String, String> toolchainEnv =
+          featureConfiguration.getEnvironmentVariables(getActionName(), buildVariables);
+
       // If the crosstool uses action_configs to configure cc compilation, collect execution info
       // from there, otherwise, use no execution info.
       // TODO(b/27903698): Assert that the crosstool has an action_config for this action.
-      ImmutableSet<String> executionRequirements = ImmutableSet.of();
-      if (featureConfiguration != null) {
-        if (featureConfiguration.actionIsConfigured(ACTION_NAME)) {
-          executionRequirements =
-              featureConfiguration.getToolForAction(ACTION_NAME).getExecutionRequirements();
-        }
+      ImmutableSet.Builder<String> executionRequirements = ImmutableSet.<String>builder();
+      if (featureConfiguration.actionIsConfigured(getActionName())) {
+        executionRequirements.addAll(
+            featureConfiguration.getToolForAction(getActionName()).getExecutionRequirements());
       }
-
+    
       return new CppLinkAction(
           getOwner(),
           inputsBuilder.deduplicate().build(),
@@ -915,7 +944,8 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo
           isLTOIndexing,
           allLTOArtifacts,
           linkCommandLine,
-          executionRequirements);
+          toolchainEnv,
+          executionRequirements.build());
     }
 
     /**
@@ -999,6 +1029,14 @@ public final class CppLinkAction extends AbstractAction implements ExecutionInfo
     }
 
     /**
+     * Sets the build variables that will be used to template the crosstool.
+     */
+    public Builder setBuildVariables(CcToolchainFeatures.Variables buildVariables) {
+      this.buildVariables = buildVariables;
+      return this;
+    }
+
+    /**
      * This is the LTO indexing step, rather than the real link.
      *
      * <p>When using this, build() will store allLTOArtifacts as a side-effect so the next build()
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 6aa888369c..2a6d88d664 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
@@ -57,6 +57,7 @@ import javax.annotation.Nullable;
  */
 @Immutable
 public final class LinkCommandLine extends CommandLine {
+  private final String actionName;
   private final BuildConfiguration configuration;
   private final CppConfiguration cppConfiguration;
   private final ActionOwner owner;
@@ -85,14 +86,8 @@ public final class LinkCommandLine extends CommandLine {
   @Nullable private final Artifact paramFile;
   @Nullable private final Artifact interfaceSoBuilder;
 
-
-  /**
-   * A string constant for the c++ link action, used to access the feature
-   * configuration.
-   */
-  public static final String CPP_LINK = "c++-link";
-
   private LinkCommandLine(
+      String actionName,
       BuildConfiguration configuration,
       ActionOwner owner,
       Artifact output,
@@ -140,6 +135,7 @@ public final class LinkCommandLine extends CommandLine {
           "the need whole archive flag must be false for static links");
     }
 
+    this.actionName = actionName;
     this.configuration = Preconditions.checkNotNull(configuration);
     this.cppConfiguration = configuration.getFragment(CppConfiguration.class);
     this.variables = variables;
@@ -691,7 +687,7 @@ public final class LinkCommandLine extends CommandLine {
     argv.addAll(cppConfiguration.getLinkOptions());
     // The feature config can be null for tests.
     if (featureConfiguration != null) {
-      argv.addAll(featureConfiguration.getCommandLine(CPP_LINK, variables));
+      argv.addAll(featureConfiguration.getCommandLine(actionName, variables));
     }
   }
 
@@ -1001,6 +997,7 @@ public final class LinkCommandLine extends CommandLine {
     private final ActionOwner owner;
     @Nullable private final RuleContext ruleContext;
 
+    private String actionName;
     @Nullable private Artifact output;
     @Nullable private Artifact interfaceOutput;
     @Nullable private Artifact symbolCountsOutput;
@@ -1063,6 +1060,7 @@ public final class LinkCommandLine extends CommandLine {
         variables = buildVariables.build();
       }
       return new LinkCommandLine(
+          actionName,
           configuration,
           owner,
           output,
@@ -1090,6 +1088,14 @@ public final class LinkCommandLine extends CommandLine {
     }
 
     /**
+     * Sets the name of the action for the purposes of querying the crosstool.
+     */
+    public Builder setActionName(String actionName) {
+      this.actionName = actionName;
+      return this;
+    }
+
+    /**
      * Sets the toolchain to use for link flags. If this is not called, the toolchain
      * is retrieved from the rule.
      */