Add `--@bazel_tools//tools/test:incompatible_use_default_test_toolchain` #25123

fmeum · 2025-01-29T10:41:33Z

Work towards #25160

RELNOTES: The new --@bazel_tools//tools/test:incompatible_use_default_test_toolchain flag can be used to have test actions select an execution platform that has all the constraints provided by the target platform instead of always selecting the first available execution platform. This supersedes the --use_target_platform_for_tests flag.

katre · 2025-01-31T18:29:28Z

Why a starlark flag and not a native flag? Starlark flags under @bazel_tools are more confusing for users and harder for use in the google-internal version.

fmeum · 2025-01-31T19:17:00Z

Why a starlark flag and not a native flag? Starlark flags under @bazel_tools are more confusing for users and harder for use in the google-internal version.

I am ultimately open to both, but I saw most benefits on the side of a Starlark flag:

Fully transparent, no magic: Everything except for the use_target_platform_constraints attribute on toolchain is implemented in unprivileged Starlark, which makes it easy for ruleset authors to adopt similar logic for their own test toolchains (think cc_test toolchain) and for users to understand what's going on.
Easier to import into google3: You can just skip everything in @bazel_tools and supply your own toolchain implementation (for example, you probably never used --use_target_platform_for_tests to begin with, so you don't need to keep that toolchain).
If needed, a --flag_alias can make it feel like a native incompatible flag.
Starlark flags are pretty common by now and there is precedent for incompatible flags in core rulesets.
If we want to, we could move all of this logic + the toolchains into a separate Bazel module. We need to do that at some point anyway to support cross-platform running of tests (Linux -> Windows, which requires a precompiled tw binary that's downloadable, not shipped with a Windows Bazel binary).

katre · 2025-02-03T16:08:08Z

Hmm, I missed the part where this isn't used in the Bazel binary, and so we can entirely skip it for internal blaze.

Adding this to my review queue, it's going to need some in-depth thinking.

fmeum · 2025-02-05T14:59:45Z

I resolved the conflict and am now making use of no_match_error.

katre

Sorry for the delay: a few comments but overall this looks reasonable.

katre · 2025-02-07T18:37:06Z

src/main/java/com/google/devtools/build/lib/analysis/test/TestConfiguration.java

@@ -332,16 +332,6 @@ public static class TestOptions extends FragmentOptions {
        help = "If true, undeclared test outputs will be archived in a zip file.")
    public boolean zipUndeclaredTestOutputs;

-    @Option(


Why is this moving to CoreOptions?

I added a comment.

katre · 2025-02-07T18:41:51Z

src/main/java/com/google/devtools/build/lib/cmdline/LabelConstants.java

@@ -62,4 +62,8 @@ public class LabelConstants {
  // As a result, external repository runfiles are symlinked to:
  // $runfiles_root/$workspace_name/../$repo_name/<path>, i.e. $runfiles_root/$repo_name/<path>.
  public static final PathFragment EXTERNAL_RUNFILES_PATH_PREFIX = PathFragment.create("..");
+
+  // The label of the toolchain type to add to the default "test" exec group.
+  public static final Label DEFAULT_TEST_TOOLCHAIN_TYPE =


Despite the name of the class, everything else in LabelConstants is a PathFragment.

Can this instead be in PlatformConstants (which I just submitted a minute ago): I can't decide if it's better logically but it will greatly simplify my job to import this.

I moved it and updated the other constant to be a Label.

katre · 2025-02-07T18:46:13Z

src/test/java/com/google/devtools/build/lib/bazel/rules/BUILD

@@ -13,7 +13,6 @@ filegroup(
        "//src/test/java/com/google/devtools/build/lib/bazel/rules/genrule:srcs",
        "//src/test/java/com/google/devtools/build/lib/bazel/rules/java:srcs",
        "//src/test/java/com/google/devtools/build/lib/bazel/rules/python:srcs",
-        "//src/test/java/com/google/devtools/build/lib/bazel/rules/sh:srcs",


Is the removal of the sh tests actually part of this change, or just a drive-by? Can it be a separate PR?

That separate PR is #25142, but it hasn't been merged yet. When it has landed, I can rebase this.

katre · 2025-02-18T14:35:58Z

Now that #25142 is merged, let me know when you've rebased and I will re-review.

fmeum · 2025-02-18T17:54:53Z

@katre Done, PTAL

src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java

src/main/java/com/google/devtools/build/lib/analysis/platform/DeclaredToolchainInfo.java

katre · 2025-02-18T19:08:37Z

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

@@ -305,6 +305,90 @@ def _impl(ctx):
    config.create(
        "embedded_tools/tools/test/BUILD",
        """
+        load(":default_test_toolchain.bzl", "bool_flag", "empty_toolchain")


This is directly copied from tools/test/BUILD.tools, correct? Can we:

Add that as a comment

Simplify this (definitely by removing comments, possibly by removing targets)?

Actually, looking at this further, isn't the minimum the definition of the toolchain type? Do we need the toolchain definition? Can we make it optional?

I dropped the comments. The existing test suite together with the new tests actually exercise all the targets I added. I don't see a way to strip them down while still getting realistic coverage of e.g. --use_target_platform_for_tests.

fmeum force-pushed the test-action-proposal-3 branch 10 times, most recently from 69512ad to 8e3a78e Compare January 31, 2025 14:25

fmeum changed the title ~~Implement "Execution platform selection for test actions in multi-platform builds"~~ Add --@bazel_tools//tools/test:incompatible_use_default_test_toolchain Jan 31, 2025

fmeum marked this pull request as ready for review January 31, 2025 18:03

fmeum requested review from a team, gregestren and fweikert as code owners January 31, 2025 18:03

fmeum requested review from katre and removed request for a team January 31, 2025 18:03

fmeum force-pushed the test-action-proposal-3 branch from a3bed45 to e46820d Compare January 31, 2025 19:22

This was referenced Jan 31, 2025

--incompatible_bazel_test_exec_run_under #23179

Open

native_{run,test}#src should be cfg = "exec" bazelbuild/bazel-skylib#558

Closed

fmeum force-pushed the test-action-proposal-3 branch from e46820d to 5322be9 Compare February 5, 2025 14:59

katre requested changes Feb 7, 2025

View reviewed changes

fmeum force-pushed the test-action-proposal-3 branch from f030b3b to 257f068 Compare February 7, 2025 21:30

fmeum requested a review from katre February 7, 2025 21:30

fmeum force-pushed the test-action-proposal-3 branch 2 times, most recently from 5343690 to 214e062 Compare February 18, 2025 17:54

katre requested changes Feb 18, 2025

View reviewed changes

fmeum added 2 commits February 18, 2025 21:31

Implementation

7dcd270

Address comments

ff976b3

fmeum force-pushed the test-action-proposal-3 branch from 214e062 to ff976b3 Compare February 18, 2025 20:37

fmeum requested a review from katre February 18, 2025 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--@bazel_tools//tools/test:incompatible_use_default_test_toolchain` #25123

Add `--@bazel_tools//tools/test:incompatible_use_default_test_toolchain` #25123

fmeum commented Jan 29, 2025 •

edited

Loading

katre commented Jan 31, 2025

fmeum commented Jan 31, 2025 •

edited

Loading

katre commented Feb 3, 2025

fmeum commented Feb 5, 2025

katre left a comment

katre Feb 7, 2025

fmeum Feb 7, 2025

katre Feb 7, 2025

fmeum Feb 7, 2025

katre Feb 7, 2025

fmeum Feb 7, 2025

katre commented Feb 18, 2025

fmeum commented Feb 18, 2025

katre Feb 18, 2025

katre Feb 18, 2025

fmeum Feb 18, 2025

Add --@bazel_tools//tools/test:incompatible_use_default_test_toolchain #25123

Are you sure you want to change the base?

Add --@bazel_tools//tools/test:incompatible_use_default_test_toolchain #25123

Conversation

fmeum commented Jan 29, 2025 • edited Loading

katre commented Jan 31, 2025

fmeum commented Jan 31, 2025 • edited Loading

katre commented Feb 3, 2025

fmeum commented Feb 5, 2025

katre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katre commented Feb 18, 2025

fmeum commented Feb 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `--@bazel_tools//tools/test:incompatible_use_default_test_toolchain` #25123

Add `--@bazel_tools//tools/test:incompatible_use_default_test_toolchain` #25123

fmeum commented Jan 29, 2025 •

edited

Loading

fmeum commented Jan 31, 2025 •

edited

Loading