Skip to content

Commit

Permalink
Get rid of logic which assumed cc_toolchain might have been analyze…
Browse files Browse the repository at this point in the history
…d in a different configuration from rules.

Since both `incompatible_require_ctx_in_configure_features` and `incompatible_enable_cc_toolchain_resolution` are no-op logic is obsolete.

PiperOrigin-RevId: 588879596
Change-Id: Iea6aa3f9fae437b1f00b386f474e40f9a52542fc
  • Loading branch information
buildbreaker2021 authored and copybara-github committed Dec 7, 2023
1 parent 8b55ad2 commit b2a8937
Show file tree
Hide file tree
Showing 5 changed files with 11 additions and 78 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType;
import com.google.devtools.build.lib.shell.ShellUtils;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -600,15 +599,10 @@ public static String computeCcFlags(RuleContext ruleContext, TransitiveInfoColle

// Determine the original value of CC_FLAGS.
String originalCcFlags = toolchainProvider.getLegacyCcFlagsMakeVariable();

// Ensure that Sysroot is set properly.
// TODO(b/129045294): We assume --incompatible_disable_genrule_cc_toolchain_dependency will
// be flipped sooner than --incompatible_enable_cc_toolchain_resolution. Then this method
// will be gone.
String sysrootCcFlags =
computeCcFlagForSysroot(
toolchainProvider.getCppConfigurationEvenThoughItCanBeDifferentThanWhatTargetHas(),
toolchainProvider);
String sysrootCcFlags = "";
if (toolchainProvider.getSysrootPathFragment() != null) {
sysrootCcFlags = SYSROOT_FLAG + toolchainProvider.getSysrootPathFragment();
}

// Fetch additional flags from the FeatureConfiguration.
List<String> featureConfigCcFlags =
Expand All @@ -633,17 +627,6 @@ private static boolean containsSysroot(String ccFlags, List<String> moreCcFlags)
.anyMatch(str -> str.contains(SYSROOT_FLAG));
}

private static String computeCcFlagForSysroot(
CppConfiguration cppConfiguration, CcToolchainProvider toolchainProvider) {
PathFragment sysroot = toolchainProvider.getSysrootPathFragment(cppConfiguration);
String sysrootFlag = "";
if (sysroot != null) {
sysrootFlag = SYSROOT_FLAG + sysroot;
}

return sysrootFlag;
}

