Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4086ec990f
ℹ️ 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".
.github/workflows/bazel.yml
Outdated
|
|
||
| - name: Check MODULE.bazel.lock is up to date | ||
| if: matrix.os == 'ubuntu-24.04' && matrix.target == 'x86_64-unknown-linux-gnu' | ||
| if: github.event_name != 'pull_request' && matrix.os == 'ubuntu-24.04' && matrix.target == 'x86_64-unknown-linux-gnu' |
There was a problem hiding this comment.
Run lockfile consistency check for pull requests
Adding github.event_name != 'pull_request' here disables the only place this workflow runs ./scripts/check-module-bazel-lock.sh on PRs, so lockfile drift can now merge undetected and only fail after landing on main. In practice, a PR that updates Cargo.lock/MODULE.bazel without regenerating MODULE.bazel.lock will no longer be blocked pre-merge, which can leave mainline CI red and require a follow-up fix commit.
Useful? React with 👍 / 👎.
c516e41 to
df257ba
Compare
ada573c to
a550414
Compare
Why
This PR is the current, consolidated follow-up to the earlier Windows Bazel attempt in #11229. The goal is no longer just to get a tiny Windows smoke job limping along: it is to make the ordinary Bazel CI path usable on
windows-latestforx86_64-pc-windows-gnullvm, with the same broad//...test shape that macOS and Linux already use.The earlier smoke-list version of this work was useful as a foothold, but it was not a good long-term landing point. Windows Bazel kept surfacing real issues outside that allowlist:
FINDSTR: Cannot open D:MANIFEST, which broke Bazel test launchers even when the manifest file existed.rules_rs,rules_rust, LLVM extraction, and Abseil still neededwindows-gnullvm-specific fixes for our hermetic toolchain.rusty_v8does not ship Windows GNU artifacts in the same shape we need, and Bazel's in-tree V8 build needed a set of Windows GNU portability fixes.Windows performance pressure also pushed this toward a full solution instead of a permanent smoke suite. During this investigation we hit targets such as
//codex-rs/shell-command:shell-command-unit-teststhat were much more expensive on Windows because they repeatedly spawn real PowerShell parsers (see #16057 for one concrete example of that pressure). That made it much more valuable to get the real Windows Bazel path working than to keep iterating on a narrowly curated subset.The net result is that this PR now aims for the same CI contract on Windows that we already expect elsewhere: keep standalone
//third_party/v8:allout of the ordinary Bazel lane, but allow V8 consumers under//codex-rs/...to build and test transitively through//....What Changed
CI and workflow wiring
windows-latest/x86_64-pc-windows-gnullvmBazel matrix entry in.github/workflows/bazel.ymlD:\band enablegit config --global core.longpaths truein.github/actions/setup-bazel-ci/action.yml//...while excluding only standalone//third_party/v8:alltargets from the normal laneToolchain and module support for
windows-gnullvmrules_rssowindows-gnullvmis modeled as a distinct Windows exec/toolchain platform instead of collapsing into the generic Windows shaperules_rustbuild-script environment handling so llvm-mingw build-script probes do not inherit unsupported-fstack-protector*flagswindows-gnullvmtoolchain instead of taking an incompatible MinGW pthread pathWindows test-launch and binary-behavior fixes
workspace_root_test_launcher.bat.tplto read the runfiles manifest directly instead of shelling out tofindstr, which was the source of theD:MANIFESTfailures on the GitHub Windows runnerdefs.bzlso Bazel-built binaries that pull in V8 behave correctly both under normal builds and underbazel test.bazelrcV8 /
rusty_v8Windows GNU supportpatches/BUILD.bazel/MODULE.bazelthird_party/v8/BUILD.bazelto build Windows GNU static archives in-tree instead of aliasing them to the MSVC prebuiltsrusty_v8does not currently publish a Windows GNU binding artifactTesting
Bazelworkflow plusv8-canary, since the hard parts are Windows-specific and depend on real GitHub runner behaviorv8-canaryin run 23675590453windows-latest/x86_64-pc-windows-gnullvmBazel job with the ordinary//...path, not the earlier Windows smoke allowlistStack created with Sapling. Best reviewed with ReviewStack.