Skip to content

Commit

Permalink
Make incompatible_enable_cc_toolchain_resolution a no-op
Browse files Browse the repository at this point in the history
Make crosstool_top flag a no-op and remove the code used to retrieve toolchain from crosstool_top.

Remove the tests related to crosstop or incompatible_enable_cc_toolchain_resolution. Those tests can't work anymore, becuase the functionality is removed.

There are some further cleanups, like removing _cc_toolchain attribute from the rules.

RELNOTES[INC]: incompatible_enable_cc_toolchain_resolution is a no-op, enabled by default (#7260)

PiperOrigin-RevId: 588363296
Change-Id: I8af466628ce9c8d9175533255faef36968570aa4
  • Loading branch information
comius authored and Copybara-Service committed Dec 6, 2023
1 parent c5493b9 commit e116bae
Show file tree
Hide file tree
Showing 24 changed files with 257 additions and 829 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1923,13 +1923,6 @@ private static <T> T nullIfNone(Object object, Class<T> type) {
return object != Starlark.NONE ? type.cast(object) : null;
}

@Override
public boolean isCcToolchainResolutionEnabled(
StarlarkRuleContext starlarkRuleContext, StarlarkThread thread) throws EvalException {
isCalledFromStarlarkCcCommon(thread);
return CppHelper.useToolchainResolution(starlarkRuleContext.getRuleContext());
}

@Override
public Tuple createLinkingContextFromCompilationOutputs(
StarlarkActionFactory starlarkActionFactoryApi,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -750,9 +750,6 @@ private CcToolchainVariables getBuildVariables(
AppleConfiguration appleConfiguration,
String cpu)
throws EvalException, InterruptedException {
if (!cppConfiguration.enableCcToolchainResolution()) {
return buildVariables;
}
// With platforms, cc toolchain is analyzed in the exec configuration, so we can only reuse the
// same build variables instance if the inputs to the construction match.
PathFragment sysroot = getSysrootPathFragment(cppConfiguration);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
Expand All @@ -34,8 +33,6 @@
import java.util.HashMap;
import java.util.Map;
import javax.annotation.Nullable;
import net.starlark.java.eval.Starlark;
import net.starlark.java.eval.StarlarkFunction;
import net.starlark.java.syntax.Location;

/**
Expand Down Expand Up @@ -77,48 +74,16 @@ public ConfiguredTarget create(RuleContext ruleContext)
Label selectedCcToolchain = toolchains.get(key);
CcToolchainProvider ccToolchainProvider;

if (CppHelper.useToolchainResolution(ruleContext)) {
// This is a platforms build (and the user requested to build this suite explicitly).
// Cc_toolchains provide CcToolchainInfo already. Let's select the CcToolchainProvider from
// toolchains and provide it here as well.
ccToolchainProvider =
selectCcToolchain(
CcToolchainProvider.PROVIDER,
ruleContext,
transformedCpu,
compiler,
selectedCcToolchain);
} else {
// This is not a platforms build, and cc_toolchain_suite is the one responsible for creating
// and providing CcToolchainInfo.
CcToolchainAttributesProvider selectedAttributes =
selectCcToolchain(
CcToolchainAttributesProvider.PROVIDER,
ruleContext,
transformedCpu,
compiler,
selectedCcToolchain);
StarlarkFunction getCcToolchainProvider =
(StarlarkFunction) ruleContext.getStarlarkDefinedBuiltin("get_cc_toolchain_provider");
ruleContext.initStarlarkRuleContext();
Object starlarkCcToolchainProvider =
ruleContext.callStarlarkOrThrowRuleError(
getCcToolchainProvider,
ImmutableList.of(
/* ctx */ ruleContext.getStarlarkRuleContext(),
/* attributes */ selectedAttributes,
/* has_apple_fragment */ true),
ImmutableMap.of());
ccToolchainProvider =
starlarkCcToolchainProvider != Starlark.NONE
? (CcToolchainProvider) starlarkCcToolchainProvider
: null;

if (ccToolchainProvider == null) {
// Skyframe restart
return null;
}
}
// This is a platforms build (and the user requested to build this suite explicitly).
// Cc_toolchains provide CcToolchainInfo already. Let's select the CcToolchainProvider from
// toolchains and provide it here as well.
ccToolchainProvider =
selectCcToolchain(
CcToolchainProvider.PROVIDER,
ruleContext,
transformedCpu,
compiler,
selectedCcToolchain);

