[bazel] Sandbox bash and git on Windows with tools/bazel.bat - #9341
Open
AustinSchuh wants to merge 16 commits into
Open
[bazel] Sandbox bash and git on Windows with tools/bazel.bat#9341AustinSchuh wants to merge 16 commits into
AustinSchuh wants to merge 16 commits into
Conversation
Bazelisk delegates to tools/bazel.bat on Windows, which lets us stop depending on whatever bash, sh, and git happen to be installed on the developer's machine. The wrapper downloads a private, SHA256 verified copy of PortableGit into %USERPROFILE%\.cache\bazel\portable_git, points BAZEL_SH, BAZEL_GIT, and GIT_BIN_PATH at it, and then clears the rest of the environment before starting Bazel so that stray variables can't leak into actions. It also shortens %USERPROFILE% to its 8.3 form so that a user name containing a space doesn't break paths downstream. BAZEL_VC, BAZEL_VS, BAZEL_VC_FULL_VERSION, and BAZEL_WINSDK_FULL_VERSION survive the scrub so that a non-default Visual Studio install can still be pointed at explicitly. .bat files are marked -text so that their CRLF line endings round-trip unchanged rather than being normalized on checkin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FkRkDjJmAEa54pYn3oWAN6 Signed-off-by: Austin Schuh <austin.linux@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bafb24a05
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Stop on a failed PortableGit extraction. The self-extractor's exit status was ignored and then clobbered by the rmdir of the temp directory, so an extraction that ran out of disk or got blocked by security software still reported success and launched Bazel against a missing or partial Git runtime. Capture the status, sanity check that git.exe and bash.exe actually landed, and delete the incomplete cache on the way out so that the next invocation retries instead of trusting it. Keep a few more variables through the scrub. rules_cc finds vswhere.exe under %ProgramFiles(x86)%, publishing_rule.bzl reads WPI_PUBLISH_CLASSIFIER_FILTER straight out of the environment, and nothing can fetch from behind a corporate proxy without HTTP_PROXY and friends. Comments inside parenthesized blocks now use rem rather than ::, since a label inside a block is a cmd.exe parser hazard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FkRkDjJmAEa54pYn3oWAN6
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe8186e32b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Key the cache directory by the pinned version. The presence of cmd\git.exe short circuited the whole download, so bumping the pin left everyone who had already bootstrapped on the old release forever, including after a security driven update. Install through a staging directory private to the invocation and publish it with a single rename. Two Bazelisk invocations racing on a cold cache used to share one temp directory and one destination, so they could delete each other's download, extract over each other, or tear down a cache the other had started using. Nothing partial is ever visible under the cache path now, and the loser of the race keeps the winner's copy, which passed the same checksum and layout checks. Turn delayed expansion back off before forwarding %* to Bazel. A "!" in any argument -- a remote cache header or a proxy password, say -- was being eaten when the command line got reparsed. Point git at nonexistent global and system config files. HOME has to keep pointing at the real profile because Bazel reads the user .bazelrc from there, but that also handed the pinned git the developer's ~/.gitconfig, where core.autocrlf or url.*.insteadOf would make fetches machine dependent. Document the one deliberate hole in the scrub: the original PATH is still appended behind the system and pinned-git directories, because dropping it breaks anyone whose tools live elsewhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FkRkDjJmAEa54pYn3oWAN6
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de5a0883dc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
An environment value containing "!" was being mangled on its way through the save and restore, because delayed expansion was enabled for the whole script and applies to whatever percent expansion just produced. A proxy password like http://user:p!ss@proxy came out the other side different from what the developer set, which is exactly the case the proxy passthrough was added for. Turn delayed expansion off instead of on, explicitly rather than by inheritance so that a shell started with /V:ON or a machine with the registry default flipped behaves the same, and move the two places that needed to read a variable assigned in the same block into subroutines. The separate scope that was wrapping the handoff is now redundant. Key the git cache by the release tag and checksum rather than the package version alone, so that a rebuild of the same upstream version -- v2.44.0.windows.1 to .2, say -- is picked up rather than being short circuited by the existing install. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FkRkDjJmAEa54pYn3oWAN6
Member
|
As much as we love batch scripts, would PowerShell be simpler? |
Contributor
Author
|
I'm not sure we get a choice. Bazel automatically loads //tools:bazel.bat. Unless I'm missing something. |
Member
|
Doesn't Bazelisk support |
Member
|
According to https://github.com/bazelbuild/bazelisk#toolsbazel, it does. |
Contributor
Author
|
Yea, it does. I didn't know it existed until you brought it up. Digging in some more, the string quoting process is a fair amount easier with bazel.bat, and bazel.ps1 support was released the beginning of this year. Every argument which gets passed through with something like a space will need to be re-quoted. Those two things make me prefer the current bazel.bat, though I'm happy to convert it over. |
auscompgeek
reviewed
Aug 24, 2026
Rewriting USERPROFILE to its 8.3 form unconditionally hands a mangled path to everyone, including the vast majority of users whose profile directory was never a problem -- 8.3 shortening applies to long names, not just to spaces. Do it only when there is a space to get rid of, and warn when the volume has 8.3 name generation disabled and there is nothing to fall back to. Also write down why the environment scrub is worth doing when --incompatible_strict_action_env already fixes the environment actions run under: it doesn't cover repository rules, which read the client environment in full. rules_cc declares thirty-odd variables as inputs to cc_autoconf and hands the whole environment to the subprocesses it runs to locate MSVC, so anything that differs between two machines ends up in local_config_cc and from there in every compile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FkRkDjJmAEa54pYn3oWAN6
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ee4c11143
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
--experimental_strict_repo_env limits repository rules and module extensions to PATH, PATHEXT, and the variables named by --repo_env, which is what the wrapper's environment purge was there to approximate. Bazel does it better: it is a whitelist rather than a blocklist, and it is scoped to repository rules, so it doesn't take the environment away from bazel run targets. Losing credentials and the like out from under a script that bazel run invokes is a real cost, and one that is hard to connect back to a wrapper the user never sees. So turn the flag on for Windows, name the variables the wrapper exports in .bazelrc, and delete the save, purge, and restore -- about seventy lines, along with its allowlist to keep up to date. What is left is the part Bazel can't do: pin a git and a bash, and put them at the front of PATH, which is one of the two variables strict repo env still inherits. Everything MSVC discovery reads has a documented fallback in windows_cc_configure.bzl -- SYSTEMROOT and TMP to C:\Windows, PROGRAMFILES(X86) to the standard location -- and setup_vc_env_vars already runs vcvarsall.bat under an environment it builds itself, so dropping the passthrough for those doesn't change how it resolves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FkRkDjJmAEa54pYn3oWAN6
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67cb1725d9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
GIT_CONFIG_GLOBAL and friends were exported by the wrapper, so every process Bazel launches inherited them, `bazel run` targets included. shared/bazel/copybara/run_copybara.py runs `bazel run //:copybara` with a git destination URL it then pushes to, and pointing git at an empty global config takes away both the credential helper it authenticates with and the identity it commits as. Move all three into --repo_env lines in .bazelrc, where they reach the repository rules they were meant for and nothing else. Same reason the environment purge came out one commit ago: what a wrapper does to the whole process tree is not what it looks like it is doing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FkRkDjJmAEa54pYn3oWAN6
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1241b543e3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
publishing_rule.bzl declares WPI_PUBLISH_CLASSIFIER_FILTER as an environ input and reads it out of the environment, and .bazelrc only sets it explicitly under the ci configs. So the shell variable is how a developer picks a subset of the artifact variants locally, and turning on --experimental_strict_repo_env stopped it from arriving on Windows. An unset filter means no filter at all -- publishing.bzl falls back to the full artifact set -- so the symptom is a much longer build rather than an error. Name only, so the value comes from the shell the way it does on Linux and macOS. CI is unaffected: it passes --config=ci on the command line, which expands after the rc files and sets the variable explicitly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FkRkDjJmAEa54pYn3oWAN6
auscompgeek
reviewed
Aug 31, 2026
auscompgeek
left a comment
auscompgeek
left a comment
Member
There was a problem hiding this comment.
I think Claude has left behind inaccurate comments.
Neither variable is read by anything. They appear nowhere in Bazel's embedded tools, nowhere in Bazel's own source, and nowhere in any external repository this build fetches; they came along from the wrapper this was adapted from. BAZEL_SH is the real one -- rules_shell's sh_config.bzl and rules_cc's windows_cc_configure.bzl both read it -- and git is found through PATH, which strict repo env passes through. Take the two suggested comment trims while here. The claim that CI relied on the --repo_env line in a bazelrc was wrong: the workflow passes the classifier filter explicitly on the command line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FkRkDjJmAEa54pYn3oWAN6
Co-authored-by: David Vo <auscompgeek@users.noreply.github.com>
Co-authored-by: David Vo <auscompgeek@users.noreply.github.com>
http.sslBackend=openssl and http.sslVerify=true came along from the wrapper this was adapted from. Nothing in this repository is fetched over git, and where a git_repository would be the consumer Bazel discards the variable before it ever runs git: _GIT_LOCAL_ENV_VARS in tools/build_defs/repo/git_worker.bzl unsets GIT_CONFIG_PARAMETERS along with the rest of `git rev-parse --local-env-vars`. GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM stay. Those aren't on that list, so they still do what they're there for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FkRkDjJmAEa54pYn3oWAN6
… bazel-windows-bash-sandbox
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b97eb0aba
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Move the pin from Git for Windows 2.44.0 to 2.55.0(5). 2.44.0 predates the May 2024 clone fixes, CVE-2024-32002 among them, and since the wrapper puts the pinned copy at the front of PATH it hands that git to everything Bazel launches -- `bazel run //:copybara` clones with it -- and shadows a patched system git while doing so. Checksum taken from the release asset digest and confirmed by downloading it. Invoke Bazelisk by name in the Windows CI jobs. The runner image installs a real bazel.exe through Chocolatey and Bazelisk through npm, and the npm package provides a `bazel` binary of its own, so which one `bazel` resolves to comes down to PATH order inside the image. Only Bazelisk dispatches tools/bazel.bat, so leaving that to chance means CI may never exercise any of this. Naming bazelisk settles it. Stop filtering certutil's output by text. A cache path containing "CertUtil" would match the header line as well as the completion line, and skip=1 would then discard the digest instead, leaving an empty hash that fails the comparison every time. Take the first line after the header instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FkRkDjJmAEa54pYn3oWAN6
github-actions
Bot
added
the
build
auscompgeek
reviewed
Sep 3, 2026
A real bazel.exe ignores .bazelversion entirely, so the runner image's Bazel and Bazelisk are interchangeable right up until the wrapper matters. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FkRkDjJmAEa54pYn3oWAN6
PeterJohnson
pushed a commit
that referenced
this pull request
Sep 5, 2026
See #9341 (comment) The `setup-bazel` action handles putting the output root in an appropriate place, so we don't have to do it ourselves. Also disable saving to GitHub Actions cache from PRs to avoid using up the limited cache space.
…ndbox setup-bazel now runs on every platform (wpilibsuite#9383), so Windows already goes through Bazelisk and gets an output base on D: without the if statement this branch had added. Take main's workflow as-is. Signed-off-by: Austin Schuh <austin.linux@gmail.com>
github-actions
Bot
removed
the
build
auscompgeek
approved these changes
Sep 6, 2026
auscompgeek
left a comment
auscompgeek
left a comment
Member
There was a problem hiding this comment.
LGTM. The PR description needs updating though.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Bazelisk delegates to tools/bazel.bat on Windows, which lets us stop depending on whatever bash, sh, and git happen to be installed on the developer's machine. The wrapper downloads a private, SHA256 verified copy of PortableGit into %USERPROFILE%.cache\bazel\portable_git, points BAZEL_SH and PATH at it, and then clears the rest of the environment before starting Bazel so that stray variables can't leak into actions. It also shortens %USERPROFILE% to its 8.3 form so that a user name containing a space doesn't break paths downstream.
BAZEL_VC, BAZEL_VS, BAZEL_VC_FULL_VERSION, and BAZEL_WINSDK_FULL_VERSION survive the scrub so that a non-default Visual Studio install can still be pointed at explicitly.
.bat files are marked -text so that their CRLF line endings round-trip unchanged rather than being normalized on checkin.