Fix test and shell execution OS logic by fmeum · Pull Request #28038 · bazelbuild/bazel

fmeum · 2025-12-17T16:50:18Z

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 renames 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.

As a consequence of this refactoring, it became clear that the shell path was determined by the host OS when the execution OS is Linux or macOS, resulting in a Windows host trying to use the Windows shell path when running e.g. a genrule on Linux. In a surprising turn of events, this behavior is what holds Bazel's own Java integration tests together on Windows - for now. As a result, the fix is accompanied by a manual override on --shell_executable in BuildIntegrationTest for a Windows host.

fmeum · 2025-12-17T16:50:37Z

@bazel-io fork 9.0.0

gemini-code-assist

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.

bharadwaj08-one · 2025-12-18T04:17:53Z

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

fmeum · 2025-12-18T10:37:08Z

@bharadwaj08-one Fixed!

gregestren · 2026-01-09T18:20:56Z

Windows CI failures might be relevant?

fmeum · 2026-01-10T09:48:08Z

@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 · 2026-01-11T11:20:45Z

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

fmeum · 2026-01-24T19:13:32Z

@gregestren I worked around the test failures for now by preserving the preexisting behavior. PTAL.

fmeum · 2026-02-12T09:27:13Z

@gregestren friendly ping

gregestren · 2026-02-20T04:50:42Z

+  @VisibleForTesting
+  public static final ImmutableMap<OS, PathFragment> SHELL_EXECUTABLES =
      ImmutableMap.<OS, PathFragment>builder()
          .put(OS.WINDOWS, PathFragment.create("c:/msys64/usr/bin/bash.exe"))


Is this related to your thoughts on refactoring shell toolchain selection? i.e. why do we need this hard-coded os -> shell mapping in the first place?

Exactly, this mapping should really be replaced by a toolchain type. But that requires an incompatible change that adds the toolchain requirement to all rules using ctx.actions.run_shell.

gregestren

Thanks for the helpful walkthrough!

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 renames 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. As a consequence of this refactoring, it became clear that the shell path was determined by the host OS when the execution OS is Linux or macOS, resulting in a Windows host trying to use the Windows shell path when running e.g. a genrule on Linux. In a surprising turn of events, this behavior is what holds Bazel's own Java integration tests together on Windows - for now. As a result, the fix is accompanied by a manual override on `--shell_executable` in `BuildIntegrationTest` for a Windows host. Closes bazelbuild#28038. PiperOrigin-RevId: 874626714 Change-Id: Icae167c5e58309258b3c73bac99796a512d2f26d

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 renames 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. As a consequence of this refactoring, it became clear that the shell path was determined by the host OS when the execution OS is Linux or macOS, resulting in a Windows host trying to use the Windows shell path when running e.g. a genrule on Linux. In a surprising turn of events, this behavior is what holds Bazel's own Java integration tests together on Windows - for now. As a result, the fix is accompanied by a manual override on `--shell_executable` in `BuildIntegrationTest` for a Windows host. Closes #28038. PiperOrigin-RevId: 874626714 Change-Id: Icae167c5e58309258b3c73bac99796a512d2f26d Commit 79fb843 Co-authored-by: Fabian Meumertzheim <[email protected]>

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 renames 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. As a consequence of this refactoring, it became clear that the shell path was determined by the host OS when the execution OS is Linux or macOS, resulting in a Windows host trying to use the Windows shell path when running e.g. a genrule on Linux. In a surprising turn of events, this behavior is what holds Bazel's own Java integration tests together on Windows - for now. As a result, the fix is accompanied by a manual override on `--shell_executable` in `BuildIntegrationTest` for a Windows host. Closes bazelbuild#28038. PiperOrigin-RevId: 874626714 Change-Id: Icae167c5e58309258b3c73bac99796a512d2f26d

fmeum requested a review from a team as a code owner December 17, 2025 16:50

fmeum requested review from mai93 and tjgq and removed request for a team and mai93 December 17, 2025 16:50

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

bazel-io mentioned this pull request Dec 17, 2025

[9.1.0] Fix test execution OS logic #28039

Closed

gemini-code-assist Bot reviewed Dec 17, 2025

View reviewed changes

fmeum force-pushed the fix-test-exec-platform branch from ed76374 to 0cffea6 Compare December 17, 2025 17:21

fmeum requested a review from lberki as a code owner December 17, 2025 17:21

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 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 requested a review from a team as a code owner December 18, 2025 10:15

fmeum force-pushed the fix-test-exec-platform branch from 373ad61 to ebe8285 Compare December 18, 2025 10:28

fmeum force-pushed the fix-test-exec-platform branch 2 times, most recently from 4a5ee51 to 17e9685 Compare December 18, 2025 10:55

fmeum requested review from Wyverald and meteorcloudy as code owners December 18, 2025 15:54

fmeum force-pushed the fix-test-exec-platform branch 2 times, most recently from 3562416 to e3aa5f9 Compare December 18, 2025 15:57

fmeum mentioned this pull request Dec 18, 2025

Choose command length limits based on exec OS rather than host OS #28052

Closed

fmeum removed request for Wyverald and meteorcloudy December 18, 2025 15:58

bharadwaj08-one added the awaiting-review PR is awaiting review from an assigned reviewer label Dec 19, 2025

fmeum added 2 commits January 9, 2026 12:16

Refactor and fix

6facd76

Simplify

bc82e10

fmeum force-pushed the fix-test-exec-platform branch from e3aa5f9 to bc82e10 Compare January 9, 2026 11:30

fmeum requested a review from gregestren January 9, 2026 11:34

gregestren approved these changes Jan 9, 2026

View reviewed changes

Attempt fix

ecc7700

fmeum requested a review from gregestren January 10, 2026 07:20

Add shell executables

01f1209

Force Windows shell path

1a66e4a

fmeum changed the title ~~Fix test execution OS logic~~ Fix test and shell execution OS logic Jan 24, 2026

Fix more tests

103306f

gregestren reviewed Feb 20, 2026

View reviewed changes

Address comments

ce8e2b6

fmeum requested a review from gregestren February 20, 2026 15:48

gregestren approved these changes Feb 20, 2026

View reviewed changes

gregestren 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 Feb 20, 2026

iancha1992 removed 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 Feb 20, 2026

copybara-service Bot closed this in 79fb843 Feb 24, 2026

github-actions Bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Feb 24, 2026

bazel-io mentioned this pull request Feb 25, 2026

[9.1.0] Fix test and shell execution OS logic (https://github.com/bazelbuild/bazel/pull/28038) #28801

Merged

Conversation

fmeum commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented Dec 17, 2025

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

bharadwaj08-one commented Dec 18, 2025

Uh oh!

fmeum commented Dec 18, 2025

Uh oh!

gregestren commented Jan 9, 2026

Uh oh!

fmeum commented Jan 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented Jan 11, 2026

Uh oh!

fmeum commented Jan 24, 2026

Uh oh!

fmeum commented Feb 12, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gregestren Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

fmeum Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gregestren left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fmeum commented Dec 17, 2025 •

edited

Loading

fmeum commented Jan 10, 2026 •

edited

Loading