CcCommon.reportInvalidOptions(ruleContext, cppConfiguration, ccToolchainProvider);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -292,14 +292,7 @@ public CppConfiguration(BuildOptions options) throws InvalidConfigurationExcepti
defaultInToolRepository = true)
@Nullable
public Label getRuleProvidingCcToolchainProvider() {
if (cppOptions.enableCcToolchainResolution) {
// In case C++ toolchain resolution is enabled, crosstool_top flags are not used.
// Returning null prevents additional work on the flags values and makes it possible to
// remove `--crosstool_top` flags.
return null;
} else {
return cppOptions.crosstoolTop;
}
}

@Nullable
Expand Down Expand Up @@ -753,20 +746,6 @@ public boolean collectCodeCoverage() {
return collectCodeCoverage;
}

@StarlarkMethod(
name = "enable_cc_toolchain_resolution",
documented = false,
useStarlarkThread = true)
public boolean enableCcToolchainResolutionForStarlark(StarlarkThread thread)
throws EvalException {
CcModule.checkPrivateStarlarkificationAllowlist(thread);
return enableCcToolchainResolution();
}

public boolean enableCcToolchainResolution() {
return cppOptions.enableCcToolchainResolution;
}

public boolean saveFeatureState() {
return cppOptions.saveFeatureState;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,8 @@

package com.google.devtools.build.lib.rules.cpp;

import static com.google.devtools.build.lib.packages.BuildType.LABEL;
import static com.google.devtools.build.lib.packages.BuildType.LABEL_LIST;

import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
Expand Down Expand Up @@ -131,45 +129,9 @@ public static ImmutableList<String> getLinkopts(RuleContext ruleContext)
return ImmutableList.of();
}

@Nullable
private static CcToolchainProvider getToolchainUsingDefaultCcToolchainAttribute(
RuleContext ruleContext) throws RuleErrorException {
if (ruleContext.attributes().has(CcToolchainRule.CC_TOOLCHAIN_DEFAULT_ATTRIBUTE_NAME)) {
return getToolchainUsingAttribute(
ruleContext, CcToolchainRule.CC_TOOLCHAIN_DEFAULT_ATTRIBUTE_NAME);
} else if (ruleContext
.attributes()
.has(CcToolchainRule.CC_TOOLCHAIN_DEFAULT_ATTRIBUTE_NAME_FOR_STARLARK)) {
return getToolchainUsingAttribute(
ruleContext, CcToolchainRule.CC_TOOLCHAIN_DEFAULT_ATTRIBUTE_NAME_FOR_STARLARK);
}
return null;
}

private static CcToolchainProvider getToolchainUsingAttribute(
RuleContext ruleContext, String toolchainAttribute) throws RuleErrorException {
if (!ruleContext.isAttrDefined(toolchainAttribute, LABEL)) {
throw ruleContext.throwWithRuleError(
String.format(
"INTERNAL BLAZE ERROR: Tried to locate a cc_toolchain via the attribute %s, but it"
+ " is not defined",
toolchainAttribute));
}
TransitiveInfoCollection dep = ruleContext.getPrerequisite(toolchainAttribute);
return getToolchainFromLegacyToolchain(ruleContext, dep);
}

