Make incompatible_enable_cc_toolchain_resolution a no-op

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
bazelbuild · Dec 6, 2023 · e116bae · e116bae
1 parent c5493b9
commit e116bae
Show file tree

Hide file tree

Showing 24 changed files with 257 additions and 829 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java
@@ -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,

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
@@ -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);

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
@@ -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;
@@ -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;
 
 /**
@@ -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);
 

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
@@ -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
@@ -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;
   }

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
@@ -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;
@@ -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) {
@@ -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);
@@ -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;
-  }
 }
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
@@ -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(
@@ -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
@@ -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(
@@ -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",
@@ -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;
     }

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoHelper.java
@@ -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;
       }
 
@@ -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;
     }
 

diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/cpp/CcModuleApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/cpp/CcModuleApi.java
@@ -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",

diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_common.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_common.bzl
@@ -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(
         *,