Give RuleContext the ability to add Make Variables

This CL also makes CcToolchain responsible for adding the sysroot to CC_FLAGS.

PiperOrigin-RevId: 154329746

8 files changed, 122 insertions, 47 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationMakeVariableContext.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationMakeVariableContext.java
index aa795bbea2..4d3b3b6be0 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationMakeVariableContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfigurationMakeVariableContext.java
@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.analysis;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.analysis.MakeVariableExpander.ExpansionException;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
@@ -29,27 +30,35 @@ import java.util.Map;
  */
 public class ConfigurationMakeVariableContext implements MakeVariableExpander.Context {
   private final Package pkg;
-  private final Map<String, String> toolchainEnv;
-  private final Map<String, String> commandLineEnv;
-  private final Map<String, String> globalEnv;
+  private final ImmutableMap<String, String> ruleEnv;
+  private final ImmutableMap<String, String> commandLineEnv;
+  private final ImmutableMap<String, String> globalEnv;
   private final String platform;
 
-  public ConfigurationMakeVariableContext(Package pkg, BuildConfiguration configuration) {
-    this(pkg, configuration, ImmutableMap.<String, String>of());
+  // TODO(b/37567440): Remove when Skylark callers can be updated to get this from
+  // CcToolchainProvider.
+  private static final ImmutableList<String> defaultMakeVariableAttributes =
+      ImmutableList.of(":cc_toolchain");
+
+  public ConfigurationMakeVariableContext(
+      RuleContext ruleContext, Package pkg, BuildConfiguration configuration) {
+    this(ruleContext.getMakeVariables(defaultMakeVariableAttributes), pkg, configuration);
   }
 
-  public ConfigurationMakeVariableContext(Package pkg, BuildConfiguration configuration,
-      ImmutableMap<String, String> toolchainEnv) {
+  public ConfigurationMakeVariableContext(
+      ImmutableMap<String, String> ruleMakeVariables,
+      Package pkg,
+      BuildConfiguration configuration) {
     this.pkg = pkg;
-    this.toolchainEnv = toolchainEnv;
-    commandLineEnv = configuration.getCommandLineBuildVariables();
-    globalEnv = configuration.getGlobalMakeEnvironment();
-    platform = configuration.getPlatformName();
+    this.ruleEnv = ruleMakeVariables;
+    this.commandLineEnv = ImmutableMap.copyOf(configuration.getCommandLineBuildVariables());
+    this.globalEnv = ImmutableMap.copyOf(configuration.getGlobalMakeEnvironment());
+    this.platform = configuration.getPlatformName();
   }
 
   @Override
   public String lookupMakeVariable(String var) throws ExpansionException {
-    String value = toolchainEnv.get(var);
+    String value = ruleEnv.get(var);
     if (value == null) {
       value = commandLineEnv.get(var);
     }
@@ -73,6 +82,7 @@ public class ConfigurationMakeVariableContext implements MakeVariableExpander.Co
     map.putAll(pkg.getAllMakeVariables(platform));
     map.putAll(globalEnv);
     map.putAll(commandLineEnv);
+    map.putAll(ruleEnv);
     return SkylarkDict.<String, String>copyOf(null, map);
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index 3ebdd78f76..f47b38d006 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -77,6 +77,7 @@ import com.google.devtools.build.lib.packages.SkylarkProviderIdentifier;
 import com.google.devtools.build.lib.packages.Target;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.rules.AliasProvider;
+import com.google.devtools.build.lib.rules.MakeVariableProvider;
 import com.google.devtools.build.lib.rules.fileset.FilesetProvider;
 import com.google.devtools.build.lib.shell.ShellUtils;
 import com.google.devtools.build.lib.syntax.ClassObject;
@@ -1015,6 +1016,30 @@ public final class RuleContext extends TargetContext
     }
   }
 
+  public ImmutableMap<String, String> getMakeVariables(Iterable<String> attributeNames) {
+    // Using an ImmutableBuilder to complain about duplicate keys. This traversal order of
+    // getPrerequisites isn't well-defined, so this makes sure providers don't seceretly stomp on
+    // each other.
+    ImmutableMap.Builder<String, String> makeVariableBuilder = ImmutableMap.builder();
+    ImmutableSet.Builder<MakeVariableProvider> makeVariableProvidersBuilder =
+        ImmutableSet.builder();
+
+    for (String attributeName : attributeNames) {
+      // TODO(b/37567440): Remove this continue statement.
+      if (!attributes().has(attributeName)) {
+        continue;
+      }
+      makeVariableProvidersBuilder.addAll(
+          getPrerequisites(attributeName, Mode.TARGET, MakeVariableProvider.class));
+    }
+
+    for (MakeVariableProvider makeVariableProvider : makeVariableProvidersBuilder.build()) {
+      makeVariableBuilder.putAll(makeVariableProvider.getMakeVariables());
+    }
+
+    return makeVariableBuilder.build();
+  }
+
   /**
    * Return a context that maps Make variable names (string) to values (string).
    *
@@ -1022,8 +1047,8 @@ public final class RuleContext extends TargetContext
    **/
   public ConfigurationMakeVariableContext getConfigurationMakeVariableContext() {
     if (configurationMakeVariableContext == null) {
-      configurationMakeVariableContext = new ConfigurationMakeVariableContext(
-          getRule().getPackage(), getConfiguration());
+      configurationMakeVariableContext =
+          new ConfigurationMakeVariableContext(this, getRule().getPackage(), getConfiguration());
     }
     return configurationMakeVariableContext;
   }
@@ -1086,8 +1111,8 @@ public final class RuleContext extends TargetContext
    */
   public String expandSingleMakeVariable(String attrName, String expression) {
     try {
-      return MakeVariableExpander.expandSingleVariable(expression,
-          new ConfigurationMakeVariableContext(getRule().getPackage(), getConfiguration()));
+      return MakeVariableExpander.expandSingleVariable(
+          expression, getConfigurationMakeVariableContext());
     } catch (MakeVariableExpander.ExpansionException e) {
       attributeError(attrName, e.getMessage());
       return expression;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/MakeVariableProvider.java b/src/main/java/com/google/devtools/build/lib/rules/MakeVariableProvider.java
index 1c56c20bfb..15d4998c7f 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/MakeVariableProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/MakeVariableProvider.java
@@ -15,34 +15,37 @@
 package com.google.devtools.build.lib.rules;
 
 import com.google.common.collect.ImmutableMap;
-import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
-import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
-import java.util.TreeMap;
+import com.google.devtools.build.lib.packages.ClassObjectConstructor;
+import com.google.devtools.build.lib.packages.NativeClassObjectConstructor;
+import com.google.devtools.build.lib.packages.SkylarkClassObject;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
 
 /** Provides access to make variables from the current fragments. */
+@SkylarkModule(name = "MakeVariables", doc = "Make variables exposed by the current target.")
 @Immutable
-public final class MakeVariableProvider implements TransitiveInfoProvider {
+public final class MakeVariableProvider extends SkylarkClassObject
+    implements TransitiveInfoProvider {
+  public static final String SKYLARK_NAME = "MakeVariableInfo";
+
+  public static final ClassObjectConstructor SKYLARK_CONSTRUCTOR =
+      new NativeClassObjectConstructor(SKYLARK_NAME) {};
+
   private final ImmutableMap<String, String> makeVariables;
 
   public MakeVariableProvider(ImmutableMap<String, String> makeVariables) {
+    super(SKYLARK_CONSTRUCTOR, ImmutableMap.<String, Object>of());
     this.makeVariables = makeVariables;
   }
 
+  @SkylarkCallable(
+    name = "make_variables",
+    doc = "Returns the make variables defined by this target.",
+    structField = true
+  )
   public ImmutableMap<String, String> getMakeVariables() {
     return makeVariables;
   }
-
-  public static ImmutableMap<String, String> getToolchainMakeVariables(
-      RuleContext ruleContext, String attributeName) {
-    // Cannot be an ImmutableMap.Builder because we want to support duplicate keys
-    TreeMap<String, String> result = new TreeMap<>();
-    for (MakeVariableProvider provider :
-        ruleContext.getPrerequisites(attributeName, Mode.TARGET, MakeVariableProvider.class)) {
-      result.putAll(provider.getMakeVariables());
-    }
-
-    return ImmutableMap.copyOf(result);
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java
index 897d77ac01..7ab8097ed9 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleContext.java
@@ -986,9 +986,11 @@ public final class SkylarkRuleContext implements SkylarkValue {
   public String expandMakeVariables(String attributeName, String command,
       final Map<String, String> additionalSubstitutions) throws EvalException {
     checkMutable("expand_make_variables");
-    return ruleContext.expandMakeVariables(attributeName,
-        command, new ConfigurationMakeVariableContext(ruleContext.getRule().getPackage(),
-            ruleContext.getConfiguration()) {
+    return ruleContext.expandMakeVariables(
+        attributeName,
+        command,
+        new ConfigurationMakeVariableContext(
+            ruleContext, ruleContext.getRule().getPackage(), ruleContext.getConfiguration()) {
           @Override
           public String lookupMakeVariable(String name) throws ExpansionException {
             if (additionalSubstitutions.containsKey(name)) {
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java
index c4d3abc469..cc01e6c7f4 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java
@@ -39,6 +39,7 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.packages.License;
+import com.google.devtools.build.lib.rules.MakeVariableProvider;
 import com.google.devtools.build.lib.rules.RuleConfiguredTargetFactory;
 import com.google.devtools.build.lib.rules.cpp.FdoSupport.FdoException;
 import com.google.devtools.build.lib.util.Pair;
@@ -49,6 +50,7 @@ import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyKey;
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
@@ -204,7 +206,7 @@ public class CcToolchain implements RuleConfiguredTargetFactory {
       coverage = crosstool;
     }
 
-    CcToolchainProvider provider =
+    CcToolchainProvider ccProvider =
         new CcToolchainProvider(
             cppConfiguration,
             crosstool,
@@ -231,10 +233,19 @@ public class CcToolchain implements RuleConfiguredTargetFactory {
             ruleContext.getPrerequisiteArtifact("$link_dynamic_library_tool", Mode.HOST),
             getEnvironment(ruleContext),
             cppConfiguration.getBuiltInIncludeDirectories());
+
+    // TODO(kmensah): Remove sysroot from cppConfiguration and calculate it here.
+    PathFragment sysroot = cppConfiguration.getSysroot();
+
+    MakeVariableProvider makeVariableProvider =
+        createMakeVariableProvider(cppConfiguration, sysroot);
+
     RuleConfiguredTargetBuilder builder =
         new RuleConfiguredTargetBuilder(ruleContext)
-            .addProvider(provider)
-            .addNativeDeclaredProvider(provider)
+            .addProvider(ccProvider)
+            .addNativeDeclaredProvider(ccProvider)
+            .addProvider(makeVariableProvider)
+            .addNativeDeclaredProvider(makeVariableProvider)
             .addProvider(fdoSupport.getFdoSupport().createFdoSupportProvider(ruleContext))
             .setFilesToBuild(new NestedSetBuilder<Artifact>(Order.STABLE_ORDER).build())
             .addProvider(RunfilesProvider.simple(Runfiles.EMPTY));
@@ -354,6 +365,22 @@ public class CcToolchain implements RuleConfiguredTargetFactory {
         : NestedSetBuilder.<Artifact>emptySet(Order.STABLE_ORDER);
   }
 
+  private MakeVariableProvider createMakeVariableProvider(
+      CppConfiguration cppConfiguration, PathFragment sysroot) {
+
+    HashMap<String, String> makeVariables =
+        new HashMap<>(cppConfiguration.getAdditionalMakeVariables());
+
+    // Overwrite the CC_FLAGS variable to include sysroot, if it's available.
+    if (sysroot != null) {
+      String sysrootFlag = "--sysroot=" + sysroot;
+      String ccFlags = makeVariables.get("CC_FLAGS");
+      ccFlags = ccFlags.isEmpty() ? sysrootFlag : ccFlags + " " + sysrootFlag;
+      makeVariables.put("CC_FLAGS", ccFlags);
+    }
+    return new MakeVariableProvider(ImmutableMap.copyOf(makeVariables));
+  }
+
   /**
    * Returns a map that should be templated into the crosstool as build variables. Is meant to
    * be overridden by subclasses of CcToolchain.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
index 95efb13dd9..d07ec2aaf8 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java
@@ -718,6 +718,7 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
     for (CrosstoolConfig.MakeVariable variable : toolchain.getMakeVariableList()) {
       makeVariablesBuilder.put(variable.getName(), variable.getValue());
     }
+    // TODO(kmensah): Remove once targets can depend on the cc_toolchain in skylark.
     if (sysrootFlag != null) {
       String ccFlags = makeVariablesBuilder.get("CC_FLAGS");
       ccFlags = ccFlags.isEmpty() ? sysrootFlag : ccFlags + " " + sysrootFlag;
@@ -1655,7 +1656,6 @@ public class CppConfiguration extends BuildConfiguration.Fragment {
    * <p>The returned map must contain an entry for {@code STACK_FRAME_UNLIMITED},
    * though the entry may be an empty string.
    */
-  @VisibleForTesting
   public ImmutableMap<String, String> getAdditionalMakeVariables() {
     return additionalMakeVariables;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraActionFactory.java b/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraActionFactory.java
index ef7062303b..f0bc1c7679 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraActionFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraActionFactory.java
@@ -64,9 +64,11 @@ public final class ExtraActionFactory implements RuleConfiguredTargetFactory {
     command = command.replace("$(OWNER_LABEL_DIGEST)", "$$(OWNER_LABEL_DIGEST)");
     command = command.replace("$(output ", "$$(output ");
     try {
-      command = MakeVariableExpander.expand(
-          command, new ConfigurationMakeVariableContext(
-              context.getTarget().getPackage(), context.getConfiguration()));
+      command =
+          MakeVariableExpander.expand(
+              command,
+              new ConfigurationMakeVariableContext(
+                  context, context.getTarget().getPackage(), context.getConfiguration()));
     } catch (MakeVariableExpander.ExpansionException e) {
       context.ruleError(String.format("Unable to expand make variables: %s",
           e.getMessage()));
diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java
index ca50836a59..49372a9c3a 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBase.java
@@ -39,7 +39,6 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.rules.AliasProvider;
-import com.google.devtools.build.lib.rules.MakeVariableProvider;
 import com.google.devtools.build.lib.rules.RuleConfiguredTargetFactory;
 import com.google.devtools.build.lib.rules.cpp.CppHelper;
 import com.google.devtools.build.lib.rules.java.JavaHelper;
@@ -290,12 +289,17 @@ public abstract class GenRuleBase implements RuleConfiguredTargetFactory {
     private final NestedSet<Artifact> resolvedSrcs;
     private final NestedSet<Artifact> filesToBuild;
 
-    public CommandResolverContext(RuleContext ruleContext, NestedSet<Artifact> resolvedSrcs,
+    private static final ImmutableList<String> makeVariableAttributes =
+        ImmutableList.of(":cc_toolchain", "toolchains");
+
+    public CommandResolverContext(
+        RuleContext ruleContext,
+        NestedSet<Artifact> resolvedSrcs,
         NestedSet<Artifact> filesToBuild) {
       super(
+          ruleContext.getMakeVariables(makeVariableAttributes),
           ruleContext.getRule().getPackage(),
-          ruleContext.getConfiguration(),
-          MakeVariableProvider.getToolchainMakeVariables(ruleContext, "toolchains"));
+          ruleContext.getConfiguration());
       this.ruleContext = ruleContext;
       this.resolvedSrcs = resolvedSrcs;
       this.filesToBuild = filesToBuild;
@@ -344,7 +348,9 @@ public abstract class GenRuleBase implements RuleConfiguredTargetFactory {
         }
       } else if (JDK_MAKE_VARIABLE.matcher("$(" + name + ")").find()) {
         return new ConfigurationMakeVariableContext(
-            ruleContext.getTarget().getPackage(), ruleContext.getHostConfiguration())
+                ruleContext.getMakeVariables(makeVariableAttributes),
+                ruleContext.getTarget().getPackage(),
+                ruleContext.getHostConfiguration())
             .lookupMakeVariable(name);
       } else {
         return super.lookupMakeVariable(name);