/** Returns C++ toolchain, using toolchain resolution or via default cc toolchain attribute */
/** Returns C++ toolchain, using toolchain resolution */
public static CcToolchainProvider getToolchain(RuleContext ruleContext)
throws RuleErrorException {
if (useToolchainResolution(ruleContext)) {
return getToolchainFromPlatformConstraints(ruleContext);
}
return getToolchainUsingDefaultCcToolchainAttribute(ruleContext);
}

private static CcToolchainProvider getToolchainFromPlatformConstraints(RuleContext ruleContext)
throws RuleErrorException {
ToolchainInfo toolchainInfo =
ruleContext.getToolchainInfo(Label.parseCanonicalUnchecked("//tools/cpp:toolchain_type"));
if (toolchainInfo == null) {
Expand All @@ -191,15 +153,6 @@ private static CcToolchainProvider getToolchainFromPlatformConstraints(RuleConte
}
}

private static CcToolchainProvider getToolchainFromLegacyToolchain(
RuleContext ruleContext, TransitiveInfoCollection dep) throws RuleErrorException {
// TODO(bazel-team): Consider checking this generally at the attribute level.
if ((dep == null) || (dep.get(CcToolchainProvider.PROVIDER) == null)) {
throw ruleContext.throwWithRuleError("The selected C++ toolchain is not a cc_toolchain rule");
}
return dep.get(CcToolchainProvider.PROVIDER);
}

/** Returns the directory where object files are created. */
public static PathFragment getObjDirectory(Label ruleLabel, boolean siblingRepositoryLayout) {
return getObjDirectory(ruleLabel, false, siblingRepositoryLayout);
Expand Down Expand Up @@ -584,12 +537,4 @@ public static boolean useInterfaceSharedLibraries(
return toolchain.supportsInterfaceSharedLibraries(featureConfiguration)
&& cppConfiguration.getUseInterfaceSharedLibraries();
}

static boolean useToolchainResolution(RuleContext ruleContext) {
CppOptions cppOptions =
Preconditions.checkNotNull(
ruleContext.getConfiguration().getOptions().get(CppOptions.class));

return cppOptions.enableCcToolchainResolution;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -108,13 +108,11 @@ public String getTypeDescription() {
name = "crosstool_top",
defaultValue = "@bazel_tools//tools/cpp:toolchain",
converter = LabelConverter.class,
documentationCategory = OptionDocumentationCategory.TOOLCHAIN,
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {
OptionEffectTag.LOADING_AND_ANALYSIS,
OptionEffectTag.CHANGES_INPUTS,
OptionEffectTag.AFFECTS_OUTPUTS
OptionEffectTag.NO_OP,
},
help = "The label of the crosstool package to be used for compiling C++ code.")
help = "No-op flag. Will be removed in a future release.")
public Label crosstoolTop;

@Option(
Expand All @@ -130,9 +128,7 @@ public String getTypeDescription() {
defaultValue = "null",
documentationCategory = OptionDocumentationCategory.TOOLCHAIN,
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS, OptionEffectTag.EXECUTION},
help =
"The C++ compiler to use for host compilation. It is ignored if --host_crosstool_top "
+ "is not set.")
help = "No-op flag. Will be removed in a future release.")
public String hostCppCompiler;

// This is different from --platform_suffix in that that one is designed to facilitate the
Expand Down Expand Up @@ -596,16 +592,9 @@ public Label getMemProfProfileLabel() {
name = "host_crosstool_top",
defaultValue = "null",
converter = LabelConverter.class,
documentationCategory = OptionDocumentationCategory.TOOLCHAIN,
effectTags = {
OptionEffectTag.LOADING_AND_ANALYSIS,
OptionEffectTag.CHANGES_INPUTS,
OptionEffectTag.AFFECTS_OUTPUTS
},
help =
"By default, the --crosstool_top and --compiler options are also used "
+ "for the exec configuration. If this flag is provided, Bazel uses the default libc "
+ "and compiler for the given crosstool_top.")
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.NO_OP},
help = "No-op flag. Will be removed in a future release.")
public Label hostCrosstoolTop;

