Remove genrule's special handling of the "toolchains" attribute.

The general handling should be fine for this.

PiperOrigin-RevId: 201352916

5 files changed, 63 insertions, 69 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 0e2cee9dff..cae9e7a83d 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
@@ -16,9 +16,11 @@ package com.google.devtools.build.lib.analysis;
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Streams;
 import com.google.devtools.build.lib.analysis.MakeVariableSupplier.MapBackedMakeVariableSupplier;
+import com.google.devtools.build.lib.analysis.MakeVariableSupplier.TemplateVariableInfoBackedMakeVariableSupplier;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
+import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.stringtemplate.ExpansionException;
 import com.google.devtools.build.lib.analysis.stringtemplate.TemplateContext;
 import com.google.devtools.build.lib.packages.Package;
@@ -33,6 +35,21 @@ import java.util.Map;
  */
 public class ConfigurationMakeVariableContext implements TemplateContext {
 
+  private static ImmutableList<TemplateVariableInfo> getRuleTemplateVariableProviders(
+      RuleContext ruleContext, Iterable<String> attributeNames) {
+
+    return Streams.stream(attributeNames)
+        // Only process this attribute it if is present in the rule.
+        .filter(attrName -> ruleContext.attributes().has(attrName))
+        // Get the TemplateVariableInfo providers from this attribute.
+        .flatMap(
+            attrName ->
+                Streams.stream(
+                    ruleContext.getPrerequisites(
+                        attrName, Mode.DONT_CHECK, TemplateVariableInfo.PROVIDER)))
+        .collect(ImmutableList.toImmutableList());
+  }
+
   private final ImmutableList<? extends MakeVariableSupplier> allMakeVariableSuppliers;
 
   // TODO(b/37567440): Remove when Skylark callers can be updated to get this from
@@ -43,18 +60,7 @@ public class ConfigurationMakeVariableContext implements TemplateContext {
 
   public ConfigurationMakeVariableContext(
       RuleContext ruleContext, Package pkg, BuildConfiguration configuration) {
-    this(
-        ruleContext.getMakeVariables(DEFAULT_MAKE_VARIABLE_ATTRIBUTES),
-        pkg,
-        configuration,
-        ImmutableList.<MakeVariableSupplier>of());
-  }
-
-  public ConfigurationMakeVariableContext(
-      ImmutableMap<String, String> ruleMakeVariables,
-      Package pkg,
-      BuildConfiguration configuration) {
-    this(ruleMakeVariables, pkg, configuration, ImmutableList.of());
+    this(ruleContext, pkg, configuration, ImmutableList.<MakeVariableSupplier>of());
   }
 
   public ConfigurationMakeVariableContext(
@@ -63,14 +69,14 @@ public class ConfigurationMakeVariableContext implements TemplateContext {
       BuildConfiguration configuration,
       Iterable<? extends MakeVariableSupplier> makeVariableSuppliers) {
     this(
-        ruleContext.getMakeVariables(DEFAULT_MAKE_VARIABLE_ATTRIBUTES),
+        getRuleTemplateVariableProviders(ruleContext, DEFAULT_MAKE_VARIABLE_ATTRIBUTES),
         pkg,
         configuration,
         makeVariableSuppliers);
   }
 
-  public ConfigurationMakeVariableContext(
-      ImmutableMap<String, String> ruleMakeVariables,
+  private ConfigurationMakeVariableContext(
+      ImmutableList<TemplateVariableInfo> ruleTemplateVariableProviders,
       Package pkg,
       BuildConfiguration configuration,
       Iterable<? extends MakeVariableSupplier> extraMakeVariableSuppliers) {
@@ -85,7 +91,7 @@ public class ConfigurationMakeVariableContext implements TemplateContext {
             .addAll(Preconditions.checkNotNull(extraMakeVariableSuppliers))
             .add(new MapBackedMakeVariableSupplier(configuration.getCommandLineBuildVariables()))
             .add(new MapBackedMakeVariableSupplier(pkg.getMakeEnvironment()))
-            .add(new MapBackedMakeVariableSupplier(ruleMakeVariables))
+            .add(new TemplateVariableInfoBackedMakeVariableSupplier(ruleTemplateVariableProviders))
             .add(new MapBackedMakeVariableSupplier(configuration.getGlobalMakeEnvironment()))
             .build();
   }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/MakeVariableSupplier.java b/src/main/java/com/google/devtools/build/lib/analysis/MakeVariableSupplier.java
index 2d9426e441..19f6c70a41 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/MakeVariableSupplier.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/MakeVariableSupplier.java
@@ -15,7 +15,11 @@
 package com.google.devtools.build.lib.analysis;
 
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import java.util.List;
+import java.util.Map;
+import java.util.TreeMap;
 import javax.annotation.Nullable;
 
 /**
@@ -31,6 +35,40 @@ public interface MakeVariableSupplier {
   /** Returns all Make variables that it supplies */
   ImmutableMap<String, String> getAllMakeVariables();
 
+  /**
+   * {@link MakeVariableSupplier} that reads variables from a list of {@link TemplateVariableInfo}
+   * providers. This implementation respects the ordering of providers, with the first listed being
+   * the highest priority.
+   */
+  class TemplateVariableInfoBackedMakeVariableSupplier implements MakeVariableSupplier {
+    private final ImmutableList<TemplateVariableInfo> templateVariableProviders;
+
+    public TemplateVariableInfoBackedMakeVariableSupplier(
+        List<TemplateVariableInfo> templateVariableProviders) {
+      this.templateVariableProviders = ImmutableList.copyOf(templateVariableProviders);
+    }
+
+    @Nullable
+    @Override
+    public String getMakeVariable(String variableName) {
+      for (TemplateVariableInfo templateVariableInfo : templateVariableProviders) {
+        if (templateVariableInfo.getVariables().containsKey(variableName)) {
+          return templateVariableInfo.getVariables().get(variableName);
+        }
+      }
+      return null;
+    }
+
+    @Override
+    public ImmutableMap<String, String> getAllMakeVariables() {
+      Map<String, String> variables = new TreeMap<>();
+      for (TemplateVariableInfo templateVariableInfo : templateVariableProviders) {
+        variables.putAll(templateVariableInfo.getVariables());
+      }
+      return ImmutableMap.copyOf(variables);
+    }
+  }
+
   /** {@link MakeVariableSupplier} that reads variables it supplies from a map. */
   class MapBackedMakeVariableSupplier implements MakeVariableSupplier {
 
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 ecac68f078..d3c2139207 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
@@ -98,11 +98,9 @@ import com.google.devtools.build.lib.util.OrderedSetMultimap;
 import com.google.devtools.build.lib.util.StringUtil;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.PathFragment;
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
@@ -1063,26 +1061,6 @@ public final class RuleContext extends TargetContext
     return new Expander(this, getConfigurationMakeVariableContext());
   }
 
-  public ImmutableMap<String, String> getMakeVariables(Iterable<String> attributeNames) {
-    ArrayList<TemplateVariableInfo> templateVariableInfos = new ArrayList<>();
-
-    for (String attributeName : attributeNames) {
-      // TODO(b/37567440): Remove this continue statement.
-      if (!attributes().has(attributeName)) {
-        continue;
-      }
-      Iterables.addAll(templateVariableInfos, getPrerequisites(
-          attributeName, Mode.DONT_CHECK, TemplateVariableInfo.PROVIDER));
-    }
-
-    LinkedHashMap<String, String> makeVariables = new LinkedHashMap<>();
-    for (TemplateVariableInfo templateVariableInfo : templateVariableInfos) {
-      makeVariables.putAll(templateVariableInfo.getVariables());
-    }
-
-    return ImmutableMap.copyOf(makeVariables);
-  }
-
   /**
    * Returns a cached context that maps Make variable names (string) to values (string) without any
    * extra {@link MakeVariableSupplier}.
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 4d0f60d26d..49c13c605d 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
@@ -35,7 +35,6 @@ import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.Runfiles;
 import com.google.devtools.build.lib.analysis.RunfilesProvider;
 import com.google.devtools.build.lib.analysis.ShToolchain;
-import com.google.devtools.build.lib.analysis.TemplateVariableInfo;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.stringtemplate.ExpansionException;
@@ -283,39 +282,25 @@ public abstract class GenRuleBase implements RuleConfiguredTargetFactory {
     private final RuleContext ruleContext;
     private final NestedSet<Artifact> resolvedSrcs;
     private final NestedSet<Artifact> filesToBuild;
-    private final Iterable<TemplateVariableInfo> toolchains;
 
     public CommandResolverContext(
         RuleContext ruleContext,
         NestedSet<Artifact> resolvedSrcs,
         NestedSet<Artifact> filesToBuild) {
       super(
-          ruleContext.getMakeVariables(ImmutableList.of(":cc_toolchain")),
+          ruleContext,
           ruleContext.getRule().getPackage(),
           ruleContext.getConfiguration(),
           ImmutableList.of(new CcFlagsSupplier(ruleContext)));
       this.ruleContext = ruleContext;
       this.resolvedSrcs = resolvedSrcs;
       this.filesToBuild = filesToBuild;
-      this.toolchains = ruleContext.getPrerequisites(
-          "toolchains", Mode.TARGET, TemplateVariableInfo.PROVIDER);
     }
 
     public RuleContext getRuleContext() {
       return ruleContext;
     }
 
-    private String resolveVariableFromToolchains(String variableName) {
-      for (TemplateVariableInfo info : toolchains) {
-        String result = info.getVariables().get(variableName);
-        if (result != null) {
-          return result;
-        }
-      }
-
-      return null;
-    }
-
     @Override
     public String lookupVariable(String variableName) throws ExpansionException {
       if (variableName.equals("SRCS")) {
@@ -363,15 +348,6 @@ public abstract class GenRuleBase implements RuleConfiguredTargetFactory {
         }
       }
 
-      // Make variables provided by the :cc_toolchain attributes should not be overridden by
-      // those provided by the toolchains attribute.
-      if (!CROSSTOOL_MAKE_VARIABLES.contains(variableName)) {
-        String valueFromToolchains = resolveVariableFromToolchains(variableName);
-        if (valueFromToolchains != null) {
-          return valueFromToolchains;
-        }
-      }
-
       return super.lookupVariable(variableName);
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStubBinary.java b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStubBinary.java
index c6c1661034..19c261fb7e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStubBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/AppleStubBinary.java
@@ -20,7 +20,6 @@ import static com.google.devtools.build.lib.syntax.Type.STRING;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
-import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.analysis.ConfigurationMakeVariableContext;
@@ -57,10 +56,7 @@ public class AppleStubBinary implements RuleConfiguredTargetFactory {
         ImmutableList.of("$(SDKROOT)", "$(PLATFORM_DIR)");
 
     public XcenvBasedPathVariableContext(RuleContext ruleContext, ApplePlatform platform) {
-      super(
-          ImmutableMap.<String, String>of(),
-          ruleContext.getRule().getPackage(),
-          ruleContext.getConfiguration());
+      super(ruleContext, ruleContext.getRule().getPackage(), ruleContext.getConfiguration());
       this.ruleContext = ruleContext;
       this.platform = platform;
     }