From b2a89372a6c5eb06fec8142c588b6270a2a5f322 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 7 Dec 2023 12:46:43 -0800 Subject: [PATCH] Get rid of logic which assumed `cc_toolchain` might have been analyzed 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 --- .../build/lib/rules/cpp/CcCommon.java | 25 ++--------- .../lib/rules/cpp/CcStarlarkInternal.java | 9 ---- .../lib/rules/cpp/CcToolchainProvider.java | 42 +++---------------- .../rules/cpp/CppCompileActionTemplate.java | 4 +- .../cc/cc_toolchain_provider_helper.bzl | 9 +--- 5 files changed, 11 insertions(+), 78 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java index d9dd07e308e4a7..3ffd8df67f8c4c 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java @@ -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; @@ -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 featureConfigCcFlags = @@ -633,17 +627,6 @@ private static boolean containsSysroot(String ccFlags, List 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 computeCcFlagsFromFeatureConfig( RuleContext ruleContext, CcToolchainProvider toolchainProvider) throws RuleErrorException, InterruptedException { diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcStarlarkInternal.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcStarlarkInternal.java index 6eb53cc1601f50..eb0d790c4acf73 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcStarlarkInternal.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcStarlarkInternal.java @@ -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), @@ -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, @@ -200,7 +196,6 @@ public CcToolchainProvider getCcToolchainProvider( .map(PathFragment::create) .collect(toImmutableList()); PathFragment sysroot = getPathfragmentOrNone(sysrootObject); - PathFragment targetSysroot = getPathfragmentOrNone(targetSysrootObject); Dict additionalMakeVariables = Dict.cast(additionalMakeVariablesDict, String.class, String.class, "tool_paths"); PathFragment defaultSysroot = getPathfragmentOrNone(defaultSysrootObject); @@ -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(), diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java index c0688feda911f9..521a8d342ae6c4 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchainProvider.java @@ -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; @@ -93,12 +92,10 @@ public final class CcToolchainProvider extends NativeInfo private final boolean supportsHeaderParsing; private final CcToolchainVariables buildVariables; private final ImmutableList builtinIncludeFiles; - private final ImmutableList targetBuiltinIncludeFiles; @Nullable private final Artifact linkDynamicLibraryTool; @Nullable private final Artifact grepIncludes; private final ImmutableList builtInIncludeDirectories; @Nullable private final PathFragment sysroot; - private final PathFragment targetSysroot; private final boolean isToolConfiguration; private final ImmutableMap toolPaths; private final CcToolchainFeatures toolchainFeatures; @@ -164,12 +161,10 @@ public CcToolchainProvider( boolean supportsHeaderParsing, CcToolchainVariables buildVariables, ImmutableList builtinIncludeFiles, - ImmutableList targetBuiltinIncludeFiles, Artifact linkDynamicLibraryTool, @Nullable Artifact grepIncludes, ImmutableList builtInIncludeDirectories, @Nullable PathFragment sysroot, - @Nullable PathFragment targetSysroot, FdoContext fdoContext, boolean isToolConfiguration, LicensesProvider licensesProvider, @@ -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; @@ -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. - * - *

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. - * - *

Once toolchain transitions are implemented, we can safely use the CppConfiguration from the - * toolchain in rules. - */ - CppConfiguration getCppConfigurationEvenThoughItCanBeDifferentThanWhatTargetHas() { + CppConfiguration getCppConfiguration() { return cppConfiguration; } @@ -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 @@ -822,11 +798,7 @@ public CcToolchainVariables getBuildVariablesForStarlark( * source file or any of the headers included by it. */ public ImmutableList getBuiltinIncludeFiles(CppConfiguration cppConfiguration) { - if (cppConfiguration.equals(getCppConfigurationEvenThoughItCanBeDifferentThanWhatTargetHas())) { - return builtinIncludeFiles; - } else { - return targetBuiltinIncludeFiles; - } + return builtinIncludeFiles; } /** @@ -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; } /** diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java index 037d04b17630d4..23e4dc6d7c55a7 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java @@ -204,9 +204,7 @@ protected void computeKey( cppCompileActionBuilder.getPrunableHeaders(), cppCompileActionBuilder.getBuiltinIncludeDirectories(), cppCompileActionBuilder.getInputsForInvalidation(), - toolchain - .getCppConfigurationEvenThoughItCanBeDifferentThanWhatTargetHas() - .validateTopLevelHeaderInclusions()); + toolchain.getCppConfiguration().validateTopLevelHeaderInclusions()); } private boolean shouldCompileHeaders() { diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_toolchain_provider_helper.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_toolchain_provider_helper.bzl index cb8f24ded50057..ee99904561c686 100644 --- a/src/main/starlark/builtins_bzl/common/cc/cc_toolchain_provider_helper.bzl +++ b/src/main/starlark/builtins_bzl/common/cc/cc_toolchain_provider_helper.bzl @@ -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 @@ -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 @@ -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,