Make Make variables from genrule.toolchains override the usual synthetic host JAVA/JAVABASE attributes.

Also fix a few lint warnings and move a class so that it's closer to where it's actually used.

RELNOTES: None.
PiperOrigin-RevId: 167501208

6 files changed, 113 insertions, 63 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 3392aff327..e4e2c63eac 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
@@ -54,7 +54,7 @@ public class ConfigurationMakeVariableContext implements MakeVariableExpander.Co
       ImmutableMap<String, String> ruleMakeVariables,
       Package pkg,
       BuildConfiguration configuration) {
-    this(ruleMakeVariables, pkg, configuration, ImmutableList.<MakeVariableSupplier>of());
+    this(ruleMakeVariables, pkg, configuration, ImmutableList.of());
   }
 
   public ConfigurationMakeVariableContext(
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 e2b14a62ce..a11f16493e 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
@@ -27,7 +27,7 @@ import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.FileProvider;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
 import com.google.devtools.build.lib.analysis.MakeVariableExpander.ExpansionException;
-import com.google.devtools.build.lib.analysis.MakeVariableSupplier;
+import com.google.devtools.build.lib.analysis.MakeVariableInfo;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetFactory;
@@ -273,19 +273,7 @@ public abstract class GenRuleBase implements RuleConfiguredTargetFactory {
     return ruleContext.expandMakeVariables(
         "cmd",
         command,
-        createCommandResolverContext(ruleContext, resolvedSrcs, filesToBuild));
-  }
-
-  /**
-   * Creates a new {@link CommandResolverContext} instance to use in {@link #resolveCommand}.
-   */
-  protected CommandResolverContext createCommandResolverContext(RuleContext ruleContext,
-      NestedSet<Artifact> resolvedSrcs, NestedSet<Artifact> filesToBuild) {
-    return new CommandResolverContext(
-        ruleContext,
-        resolvedSrcs,
-        filesToBuild,
-        ImmutableList.of(new CcFlagsSupplier(ruleContext)));
+        new CommandResolverContext(ruleContext, resolvedSrcs, filesToBuild));
   }
 
   /**
@@ -297,37 +285,58 @@ public abstract class GenRuleBase implements RuleConfiguredTargetFactory {
     private final RuleContext ruleContext;
     private final NestedSet<Artifact> resolvedSrcs;
     private final NestedSet<Artifact> filesToBuild;
+    private final Iterable<MakeVariableInfo> toolchains;
 
     public CommandResolverContext(
         RuleContext ruleContext,
         NestedSet<Artifact> resolvedSrcs,
-        NestedSet<Artifact> filesToBuild,
-        Iterable<? extends MakeVariableSupplier> makeVariableSuppliers) {
+        NestedSet<Artifact> filesToBuild) {
       super(
-          ruleContext,
+          ruleContext.getMakeVariables(ImmutableList.of(":cc_toolchain")),
           ruleContext.getRule().getPackage(),
           ruleContext.getConfiguration(),
-          makeVariableSuppliers);
+          ImmutableList.of(new CcFlagsSupplier(ruleContext)));
       this.ruleContext = ruleContext;
       this.resolvedSrcs = resolvedSrcs;
       this.filesToBuild = filesToBuild;
+      this.toolchains = ruleContext.getPrerequisites(
+          "toolchains", Mode.TARGET, MakeVariableInfo.PROVIDER);
     }
 
     public RuleContext getRuleContext() {
       return ruleContext;
     }
 
+    private String resolveVariableFromToolchains(String variableName) {
+      for (MakeVariableInfo info : toolchains) {
+        String result = info.getMakeVariables().get(variableName);
+        if (result != null) {
+          return result;
+        }
+      }
+
+      return null;
+    }
+
     @Override
     public String lookupMakeVariable(String variableName) throws ExpansionException {
       if (variableName.equals("SRCS")) {
         return Artifact.joinExecPaths(" ", resolvedSrcs);
-      } else if (variableName.equals("<")) {
+      }
+
+      if (variableName.equals("<")) {
         return expandSingletonArtifact(resolvedSrcs, "$<", "input file");
-      } else if (variableName.equals("OUTS")) {
+      }
+
+      if (variableName.equals("OUTS")) {
         return Artifact.joinExecPaths(" ", filesToBuild);
-      } else if (variableName.equals("@")) {
+      }
+
+      if (variableName.equals("@")) {
         return expandSingletonArtifact(filesToBuild, "$@", "output file");
-      } else if (variableName.equals("@D")) {
+      }
+
+      if (variableName.equals("@D")) {
         // The output directory. If there is only one filename in outs,
         // this expands to the directory containing that file. If there are
         // multiple filenames, this variable instead expands to the
@@ -354,7 +363,14 @@ public abstract class GenRuleBase implements RuleConfiguredTargetFactory {
               ruleContext.getRule().getLabel().getPackageIdentifier().getSourceRoot();
           return dir.getRelative(relPath).getPathString();
         }
-      } else if (JDK_MAKE_VARIABLE.matcher("$(" + variableName + ")").find()) {
+      }
+
+      String valueFromToolchains = resolveVariableFromToolchains(variableName);
+      if (valueFromToolchains != null) {
+        return valueFromToolchains;
+      }
+
+      if (JDK_MAKE_VARIABLE.matcher("$(" + variableName + ")").find()) {
         List<String> attributes = new ArrayList<>();
         attributes.addAll(ConfigurationMakeVariableContext.DEFAULT_MAKE_VARIABLE_ATTRIBUTES);
         attributes.add(":host_jdk");
@@ -363,9 +379,9 @@ public abstract class GenRuleBase implements RuleConfiguredTargetFactory {
                 ruleContext.getTarget().getPackage(),
                 ruleContext.getHostConfiguration())
             .lookupMakeVariable(variableName);
-      } else {
-        return super.lookupMakeVariable(variableName);
       }
+
+      return super.lookupMakeVariable(variableName);
     }
 
     /**
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java
index 03ad2773a7..a17d17d05d 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleConfiguredTargetTest.java
@@ -104,14 +104,24 @@ public class GenRuleConfiguredTargetTest extends BuildViewTestCase {
   @Test
   public void testToolchainMakeVariableExpansion() throws Exception {
     scratch.file("a/BUILD",
-        "genrule(name='gr', srcs=[], outs=['out'], cmd='$(TEST_VARIABLE)', toolchains=[':v'])",
-        "make_variable_tester(name='v')");
+        "genrule(name='gr', srcs=[], outs=['out'], cmd='$(FOO)', toolchains=[':v'])",
+        "make_variable_tester(name='v', variables={'FOO': 'FOOBAR'})");
 
     String cmd = getCommand("//a:gr");
     assertThat(cmd).endsWith("FOOBAR");
   }
 
   @Test
+  public void testToolchainOverridesConfiguration() throws Exception {
+    scratch.file("a/BUILD",
+        "genrule(name='gr', srcs=[], outs=['out'], cmd='JAVABASE=$(JAVABASE)', toolchains=[':v'])",
+        "make_variable_tester(name='v', variables={'JAVABASE': 'REPLACED'})");
+
+    String cmd = getCommand("//a:gr");
+    assertThat(cmd).endsWith("JAVABASE=REPLACED");
+  }
+
+  @Test
   public void testD() throws Exception {
     createFiles();
     ConfiguredTarget z = getConfiguredTarget("//hello:z");
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
index 084b03325c..98ba0c223f 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CcLibraryConfiguredTargetTest.java
@@ -102,9 +102,9 @@ public class CcLibraryConfiguredTargetTest extends BuildViewTestCase {
   @Test
   public void testDefinesAndMakeVariables() throws Exception {
     ConfiguredTarget l = scratchConfiguredTarget("a", "l",
-        "cc_library(name='l', srcs=['l.cc'], defines=['V=$(TEST_VARIABLE)'], toolchains=[':v'])",
-        "make_variable_tester(name='v')");
-    assertThat(l.getProvider(CppCompilationContext.class).getDefines()).contains("V=FOOBAR");
+        "cc_library(name='l', srcs=['l.cc'], defines=['V=$(FOO)'], toolchains=[':v'])",
+        "make_variable_tester(name='v', variables={'FOO': 'BAR'})");
+    assertThat(l.getProvider(CppCompilationContext.class).getDefines()).contains("V=BAR");
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
index 10402c25ca..a18a7d637c 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
@@ -16,6 +16,8 @@ package com.google.devtools.build.lib.skylark;
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
+import static com.google.devtools.build.lib.packages.Attribute.attr;
+import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;
 import static org.junit.Assert.fail;
 
 import com.google.common.collect.ImmutableList;
@@ -23,13 +25,20 @@ import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.ActionsProvider;
+import com.google.devtools.build.lib.analysis.BaseRuleClasses;
+import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
 import com.google.devtools.build.lib.analysis.FileConfiguredTarget;
+import com.google.devtools.build.lib.analysis.RuleDefinition;
+import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
 import com.google.devtools.build.lib.analysis.skylark.SkylarkRuleContext;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.Info;
+import com.google.devtools.build.lib.packages.RuleClass;
+import com.google.devtools.build.lib.packages.RuleClass.Builder;
+import com.google.devtools.build.lib.packages.SkylarkProviderIdentifier;
 import com.google.devtools.build.lib.rules.java.JavaInfo;
 import com.google.devtools.build.lib.rules.java.JavaSourceJarsProvider;
 import com.google.devtools.build.lib.rules.python.PyCommon;
@@ -38,6 +47,9 @@ import com.google.devtools.build.lib.syntax.Runtime;
 import com.google.devtools.build.lib.syntax.SkylarkDict;
 import com.google.devtools.build.lib.syntax.SkylarkList;
 import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
+import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
+import com.google.devtools.build.lib.testutil.UnknownRuleConfiguredTarget;
+import com.google.devtools.build.lib.util.FileTypeSet;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
@@ -53,6 +65,42 @@ import org.junit.runners.JUnit4;
 @RunWith(JUnit4.class)
 public class SkylarkRuleContextTest extends SkylarkTestCase {
 
+  /**
+   * A test rule that exercises the semantics of mandatory providers.
+   */
+  public static final class TestingRuleForMandatoryProviders implements RuleDefinition {
+    @Override
+    public RuleClass build(Builder builder, RuleDefinitionEnvironment env) {
+      return builder
+          .setUndocumented()
+          .add(attr("srcs", LABEL_LIST).allowedFileTypes(FileTypeSet.ANY_FILE))
+          .override(builder.copy("deps").mandatoryProvidersList(
+              ImmutableList.of(
+                  ImmutableList.of(SkylarkProviderIdentifier.forLegacy("a")),
+                  ImmutableList.of(
+                      SkylarkProviderIdentifier.forLegacy("b"),
+                      SkylarkProviderIdentifier.forLegacy("c")))))
+          .build();
+    }
+
+    @Override
+    public Metadata getMetadata() {
+      return RuleDefinition.Metadata.builder()
+          .name("testing_rule_for_mandatory_providers")
+          .ancestors(BaseRuleClasses.RuleBase.class)
+          .factoryClass(UnknownRuleConfiguredTarget.class)
+          .build();
+    }
+  }
+
+  @Override
+  protected ConfiguredRuleClassProvider getRuleClassProvider() {
+    ConfiguredRuleClassProvider.Builder builder = new ConfiguredRuleClassProvider.Builder()
+        .addRuleDefinition(new TestingRuleForMandatoryProviders());
+    TestRuleClassProvider.addStandardRules(builder);
+    return builder.build();
+  }
+
   @Before
   public final void generateBuildFile() throws Exception {
     scratch.file(
@@ -2090,5 +2138,4 @@ public class SkylarkRuleContextTest extends SkylarkTestCase {
       }
     }
   }
