Skip to content

Fix test execution OS logic #28038

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
fmeum wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fix test execution OS logic #28038

fmeum wants to merge 5 commits into from
+277 −155

Conversation

@fmeum
Copy link
Collaborator

@fmeum fmeum commented Dec 17, 2025 •
edited
Loading

The execution OS is determined by the specified test exec group, which can differ from the test exec group as well as the test rule's default execution group.

Also rename some helper methods to make this class of bugs less likely and clean up ConstraintConstants to support the macos alias for the darwin OS constraint value.

@fmeum fmeum requested a review from a team as a code owner December 17, 2025 16:50
@fmeum fmeum requested review from mai93 and tjgq and removed request for a team and mai93 December 17, 2025 16:50
@github-actions github-actions bot added team-Configurability platforms, toolchains, cquery, select(), config transitions awaiting-review PR is awaiting review from an assigned reviewer labels Dec 17, 2025
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 17, 2025

@bazel-io fork 9.0.0

@bazel-io bazel-io mentioned this pull request Dec 17, 2025
Open
gemini-code-assist[bot]
gemini-code-assist bot reviewed Dec 17, 2025
View reviewed changes
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request correctly determines the execution OS for tests by using the execution platform associated with the specific test execution group, rather than a default platform. The changes involve refactoring how the ActionOwner and its corresponding PlatformInfo are obtained and passed down to where the execution OS is needed. This fixes a bug where the wrong OS was assumed for tests with custom execution groups. The changes are well-structured and include new tests that validate the corrected logic for different scenarios, including exec_group on the test rule and ExecutionInfo from the implementation. The code is cleaner and more correct.

@fmeum fmeum force-pushed the fix-test-exec-platform branch from ed76374 to 0cffea6 Compare December 17, 2025 17:21
@fmeum fmeum requested a review from lberki as a code owner December 17, 2025 17:21
@fmeum fmeum force-pushed the fix-test-exec-platform branch 2 times, most recently from 24bb4be to fad452c Compare December 17, 2025 17:25
@bharadwaj08-one
Copy link

bharadwaj08-one commented Dec 18, 2025

@fmeum Could you please take a look at the failing checks?

@bharadwaj08-one bharadwaj08-one added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Dec 18, 2025
@fmeum fmeum requested a review from a team as a code owner December 18, 2025 10:15
@fmeum fmeum force-pushed the fix-test-exec-platform branch from 373ad61 to ebe8285 Compare December 18, 2025 10:28
@fmeum
Copy link
Collaborator Author

fmeum commented Dec 18, 2025

@bharadwaj08-one Fixed!

@fmeum fmeum force-pushed the fix-test-exec-platform branch 2 times, most recently from 4a5ee51 to 17e9685 Compare December 18, 2025 10:55
@fmeum fmeum force-pushed the fix-test-exec-platform branch 2 times, most recently from 3562416 to e3aa5f9 Compare December 18, 2025 15:57
@fmeum fmeum mentioned this pull request Dec 18, 2025
Open
@bharadwaj08-one bharadwaj08-one added the awaiting-review PR is awaiting review from an assigned reviewer label Dec 19, 2025
@bharadwaj08-one bharadwaj08-one removed the awaiting-user-response Awaiting a response from the author label Dec 19, 2025
@fmeum fmeum mentioned this pull request Dec 20, 2025
Open
@fmeum fmeum added the soft-release-blocker Soft release blockers that are nice to have, but shouldn't block the release if it's the last one. label Jan 8, 2026
@lberki lberki requested review from gregestren and removed request for lberki and tjgq January 8, 2026 19:42
@lberki
Copy link
Contributor

lberki commented Jan 8, 2026

I'll bounce this over to @gregestren ; firstly, it's quite late here, secondly, he is much more familiar with the intricacies of platform selection than I am. Superficially, it looks good to me, but I'd rather someone say that non-superficially also.

gregestren
gregestren reviewed Jan 8, 2026
View reviewed changes
src/test/java/com/google/devtools/build/lib/analysis/test/TestActionBuilderTest.java
)
return [
DefaultInfo(executable = script),
testing.ExecutionInfo(exec_group = "alternative_test"),
Copy link
Contributor

@gregestren gregestren Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the context is #15664.

Does ExecutionInfo need to be anything more than a reference to an exec group now? i.e. is requirements still important? The main difference is ExecutionInfo can dynamically set them in the rule implementation context?

This reads subtle to me. It'd be great to clarify the relationships in https://bazel.build/extending/exec-groups and/or https://bazel.build/reference/test-encyclopedia#execution-platform. And why one would choose one over the other.

Did a specific experience motivate this that exec groups couldn't handle?

Copy link
Collaborator Author

@fmeum fmeum Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. To be honest, I think that #15664 should not have landed, at least not until test actions have become less special than they currently are. Note that the current PR doesn't touch that feature, it just ensures that the new logic works correctly when it's used.

Copy link
Contributor

@gregestren gregestren Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged, and I realize this PR is just a fix. I was just trying to get a better intuition of the wider context.

fmeum added 3 commits January 9, 2026 12:16
@fmeum
Fix test execution OS logic
cdf7a80 
The execution OS is determined by the specified test exec group, which can differ from the `test` exec group as well as the test rule's default execution group.
@fmeum
Refactor and fix
6facd76
@fmeum
Simplify
bc82e10
@fmeum fmeum force-pushed the fix-test-exec-platform branch from e3aa5f9 to bc82e10 Compare January 9, 2026 11:30
@fmeum fmeum requested a review from gregestren January 9, 2026 11:34
@gregestren
Copy link
Contributor

gregestren commented Jan 9, 2026

Windows CI failures might be relevant?

@fmeum fmeum requested a review from gregestren January 10, 2026 07:20
@fmeum
Copy link
Collaborator Author

fmeum commented Jan 10, 2026 •
edited
Loading

@gregestren As far as I can tell these tests are failing because Bazel's integration test setup pretends that the host platform is Linux even when running tests on Windows. The logic in ShToolchain was subtly incorrect and fell back to using the host OS to determine the shell path when the exec platform is Linux and that is needed to work with that test setup. Do you have an idea how the setup can be fixed?

@lberki in case you have an idea :-)

@fmeum
Copy link
Collaborator Author

fmeum commented Jan 11, 2026

#28224 shows the failures with the proper host platform set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gregestren gregestren Awaiting requested review from gregestren

1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting-review PR is awaiting review from an assigned reviewer soft-release-blocker Soft release blockers that are nice to have, but shouldn't block the release if it's the last one. team-Configurability platforms, toolchains, cquery, select(), config transitions

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@fmeum @bharadwaj08-one @lberki @gregestren