From b54c9adbb9a31e5af4b4627e4179407cb54084c0 Mon Sep 17 00:00:00 2001 From: lberki Date: Mon, 4 Sep 2017 17:40:32 +0200 Subject: 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 --- .../analysis/ConfigurationMakeVariableContext.java | 2 +- .../build/lib/rules/genrule/GenRuleBase.java | 66 ++++++++++++++-------- .../rules/genrule/GenRuleConfiguredTargetTest.java | 14 ++++- .../rules/cpp/CcLibraryConfiguredTargetTest.java | 6 +- .../build/lib/skylark/SkylarkRuleContextTest.java | 49 +++++++++++++++- .../build/lib/testutil/TestRuleClassProvider.java | 39 +++---------- 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 ruleMakeVariables, Package pkg, BuildConfiguration configuration) { - this(ruleMakeVariables, pkg, configuration, ImmutableList.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 resolvedSrcs, NestedSet 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 resolvedSrcs; private final NestedSet filesToBuild; + private final Iterable toolchains; public CommandResolverContext( RuleContext ruleContext, NestedSet resolvedSrcs, - NestedSet filesToBuild, - Iterable makeVariableSuppliers) { + NestedSet 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 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,13 +104,23 @@ 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(); 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 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(); } -- cgit v1.2.3