-
 }
diff --git a/src/test/java/com/google/devtools/build/lib/testutil/TestRuleClassProvider.java b/src/test/java/com/google/devtools/build/lib/testutil/TestRuleClassProvider.java
index 7e5263454e..21661e446a 100644
--- a/src/test/java/com/google/devtools/build/lib/testutil/TestRuleClassProvider.java
+++ b/src/test/java/com/google/devtools/build/lib/testutil/TestRuleClassProvider.java
@@ -20,7 +20,6 @@ import static com.google.devtools.build.lib.packages.BuildType.OUTPUT_LIST;
 import static com.google.devtools.build.lib.syntax.Type.INTEGER;
 import static com.google.devtools.build.lib.syntax.Type.STRING_LIST;
 
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.analysis.BaseRuleClasses;
 import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
@@ -36,9 +35,10 @@ 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.RuleClass;
 import com.google.devtools.build.lib.packages.RuleClass.Builder;
-import com.google.devtools.build.lib.packages.SkylarkProviderIdentifier;
+import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.FileTypeSet;
 import java.lang.reflect.Method;
+import java.util.Map;
 
 /**
  * Helper class to provide a RuleClassProvider for tests.
@@ -72,12 +72,14 @@ public class TestRuleClassProvider {
           new ConfiguredRuleClassProvider.Builder();
       addStandardRules(builder);
       builder.addRuleDefinition(new TestingDummyRule());
-      builder.addRuleDefinition(new TestingRuleForMandatoryProviders());
       ruleProvider = builder.build();
     }
     return ruleProvider;
   }
 
+  /**
+   * A dummy rule with some dummy attributes.
+   */
   public static final class TestingDummyRule implements RuleDefinition {
     @Override
     public RuleClass build(Builder builder, RuleDefinitionEnvironment env) {
@@ -100,31 +102,6 @@ public class TestRuleClassProvider {
     }
   }
 
-  public static final class TestingRuleForMandatoryProviders implements RuleDefinition {
-    @Override
-    public RuleClass build(Builder builder, RuleDefinitionEnvironment env) {
-      return builder
-          .setUndocumented()
-          .add(attr("srcs", LABEL_LIST).allowedFileTypes(FileTypeSet.ANY_FILE))
-          .override(builder.copy("deps").mandatoryProvidersList(
-              ImmutableList.of(
-                ImmutableList.of(SkylarkProviderIdentifier.forLegacy("a")),
-                ImmutableList.of(
-                    SkylarkProviderIdentifier.forLegacy("b"),
-                    SkylarkProviderIdentifier.forLegacy("c")))))
-          .build();
-    }
-
-    @Override
-    public Metadata getMetadata() {
-      return RuleDefinition.Metadata.builder()
-          .name("testing_rule_for_mandatory_providers")
-          .ancestors(BaseRuleClasses.RuleBase.class)
-          .factoryClass(UnknownRuleConfiguredTarget.class)
-          .build();
-    }
-  }
-
   /**
    * Stub rule to test Make variable expansion.
    */
@@ -133,12 +110,11 @@ public class TestRuleClassProvider {
     @Override
     public ConfiguredTarget create(RuleContext ruleContext)
         throws InterruptedException, RuleErrorException {
-      MakeVariableInfo variables = new MakeVariableInfo(ImmutableMap.of(
-          "TEST_VARIABLE", "FOOBAR"));
+      Map<String, String> variables = ruleContext.attributes().get("variables", Type.STRING_DICT);
       return new RuleConfiguredTargetBuilder(ruleContext)
           .setFilesToBuild(NestedSetBuilder.emptySet(Order.STABLE_ORDER))
           .addProvider(RunfilesProvider.EMPTY)
-          .addNativeDeclaredProvider(variables)
+          .addNativeDeclaredProvider(new MakeVariableInfo(ImmutableMap.copyOf(variables)))
           .build();
     }
   }
@@ -151,6 +127,7 @@ public class TestRuleClassProvider {
     public RuleClass build(Builder builder, RuleDefinitionEnvironment environment) {
       return builder
           .advertiseProvider(MakeVariableInfo.class)
+          .add(attr("variables", Type.STRING_DICT))
           .build();
     }