Forward `exec_properties` to main test spawn's execution info #24979

fmeum · 2025-01-19T15:33:32Z

This makes exec_properties such as no-remote-exec effective for the main test spawn, where they previously only affected post-processing spawns such as test XML generation.

Pros:

relatively simple (and more precise than tags) way to affect test execution using existing API
provides consistency between test spawns
provides consistency between test spawns and spawns produced by Starlark actions

Cons:

similar to Starlark actions, execution info is "polluted" with exec_properties such as OSFamily and container-image, which shows up in aquery and may increase memory usage
the fact that exec_properties end up in execution info appears to have been an unintentional side effect of adb56cc rather than a conscious design decision
provides functionality may be better addressed by Execution Platform Scoped Spawn Strategies

This makes `exec_properties` such as `no-remote-exec` effective for the main test spawn, where it previously only affected post-processing spawns such as test XML generation.

fmeum · 2025-01-20T21:21:23Z

cc @sluongng @Silic0nS0ldier @EdSchouten @keith

sluongng

This makes sense to me.

cc: @mortenmj @EdSchouten who might be relying platforms with "no-remote-exec" set.

sluongng · 2025-01-21T09:30:49Z

src/test/shell/bazel/remote/remote_execution_test.sh

+  # A my_test target includes 2 actions: 1 build action (a) and 1 test action (b),
+  # with (b) running two spawns (test execution, test XML generation).


Nit: I think it's less confusing to use either spawn or action and not both.
I thought that using the mnemonics directly might help, but afaik the test execution and xml generation have the same "TestRunner" mnemonic?

I think that we need to talk about both (with their precise meaning as in the Bazel codebase, not the user-facing docs) specifically because the bug this PR fixes is not visible on the action level (test actions have the correct execution info set), but shows up on the spawn level (that may or may not correctly inherit this information).

Yes, this is an unfortunately important implementation detail for tests like this.

sluongng · 2025-01-21T09:32:58Z

src/test/shell/bazel/remote/remote_execution_test.sh

-    target_compatible_with = [":has_foo"],
+    exec_properties = {
+        "test.no-remote-exec": "1",
+    },


Nit: I think it would be better to keep the old test setup and add new targets for my_test rule. That would show that the old behavior is unchanged for sh_test(?)

sh_test no longer lives in the Bazel codebase and isn't versioned together with Bazel. Since this test necessarily depends on internals of the rule (the actions it registers), it wouldn't be self-contained if it used that rule.

In fact, the test in its previous state relied on sh_test to register a "build action" that is also backed by a spawn, but sh_test uses ctx.actions.symlink, which is spawn-less. This hid the bug that the different test spawns used different execution infos. Using a custom rule defined within the test avoids this class of issues.

+1 to self-contained tests.

src/test/shell/bazel/remote/remote_execution_test.sh

src/test/java/com/google/devtools/build/lib/analysis/StarlarkExecGroupTest.java

fmeum · 2025-01-21T16:05:44Z

@katre What do you think about the issue of exec info pollution? Should we perhaps filter out exec_properties that aren't valid exec info keys (similar to what we do for tags) before propagating them to exec info? That would of course be a separate change.

fmeum · 2025-01-21T16:05:50Z

@bazel-io fork 8.1.0

katre · 2025-01-21T16:09:47Z

@katre What do you think about the issue of exec info pollution? Should we perhaps filter out exec_properties that aren't valid exec info keys (similar to what we do for tags) before propagating them to exec info? That would of course be a separate change.

