From 1c07a85f03f6cf567443e57701d2ea0f8d58614c Mon Sep 17 00:00:00 2001 From: jcater Date: Tue, 11 Dec 2018 10:55:43 -0800 Subject: [PATCH] Add the new incompatible flag to control genrule's cc toolchain dependency. Part of #6867. Future work will tie this into the actual genrule internals. PiperOrigin-RevId: 225032807 --- .../build/lib/rules/cpp/CcToolchain.java | 7 +++-- .../rules/cpp/CcToolchainProviderHelper.java | 6 ++-- .../build/lib/rules/cpp/CcToolchainSuite.java | 1 - .../build/lib/rules/cpp/CppConfiguration.java | 4 +++ .../build/lib/rules/cpp/CppOptions.java | 17 ++++++++++- .../build/lib/rules/cpp/CppToolchainInfo.java | 16 +++++++--- .../build/lib/rules/genrule/GenRuleBase.java | 26 ++++++++++++----- .../lib/rules/genrule/GenRuleBaseRule.java | 29 +++++++++++++++---- .../genrule/GenRuleConfiguredTargetTest.java | 21 ++++++++++++++ 9 files changed, 103 insertions(+), 24 deletions(-) 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 977321cf502880..70c3ba1cb66636 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 @@ -81,7 +81,6 @@ public ConfiguredTarget create(RuleContext ruleContext) TemplateVariableInfo templateVariableInfo = createMakeVariableProvider( ccToolchainProvider, - ccToolchainProvider.getSysrootPathFragment(), ruleContext.getRule().getLocation()); ruleConfiguredTargetBuilder @@ -94,7 +93,6 @@ public ConfiguredTarget create(RuleContext ruleContext) static TemplateVariableInfo createMakeVariableProvider( CcToolchainProvider toolchainProvider, - PathFragment sysroot, Location location) { HashMap makeVariables = @@ -106,7 +104,10 @@ static TemplateVariableInfo createMakeVariableProvider( makeVariables.putAll(ccProviderMakeVariables.build()); // Overwrite the CC_FLAGS variable to include sysroot, if it's available. - if (sysroot != null) { + // TODO(katre): move this into CcFlagsSupplier. + PathFragment sysroot = toolchainProvider.getSysrootPathFragment(); + if (sysroot != null + && makeVariables.containsKey(CppConfiguration.CC_FLAGS_MAKE_VARIABLE_NAME)) { String sysrootFlag = "--sysroot=" + sysroot; String ccFlags = makeVariables.get(CppConfiguration.CC_FLAGS_MAKE_VARIABLE_NAME); ccFlags = ccFlags.isEmpty() ? sysrootFlag : ccFlags + " " + sysrootFlag; diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProviderHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProviderHelper.java index c3b7ea23239ed6..05fda7b49127c9 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProviderHelper.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProviderHelper.java @@ -693,7 +693,8 @@ private static CppToolchainInfo getCppToolchainInfo( configInfo, cppConfiguration.disableLegacyCrosstoolFields(), cppConfiguration.disableCompilationModeFlags(), - cppConfiguration.disableLinkingModeFlags()); + cppConfiguration.disableLinkingModeFlags(), + cppConfiguration.disableGenruleCcToolchainDependency()); } catch (EvalException e) { throw ruleContext.throwWithRuleError(e.getMessage()); } @@ -723,7 +724,8 @@ private static CppToolchainInfo getCppToolchainInfo( ccToolchainConfigInfo, cppConfiguration.disableLegacyCrosstoolFields(), cppConfiguration.disableCompilationModeFlags(), - cppConfiguration.disableLinkingModeFlags()); + cppConfiguration.disableLinkingModeFlags(), + cppConfiguration.disableGenruleCcToolchainDependency()); } catch (EvalException e) { throw ruleContext.throwWithRuleError(e.getMessage()); } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainSuite.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainSuite.java index f81ac30016534d..3502b3815169cf 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainSuite.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainSuite.java @@ -103,7 +103,6 @@ public ConfiguredTarget create(RuleContext ruleContext) TemplateVariableInfo templateVariableInfo = CcToolchain.createMakeVariableProvider( ccToolchainProvider, - ccToolchainProvider.getSysrootPathFragment(), ruleContext.getRule().getLocation()); RuleConfiguredTargetBuilder builder = 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 1aaa7f926fdd40..bf5646a9d9b69a 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 @@ -589,4 +589,8 @@ boolean enableCcToolchainConfigInfoFromSkylark() { public Label getLibcTopLabel() { return cppOptions.libcTopLabel; } + + public boolean disableGenruleCcToolchainDependency() { + return cppOptions.disableGenruleCcToolchainDependency; + } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java index 58d2793bfcd3f2..0f4c1f93a0d775 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java @@ -787,6 +787,21 @@ public Label getFdoPrefetchHintsLabel() { help = "If enabled, cpu transformer is not used for CppConfiguration") public boolean doNotUseCpuTransformer; + @Option( + name = "incompatible_disable_genrule_cc_toolchain_dependency", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.UNDOCUMENTED, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, + help = + "If true, genrule will no longer automatically depend on the cc toolchain. Specifically, " + + "this means that the CC_FLAGS Make variable will not be available without using " + + "the new cc_flags_supplier rule.") + public boolean disableGenruleCcToolchainDependency; + @Override public FragmentOptions getHost() { CppOptions host = (CppOptions) getDefault(); @@ -828,8 +843,8 @@ public FragmentOptions getHost() { host.inmemoryDotdFiles = inmemoryDotdFiles; host.doNotUseCpuTransformer = doNotUseCpuTransformer; - host.enableCcToolchainConfigInfoFromSkylark = enableCcToolchainConfigInfoFromSkylark; + host.disableGenruleCcToolchainDependency = disableGenruleCcToolchainDependency; return host; } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppToolchainInfo.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppToolchainInfo.java index 69337be30a1813..51fc07dbfd5e7b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppToolchainInfo.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppToolchainInfo.java @@ -114,7 +114,8 @@ public static CppToolchainInfo create( CcToolchainConfigInfo ccToolchainConfigInfo, boolean disableLegacyCrosstoolFields, boolean disableCompilationModeFlags, - boolean disableLinkingModeFlags) + boolean disableLinkingModeFlags, + boolean disableGenruleCcToolchainDependency) throws EvalException { ImmutableMap toolPaths = computeToolPaths(ccToolchainConfigInfo, getToolsDirectory(toolchainLabel)); @@ -225,7 +226,8 @@ public static CppToolchainInfo create( "_solib_" + ccToolchainConfigInfo.getTargetCpu(), ccToolchainConfigInfo.getAbiVersion(), ccToolchainConfigInfo.getTargetSystemName(), - computeAdditionalMakeVariables(ccToolchainConfigInfo), + computeAdditionalMakeVariables( + ccToolchainConfigInfo, disableGenruleCcToolchainDependency), disableLegacyCrosstoolFields ? ImmutableList.of() : ccToolchainConfigInfo.getCompilerFlags(), @@ -743,15 +745,21 @@ public ImmutableList getUnfilteredCompilerOptions(@Nullable PathFragment } private static ImmutableMap computeAdditionalMakeVariables( - CcToolchainConfigInfo ccToolchainConfigInfo) { + CcToolchainConfigInfo ccToolchainConfigInfo, boolean disableGenruleCcToolchainDependency) { Map makeVariablesBuilder = new HashMap<>(); // The following are to be used to allow some build rules to avoid the limits on stack frame - // sizes and variable-length arrays. Ensure that these are always set. + // sizes and variable-length arrays. + // These variables are initialized here, but may be overridden by the getMakeVariables() checks. makeVariablesBuilder.put("STACK_FRAME_UNLIMITED", ""); makeVariablesBuilder.put(CppConfiguration.CC_FLAGS_MAKE_VARIABLE_NAME, ""); for (Pair variable : ccToolchainConfigInfo.getMakeVariables()) { makeVariablesBuilder.put(variable.getFirst(), variable.getSecond()); } + + if (disableGenruleCcToolchainDependency) { + makeVariablesBuilder.remove(CppConfiguration.CC_FLAGS_MAKE_VARIABLE_NAME); + } + return ImmutableMap.copyOf(makeVariablesBuilder); } 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 a08f22e4f9f45c..79e1cf4e9113e6 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 @@ -29,6 +29,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.MakeVariableSupplier; import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder; import com.google.devtools.build.lib.analysis.RuleConfiguredTargetFactory; import com.google.devtools.build.lib.analysis.RuleContext; @@ -148,10 +149,19 @@ public ConfiguredTarget create(RuleContext ruleContext) String baseCommand = ruleContext.attributes().get("cmd", Type.STRING); // Expand template variables and functions. - String command = ruleContext - .getExpander(new CommandResolverContext(ruleContext, resolvedSrcs, filesToBuild)) - .withExecLocations(commandHelper.getLabelMap()) - .expand("cmd", baseCommand); + ImmutableList.Builder makeVariableSuppliers = + new ImmutableList.Builder<>(); + if (GenRuleBaseRule.enableCcToolchain(ruleContext.getConfiguration())) { + makeVariableSuppliers.add(new CcFlagsSupplier(ruleContext)); + } + CommandResolverContext commandResolverContext = + new CommandResolverContext( + ruleContext, resolvedSrcs, filesToBuild, makeVariableSuppliers.build()); + String command = + ruleContext + .getExpander(commandResolverContext) + .withExecLocations(commandHelper.getLabelMap()) + .expand("cmd", baseCommand); // Heuristically expand things that look like labels. if (ruleContext.attributes().get("heuristic_label_expansion", Type.BOOLEAN)) { @@ -203,7 +213,8 @@ public String toString() { ImmutableMap.copyOf(executionInfo)); // TODO(bazel-team): Make the make variable expander pass back a list of these. - if (requiresCrosstool(baseCommand)) { + if (GenRuleBaseRule.enableCcToolchain(ruleContext.getConfiguration()) + && requiresCrosstool(baseCommand)) { // If cc is used, silently throw in the crosstool filegroup as a dependency. inputs.addTransitive( CppHelper.getToolchainUsingDefaultCcToolchainAttribute(ruleContext) @@ -276,12 +287,13 @@ protected static class CommandResolverContext extends ConfigurationMakeVariableC public CommandResolverContext( RuleContext ruleContext, NestedSet resolvedSrcs, - NestedSet filesToBuild) { + NestedSet filesToBuild, + Iterable makeVariableSuppliers) { super( ruleContext, ruleContext.getRule().getPackage(), ruleContext.getConfiguration(), - ImmutableList.of(new CcFlagsSupplier(ruleContext))); + makeVariableSuppliers); this.ruleContext = ruleContext; this.resolvedSrcs = resolvedSrcs; this.filesToBuild = filesToBuild; diff --git a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBaseRule.java b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBaseRule.java index 0b4dcb925d7aa8..c0287c3da63b0c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBaseRule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/genrule/GenRuleBaseRule.java @@ -23,6 +23,7 @@ import com.google.devtools.build.lib.analysis.BaseRuleClasses; import com.google.devtools.build.lib.analysis.RuleDefinition; import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment; +import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.HostTransition; import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.Attribute.ComputedDefault; @@ -43,6 +44,18 @@ * as a setup script target. */ public class GenRuleBaseRule implements RuleDefinition { + public static boolean enableCcToolchain(BuildConfiguration configuration) { + CppConfiguration cppConfiguration = configuration.getFragment(CppConfiguration.class); + if (cppConfiguration != null) { + return enableCcToolchain(cppConfiguration); + } + return true; + } + + public static boolean enableCcToolchain(CppConfiguration cppConfiguration) { + return !cppConfiguration.disableGenruleCcToolchainDependency(); + } + /** * Late-bound dependency on the C++ toolchain iff the genrule has make variables that need * that toolchain. @@ -52,12 +65,16 @@ public static LabelLateBoundDefault ccToolchainAttribute(RuleDefinitionEnviro CppConfiguration.class, env.getToolsLabel(CppRuleClasses.CROSSTOOL_LABEL), // null guards are needed for LateBoundAttributeTest - (rule, attributes, cppConfig) -> - attributes != null - && attributes.get("cmd", Type.STRING) != null - && GenRuleBase.requiresCrosstool(attributes.get("cmd", Type.STRING)) - ? CppRuleClasses.ccToolchainAttribute(env).resolve(rule, attributes, cppConfig) - : null); + (rule, attributes, cppConfig) -> { + if (!enableCcToolchain(cppConfig)) { + return null; + } + return attributes != null + && attributes.get("cmd", Type.STRING) != null + && GenRuleBase.requiresCrosstool(attributes.get("cmd", Type.STRING)) + ? CppRuleClasses.ccToolchainAttribute(env).resolve(rule, attributes, cppConfig) + : null; + }); } /** Computed dependency on the C++ toolchain type. */ 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 fa81d5f1c64315..ece88bdf6fdbe8 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 @@ -652,4 +652,25 @@ public void testDuplicateLocalFlags() throws Exception { getConfiguredTarget("//foo:g"); assertNoEvents(); } + + @Test + public void testDisableGenruleCcToolchainDependency() throws Exception { + reporter.removeHandler(failFastHandler); + scratch.file( + "a/BUILD", + "genrule(", + " name = 'a',", + " outs = ['out.log'],", + " cmd = 'echo $(CC_FLAGS) > $@',", + ")"); + + // Legacy behavior: CC_FLAGS is implicitly defined. + getConfiguredTarget("//a:a"); + assertDoesNotContainEvent("$(CC_FLAGS) not defined"); + + // Updated behavior: CC_FLAGS must be explicitly supplied by a dependency. + useConfiguration("--incompatible_disable_genrule_cc_toolchain_dependency"); + getConfiguredTarget("//a:a"); + assertContainsEvent("$(CC_FLAGS) not defined"); + } }