Introduce hermetic MinGW, MSYS and bash executable for Windows#51153
Introduce hermetic MinGW, MSYS and bash executable for Windows#51153JSGette wants to merge 13 commits into
Conversation
JSGette
added
changelog/no-changelog
|
@codex review |
|
![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: 639e56f3bb
ℹ️ 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".
| set "MSYS2_ROOT=%cd%\..\..\external\+msys2_base_repository+msys2_base" | ||
| set "MINGW_ROOT=%cd%\..\..\external\+winlibs_mingw_repository+winlibs_mingw64" |
There was a problem hiding this comment.
Compute paths from stable repo mappings instead of embedding external\+...+... names. The shim currently hardcodes canonical repository directory names, but Bzlmod canonical-name format is explicitly non-API and can change across Bazel versions or extension identity changes; when that happens, the shim won't find bash.exe/toolchain roots and all Windows run_shell/genrule actions fail before execution.
Useful? React with 👍 / 👎.
| if not exist "%MINGW_ROOT%\bin\gcc.exe" ( | ||
| echo bash_shim: hermetic gcc not found at %MINGW_ROOT%\bin\gcc.exe 1>&2 | ||
| echo bash_shim: run 'bazelisk fetch @winlibs_mingw64//...' to materialise it 1>&2 | ||
| exit /b 1 |
There was a problem hiding this comment.
The shim exits if @winlibs_mingw64 is missing even when the action only needs Bash. Since toolchain dependencies are loaded lazily, a Windows build that runs shell actions without resolving the C++ toolchain can hit this check and fail unnecessarily; this turns unrelated shell steps into hard failures and forces an unnecessary fetch/availability requirement for MinGW.
Useful? React with 👍 / 👎.
Files inventory check summaryFile checks results against ancestor 7e031281: Results for datadog-agent_7.81.0~devel.git.151.aa12205.pipeline.114506598-1_amd64.deb:No change detected |
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 7e03128 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | docker_containers_cpu | % cpu utilization | +1.40 | [-1.56, +4.37] | 1 | Logs |
Fine details of change detection per experiment
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | docker_containers_cpu | % cpu utilization | +1.40 | [-1.56, +4.37] | 1 | Logs |
| ➖ | otlp_ingest_metrics | memory utilization | +0.60 | [+0.45, +0.76] | 1 | Logs |
| ➖ | quality_gate_logs | % cpu utilization | +0.52 | [-0.51, +1.55] | 1 | Logs bounds checks dashboard |
| ➖ | ddot_metrics_sum_cumulative | memory utilization | +0.49 | [+0.33, +0.65] | 1 | Logs |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.43 | [+0.37, +0.49] | 1 | Logs bounds checks dashboard |
| ➖ | ddot_metrics_sum_delta | memory utilization | +0.21 | [+0.02, +0.41] | 1 | Logs |
| ➖ | quality_gate_idle | memory utilization | +0.17 | [+0.12, +0.22] | 1 | Logs bounds checks dashboard |
| ➖ | file_to_blackhole_500ms_latency | egress throughput | +0.04 | [-0.36, +0.44] | 1 | Logs |
| ➖ | file_tree | memory utilization | +0.04 | [-0.00, +0.08] | 1 | Logs |
| ➖ | quality_gate_metrics_logs | memory utilization | +0.03 | [-0.21, +0.27] | 1 | Logs bounds checks dashboard |
| ➖ | file_to_blackhole_0ms_latency | egress throughput | +0.03 | [-0.51, +0.56] | 1 | Logs |
| ➖ | ddot_metrics_sum_cumulativetodelta_exporter | memory utilization | +0.03 | [-0.21, +0.26] | 1 | Logs |
| ➖ | ddot_metrics | memory utilization | +0.02 | [-0.18, +0.21] | 1 | Logs |
| ➖ | ddot_logs | memory utilization | +0.01 | [-0.06, +0.07] | 1 | Logs |
| ➖ | otlp_ingest_logs | memory utilization | +0.00 | [-0.09, +0.09] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api | ingress throughput | +0.00 | [-0.20, +0.20] | 1 | Logs |
| ➖ | tcp_dd_logs_filter_exclude | ingress throughput | -0.00 | [-0.10, +0.10] | 1 | Logs |
| ➖ | file_to_blackhole_100ms_latency | egress throughput | -0.01 | [-0.15, +0.14] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api_v3 | ingress throughput | -0.04 | [-0.25, +0.17] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency | egress throughput | -0.14 | [-0.61, +0.32] | 1 | Logs |
| ➖ | tcp_syslog_to_blackhole | ingress throughput | -0.20 | [-0.38, -0.01] | 1 | Logs |
| ➖ | uds_dogstatsd_20mb_12k_contexts_20_senders | memory utilization | -0.24 | [-0.29, -0.20] | 1 | Logs |
| ➖ | docker_containers_memory | memory utilization | -0.34 | [-0.44, -0.24] | 1 | Logs |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | docker_containers_cpu | simple_check_run | 10/10 | 716 ≥ 26 | |
| ✅ | docker_containers_memory | memory_usage | 10/10 | 245.44MiB ≤ 370MiB | |
| ✅ | docker_containers_memory | simple_check_run | 10/10 | 684 ≥ 26 | |
| ✅ | file_to_blackhole_0ms_latency | memory_usage | 10/10 | 0.16GiB ≤ 1.20GiB | |
| ✅ | file_to_blackhole_0ms_latency | missed_bytes | 10/10 | 0B = 0B | |
| ✅ | file_to_blackhole_1000ms_latency | memory_usage | 10/10 | 0.20GiB ≤ 1.20GiB | |
| ✅ | file_to_blackhole_1000ms_latency | missed_bytes | 10/10 | 0B = 0B | |
| ✅ | file_to_blackhole_100ms_latency | memory_usage | 10/10 | 0.17GiB ≤ 1.20GiB | |
| ✅ | file_to_blackhole_100ms_latency | missed_bytes | 10/10 | 0B = 0B | |
| ✅ | file_to_blackhole_500ms_latency | memory_usage | 10/10 | 0.18GiB ≤ 1.20GiB | |
| ✅ | file_to_blackhole_500ms_latency | missed_bytes | 10/10 | 0B = 0B | |
| ✅ | quality_gate_idle | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 144.21MiB ≤ 147MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 738.84KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 2 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 428.23MiB ≤ 495MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.12MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 4 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 179.59MiB ≤ 195MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 264.38MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 334.30 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 4 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 385.61MiB ≤ 430MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | total_bytes_received | 10/10 | 0.94GiB ≤ 1.04GiB | bounds checks dashboard |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_idle, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f48112f1d1
ℹ️ 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".
| filegroup( | ||
| name = tool_name, | ||
| srcs = ["bin/" + tool_name + ".exe"], | ||
| ) |
There was a problem hiding this comment.
win_resource/win_messagetable now require _windres and _windmc with executable = True, but the defaults point to filegroup targets created here. filegroup exports files but does not provide an executable target, so Bazel analysis will fail when these rules are used (for example under cmd/*/windows_resources) with a “target is not executable” error. Make these defaults point at executable file labels (or wrapper executable rules) instead of filegroups.
Useful? React with 👍 / 👎.
| set "MSYS2_ROOT=%cd%\..\..\external\+msys2_base_repository+msys2_base" | ||
| set "MINGW_ROOT=%cd%\..\..\external\+winlibs_mingw_repository+winlibs_mingw64" |
There was a problem hiding this comment.
This shim resolves tool locations via hard-coded canonical repo directory names under external/. In bzlmod those canonical names are explicitly not stable, so a Bazel/bzlmod change (or repo remapping/override) can make these paths wrong and break every Windows run_shell/genrule invocation with “hermetic bash/gcc not found”. The shim should derive repo locations from stable label-based mappings instead of embedding canonical directory strings.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e2f5af56d
ℹ️ 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".
| patches = [ | ||
| # Stops rustc_compile_action from leaking default_shell_env into | ||
| # CrateInfo.rustc_env, which otherwise clobbers cc_toolchain link_env | ||
| # for rust_test(crate = ...). Upstream issue pending. | ||
| "//bazel/patches:rules_rust-crateinfo-env-leak.patch", | ||
| # Derives `-Cdlltool=<linker-dir>/dlltool.exe` automatically | ||
| "//bazel/patches:rules_rust-derive-dlltool-from-linker.patch", | ||
| ], |
There was a problem hiding this comment.
Removing //bazel/patches:rules_rust-crateinfo-env-leak.patch here reintroduces the Windows GNU link-env regression that patch was preventing: upstream rules_rust still stores CrateInfo.rustc_env from the merged action env (including default shell env), so downstream rust_test(crate=...) can inherit host PATH/LIB/INCLUDE and override the MinGW toolchain link env instead of using hermetic tool paths. This breaks Windows Rust test/link actions on machines where host env differs from CI, so the patch needs to remain until upstream fixes that provider behavior.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c44e5198fa
ℹ️ 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".
| patches = [ | ||
| # Stops rustc_compile_action from leaking default_shell_env into | ||
| # CrateInfo.rustc_env, which otherwise clobbers cc_toolchain link_env | ||
| # for rust_test(crate = ...). Upstream issue pending. | ||
| "//bazel/patches:rules_rust-crateinfo-env-leak.patch", | ||
| # Derives `-Cdlltool=<linker-dir>/dlltool.exe` automatically | ||
| "//bazel/patches:rules_rust-derive-dlltool-from-linker.patch", | ||
| ], |
There was a problem hiding this comment.
Dropping //bazel/patches:rules_rust-crateinfo-env-leak.patch reintroduces the environment propagation bug that patch was fixing: CrateInfo.rustc_env includes default_shell_env instead of only Rust-specific vars, so downstream rust_test(crate = ...) can inherit host PATH/LIB/INCLUDE and override the MinGW cc_toolchain link environment. In Windows GNU builds this makes linking depend on whatever toolchain is installed on the host and can cause non-hermetic or wrong-linker failures that were previously prevented by the removed patch.
Useful? React with 👍 / 👎.
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
32 successful checks with minimal change (< 2 KiB)
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a4869485d
ℹ️ 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".
| # multi-line `-c "..."` arguments at the first newline; the .exe forwards | ||
| # GetCommandLineW() verbatim to the bash.exe extracted by @msys2_base. | ||
| # Needed until bazelbuild/bazel#21089 wires sh_toolchain into run_shell. | ||
| common:windows --shell_executable=bazel/toolchains/msys2/bash_shim.exe |
There was a problem hiding this comment.
This change points --shell_executable to bazel/toolchains/msys2/bash_shim.exe, but that file is not added anywhere in the commit (only bash_shim.c is). On Windows, any genrule/ctx.actions.run_shell/rules_foreign_cc action will try to launch this path and fail because the executable is missing, which blocks builds as soon as a shell action is needed.
Useful? React with 👍 / 👎.
JSGette
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa12205471
ℹ️ 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".
| #define MSYS2_REL L"..\\..\\external\\+msys2_base_repository+msys2_base" | ||
| #define MINGW_REL L"..\\..\\external\\+winlibs_mingw_repository+winlibs_mingw64" |
There was a problem hiding this comment.
The shim bakes canonical external repo directory names (+msys2_base_repository+msys2_base, +winlibs_mingw_repository+winlibs_mingw64) into filesystem paths. Bazel’s canonical repo-name format is explicitly not a stable API, and bazel/AGENTS.md also calls out that runfile/canonical-name paths must not be hardcoded. When canonical naming changes (or repo mapping/layout differs), the shim will no longer locate bash.exe/gcc.exe, causing Windows run_shell/genrule actions to fail before command execution.
Useful? React with 👍 / 👎.
| common:windows --repo_env=USERPROFILE # used by MSYS2 bash to emulate HOME, needed by git to fetch repositories | ||
| common:windows --repo_env=VSTUDIO_ROOT # visual_studio(path_variable) in MODULE.bazel | ||
| # Hermetic bash discovery for ctx.actions.run_shell / genrule / rules_foreign_cc. | ||
| common:windows --shell_executable=bazel/toolchains/msys2/bash_shim.exe |
There was a problem hiding this comment.
--shell_executable is documented by Bazel as an absolute path, but this change sets it to a relative path. That makes behavior dependent on the caller’s working directory and can break Windows builds when Bazel is launched from a subdirectory or wrapper that doesn’t run at the repo root, because the shim path won’t resolve and shell-backed actions won’t start.
Useful? React with 👍 / 👎.
| wchar_t cwd[MAX_PATH]; | ||
| if (!GetCurrentDirectoryW(MAX_PATH, cwd)) { | ||
| fwprintf(stderr, L"bash_shim: GetCurrentDirectory failed: %lu\n", GetLastError()); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
The shim allocates cwd with MAX_PATH and calls GetCurrentDirectoryW(MAX_PATH, cwd) without handling the documented “buffer too small” return value. On long workspaces/output-base paths, this yields truncated/invalid paths that then propagate into bash_path/gcc_path, producing false “hermetic tool not found” errors and failing shell actions.
Useful? React with 👍 / 👎.
aiuto
left a comment
aiuto
left a comment
There was a problem hiding this comment.
Overall this seems good.
- Some CRLF differences should get fixed.
- you might split it in two for easier review and smaller rollback
- codex left some comments. I did not investigate if they are valid or not.
2 participants
What does this PR do?
Right now we consume MinGW that is installed inside of the Windows Build Image with hard coded paths to MSYS2 and MinGW that are different to the default install location. This PR consumes MSYS2 and MinGW as packages to ensure that the version of the tooling is exactly the same across all environments (including developers) and makes a first major step to make Bazel builds work without the need to use Windows containers.
However, Visual Studio still has to be installed manually and ideally exactly the same version as pinned in
visual_studiorepository rule inMODULE.bazel.List of changes worth mentioning:
winlibs_mingw_repositoryrepository rule to download MinGWmsys2_base_repositoryrepository rule to download MSYS2 and a bunch of tools (e.g. autoconf, make, etc.)Motivation
Describe how you validated your changes
Additional Notes