private static List<String> computeCcFlagsFromFeatureConfig(
RuleContext ruleContext, CcToolchainProvider toolchainProvider)
throws RuleErrorException, InterruptedException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,8 @@ public CcToolchainAttributesProvider constructCcToolchainAttributesInfo(
@Param(name = "runtime_solib_dir", positional = false, named = true),
@Param(name = "cc_compilation_context", positional = false, named = true),
@Param(name = "builtin_include_files", positional = false, named = true),
@Param(name = "target_builtin_include_files", positional = false, named = true),
@Param(name = "builtin_include_directories", positional = false, named = true),
@Param(name = "sysroot", positional = false, named = true),
@Param(name = "target_sysroot", positional = false, named = true),
@Param(name = "fdo_context", positional = false, named = true),
@Param(name = "is_tool_configuration", positional = false, named = true),
@Param(name = "tool_paths", positional = false, named = true),
Expand Down Expand Up @@ -153,10 +151,8 @@ public CcToolchainProvider getCcToolchainProvider(
String dynamicRuntimeSolibDirStr,
CcCompilationContext ccCompilationContext,
Sequence<?> builtinIncludeFiles,
Sequence<?> targetBuiltinIncludeFiles,
Sequence<?> builtInIncludeDirectoriesStr,
Object sysrootObject,
Object targetSysrootObject,
FdoContext fdoContext,
boolean isToolConfiguration,
Dict<?, ?> toolPathsDict,
Expand Down Expand Up @@ -200,7 +196,6 @@ public CcToolchainProvider getCcToolchainProvider(
.map(PathFragment::create)
.collect(toImmutableList());
PathFragment sysroot = getPathfragmentOrNone(sysrootObject);
PathFragment targetSysroot = getPathfragmentOrNone(targetSysrootObject);
Dict<String, String> additionalMakeVariables =
Dict.cast(additionalMakeVariablesDict, String.class, String.class, "tool_paths");
PathFragment defaultSysroot = getPathfragmentOrNone(defaultSysrootObject);
Expand Down Expand Up @@ -232,14 +227,10 @@ public CcToolchainProvider getCcToolchainProvider(
/* builtinIncludeFiles= */ Sequence.cast(
builtinIncludeFiles, Artifact.class, "builtin_include_files")
.getImmutableList(),
/* targetBuiltinIncludeFiles= */ Sequence.cast(
targetBuiltinIncludeFiles, Artifact.class, "target_builtin_include_files")
.getImmutableList(),
/* linkDynamicLibraryTool= */ attributes.getLinkDynamicLibraryTool(),
/* grepIncludes= */ attributes.getGrepIncludes(),
/* builtInIncludeDirectories= */ builtInIncludeDirectories,
/* sysroot= */ sysroot,
/* targetSysroot= */ targetSysroot,
/* fdoContext= */ fdoContext,
/* isToolConfiguration= */ isToolConfiguration,
/* licensesProvider= */ attributes.getLicensesProvider(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
import com.google.devtools.build.lib.rules.cpp.FdoContext.BranchFdoProfile;
import com.google.devtools.build.lib.starlarkbuildapi.cpp.CcToolchainProviderApi;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Objects;
import javax.annotation.Nullable;
import net.starlark.java.eval.Dict;
import net.starlark.java.eval.EvalException;
Expand Down Expand Up @@ -93,12 +92,10 @@ public final class CcToolchainProvider extends NativeInfo
private final boolean supportsHeaderParsing;
private final CcToolchainVariables buildVariables;
private final ImmutableList<Artifact> builtinIncludeFiles;
private final ImmutableList<Artifact> targetBuiltinIncludeFiles;
@Nullable private final Artifact linkDynamicLibraryTool;
@Nullable private final Artifact grepIncludes;
private final ImmutableList<PathFragment> builtInIncludeDirectories;
@Nullable private final PathFragment sysroot;
private final PathFragment targetSysroot;
private final boolean isToolConfiguration;
private final ImmutableMap<String, String> toolPaths;
private final CcToolchainFeatures toolchainFeatures;
Expand Down Expand Up @@ -164,12 +161,10 @@ public CcToolchainProvider(
boolean supportsHeaderParsing,
CcToolchainVariables buildVariables,
ImmutableList<Artifact> builtinIncludeFiles,
ImmutableList<Artifact> targetBuiltinIncludeFiles,
Artifact linkDynamicLibraryTool,
@Nullable Artifact grepIncludes,
ImmutableList<PathFragment> builtInIncludeDirectories,
@Nullable PathFragment sysroot,
@Nullable PathFragment targetSysroot,
FdoContext fdoContext,
boolean isToolConfiguration,
LicensesProvider licensesProvider,
Expand Down Expand Up @@ -226,11 +221,9 @@ public CcToolchainProvider(
this.supportsHeaderParsing = supportsHeaderParsing;
this.buildVariables = buildVariables;
this.builtinIncludeFiles = builtinIncludeFiles;
this.targetBuiltinIncludeFiles = targetBuiltinIncludeFiles;
this.linkDynamicLibraryTool = linkDynamicLibraryTool;
this.builtInIncludeDirectories = builtInIncludeDirectories;
this.sysroot = sysroot;
this.targetSysroot = targetSysroot;
this.defaultSysroot = defaultSysroot;
this.runtimeSysroot = runtimeSysroot;
this.fdoContext = fdoContext == null ? FdoContext.getDisabledContext() : fdoContext;
Expand Down Expand Up @@ -721,20 +714,7 @@ public boolean supportsInterfaceSharedLibraries(FeatureConfiguration featureConf
return featureConfiguration.isEnabled(CppRuleClasses.SUPPORTS_INTERFACE_SHARED_LIBRARIES);
}

/**
* Return CppConfiguration instance that was used to configure CcToolchain.
*
* <p>If C++ rules use platforms/toolchains without
* https://github.com/bazelbuild/proposals/blob/master/designs/2019-02-12-toolchain-transitions.md
* implemented, CcToolchain is analyzed in the exec configuration. This configuration is not what
* should be used by rules using the toolchain. This method should only be used to access stuff
* from CppConfiguration that is identical between exec and target (e.g. incompatible flag
* values). Don't use it if you don't know what you're doing.
*
* <p>Once toolchain transitions are implemented, we can safely use the CppConfiguration from the
* toolchain in rules.
*/
CppConfiguration getCppConfigurationEvenThoughItCanBeDifferentThanWhatTargetHas() {
CppConfiguration getCppConfiguration() {
return cppConfiguration;
}

Expand All @@ -752,11 +732,7 @@ private CcToolchainVariables getBuildVariables(
throws EvalException, InterruptedException {
// 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);
String minOsVersion = cppConfiguration.getMinimumOsVersion();
if (Objects.equals(sysroot, this.sysroot)
&& Objects.equals(minOsVersion, this.cppConfiguration.getMinimumOsVersion())
&& ccToolchainBuildVariablesFunc.getName().equals("cc_toolchain_build_variables")) {
if (ccToolchainBuildVariablesFunc.getName().equals("cc_toolchain_build_variables")) {
return buildVariables;
}
// With platforms, cc toolchain is analyzed in the exec configuration, so we cannot reuse
Expand Down Expand Up @@ -822,11 +798,7 @@ public CcToolchainVariables getBuildVariablesForStarlark(
* source file or any of the headers included by it.
*/
public ImmutableList<Artifact> getBuiltinIncludeFiles(CppConfiguration cppConfiguration) {
if (cppConfiguration.equals(getCppConfigurationEvenThoughItCanBeDifferentThanWhatTargetHas())) {
return builtinIncludeFiles;
} else {
return targetBuiltinIncludeFiles;
}
return builtinIncludeFiles;
}

/**
Expand Down Expand Up @@ -854,12 +826,8 @@ public String getSysroot() {
return sysroot != null ? sysroot.getPathString() : null;
}

public PathFragment getSysrootPathFragment(CppConfiguration cppConfiguration) {
if (cppConfiguration.equals(getCppConfigurationEvenThoughItCanBeDifferentThanWhatTargetHas())) {
return sysroot;
} else {
return targetSysroot;
}
public PathFragment getSysrootPathFragment() {
return sysroot;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -204,9 +204,7 @@ protected void computeKey(
cppCompileActionBuilder.getPrunableHeaders(),
cppCompileActionBuilder.getBuiltinIncludeDirectories(),
cppCompileActionBuilder.getInputsForInvalidation(),
toolchain
.getCppConfigurationEvenThoughItCanBeDifferentThanWhatTargetHas()
.validateTopLevelHeaderInclusions());
toolchain.getCppConfiguration().validateTopLevelHeaderInclusions());
}

private boolean shouldCompileHeaders() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@

"""A helper for creating CcToolchainProvider."""

load(":common/cc/cc_common.bzl", "cc_common")
load(":common/cc/cc_helper.bzl", "cc_helper")
load(":common/paths.bzl", "paths")
load(":common/cc/cc_common.bzl", "cc_common")

cc_internal = _builtins.internal.cc_internal

Expand Down Expand Up @@ -192,11 +192,6 @@ def get_cc_toolchain_provider(ctx, attributes, has_apple_fragment):
else:
sysroot = attributes.libc_top_label().package

if attributes.target_libc_top_label() == None:
target_sysroot = sysroot
else:
target_sysroot = attributes.target_libc_top_label().package

static_runtime_lib = attributes.static_runtime_lib()
if static_runtime_lib != None:
static_runtime_link_inputs = static_runtime_lib[DefaultInfo].files
Expand Down Expand Up @@ -247,10 +242,8 @@ def get_cc_toolchain_provider(ctx, attributes, has_apple_fragment):
runtime_solib_dir = runtime_solib_dir,
cc_compilation_context = cc_compilation_context,
builtin_include_files = _builtin_includes(attributes.libc()),
target_builtin_include_files = _builtin_includes(attributes.target_libc()),
builtin_include_directories = builtin_include_directories,
sysroot = sysroot,
target_sysroot = target_sysroot,
fdo_context = fdo_context,
is_tool_configuration = ctx.configuration.is_tool_configuration(),
tool_paths = tool_paths,
Expand Down

0 comments on commit b2a8937

Please sign in to comment.