Conversation
…fig.h. e…" This reverts commit f902668.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48eaaf3de8
ℹ️ 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".
| @@ -41,12 +41,6 @@ common:macos --remote_local_fallback_strategy=sandboxed | |||
|
|
|||
| # Windows config ------------------------------------------------------------------------------------------------------- | |||
| common:windows --strategy=standalone # Valid values are: [dynamic_worker, standalone, dynamic, remote, worker, local] | |||
There was a problem hiding this comment.
Restore Windows runfiles flag to avoid dual python_win builds
Removing common:windows --enable_runfiles reintroduces the Windows race that the reverted commit was masking: this repo still pins rules_python 1.9.0 (see MODULE.bazel), and deps/cpython/build_python.bat still writes intermediates into shared PCbuild paths under the source tree (PCbuild/amd64, PCbuild/obj, msbuild.rsp) instead of per-action output dirs. In that state, Bazel can schedule two python_win builds in different configs on Windows, which intermittently fails with missing/locked build artifacts (e.g., pyconfig.h), so this revert can make CI/builds flaky again until the hermetic build script fix lands.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
No other short-term option, alas.
Files inventory check summaryFile checks results against ancestor c1d67a90: Results for datadog-agent_7.79.0~devel.git.55.48eaaf3.pipeline.103966546-1_amd64.deb:No change detected |
Static quality checks✅ Please find below the results from static quality gates 30 successful checks with minimal change (< 2 KiB)
On-wire sizes (compressed)
|
### What does this PR do? Add `.bazel*` to the `windows_path_2` change filter in `.gitlab-ci.yml`. ### Motivation `.bazelrc` and `.bazelversion` live at the repo root, not under `bazel/**/*`. Changes to those files can affect Windows builds and tests (#48207) without triggering Windows CI jobs, so regressions go undetected until the change lands on `main`. Follows #48087 - more to come?
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: d7a7373 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | docker_containers_cpu | % cpu utilization | +0.22 | [-2.85, +3.29] | 1 | Logs |
Fine details of change detection per experiment
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | docker_containers_cpu | % cpu utilization | +0.22 | [-2.85, +3.29] | 1 | Logs |
| ➖ | docker_containers_memory | memory utilization | +0.16 | [+0.09, +0.23] | 1 | Logs |
| ➖ | quality_gate_metrics_logs | memory utilization | +0.11 | [-0.13, +0.34] | 1 | Logs bounds checks dashboard |
| ➖ | uds_dogstatsd_20mb_12k_contexts_20_senders | memory utilization | +0.08 | [+0.02, +0.14] | 1 | Logs |
| ➖ | otlp_ingest_metrics | memory utilization | +0.07 | [-0.09, +0.23] | 1 | Logs |
| ➖ | quality_gate_idle | memory utilization | +0.06 | [+0.01, +0.11] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.03 | [-0.01, +0.07] | 1 | Logs bounds checks dashboard |
| ➖ | uds_dogstatsd_to_api_v3 | ingress throughput | +0.01 | [-0.19, +0.20] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api | ingress throughput | +0.00 | [-0.19, +0.20] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency | egress throughput | +0.00 | [-0.45, +0.45] | 1 | Logs |
| ➖ | tcp_dd_logs_filter_exclude | ingress throughput | -0.00 | [-0.11, +0.11] | 1 | Logs |
| ➖ | ddot_metrics_sum_delta | memory utilization | -0.00 | [-0.17, +0.17] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency | egress throughput | -0.01 | [-0.44, +0.42] | 1 | Logs |
| ➖ | file_to_blackhole_500ms_latency | egress throughput | -0.02 | [-0.40, +0.36] | 1 | Logs |
| ➖ | ddot_metrics_sum_cumulativetodelta_exporter | memory utilization | -0.04 | [-0.26, +0.19] | 1 | Logs |
| ➖ | file_to_blackhole_100ms_latency | egress throughput | -0.06 | [-0.14, +0.01] | 1 | Logs |
| ➖ | otlp_ingest_logs | memory utilization | -0.11 | [-0.20, -0.01] | 1 | Logs |
| ➖ | ddot_logs | memory utilization | -0.15 | [-0.21, -0.09] | 1 | Logs |
| ➖ | ddot_metrics | memory utilization | -0.23 | [-0.41, -0.06] | 1 | Logs |
| ➖ | file_tree | memory utilization | -0.25 | [-0.31, -0.19] | 1 | Logs |
| ➖ | ddot_metrics_sum_cumulative | memory utilization | -0.42 | [-0.56, -0.28] | 1 | Logs |
| ➖ | tcp_syslog_to_blackhole | ingress throughput | -1.59 | [-1.73, -1.45] | 1 | Logs |
| ➖ | quality_gate_logs | % cpu utilization | -1.92 | [-3.51, -0.33] | 1 | Logs bounds checks dashboard |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | docker_containers_cpu | simple_check_run | 10/10 | 553 ≥ 26 | |
| ✅ | docker_containers_memory | memory_usage | 10/10 | 274.53MiB ≤ 370MiB | |
| ✅ | docker_containers_memory | simple_check_run | 10/10 | 698 ≥ 26 | |
| ✅ | file_to_blackhole_0ms_latency | memory_usage | 10/10 | 0.19GiB ≤ 1.20GiB | |
| ✅ | file_to_blackhole_0ms_latency | missed_bytes | 10/10 | 0B = 0B | |
| ✅ | file_to_blackhole_1000ms_latency | memory_usage | 10/10 | 0.24GiB ≤ 1.20GiB | |
| ✅ | file_to_blackhole_1000ms_latency | missed_bytes | 10/10 | 0B = 0B | |
| ✅ | file_to_blackhole_100ms_latency | memory_usage | 10/10 | 0.20GiB ≤ 1.20GiB | |
| ✅ | file_to_blackhole_100ms_latency | missed_bytes | 10/10 | 0B = 0B | |
| ✅ | file_to_blackhole_500ms_latency | memory_usage | 10/10 | 0.21GiB ≤ 1.20GiB | |
| ✅ | file_to_blackhole_500ms_latency | missed_bytes | 10/10 | 0B = 0B | |
| ✅ | quality_gate_idle | intake_connections | 10/10 | 3 = 3 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 174.94MiB ≤ 175MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 2 ≤ 3 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 494.07MiB ≤ 550MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 3 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 207.96MiB ≤ 220MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 364.65 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 3 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 404.66MiB ≤ 475MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | missed_bytes | 10/10 | 0B = 0B | 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 memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, 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_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_metrics_logs, bounds check memory_usage: 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 missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
### What does this PR do? Add `.bazel*` to the `windows_path_2` change filter in `.gitlab-ci.yml`. ### Motivation `.bazelrc` and `.bazelversion` live at the repo root, not under `bazel/**/*`. Changes to those files can affect Windows builds and tests (#48207) without triggering Windows CI jobs, so regressions go undetected until the change lands on `main`. Follows #48087 - more to come?
### What does this PR do? Add `.bazel*` to the `windows_path_2` change filter in `.gitlab-ci.yml`. ### Motivation `.bazelrc` and `.bazelversion` live at the repo root, not under `bazel/**/*`. Changes to those files can affect Windows builds and tests (#48207) without triggering Windows CI jobs, so regressions go undetected until the change lands on `main`. Follows #48087 - more to come? Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com>
### Motivation This job is critical to ensure _any_ change, whether strictly `bazel`-related or in the source code being built/tested, doesn't break Windows builds. It's the second time a breaking change is not gated properly (this time leading to #48207), so we're kindly asking to lift the Windows pipeline exclusion and always run this job in CI. Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com>
…fig.h, etc.)" (#48207) Automatic revert of commit f902668. This PR is created to anticipate the resolution of `bazel:test:windows-amd64` job failure in `main` branch in case of erroneous code change. If this revert is incorrect, do not hesitate to close the PR. Co-authored-by: datadog-datadog-prod-us1-2[bot] <261164178+datadog-datadog-prod-us1-2[bot]@users.noreply.github.com>
### What does this PR do? Add `.bazel*` to the `windows_path_2` change filter in `.gitlab-ci.yml`. ### Motivation `.bazelrc` and `.bazelversion` live at the repo root, not under `bazel/**/*`. Changes to those files can affect Windows builds and tests (#48207) without triggering Windows CI jobs, so regressions go undetected until the change lands on `main`. Follows #48087 - more to come? Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com>
### Motivation This job is critical to ensure _any_ change, whether strictly `bazel`-related or in the source code being built/tested, doesn't break Windows builds. It's the second time a breaking change is not gated properly (this time leading to #48207), so we're kindly asking to lift the Windows pipeline exclusion and always run this job in CI. Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com>
### What does this PR do? - add `common:windows --enable_runfiles` to `.bazelrc` - widen `bazel:test:windows-amd64` from `//bazel/tests/... //rtloader/...` to `//...` with exclusions for Linux-, eBPF-, and gopatch-only targets ### Motivation rules_python 1.9.0 (#48082) transitions every `py_binary` and `py_test` on Windows from `enable_runfiles=auto` to `enable_runfiles=true`. With Bazel's default (`enable_runfiles=false` on Windows), this creates a second Bazel configuration, causing `python_win` to be built twice concurrently. `build_python.bat` writes MSBuild intermediates to the shared execroot source tree rather than the action's output directory, so both builds race on those files, causing intermittent `pyconfig.h` failures. This is a redo of #48188, which was preemptively reverted (#48207). Pre-setting `--enable_runfiles` makes the transition a no-op, so Bazel sees a single configuration and builds `python_win` once. Two prerequisites are now in place: #48281 provides `PYTHON_FOR_BUILD` to MSBuild, preventing `find_python.bat` from falling back to NuGet and other external sources under `--incompatible_strict_action_env`; #48087 and #48209 trigger Windows CI on `MODULE.bazel*` and `.bazel*` changes respectively, so the widened test surface below will catch regressions before they reach `main`. ### Describe how you validated your changes Local VM and, of course, CI. ### Additional Notes `//pkg/template/...` is excluded on Windows: gopatch v0.4.0 errors on `@@\r` in hunk markers when patch files have CRLF line endings, an unreported upstream bug with no workaround in gopatch itself.
Prior art: 1. #48082 2. #48087 3. #48188 (1st attempt) 4. #48207 5. #48281 6. #48209 ### What does this PR do? This is a redo of #48188 (preemptively reverted by #48207) and therefore merely consists in re-adding `common:windows --enable_runfiles` to `.bazelrc`. ... with lessons learned, thanks to earlier: - #48209 covers the present change to `.bazelrc`, - #48281 prevents `find_python.bat` from falling back to `NuGet` or other non hermetic sources. .... and now a widened `bazel:test:windows-amd64`, evolving from just `//bazel/tests/... //rtloader/...` to `//...` **- except currently failing targets** (for the time being, of course). ### Motivation Summary of #48188: - `rules_python` 1.9.0 (#48082) transitions every `py_binary` and `py_test` on Windows from `enable_runfiles=auto` to `enable_runfiles=true`, - with Bazel's default (`enable_runfiles=false` on Windows), this creates a second Bazel configuration, causing `python_win` to be built twice concurrently, - `build_python.bat` writes `MSBuild` intermediates to the shared execroot source tree rather than the action's output directory, so both builds race on those files, causing [intermittent failures](https://gitlab.ddbuild.io/DataDog/datadog-agent/-/jobs/1533753039) (`pyconfig.h` not found, etc.). **Pre-setting `--enable_runfiles` makes the transition a no-op, so Bazel sees a single configuration and builds `python_win` once.** ### Describe how you validated your changes Local VM and, of course, CI. ### Additional Notes For instance, `//pkg/template/...` is excluded on Windows because `gopatch` errors on `@@\r` in hunk markers when patch files have CRLF line endings, which deserves a distinct PR (likely adjusting `.gitattributes`). Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com>
…fig.h, etc.)" (#48207) Automatic revert of commit f902668. This PR is created to anticipate the resolution of `bazel:test:windows-amd64` job failure in `main` branch in case of erroneous code change. If this revert is incorrect, do not hesitate to close the PR. Co-authored-by: datadog-datadog-prod-us1-2[bot] <261164178+datadog-datadog-prod-us1-2[bot]@users.noreply.github.com>
### What does this PR do? Add `.bazel*` to the `windows_path_2` change filter in `.gitlab-ci.yml`. ### Motivation `.bazelrc` and `.bazelversion` live at the repo root, not under `bazel/**/*`. Changes to those files can affect Windows builds and tests (#48207) without triggering Windows CI jobs, so regressions go undetected until the change lands on `main`. Follows #48087 - more to come? Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com>
### Motivation This job is critical to ensure _any_ change, whether strictly `bazel`-related or in the source code being built/tested, doesn't break Windows builds. It's the second time a breaking change is not gated properly (this time leading to #48207), so we're kindly asking to lift the Windows pipeline exclusion and always run this job in CI. Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com>
Prior art: 1. #48082 2. #48087 3. #48188 (1st attempt) 4. #48207 5. #48281 6. #48209 ### What does this PR do? This is a redo of #48188 (preemptively reverted by #48207) and therefore merely consists in re-adding `common:windows --enable_runfiles` to `.bazelrc`. ... with lessons learned, thanks to earlier: - #48209 covers the present change to `.bazelrc`, - #48281 prevents `find_python.bat` from falling back to `NuGet` or other non hermetic sources. .... and now a widened `bazel:test:windows-amd64`, evolving from just `//bazel/tests/... //rtloader/...` to `//...` **- except currently failing targets** (for the time being, of course). ### Motivation Summary of #48188: - `rules_python` 1.9.0 (#48082) transitions every `py_binary` and `py_test` on Windows from `enable_runfiles=auto` to `enable_runfiles=true`, - with Bazel's default (`enable_runfiles=false` on Windows), this creates a second Bazel configuration, causing `python_win` to be built twice concurrently, - `build_python.bat` writes `MSBuild` intermediates to the shared execroot source tree rather than the action's output directory, so both builds race on those files, causing [intermittent failures](https://gitlab.ddbuild.io/DataDog/datadog-agent/-/jobs/1533753039) (`pyconfig.h` not found, etc.). **Pre-setting `--enable_runfiles` makes the transition a no-op, so Bazel sees a single configuration and builds `python_win` once.** ### Describe how you validated your changes Local VM and, of course, CI. ### Additional Notes For instance, `//pkg/template/...` is excluded on Windows because `gopatch` errors on `@@\r` in hunk markers when patch files have CRLF line endings, which deserves a distinct PR (likely adjusting `.gitattributes`). Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com>
…fig.h, etc.)" (#48207) Automatic revert of commit f902668. This PR is created to anticipate the resolution of `bazel:test:windows-amd64` job failure in `main` branch in case of erroneous code change. If this revert is incorrect, do not hesitate to close the PR. Co-authored-by: datadog-datadog-prod-us1-2[bot] <261164178+datadog-datadog-prod-us1-2[bot]@users.noreply.github.com>
### What does this PR do? Add `.bazel*` to the `windows_path_2` change filter in `.gitlab-ci.yml`. ### Motivation `.bazelrc` and `.bazelversion` live at the repo root, not under `bazel/**/*`. Changes to those files can affect Windows builds and tests (#48207) without triggering Windows CI jobs, so regressions go undetected until the change lands on `main`. Follows #48087 - more to come? Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com>
### Motivation This job is critical to ensure _any_ change, whether strictly `bazel`-related or in the source code being built/tested, doesn't break Windows builds. It's the second time a breaking change is not gated properly (this time leading to #48207), so we're kindly asking to lift the Windows pipeline exclusion and always run this job in CI. Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com>
Prior art: 1. #48082 2. #48087 3. #48188 (1st attempt) 4. #48207 5. #48281 6. #48209 ### What does this PR do? This is a redo of #48188 (preemptively reverted by #48207) and therefore merely consists in re-adding `common:windows --enable_runfiles` to `.bazelrc`. ... with lessons learned, thanks to earlier: - #48209 covers the present change to `.bazelrc`, - #48281 prevents `find_python.bat` from falling back to `NuGet` or other non hermetic sources. .... and now a widened `bazel:test:windows-amd64`, evolving from just `//bazel/tests/... //rtloader/...` to `//...` **- except currently failing targets** (for the time being, of course). ### Motivation Summary of #48188: - `rules_python` 1.9.0 (#48082) transitions every `py_binary` and `py_test` on Windows from `enable_runfiles=auto` to `enable_runfiles=true`, - with Bazel's default (`enable_runfiles=false` on Windows), this creates a second Bazel configuration, causing `python_win` to be built twice concurrently, - `build_python.bat` writes `MSBuild` intermediates to the shared execroot source tree rather than the action's output directory, so both builds race on those files, causing [intermittent failures](https://gitlab.ddbuild.io/DataDog/datadog-agent/-/jobs/1533753039) (`pyconfig.h` not found, etc.). **Pre-setting `--enable_runfiles` makes the transition a no-op, so Bazel sees a single configuration and builds `python_win` once.** ### Describe how you validated your changes Local VM and, of course, CI. ### Additional Notes For instance, `//pkg/template/...` is excluded on Windows because `gopatch` errors on `@@\r` in hunk markers when patch files have CRLF line endings, which deserves a distinct PR (likely adjusting `.gitattributes`). Co-authored-by: regis.desgroppes <regis.desgroppes@datadoghq.com>
Automatic revert of commit f902668.
This PR is created to anticipate the resolution of
bazel:test:windows-amd64job failure inmainbranch in case of erroneous code change.If this revert is incorrect, do not hesitate to close the PR.