Skip to content

Commit

Permalink
Add the new incompatible flag to control genrule's cc toolchain depen…
Browse files Browse the repository at this point in the history
…dency.

Part of #6867.

Future work will tie this into the actual genrule internals.

PiperOrigin-RevId: 225032807
  • Loading branch information
katre authored and Copybara-Service committed Dec 11, 2018
1 parent d033c2f commit 1c07a85
Show file tree
Hide file tree
Showing 9 changed files with 103 additions and 24 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,6 @@ public ConfiguredTarget create(RuleContext ruleContext)
TemplateVariableInfo templateVariableInfo =
createMakeVariableProvider(
ccToolchainProvider,
ccToolchainProvider.getSysrootPathFragment(),
ruleContext.getRule().getLocation());

ruleConfiguredTargetBuilder
Expand All @@ -94,7 +93,6 @@ public ConfiguredTarget create(RuleContext ruleContext)

static TemplateVariableInfo createMakeVariableProvider(
CcToolchainProvider toolchainProvider,
PathFragment sysroot,
Location location) {

HashMap<String, String> makeVariables =
Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down Expand Up @@ -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());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ public ConfiguredTarget create(RuleContext ruleContext)
TemplateVariableInfo templateVariableInfo =
CcToolchain.createMakeVariableProvider(
ccToolchainProvider,
ccToolchainProvider.getSysrootPathFragment(),
ruleContext.getRule().getLocation());

RuleConfiguredTargetBuilder builder =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -589,4 +589,8 @@ boolean enableCcToolchainConfigInfoFromSkylark() {
public Label getLibcTopLabel() {
return cppOptions.libcTopLabel;
}

public boolean disableGenruleCcToolchainDependency() {
return cppOptions.disableGenruleCcToolchainDependency;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -828,8 +843,8 @@ public FragmentOptions getHost() {
host.inmemoryDotdFiles = inmemoryDotdFiles;

host.doNotUseCpuTransformer = doNotUseCpuTransformer;

host.enableCcToolchainConfigInfoFromSkylark = enableCcToolchainConfigInfoFromSkylark;
host.disableGenruleCcToolchainDependency = disableGenruleCcToolchainDependency;

return host;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,8 @@ public static CppToolchainInfo create(
CcToolchainConfigInfo ccToolchainConfigInfo,
boolean disableLegacyCrosstoolFields,
boolean disableCompilationModeFlags,
boolean disableLinkingModeFlags)
boolean disableLinkingModeFlags,
boolean disableGenruleCcToolchainDependency)
throws EvalException {
ImmutableMap<String, PathFragment> toolPaths =
computeToolPaths(ccToolchainConfigInfo, getToolsDirectory(toolchainLabel));
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -743,15 +745,21 @@ public ImmutableList<String> getUnfilteredCompilerOptions(@Nullable PathFragment
}

private static ImmutableMap<String, String> computeAdditionalMakeVariables(
CcToolchainConfigInfo ccToolchainConfigInfo) {
CcToolchainConfigInfo ccToolchainConfigInfo, boolean disableGenruleCcToolchainDependency) {
Map<String, String> 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<String, String> variable : ccToolchainConfigInfo.getMakeVariables()) {
makeVariablesBuilder.put(variable.getFirst(), variable.getSecond());
}

if (disableGenruleCcToolchainDependency) {
makeVariablesBuilder.remove(CppConfiguration.CC_FLAGS_MAKE_VARIABLE_NAME);
}

return ImmutableMap.copyOf(makeVariablesBuilder);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<MakeVariableSupplier> 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)) {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -276,12 +287,13 @@ protected static class CommandResolverContext extends ConfigurationMakeVariableC
public CommandResolverContext(
RuleContext ruleContext,
NestedSet<Artifact> resolvedSrcs,
NestedSet<Artifact> filesToBuild) {
NestedSet<Artifact> filesToBuild,
Iterable<? extends MakeVariableSupplier> makeVariableSuppliers) {
super(
ruleContext,
ruleContext.getRule().getPackage(),
ruleContext.getConfiguration(),
ImmutableList.of(new CcFlagsSupplier(ruleContext)));
makeVariableSuppliers);
this.ruleContext = ruleContext;
this.resolvedSrcs = resolvedSrcs;
this.filesToBuild = filesToBuild;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 <i>iff</i> the genrule has make variables that need
* that toolchain.
Expand All @@ -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. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
}

0 comments on commit 1c07a85

Please sign in to comment.