[WIP] Change --extra_toolchains precedence to last-wins, instead of first…

site/en/extending/toolchains.md

@@ -508,6 +508,18 @@ The host platform is automatically included as an available execution platform.
 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 via `--extra_toolchains` are added first.
+  1. Within these, the **last** toolchain has highest priority.
+2. Toolchains registered via `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 is currently using a lexicographic ordering.
+
 The resolution steps are as follows.
 
 1. A `target_compatible_with` or `exec_compatible_with` clause *matches* a

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

@@ -83,12 +83,13 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     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);

src/test/java/com/google/devtools/build/lib/skyframe/toolchains/RegisteredToolchainsFunctionTest.java

@@ -153,8 +153,8 @@ public void testRegisteredToolchains_flagOverride_multiple() throws Exception {
     // 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();
   }

src/test/shell/integration/java_integration_test.sh

@@ -267,7 +267,8 @@ EOF
   # 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"

src/test/shell/integration/toolchain_test.sh

@@ -2781,6 +2781,80 @@ EOF
   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
+  add_to_bazelrc "build --extra_toolchains=//${pkg}:toolchain_2"
+  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_2"'
+
+  # Test that command-line options take precedence over other toolchains
+  bazel \
+    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 \
+    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"