Conversation
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9dd4e4075
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Bojun-Vvibe
added a commit
to Bojun-Vvibe/oss-contributions
that referenced
this pull request
May 1, 2026
- anomalyco/opencode#25265 merge-as-is: truecolor capability gate for logo subpixel rendering - openai/codex#20585 merge-after-nits: cross-compile Windows Bazel CI mirroring macOS pattern - openai/codex#20575 merge-after-nits: app-server thread reads migrated to ThreadStore
bdbe9b6 to
97731fc
Compare
b5683ea to
424b183
Compare
starr-openai
approved these changes
May 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Status
This is the Bazel PR-CI cross-compilation follow-up to #20485. It is intentionally split from the Cargo/cargo-xwin release-build PoC so #20485 can stay as the historical release-build exploration. The unrelated async-utils test cleanup has been moved to #20686, so this PR is focused on the Windows Bazel CI path.
The intended tradeoff is now explicit in
.github/workflows/bazel.yml: pull requests get the fast Windows cross-compiled Bazel test leg, while post-merge pushes tomainrun both that fast cross leg and a fully native Windows Bazel test leg. The native main-only job keeps full V8/code-mode coverage and gets a 40-minute timeout because it is less latency-sensitive than PR CI. All other Bazel jobs remain at 30 minutes.Why
Windows Bazel PR CI currently does the expensive part of the build on Windows. A native Windows Bazel test job on
maincompleted in about 28m12s, leaving very little headroom under the 30-minute job timeout and making Windows the slowest PR signal.#20485 showed that Windows cross-compilation can be materially faster for Cargo release builds, but PR CI needs Bazel because Bazel owns our test sharding, flaky-test retries, and integration-test layout. This PR applies the same high-level shape we already use for macOS Bazel CI: compile with remote Linux execution, then run platform-specific tests on the platform runner.
The compromise is deliberately signal-aware: code-mode/V8 changes are rare enough that PR CI can accept losing the direct V8/code-mode smoke-test signal temporarily, while
mainstill runs the native Windows job post-merge to catch that class of regression. A follow-up PR should investigate making the cross-built Windows gnullvm V8 archive pass the direct V8/code-mode tests so this tradeoff can eventually go away.What Changed
ci-windows-crossBazel config that targetsx86_64-pc-windows-gnullvm, uses Linux RBE for build actions, and keepsTestRunneractions local on the Windows runner.windows_x86_64_gnullvm,windows_x86_64_msvc, and a bridge toolchain that lets gnullvm test targets execute under the Windows MSVC host platform.--windows-cross-compileand--remote-download-toplevel.test-windows-native-mainjob that runs only forpushevents onrefs/heads/main, uses the native Windows Bazel path, includes V8/code-mode smoke tests, and hastimeout-minutes: 40.BUILDBUDDY_API_KEYon the previous local Windows MSVC-host fallback, including--host_platform=//:local_windows_msvcand--jobs=8.windows_gnullvm.CARGO_BIN_EXE_*values from runfiles at test runtime, avoiding hard-coded Cargo paths and duplicate test runfiles.x86_64-pc-windows-gnullvmtarget and Linux remote execution path.INSTA_WORKSPACE_ROOTat runtime when Bazel provides it, because cross-compiled binaries may contain Linux compile-time paths.Command Shape
The fast Windows PR test leg invokes the normal Bazel CI wrapper like this:
With the BuildBuddy secret available on Windows, the wrapper selects
--config=ci-windows-crossand appends the important Windows-cross overrides after rc expansion:--host_platform=//:rbe --shell_executable=/bin/bash --action_env=PATH=/usr/bin:/bin --host_action_env=PATH=/usr/bin:/bin --test_env=PATH=${CODEX_BAZEL_WINDOWS_PATH}The native post-merge Windows job intentionally omits
--windows-cross-compileand does not exclude the V8/code-mode unit targets:Research Notes
The existing macOS Bazel CI config already uses the model we want here: build actions run remotely with
--strategy=remote, butTestRunneractions execute on the macOS runner. This PR mirrors that pattern for Windows with--strategy=TestRunner=local.The important Bazel detail is that
rules_rsis already targetingx86_64-pc-windows-gnullvmfor Windows Bazel PR tests. This PR changes where the build actions execute; it does not switch the Bazel PR test target to Cargo,cargo-nextest, or the MSVC release target.Cargo release builds differ from this Bazel path for V8: the normal Windows Cargo release target is MSVC, and
rusty_v8publishes prebuilt Windows MSVC.lib.gzarchives. The Bazel PR path targetswindows-gnullvm;rusty_v8does not publish a prebuilt Windows GNU/gnullvm archive, so this PR builds that archive in-tree. That Linux-RBE-built gnullvm archive currently crashes in direct V8/code-mode smoke tests, which is why the workflow keeps native Windows coverage onmain.The less obvious Bazel detail is test wrapper selection. Bazel chooses the Windows test wrapper (
tw.exe) from the test action execution platform, not merely from the Rust target triple. The outerworkspace_root_testtherefore declares the default test toolchain and uses the bridge toolchain above so the test action executes on Windows while its inner Rust binary is built for gnullvm.The V8 investigation exposed a Windows-client gotcha: even when an action execution platform is Linux RBE, Bazel can still derive the genrule shell path from the Windows client. That produced remote commands trying to run
C:\Program Files\Git\usr\bin\bash.exeon Linux workers. The wrapper now passes--shell_executable=/bin/bashwith--host_platform=//:rbefor the Windows cross path.The same Windows-client/Linux-RBE boundary also affected
third_party/v8:binding_cc: a multiline genrule command can carry CRLF line endings into Linux remote bash, which failed as$'\r'. That genrule now keeps thesedcommand on one physical shell line while using an explicit Starlark join so the shell arguments stay readable.Verification
Local checks included:
bash -n .github/scripts/run-bazel-ci.sh bash -n workspace_root_test_launcher.sh.tpl ruby -e "require %q{yaml}; YAML.load_file(%q{.github/workflows/bazel.yml}); puts %q{ok}" RUNNER_OS=Linux ./scripts/list-bazel-clippy-targets.sh RUNNER_OS=Windows ./scripts/list-bazel-clippy-targets.sh RUNNER_OS=Linux ./tools/argument-comment-lint/list-bazel-targets.sh RUNNER_OS=Windows ./tools/argument-comment-lint/list-bazel-targets.shThe Linux clippy and argument-comment target lists contain zero
*-windows-cross-binlabels, while the Windows lists still include 47 Windows-cross internal test binaries.CI evidence:
main: success in about 28m12s, https://github.com/openai/codex/actions/runs/25206257208/job/73907325959bazel.yml; CI is rerunning on that focused diff.Follow-Up
A subsequent PR should investigate making a cross-built Windows binary work with V8/code-mode enabled. Likely options are either making the Linux-RBE-built
windows-gnullvmV8 archive correct at runtime, or evaluating whether a Bazel MSVC target/toolchain can reuse the same prebuilt MSVCrusty_v8archive shape that Cargo release builds already use.