Conversation
![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: 48948ea9e8
ℹ️ 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".
| IFS=';' read -r -a windows_path_entries <<< "${PATH}" | ||
| for path_entry in "${windows_path_entries[@]}"; do | ||
| case "${path_entry}" in |
There was a problem hiding this comment.
On Windows CI this wrapper is invoked with shell: bash (.github/workflows/bazel.yml), so PATH is provided in MSYS format (colon-delimited /c/... entries), which is why the script already has to convert which node output via cygpath -w earlier. Splitting PATH on ; and matching only C:\... patterns means the allow-list will typically stay empty, so the new cache-stable PATH branch is skipped and Bazel still receives the full volatile PATH, defeating the cache-stabilization change. Convert PATH to Windows form first (or match MSYS-form paths) before filtering.
Useful? React with 👍 / 👎.
| elif [[ -n "${PATH:-}" ]]; then | ||
| post_config_bazel_args+=("--action_env=PATH" "--host_action_env=PATH" "--test_env=PATH") | ||
| fi |
There was a problem hiding this comment.
I think you want to remove this part, since this shouldn't apply to non-windows platforms
bolinfest
changed the title
bolinfest
requested review from
keith and
zbarsky-openai
and removed request for
keith
bolinfest
changed the title
4 participants
Why
The BuildBuddy runs for PR #19086 and the later
mainbuild had the same source tree, but their Windows Bazel action and test cache keys did not line up. Comparing the downloaded execution logs showed the full GitHub-hosted Windows runnerPATHhad changed fromapache-maven-3.9.14toapache-maven-3.9.15.This repo is not using Maven; the Maven entry was just ambient hosted-runner state. The problem was that Windows Bazel CI was still forwarding the whole runner
PATHinto Bazel via--action_env=PATH,--host_action_env=PATH, and--test_env=PATH, which made otherwise reusable cache entries sensitive to unrelated runner image churn.After discussion with the Bazel and BuildBuddy folks, the better shape for this change was to stop asking Bazel to inherit the ambient Windows
PATHand instead compute one explicit cache-stablePATHin the Windows setup action that already prepares the CI toolchain environment.What
PATHpassthrough from.bazelrcCODEX_BAZEL_WINDOWS_PATHfrom.github/actions/setup-bazel-ci/action.yml.github/scripts/compute-bazel-windows-path.ps1so the allow-list is easier to review and document.github/scripts/run-bazel-ci.shto require that explicit value and forward it to Bazel action, host action, and test environmentsCODEX_BAZEL_WINDOWS_PATHin the setup step to simplify cache-key debuggingWindows
%PATH%On the Windows CI jobs for this PR, note the
%CODEX_BAZEL_WINDOWS_PATH%we derive is quite long, and many of the path entries have version numbers, so upgrading any of them would result in a full rebuild because the change to%PATH%would invalidate the existing cache entries:Verification
bash -n .github/scripts/run-bazel-ci.shruby -e 'require "yaml"; YAML.load_file(ARGV[0])' .github/actions/setup-bazel-ci/action.yml.github/scripts/compute-bazel-windows-path.ps1PATHin PowerShell; the allow-list retained MSVC, Git, PowerShell, Node, Windows, and DotSlash entries while dropping Maven