Give WORKSPACE toolchains and platforms precedence over non-root modules

RELNOTES[INC]: Toolchains and execution platforms are now registered in the following order with `--enable_bzlmod`:
1. root module's module file
2. `WORKSPACE` or `WORKSPACE.bzlmod`
3. non-root modules' module files
4. default toolchains registered by Bazel (does not apply with `WORKSPACE.bzlmod` or execution platforms)

Fixes #20354

Closes #20407.

Co-authored-by: Xudong Yang <[email protected]>
PiperOrigin-RevId: 587826082
Change-Id: Ia98da6ef07b2fbf589ef369d986af2323af6f72a

Author: 2 people

site/en/external/migration.md

@@ -453,6 +453,19 @@ toolchain.
     register_toolchains("@local_config_sh//:local_sh_toolchain")
     ```
 
+The toolchains and execution platforms registered in `WORKSPACE`,
+`WORKSPACE.bzlmod` and each Bazel module's `MODULE.bazel` file follow this
+order of precedence during toolchain selection (from highest to lowest):
+
+1. toolchains and execution platforms registered in the root module's
+   `MODULE.bazel` file.
+2. toolchains and execution platforms registered in the `WORKSPACE` or
+   `WORKSPACE.bzlmod` file.
+3. toolchains and execution platforms registered by modules that are
+   (transitive) dependencies of the root module.
+4. when not using `WORKSPACE.bzlmod`: toolchains registered in the `WORKSPACE`
+   [suffix](/external/migration#builtin-default-deps).
+
 [register_execution_platforms]: /rules/lib/globals/module#register_execution_platforms
 
 ### Introduce local repositories {:#introduce-local-deps}

src/main/java/com/google/devtools/build/lib/packages/Package.java

@@ -72,6 +72,7 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.OptionalInt;
 import java.util.OptionalLong;
 import java.util.Set;
 import java.util.TreeMap;
@@ -262,7 +263,7 @@ void setPackageOverhead(OptionalLong packageOverhead) {
 
   private ImmutableList<TargetPattern> registeredExecutionPlatforms;
   private ImmutableList<TargetPattern> registeredToolchains;
-
+  private OptionalInt firstWorkspaceSuffixRegisteredToolchain;
   private long computationSteps;
 
   // These two fields are mutually exclusive. Which one is set depends on
@@ -425,6 +426,7 @@ private void finishInit(Builder builder) {
     this.failureDetail = builder.getFailureDetail();
     this.registeredExecutionPlatforms = ImmutableList.copyOf(builder.registeredExecutionPlatforms);
     this.registeredToolchains = ImmutableList.copyOf(builder.registeredToolchains);
+    this.firstWorkspaceSuffixRegisteredToolchain = builder.firstWorkspaceSuffixRegisteredToolchain;
     this.repositoryMapping = Preconditions.checkNotNull(builder.repositoryMapping);
     this.mainRepositoryMapping = Preconditions.checkNotNull(builder.mainRepositoryMapping);
     ImmutableMap.Builder<RepositoryName, ImmutableMap<String, RepositoryName>>
@@ -764,6 +766,23 @@ public ImmutableList<TargetPattern> getRegisteredToolchains() {
     return registeredToolchains;
   }
 
+  public ImmutableList<TargetPattern> getUserRegisteredToolchains() {
+    return getRegisteredToolchains()
+        .subList(
+            0, firstWorkspaceSuffixRegisteredToolchain.orElse(getRegisteredToolchains().size()));
+  }
+
+  public ImmutableList<TargetPattern> getWorkspaceSuffixRegisteredToolchains() {
+    return getRegisteredToolchains()
+        .subList(
+            firstWorkspaceSuffixRegisteredToolchain.orElse(getRegisteredToolchains().size()),
+            getRegisteredToolchains().size());
+  }
+
+  OptionalInt getFirstWorkspaceSuffixRegisteredToolchain() {
+    return firstWorkspaceSuffixRegisteredToolchain;
+  }
+
   @Override
   public String toString() {
     return "Package("
@@ -984,8 +1003,16 @@ default boolean precomputeTransitiveLoads() {
     private final List<TargetPattern> registeredToolchains = new ArrayList<>();
 
     /**
-     * True iff the "package" function has already been called in this package.
+     * Tracks the index within {@link #registeredToolchains} of the first toolchain registered from
+     * the WORKSPACE suffixes rather than the WORKSPACE file (if any).
+     *
+     * <p>This is needed to distinguish between these toolchains during resolution: toolchains
+     * registered in WORKSPACE have precedence over those defined in non-root Bazel modules, which
+     * in turn have precedence over those from the WORKSPACE suffixes.
      */
+    private OptionalInt firstWorkspaceSuffixRegisteredToolchain = OptionalInt.empty();
+
+    /** True iff the "package" function has already been called in this package. */
     private boolean packageFunctionUsed;
 
     /**
@@ -1670,10 +1697,18 @@ void addRegisteredExecutionPlatforms(List<TargetPattern> platforms) {
       this.registeredExecutionPlatforms.addAll(platforms);
     }
 
-    void addRegisteredToolchains(List<TargetPattern> toolchains) {
+    void addRegisteredToolchains(List<TargetPattern> toolchains, boolean forWorkspaceSuffix) {
+      if (forWorkspaceSuffix && firstWorkspaceSuffixRegisteredToolchain.isEmpty()) {
+        firstWorkspaceSuffixRegisteredToolchain = OptionalInt.of(registeredToolchains.size());
+      }
       this.registeredToolchains.addAll(toolchains);
     }
 
+    void setFirstWorkspaceSuffixRegisteredToolchain(
+        OptionalInt firstWorkspaceSuffixRegisteredToolchain) {
+      this.firstWorkspaceSuffixRegisteredToolchain = firstWorkspaceSuffixRegisteredToolchain;
+    }
+
     @CanIgnoreReturnValue
     private Builder beforeBuild(boolean discoverAssumedInputFiles) throws NoSuchPackageException {
       Preconditions.checkNotNull(pkg);

src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java

@@ -185,7 +185,10 @@ public void setParent(
       builder.setFailureDetailOverride(aPackage.getFailureDetail());
     }
     builder.addRegisteredExecutionPlatforms(aPackage.getRegisteredExecutionPlatforms());
-    builder.addRegisteredToolchains(aPackage.getRegisteredToolchains());
+    builder.addRegisteredToolchains(
+        aPackage.getRegisteredToolchains(), /* forWorkspaceSuffix= */ false);
+    builder.setFirstWorkspaceSuffixRegisteredToolchain(
+        aPackage.getFirstWorkspaceSuffixRegisteredToolchain());
     builder.addRepositoryMappings(aPackage);
     for (Rule rule : aPackage.getTargets(Rule.class)) {
       try {

src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java

@@ -36,6 +36,13 @@
 /** A helper for the {@link WorkspaceFactory} to create repository rules */
 public final class WorkspaceFactoryHelper {
 
+  public static final String DEFAULT_WORKSPACE_SUFFIX_FILE = "/DEFAULT.WORKSPACE.SUFFIX";
+
+  public static boolean originatesInWorkspaceSuffix(
+      ImmutableList<StarlarkThread.CallStackEntry> callstack) {
+    return callstack.get(0).location.file().equals(DEFAULT_WORKSPACE_SUFFIX_FILE);
+  }
+
   @CanIgnoreReturnValue
   public static Rule createAndAddRepositoryRule(
       Package.Builder pkg,
@@ -70,7 +77,7 @@ public static Rule createAndAddRepositoryRule(
         throw new LabelSyntaxException(e.getMessage());
       }
     }
-    pkg.addRegisteredToolchains(toolchains.build());
+    pkg.addRegisteredToolchains(toolchains.build(), originatesInWorkspaceSuffix(callstack));
     return rule;
   }
 

src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java

@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.packages;
 
+import static com.google.devtools.build.lib.packages.WorkspaceFactoryHelper.originatesInWorkspaceSuffix;
 import static net.starlark.java.eval.Starlark.NONE;
 
 import com.google.common.collect.ImmutableList;
@@ -114,7 +115,9 @@ public void registerToolchains(Sequence<?> toolchainLabels, StarlarkThread threa
     // Add to the package definition for later.
     Package.Builder builder = PackageFactory.getContext(thread).pkgBuilder;
     List<String> patterns = Sequence.cast(toolchainLabels, String.class, "toolchain_labels");
-    builder.addRegisteredToolchains(parsePatterns(patterns, builder, thread));
+    builder.addRegisteredToolchains(
+        parsePatterns(patterns, builder, thread),
+        originatesInWorkspaceSuffix(thread.getCallStack()));
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java

@@ -14,6 +14,7 @@
 
 package com.google.devtools.build.lib.skyframe;
 
+import static com.google.devtools.build.lib.packages.WorkspaceFactoryHelper.DEFAULT_WORKSPACE_SUFFIX_FILE;
 import static com.google.devtools.build.lib.rules.repository.ResolvedFileValue.ATTRIBUTES;
 import static com.google.devtools.build.lib.rules.repository.ResolvedFileValue.NATIVE;
 import static com.google.devtools.build.lib.rules.repository.ResolvedFileValue.REPOSITORIES;
@@ -220,7 +221,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
       StarlarkFile file =
           StarlarkFile.parse(
               ParserInput.fromString(
-                  ruleClassProvider.getDefaultWorkspaceSuffix(), "/DEFAULT.WORKSPACE.SUFFIX"),
+                  ruleClassProvider.getDefaultWorkspaceSuffix(), DEFAULT_WORKSPACE_SUFFIX_FILE),
               // The DEFAULT.WORKSPACE.SUFFIX file breaks through the usual privacy mechanism.
               options.toBuilder().allowLoadPrivateSymbols(true).build());
       if (!file.ok()) {

src/main/java/com/google/devtools/build/lib/skyframe/toolchains/BUILD

@@ -90,6 +90,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:platform_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis/platform",
         "//src/main/java/com/google/devtools/build/lib/analysis/platform:utils",
+        "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:common",
         "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/packages",
@@ -161,6 +162,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/analysis:platform_configuration",
         "//src/main/java/com/google/devtools/build/lib/analysis/platform",
         "//src/main/java/com/google/devtools/build/lib/analysis/platform:utils",
+        "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:common",
         "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:exception",
         "//src/main/java/com/google/devtools/build/lib/bazel/bzlmod:resolution",
         "//src/main/java/com/google/devtools/build/lib/cmdline",

src/main/java/com/google/devtools/build/lib/skyframe/toolchains/RegisteredExecutionPlatformsFunction.java

@@ -26,6 +26,7 @@
 import com.google.devtools.build.lib.analysis.platform.PlatformProviderUtils;
 import com.google.devtools.build.lib.bazel.bzlmod.BazelDepGraphValue;
 import com.google.devtools.build.lib.bazel.bzlmod.Module;
+import com.google.devtools.build.lib.bazel.bzlmod.ModuleKey;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelConstants;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
@@ -104,21 +105,31 @@ public SkyValue compute(SkyKey skyKey, Environment env)
       }
     }
 
-    // Get registered execution platforms from bzlmod.
-    ImmutableList<TargetPattern> bzlmodExecutionPlatforms =
-        getBzlmodExecutionPlatforms(starlarkSemantics, env);
-    if (bzlmodExecutionPlatforms == null) {
+    // Get registered execution platforms from the root Bazel module.
+    ImmutableList<TargetPattern> bzlmodRootModuleExecutionPlatforms =
+        getBzlmodExecutionPlatforms(starlarkSemantics, env, /* forRootModule= */ true);
+    if (bzlmodRootModuleExecutionPlatforms == null) {
       return null;
     }
-    targetPatternBuilder.addAll(TargetPatternUtil.toSigned(bzlmodExecutionPlatforms));
+    targetPatternBuilder.addAll(TargetPatternUtil.toSigned(bzlmodRootModuleExecutionPlatforms));
 
     // Get the registered execution platforms from the WORKSPACE.
+    // The WORKSPACE suffixes don't register any execution platforms, so we can register all
+    // platforms in WORKSPACE before those in non-root Bazel modules.
     ImmutableList<TargetPattern> workspaceExecutionPlatforms = getWorkspaceExecutionPlatforms(env);
     if (workspaceExecutionPlatforms == null) {
       return null;
     }
     targetPatternBuilder.addAll(TargetPatternUtil.toSigned(workspaceExecutionPlatforms));
 
+    // Get registered execution platforms from the non-root Bazel modules.
+    ImmutableList<TargetPattern> bzlmodNonRootModuleExecutionPlatforms =
+        getBzlmodExecutionPlatforms(starlarkSemantics, env, /* forRootModule= */ false);
+    if (bzlmodNonRootModuleExecutionPlatforms == null) {
+      return null;
+    }
+    targetPatternBuilder.addAll(TargetPatternUtil.toSigned(bzlmodNonRootModuleExecutionPlatforms));
+
     // Expand target patterns.
     ImmutableList<Label> platformLabels;
     try {
@@ -164,7 +175,7 @@ public static ImmutableList<TargetPattern> getWorkspaceExecutionPlatforms(Enviro
 
   @Nullable
   private static ImmutableList<TargetPattern> getBzlmodExecutionPlatforms(
-      StarlarkSemantics semantics, Environment env)
+      StarlarkSemantics semantics, Environment env, boolean forRootModule)
       throws InterruptedException, RegisteredExecutionPlatformsFunctionException {
     if (!semantics.getBool(BuildLanguageOptions.ENABLE_BZLMOD)) {
       return ImmutableList.of();
@@ -176,6 +187,9 @@ private static ImmutableList<TargetPattern> getBzlmodExecutionPlatforms(
     }
     ImmutableList.Builder<TargetPattern> executionPlatforms = ImmutableList.builder();
     for (Module module : bazelDepGraphValue.getDepGraph().values()) {
+      if (forRootModule != module.getKey().equals(ModuleKey.ROOT)) {
+        continue;
+      }
       TargetPattern.Parser parser =
           new TargetPattern.Parser(
               PathFragment.EMPTY_FRAGMENT,

src/main/java/com/google/devtools/build/lib/skyframe/toolchains/RegisteredToolchainsFunction.java

@@ -28,6 +28,7 @@
 import com.google.devtools.build.lib.bazel.bzlmod.BazelDepGraphValue;
 import com.google.devtools.build.lib.bazel.bzlmod.ExternalDepsException;
 import com.google.devtools.build.lib.bazel.bzlmod.Module;
+import com.google.devtools.build.lib.bazel.bzlmod.ModuleKey;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelConstants;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
@@ -95,19 +96,37 @@ public SkyValue compute(SkyKey skyKey, Environment env)
           new InvalidToolchainLabelException(e), Transience.PERSISTENT);
     }
 
-    // Get registered toolchains from bzlmod.
-    ImmutableList<TargetPattern> bzlmodToolchains = getBzlmodToolchains(starlarkSemantics, env);
-    if (bzlmodToolchains == null) {
+    // Get registered toolchains from the root Bazel module.
+    ImmutableList<TargetPattern> bzlmodRootModuleToolchains =
+        getBzlmodToolchains(starlarkSemantics, env, /* forRootModule= */ true);
+    if (bzlmodRootModuleToolchains == null) {
       return null;
     }
-    targetPatternBuilder.addAll(TargetPatternUtil.toSigned(bzlmodToolchains));
+    targetPatternBuilder.addAll(TargetPatternUtil.toSigned(bzlmodRootModuleToolchains));
 
-    // Get the registered toolchains from the WORKSPACE.
-    ImmutableList<TargetPattern> workspaceToolchains = getWorkspaceToolchains(env);
-    if (workspaceToolchains == null) {
+    // Get the toolchains from the user-supplied WORKSPACE file.
+    ImmutableList<TargetPattern> userRegisteredWorkspaceToolchains =
+        getWorkspaceToolchains(env, /* userRegistered= */ true);
+    if (userRegisteredWorkspaceToolchains == null) {
       return null;
     }
-    targetPatternBuilder.addAll(TargetPatternUtil.toSigned(workspaceToolchains));
+    targetPatternBuilder.addAll(TargetPatternUtil.toSigned(userRegisteredWorkspaceToolchains));
+
+    // Get registered toolchains from non-root Bazel modules.
+    ImmutableList<TargetPattern> bzlmodNonRootModuleToolchains =
+        getBzlmodToolchains(starlarkSemantics, env, /* forRootModule= */ false);
+    if (bzlmodNonRootModuleToolchains == null) {
+      return null;
+    }
+    targetPatternBuilder.addAll(TargetPatternUtil.toSigned(bzlmodNonRootModuleToolchains));
+
+    // Get the toolchains from the Bazel-supplied WORKSPACE suffix.
+    ImmutableList<TargetPattern> workspaceSuffixToolchains =
+        getWorkspaceToolchains(env, /* userRegistered= */ false);
+    if (workspaceSuffixToolchains == null) {
+      return null;
+    }
+    targetPatternBuilder.addAll(TargetPatternUtil.toSigned(workspaceSuffixToolchains));
 
     // Expand target patterns.
     ImmutableList<Label> toolchainLabels;
@@ -140,21 +159,25 @@ public SkyValue compute(SkyKey skyKey, Environment env)
    */
   @Nullable
   @VisibleForTesting
-  public static ImmutableList<TargetPattern> getWorkspaceToolchains(Environment env)
-      throws InterruptedException {
+  public static ImmutableList<TargetPattern> getWorkspaceToolchains(
+      Environment env, boolean userRegistered) throws InterruptedException {
     PackageValue externalPackageValue =
         (PackageValue) env.getValue(LabelConstants.EXTERNAL_PACKAGE_IDENTIFIER);
     if (externalPackageValue == null) {
       return null;
     }
 
     Package externalPackage = externalPackageValue.getPackage();
-    return externalPackage.getRegisteredToolchains();
+    if (userRegistered) {
+      return externalPackage.getUserRegisteredToolchains();
+    } else {
+      return externalPackage.getWorkspaceSuffixRegisteredToolchains();
+    }
   }
 
   @Nullable
   private static ImmutableList<TargetPattern> getBzlmodToolchains(
-      StarlarkSemantics semantics, Environment env)
+      StarlarkSemantics semantics, Environment env, boolean forRootModule)
       throws InterruptedException, RegisteredToolchainsFunctionException {
     if (!semantics.getBool(BuildLanguageOptions.ENABLE_BZLMOD)) {
       return ImmutableList.of();
@@ -166,6 +189,9 @@ private static ImmutableList<TargetPattern> getBzlmodToolchains(
     }
     ImmutableList.Builder<TargetPattern> toolchains = ImmutableList.builder();
     for (Module module : bazelDepGraphValue.getDepGraph().values()) {
+      if (forRootModule != module.getKey().equals(ModuleKey.ROOT)) {
+        continue;
+      }
       TargetPattern.Parser parser =
           new TargetPattern.Parser(
               PathFragment.EMPTY_FRAGMENT,

src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java

@@ -462,7 +462,8 @@ public void setupMockClient(MockToolsConfig config, List<String> workspaceConten
         "def rules_java_dependencies():",
         "    pass",
         "def rules_java_toolchains():",
-        "    pass");
+        "    native.register_toolchains('//java/toolchains/runtime:all')",
+        "    native.register_toolchains('//java/toolchains/javac:all')");
 
     config.create(
         "rules_java_workspace/java/toolchains/runtime/BUILD",

src/test/java/com/google/devtools/build/lib/packages/util/LoadingMock.java

@@ -39,4 +39,8 @@ public PackageFactoryBuilderWithSkyframeForTesting getPackageFactoryBuilderForTe
   public ConfiguredRuleClassProvider createRuleClassProvider() {
     return TestRuleClassProvider.getRuleClassProviderWithClearedSuffix();
   }
+
+  public ConfiguredRuleClassProvider createRuleClassProviderWithSuffix() {
+    return TestRuleClassProvider.getRuleClassProvider();
+  }
 }