Change `--extra_toolchains` precedence to last-wins, instead of first-wins.
This only changes the precedence within all uses of `--extra_toolchains`, it does not change the precedence for `register_toolchains` calls.
After this, the priority order for toolchains is:
1. Consider toolchains registered via `--extra_toolchains`
1. Within this set, the last mentioned toolchain has highest priority
2. Consider toolchains registered via `register_toolchains`
1. Within this set, the first mentioned toolchain has highest priority
In all cases, pseudo-targets like `:all` and `/...` are ordered by Bazel's package loading mechanism, which is currently using a lexicographic ordering.
Fixes #19945.
Closes #19942. Closes #20031.
PiperOrigin-RevId: 579845262
Change-Id: Ibd9c32e5479a6a27717734852b875f9b3f31b510
diff --git a/site/en/extending/toolchains.md b/site/en/extending/toolchains.md
index d89e66d..c193683 100644
--- a/site/en/extending/toolchains.md
+++ b/site/en/extending/toolchains.md
@@ -508,6 +508,18 @@
Available platforms and toolchains are tracked as ordered lists for determinism,
with preference given to earlier items in the list.
+The set of available toolchains, in priority order, is created from
+`--extra_toolchains` and `register_toolchains`:
+
+1. Toolchains registered using `--extra_toolchains` are added first.
+ 1. Within these, the **last** toolchain has highest priority.
+2. Toolchains registered using `register_toolchains`
+ 1. Within these, the **first** mentioned toolchain has highest priority.
+
+**NOTE:** [Pseudo-targets like `:all`, `:*`, and
+`/...`](/run/build#specifying-build-targets) are ordered by Bazel's package
+loading mechanism, which uses a lexicographic ordering.
+
The resolution steps are as follows.
1. A `target_compatible_with` or `exec_compatible_with` clause *matches* a
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/toolchains/RegisteredToolchainsFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/toolchains/RegisteredToolchainsFunction.java
index 2b40720..cec918f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/toolchains/RegisteredToolchainsFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/toolchains/RegisteredToolchainsFunction.java
@@ -83,12 +83,13 @@
ImmutableList.Builder<SignedTargetPattern> targetPatternBuilder = new ImmutableList.Builder<>();
// Get the toolchains from the configuration.
+ // Reverse the list so the last one defined takes precedences.
PlatformConfiguration platformConfiguration =
configuration.getFragment(PlatformConfiguration.class);
try {
targetPatternBuilder.addAll(
TargetPatternUtil.parseAllSigned(
- platformConfiguration.getExtraToolchains(), mainRepoParser));
+ platformConfiguration.getExtraToolchains().reverse(), mainRepoParser));
} catch (InvalidTargetPatternException e) {
throw new RegisteredToolchainsFunctionException(
new InvalidToolchainLabelException(e), Transience.PERSISTENT);
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/toolchains/RegisteredToolchainsFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/toolchains/RegisteredToolchainsFunctionTest.java
index cb3e364..0892a30 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/toolchains/RegisteredToolchainsFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/toolchains/RegisteredToolchainsFunctionTest.java
@@ -153,8 +153,8 @@
// Verify that the target registered with the extra_toolchains flag is first in the list.
assertToolchainLabels(result.get(toolchainsKey))
.containsAtLeast(
- Label.parseCanonicalUnchecked("//extra:extra_toolchain_impl_1"),
Label.parseCanonicalUnchecked("//extra:extra_toolchain_impl_2"),
+ Label.parseCanonicalUnchecked("//extra:extra_toolchain_impl_1"),
Label.parseCanonicalUnchecked("//toolchain:toolchain_1_impl"))
.inOrder();
}
diff --git a/src/test/shell/integration/java_integration_test.sh b/src/test/shell/integration/java_integration_test.sh
index 59d134d..758ce8a 100755
--- a/src/test/shell/integration/java_integration_test.sh
+++ b/src/test/shell/integration/java_integration_test.sh
@@ -267,7 +267,8 @@
# Set javabase to an absolute path.
bazel build //$pkg/java/hello:hello //$pkg/java/hello:hello_deploy.jar \
"$stamp_arg" \
- --extra_toolchains="//$pkg/jvm:all,//tools/jdk:all" \
+ --extra_toolchains="//tools/jdk:all" \
+ --extra_toolchains="//$pkg/jvm:all" \
--platforms="//$pkg/jvm:platform" \
"$embed_label" >&"$TEST_log" \
|| fail "Build failed"
diff --git a/src/test/shell/integration/toolchain_test.sh b/src/test/shell/integration/toolchain_test.sh
index d81aa14..fa15c4a 100755
--- a/src/test/shell/integration/toolchain_test.sh
+++ b/src/test/shell/integration/toolchain_test.sh
@@ -2781,6 +2781,87 @@
expect_log "Selected execution platform //${pkg}/platforms:platform2"
}
+# Regression test for https://github.com/bazelbuild/bazel/issues/19945.
+function test_extra_toolchain_precedence {
+ local -r pkg="${FUNCNAME[0]}"
+
+ write_test_toolchain "${pkg}"
+ write_test_rule "${pkg}"
+
+ cat > WORKSPACE <<EOF
+register_toolchains('//${pkg}:toolchain_1')
+EOF
+
+ cat > "${pkg}/BUILD" <<EOF
+load('//${pkg}/toolchain:toolchain_test_toolchain.bzl', 'test_toolchain')
+
+package(default_visibility = ["//visibility:public"])
+
+# Define and declare four identical toolchains.
+[
+ [
+ test_toolchain(
+ name = 'toolchain_impl_' + str(i),
+ extra_str = 'foo from toolchain_' + str(i),
+ ),
+ toolchain(
+ name = 'toolchain_' + str(i),
+ toolchain_type = '//${pkg}/toolchain:test_toolchain',
+ toolchain = ':toolchain_impl_' + str(i)
+ ),
+ ]
+ for i in range(1, 5)
+]
+EOF
+
+ mkdir -p "${pkg}/demo"
+ cat > "${pkg}/demo/BUILD" <<EOF
+load('//${pkg}/toolchain:rule_use_toolchain.bzl', 'use_toolchain')
+package(default_visibility = ["//visibility:public"])
+
+# Use the toolchain.
+use_toolchain(
+ name = 'use',
+ message = 'this is the rule')
+EOF
+
+ bazel query "//${pkg}:*"
+
+ bazel \
+ build \
+ "//${pkg}/demo:use" &> $TEST_log || fail "Build failed"
+ expect_log 'Using toolchain: rule message: "this is the rule", toolchain extra_str: "foo from toolchain_1"'
+
+ # Test that bazelrc options take precedence over registered toolchains
+ cat > "${pkg}/toolchain_rc" <<EOF
+import ${bazelrc}
+build --extra_toolchains=//${pkg}:toolchain_2
+EOF
+
+ bazel \
+ --${PRODUCT_NAME}rc="${pkg}/toolchain_rc" \
+ build \
+ "//${pkg}/demo:use" &> $TEST_log || fail "Build failed"
+ expect_log 'Using toolchain: rule message: "this is the rule", toolchain extra_str: "foo from toolchain_2"'
+
+ # Test that command-line options take precedence over other toolchains
+ bazel \
+ --${PRODUCT_NAME}rc="${pkg}/toolchain_rc" \
+ build \
+ --extra_toolchains=//${pkg}:toolchain_3 \
+ "//${pkg}/demo:use" &> $TEST_log || fail "Build failed"
+ expect_log 'Using toolchain: rule message: "this is the rule", toolchain extra_str: "foo from toolchain_3"'
+
+ # Test that the last --extra_toolchains takes precedence
+ bazel \
+ --${PRODUCT_NAME}rc="${pkg}/toolchain_rc" \
+ build \
+ --extra_toolchains=//${pkg}:toolchain_3 \
+ --extra_toolchains=//${pkg}:toolchain_4 \
+ "//${pkg}/demo:use" &> $TEST_log || fail "Build failed"
+ expect_log 'Using toolchain: rule message: "this is the rule", toolchain extra_str: "foo from toolchain_4"'
+}
+
# TODO(katre): Test using toolchain-provided make variables from a genrule.
run_suite "toolchain tests"
