Give RuleContext the ability to add make variables. This CL also makes CcToolchain responsible for adding the sysroot to CC_FLAGS. PiperOrigin-RevId: 155171725
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 aa795bb..4d3b3b6 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 @@ */ 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 @@ 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 13ed25c..a6c43d4 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.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; @@ -1017,6 +1018,30 @@ } } + 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). * @@ -1024,8 +1049,8 @@ **/ public ConfigurationMakeVariableContext getConfigurationMakeVariableContext() { if (configurationMakeVariableContext == null) { - configurationMakeVariableContext = new ConfigurationMakeVariableContext( - getRule().getPackage(), getConfiguration()); + configurationMakeVariableContext = + new ConfigurationMakeVariableContext(this, getRule().getPackage(), getConfiguration()); } return configurationMakeVariableContext; } @@ -1088,8 +1113,8 @@ */ 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 1c56c20..15d4998 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 897d77a..7ab8097 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 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 69924f7..6fc4c92 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
@@ -41,6 +41,7 @@ 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; @@ -52,6 +53,7 @@ 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; @@ -282,7 +284,7 @@ coverage = crosstool; } - CcToolchainProvider provider = + CcToolchainProvider ccProvider = new CcToolchainProvider( cppConfiguration, crosstool, @@ -309,10 +311,18 @@ ruleContext.getPrerequisiteArtifact("$link_dynamic_library_tool", Mode.HOST), getEnvironment(ruleContext), cppConfiguration.getBuiltInIncludeDirectories()); + + 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, profileArtifact)) .setFilesToBuild(new NestedSetBuilder<Artifact>(Order.STABLE_ORDER).build()) @@ -433,6 +443,22 @@ : 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 f9ddfba..b911026 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
@@ -727,6 +727,7 @@ 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; @@ -1664,7 +1665,6 @@ * <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 ef70623..f0bc1c7 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 @@ 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 ca50836..49372a9 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.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 @@ 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 @@ } } 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);