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

--
MOS_MIGRATED_REVID=122735641

4 files changed, 158 insertions, 45 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.
      */
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java
index 8707f46565..4d64a293cb 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcToolchainFeaturesTest.java
@@ -77,7 +77,10 @@ public class CcToolchainFeaturesTest {
     return variables.build();
   }
   
-  private CcToolchainFeatures buildFeatures(String... toolchain) throws Exception {
+  /**
+   * Creates a CcToolchainFeatures from features described in the given toolchain fragment.
+   */
+  public static CcToolchainFeatures buildFeatures(String... toolchain) throws Exception {
     CToolchain.Builder toolchainBuilder = CToolchain.newBuilder();
     TextFormat.merge(Joiner.on("").join(toolchain), toolchainBuilder);      
     return new CcToolchainFeatures(toolchainBuilder.buildPartial());    
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java
index 54bc4919d6..74ba8c565a 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.rules.cpp;
 
+import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
@@ -31,6 +32,7 @@ import com.google.devtools.build.lib.analysis.util.ActionTester.ActionCombinatio
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
+import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
 import com.google.devtools.build.lib.rules.cpp.CppLinkAction.Builder;
 import com.google.devtools.build.lib.rules.cpp.Link.LinkStaticness;
 import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType;
@@ -65,6 +67,54 @@ public class CppLinkActionTest extends BuildViewTestCase {
         }, masterConfig);
   }
 
+  @Test
+  public void testToolchainFeatureFlags() throws Exception {
+    FeatureConfiguration featureConfiguration =
+        CcToolchainFeaturesTest.buildFeatures(
+                "feature {",
+                "   name: 'a'",
+                "   flag_set {",
+                "      action: 'c++-link'",
+                "      flag_group { flag: 'some_flag' }",
+                "   }",
+                "}")
+            .getFeatureConfiguration("a");
+
+    CppLinkAction linkAction =
+        createLinkBuilder(
+                Link.LinkTargetType.EXECUTABLE,
+                "out",
+                ImmutableList.<Artifact>of(),
+                ImmutableList.<LibraryToLink>of(),
+                featureConfiguration)
+            .build();
+    assertThat(linkAction.getArgv()).contains("some_flag");
+  }
+
+  @Test
+  public void testToolchainFeatureEnv() throws Exception {
+     FeatureConfiguration featureConfiguration =
+        CcToolchainFeaturesTest.buildFeatures(
+                "feature {",
+                "   name: 'a'",
+                "   env_set {",
+                "      action: 'c++-link'",
+                "      env_entry { key: 'foo', value: 'bar' }",
+                "   }",
+                "}")
+            .getFeatureConfiguration("a");
+
+    CppLinkAction linkAction =
+        createLinkBuilder(
+                Link.LinkTargetType.EXECUTABLE,
+                "out",
+                ImmutableList.<Artifact>of(),
+                ImmutableList.<LibraryToLink>of(),
+                featureConfiguration)
+            .build();
+    assertThat(linkAction.getEnvironment()).containsEntry("foo", "bar");
+  }
+  
   /**
    * This mainly checks that non-static links don't have identical keys. Many options are only
    * allowed on non-static links, and we test several of them here.
@@ -100,6 +150,8 @@ public class CppLinkActionTest extends BuildViewTestCase {
             builder.setWholeArchive((i & 16) == 0);
             builder.setFake((i & 32) == 0);
             builder.setRuntimeSolibDir((i & 64) == 0 ? null : new PathFragment("so1"));
+            builder.setFeatureConfiguration(new FeatureConfiguration());
+
             return builder.build();
           }
         });
@@ -134,6 +186,7 @@ public class CppLinkActionTest extends BuildViewTestCase {
                 (i & 1) == 0 ? ImmutableList.of(oFile) : ImmutableList.of(oFile2));
             builder.setLinkType(
                 (i & 2) == 0 ? LinkTargetType.STATIC_LIBRARY : LinkTargetType.DYNAMIC_LIBRARY);
+            builder.setFeatureConfiguration(new FeatureConfiguration());
             return builder.build();
           }
         });
@@ -188,11 +241,15 @@ public class CppLinkActionTest extends BuildViewTestCase {
       objects.add(getOutputArtifact("object" + i + ".o"));
     }
 
-    CppLinkAction linkAction = createLinkBuilder(
-        Link.LinkTargetType.EXECUTABLE, "binary2", objects.build(),
-        ImmutableList.<LibraryToLink>of())
-        .setFake(true)
-        .build();
+    CppLinkAction linkAction =
+        createLinkBuilder(
+                Link.LinkTargetType.EXECUTABLE,
+                "binary2",
+                objects.build(),
+                ImmutableList.<LibraryToLink>of(),
+                new FeatureConfiguration())
+            .setFake(true)
+            .build();
 
     // Ensure that minima are enforced.
     ResourceSet resources = linkAction.estimateResourceConsumptionLocal();
@@ -215,22 +272,31 @@ public class CppLinkActionTest extends BuildViewTestCase {
     assertTrue(resources.getIoUsage() == CppLinkAction.MIN_STATIC_LINK_RESOURCES.getIoUsage()
       || resources.getIoUsage() == scaledSet.getIoUsage());
   }
-  private Builder createLinkBuilder(Link.LinkTargetType type, String outputPath,
-      Iterable<Artifact> nonLibraryInputs, ImmutableList<LibraryToLink> libraryInputs)
+
+  private Builder createLinkBuilder(
+      Link.LinkTargetType type,
+      String outputPath,
+      Iterable<Artifact> nonLibraryInputs,
+      ImmutableList<LibraryToLink> libraryInputs,
+      FeatureConfiguration featureConfiguration)
       throws Exception {
     RuleContext ruleContext = createDummyRuleContext();
-    Builder builder = new CppLinkAction.Builder(
-        ruleContext,
-        new Artifact(new PathFragment(outputPath), getTargetConfiguration().getBinDirectory()),
-        ruleContext.getConfiguration(),
-        null)
-        .addNonLibraryInputs(nonLibraryInputs)
-        .addLibraries(NestedSetBuilder.wrap(Order.LINK_ORDER, libraryInputs))
-        .setLinkType(type)
-        .setCrosstoolInputs(NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER))
-        .setLinkStaticness(type.isStaticLibraryLink()
-            ? LinkStaticness.FULLY_STATIC
-            : LinkStaticness.MOSTLY_STATIC);
+    Builder builder =
+        new CppLinkAction.Builder(
+                ruleContext,
+                new Artifact(
+                    new PathFragment(outputPath), getTargetConfiguration().getBinDirectory()),
+                ruleContext.getConfiguration(),
+                null)
+            .addNonLibraryInputs(nonLibraryInputs)
+            .addLibraries(NestedSetBuilder.wrap(Order.LINK_ORDER, libraryInputs))
+            .setLinkType(type)
+            .setCrosstoolInputs(NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER))
+            .setLinkStaticness(
+                type.isStaticLibraryLink()
+                    ? LinkStaticness.FULLY_STATIC
+                    : LinkStaticness.MOSTLY_STATIC)
+            .setFeatureConfiguration(featureConfiguration);
     return builder;
   }