From 96b361205ee05dcacdcf5055ca9cc3e5ca5d126c Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 4 Dec 2023 13:36:57 -0800 Subject: [PATCH] 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 PiperOrigin-RevId: 587826082 Change-Id: Ia98da6ef07b2fbf589ef369d986af2323af6f72a --- site/en/external/migration.md | 13 +++++ .../devtools/build/lib/packages/Package.java | 41 +++++++++++++-- .../build/lib/packages/WorkspaceFactory.java | 5 +- .../lib/packages/WorkspaceFactoryHelper.java | 9 +++- .../build/lib/packages/WorkspaceGlobals.java | 5 +- .../lib/skyframe/WorkspaceFileFunction.java | 3 +- .../build/lib/skyframe/toolchains/BUILD | 2 + .../RegisteredExecutionPlatformsFunction.java | 26 +++++++--- .../RegisteredToolchainsFunction.java | 50 ++++++++++++++----- .../lib/analysis/mock/BazelAnalysisMock.java | 3 +- .../build/lib/packages/util/LoadingMock.java | 4 ++ .../repository/ExternalPackageHelperTest.java | 15 ++++-- .../lib/rules/platform/ToolchainTestCase.java | 19 ++++++- .../build/lib/skyframe/toolchains/BUILD | 9 +--- ...isteredExecutionPlatformsFunctionTest.java | 17 +++++-- .../RegisteredToolchainsFunctionTest.java | 43 ++++++++++++++-- 16 files changed, 219 insertions(+), 45 deletions(-) diff --git a/site/en/external/migration.md b/site/en/external/migration.md index efd1cd1bc51bd9..c4eb0c4050dbab 100644 --- a/site/en/external/migration.md +++ b/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} diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java index b861df2ae512e4..02d0cabaf28cf4 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Package.java +++ b/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 registeredExecutionPlatforms; private ImmutableList 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> @@ -764,6 +766,23 @@ public ImmutableList getRegisteredToolchains() { return registeredToolchains; } + public ImmutableList getUserRegisteredToolchains() { + return getRegisteredToolchains() + .subList( + 0, firstWorkspaceSuffixRegisteredToolchain.orElse(getRegisteredToolchains().size())); + } + + public ImmutableList 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 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). + * + *

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 platforms) { this.registeredExecutionPlatforms.addAll(platforms); } - void addRegisteredToolchains(List toolchains) { + void addRegisteredToolchains(List 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); diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java index d5901190b10058..095fd305412c78 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java +++ b/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 { diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java index 1f514a4f43cbcc..0f2847470d452d 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactoryHelper.java +++ b/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 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; } diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java index 61255637295fc9..1ef44d40f422d2 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java +++ b/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 patterns = Sequence.cast(toolchainLabels, String.class, "toolchain_labels"); - builder.addRegisteredToolchains(parsePatterns(patterns, builder, thread)); + builder.addRegisteredToolchains( + parsePatterns(patterns, builder, thread), + originatesInWorkspaceSuffix(thread.getCallStack())); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java index 8ae2ad808423fc..6d6186548494a0 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java +++ b/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()) { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/toolchains/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/toolchains/BUILD index ab6d26d17d3cc0..ae29a94a24983e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/toolchains/BUILD +++ b/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", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/toolchains/RegisteredExecutionPlatformsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/toolchains/RegisteredExecutionPlatformsFunction.java index 4fb87c27f2b02e..671f9f906d676e 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/toolchains/RegisteredExecutionPlatformsFunction.java +++ b/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 bzlmodExecutionPlatforms = - getBzlmodExecutionPlatforms(starlarkSemantics, env); - if (bzlmodExecutionPlatforms == null) { + // Get registered execution platforms from the root Bazel module. + ImmutableList 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 workspaceExecutionPlatforms = getWorkspaceExecutionPlatforms(env); if (workspaceExecutionPlatforms == null) { return null; } targetPatternBuilder.addAll(TargetPatternUtil.toSigned(workspaceExecutionPlatforms)); + // Get registered execution platforms from the non-root Bazel modules. + ImmutableList bzlmodNonRootModuleExecutionPlatforms = + getBzlmodExecutionPlatforms(starlarkSemantics, env, /* forRootModule= */ false); + if (bzlmodNonRootModuleExecutionPlatforms == null) { + return null; + } + targetPatternBuilder.addAll(TargetPatternUtil.toSigned(bzlmodNonRootModuleExecutionPlatforms)); + // Expand target patterns. ImmutableList