@Option(
Expand Down Expand Up @@ -864,8 +853,8 @@ public Label getMemProfProfileLabel() {
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
help = "If true, cc rules use toolchain resolution to find the cc_toolchain.")
public boolean enableCcToolchainResolution;
help = "No-op flag. Will be removed in a future release.")
public boolean enableCcToolchainResolutionNoOp;

@Option(
name = "experimental_save_feature_state",
Expand Down Expand Up @@ -1090,14 +1079,6 @@ public FragmentOptions getNormalized() {
newOptions.targetLibcTopLabel = libcTopLabel;
changed = true;
}
if (hostCrosstoolTop == null) {
// Default to the initial target crosstoolTop.
newOptions.hostCrosstoolTop = crosstoolTop;
// Reset this, also, to maintain the invariant that host_compiler is ignored if
// host_crosstool_top is unset.
newOptions.hostCppCompiler = cppCompiler;
changed = true;
}
if (changed) {
return newOptions;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -355,13 +355,9 @@ private static Artifact convertLLVMRawProfileToIndexed(
// Get the zipper binary for unzipping the profile.
Artifact zipperBinaryArtifact = attributes.getZipper();
if (zipperBinaryArtifact == null) {
if (CppHelper.useToolchainResolution(ruleContext)) {
ruleContext.ruleError(
"Zipped profiles are not supported with platforms/toolchains before "
+ "toolchain-transitions are implemented.");
} else {
ruleContext.ruleError("Cannot find zipper binary to unzip the profile");
}
ruleContext.ruleError(
"Zipped profiles are not supported with platforms/toolchains before "
+ "toolchain-transitions are implemented.");
return null;
}

Expand Down Expand Up @@ -490,13 +486,9 @@ private static Artifact getMemProfProfileArtifact(
// Get the zipper binary for unzipping the profile.
Artifact zipperBinaryArtifact = attributes.getZipper();
if (zipperBinaryArtifact == null) {
if (CppHelper.useToolchainResolution(ruleContext)) {
ruleContext.ruleError(
"Zipped profiles are not supported with platforms/toolchains before "
+ "toolchain-transitions are implemented.");
} else {
ruleContext.ruleError("Cannot find zipper binary to unzip the profile");
}
ruleContext.ruleError(
"Zipped profiles are not supported with platforms/toolchains before "
+ "toolchain-transitions are implemented.");
return null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1420,16 +1420,6 @@ CppModuleMapT createCppModuleMap(
String legacyCcFlagsMakeVariable(CcToolchainProviderT ccToolchain, StarlarkThread thread)
throws EvalException;

@StarlarkMethod(
name = "is_cc_toolchain_resolution_enabled_do_not_use",
documented = false,
parameters = {
@Param(name = "ctx", positional = false, named = true, doc = "The rule context."),
},
doc = "Returns true if the --incompatible_enable_cc_toolchain_resolution flag is enabled.",
useStarlarkThread = true)
boolean isCcToolchainResolutionEnabled(StarlarkRuleContextT ruleContext, StarlarkThread thread)
throws EvalException;

@StarlarkMethod(
name = "create_cc_toolchain_config_info",
Expand Down
4 changes: 3 additions & 1 deletion src/main/starlark/builtins_bzl/common/cc/cc_common.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,9 @@ def _legacy_cc_flags_make_variable_do_not_use(*, cc_toolchain):
return cc_common_internal.legacy_cc_flags_make_variable_do_not_use(cc_toolchain = cc_toolchain)

def _is_cc_toolchain_resolution_enabled_do_not_use(*, ctx):
return cc_common_internal.is_cc_toolchain_resolution_enabled_do_not_use(ctx = ctx)
# Supports public is_cc_toolchain_resolution_enabled_do_not_use
# TODO(b/218795674): remove once uses are cleaned up
return True

def _create_cc_toolchain_config_info(
*,
Expand Down

0 comments on commit e116bae

Please sign in to comment.