Ideally spawn strategies would filter out the properties they are interested in and only forward those. I'm not sure there's a good principled answer to this. As I said in another issue, I think that we need to collapse the distinctions between requirements, exec info, and exec properties (and remove magic tags as much as possible), but no one has the time to take that on (and it's complicated enough by internal blaze code that I'm not sure it's feasible for an external contributor to make headway on).

fmeum · 2025-01-21T17:41:15Z

@bazel-io fork 7.5.0

Silic0nS0ldier · 2025-01-22T01:19:05Z

Going off the description, this makes sense to me. There is a capability missing here however. Specifying exec properties for the ~~text~~ test action (which is indirectly constructed by Bazel) without affecting rule actions.

fmeum · 2025-01-22T06:11:11Z

Specifying exec properties for the text action (which is indirectly constructed by Bazel) without affecting rule actions.

That is possible via the test. prefix. This applies to all test spawns, but that could be solved in the future via an additional exec group.

Silic0nS0ldier · 2025-01-22T22:41:13Z

Ah, was not aware of that. Thanks for letting me know.

fmeum force-pushed the fix-test-toolchain branch from 89398ca to 9dad1d3 Compare January 20, 2025 15:49

Forward exec_properties to main test spawn's execution info

5091e1d

This makes `exec_properties` such as `no-remote-exec` effective for the main test spawn, where it previously only affected post-processing spawns such as test XML generation.

fmeum force-pushed the fix-test-toolchain branch from 9dad1d3 to 5091e1d Compare January 20, 2025 20:32

fmeum changed the title ~~Fix test toolchain~~ Forward exec_properties to main test spawn's execution info Jan 20, 2025

fmeum marked this pull request as ready for review January 20, 2025 21:20

fmeum requested review from a team as code owners January 20, 2025 21:20

fmeum requested review from aranguyen and katre and removed request for a team and aranguyen January 20, 2025 21:20

github-actions bot added team-Configurability platforms, toolchains, cquery, select(), config transitions team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels Jan 20, 2025

sluongng approved these changes Jan 21, 2025

View reviewed changes

Add references

9160b4e

katre approved these changes Jan 21, 2025

View reviewed changes

katre added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jan 21, 2025

bazel-io mentioned this pull request Jan 21, 2025

[8.1.0] Forward exec_properties to main test spawn's execution info #25002

Open

bazel-io mentioned this pull request Jan 21, 2025

[7.5.0] Forward exec_properties to main test spawn's execution info #25003

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Forward `exec_properties` to main test spawn's execution info #24979

Forward `exec_properties` to main test spawn's execution info #24979

fmeum commented Jan 19, 2025 •

edited

Loading

fmeum commented Jan 20, 2025

sluongng left a comment

sluongng Jan 21, 2025

fmeum Jan 21, 2025

katre Jan 21, 2025

sluongng Jan 21, 2025

fmeum Jan 21, 2025

katre Jan 21, 2025

fmeum commented Jan 21, 2025

fmeum commented Jan 21, 2025

katre commented Jan 21, 2025

fmeum commented Jan 21, 2025

Silic0nS0ldier commented Jan 22, 2025 •

edited

Loading

fmeum commented Jan 22, 2025

Silic0nS0ldier commented Jan 22, 2025

		# A my_test target includes 2 actions: 1 build action (a) and 1 test action (b),
		# with (b) running two spawns (test execution, test XML generation).

Forward exec_properties to main test spawn's execution info #24979

Are you sure you want to change the base?

Forward exec_properties to main test spawn's execution info #24979

Conversation

fmeum commented Jan 19, 2025 • edited Loading

fmeum commented Jan 20, 2025

sluongng left a comment

Choose a reason for hiding this comment

sluongng Jan 21, 2025

Choose a reason for hiding this comment

fmeum Jan 21, 2025

Choose a reason for hiding this comment

katre Jan 21, 2025

Choose a reason for hiding this comment

sluongng Jan 21, 2025

Choose a reason for hiding this comment

fmeum Jan 21, 2025

Choose a reason for hiding this comment

katre Jan 21, 2025

Choose a reason for hiding this comment

fmeum commented Jan 21, 2025

fmeum commented Jan 21, 2025

katre commented Jan 21, 2025

fmeum commented Jan 21, 2025

Silic0nS0ldier commented Jan 22, 2025 • edited Loading

fmeum commented Jan 22, 2025

Silic0nS0ldier commented Jan 22, 2025

Forward `exec_properties` to main test spawn's execution info #24979

Forward `exec_properties` to main test spawn's execution info #24979

fmeum commented Jan 19, 2025 •

edited

Loading

Silic0nS0ldier commented Jan 22, 2025 •

edited

Loading