Provide PYTHON_FOR_BUILD to CPython's Windows build#48281
Conversation
### What does this PR do? - add `@python_3_12//:python3` (current `rules_python`'s hermetic interpreter) to `python_win`'s srcs and expose it as [PYTHON_FOR_BUILD](https://github.com/python/cpython/blob/3.12/PCbuild/readme.txt#L230), - resolve it to an absolute path in `build_python.bat` before `build.bat` is called, so MSBuild can use it regardless of its working directory ### Motivation CPython's `PCbuild/find_python.bat` looks for a Python interpreter to perform pre-compilation code generation (opcodes, grammar, token tables) and to orchestrate PGO training runs. When `PYTHON_FOR_BUILD` is not set, [find_python.bat](https://github.com/python/cpython/blob/3.12/PCbuild/find_python.bat) falls back through the following chain, each of which fails in a Bazel action running under `--incompatible_strict_action_env` (Bazel 9's default BTW): 1. `VIRTUAL_ENV`, 2. `externals` directory, 3. `HOST_PYTHON`, 4. `py.exe` launcher, 5. `NuGet` download: **failed for me**, which surfaced the hermeticity issue. `PYTHON_FOR_BUILD` is CPython's dedicated variable for the interpreter used to **build** CPython itself, as opposed to the one **being built**. ### Describe how you validated your changes Confirmed on a local Windows VM that `find_python.bat` reports "found as PYTHON_FOR_BUILD" and proceeds past code generation. ### Additional Notes Found while making progress towards #48206; split out so it can be reviewed/merged/reverted independently.
05e8a82 to
894c67d
Compare
Files inventory check summaryFile checks results against ancestor 70d7d910: Results for datadog-agent_7.79.0~devel.git.98.894c67d.pipeline.104148767-1_amd64.deb:No change detected |
| "libffi_win_dir", | ||
| "openssl-bin_win_dir", | ||
| "tcltk_win_dir", | ||
| "@python_3_12//:python3", |
There was a problem hiding this comment.
does that work while building python 3.13? I thought python required the local interpreter to match the version being built
There was a problem hiding this comment.
guess who didn't read the PR description before reviewing the change...
There was a problem hiding this comment.
https://github.com/python/cpython/blob/3.13/PCbuild/readme.txt#L6-L8:
1a. Optionally install Python 3.6 or later. If not installed,
get_externals.bat (via build.bat) will download and use Python via
NuGet.
There was a problem hiding this comment.
I suppose the requirement for matching version only applies to cross compilation then, my bad!
There was a problem hiding this comment.
According to the readme, as linked in the PR description, yes: https://github.com/python/cpython/blob/3.13/PCbuild/readme.txt#L6
As far as I remember this python is used to run specific scripts during the build and doesn't need to match the python being built. https://github.com/search?q=repo%3Apython%2Fcpython%20PythonForBuild&type=code
The only concern with this reference now is that iiuc it needs to stay in sync with
Line 235 in 281dbad
EDIT: And of course my comment came in late :D
There was a problem hiding this comment.
That being said, Python 3.10+ will be required for Python 3.14, https://github.com/python/cpython/blob/3.14/PCbuild/readme.txt#L6-L8:
1a. Optionally install Python 3.10 or later. If not installed,
get_externals.bat (via build.bat) will download and use Python via
NuGet.
There was a problem hiding this comment.
TBH, I didn't find a quick alias to the configured toolchain's Python interpreter. It's possible by mean of a repo rule... overkill, I guess.
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 89faa04 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | docker_containers_cpu | % cpu utilization | -0.57 | [-3.66, +2.52] | 1 | Logs |
Fine details of change detection per experiment
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_logs | % cpu utilization | +2.02 | [+0.37, +3.68] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | +0.31 | [+0.26, +0.37] | 1 | Logs bounds checks dashboard |
| ➖ | docker_containers_memory | memory utilization | +0.28 | [+0.20, +0.36] | 1 | Logs |
| ➖ | quality_gate_metrics_logs | memory utilization | +0.22 | [-0.01, +0.45] | 1 | Logs bounds checks dashboard |
| ➖ | ddot_logs | memory utilization | +0.13 | [+0.06, +0.19] | 1 | Logs |
| ➖ | file_tree | memory utilization | +0.09 | [+0.04, +0.14] | 1 | Logs |
| ➖ | file_to_blackhole_100ms_latency | egress throughput | +0.06 | [-0.03, +0.15] | 1 | Logs |
| ➖ | file_to_blackhole_1000ms_latency | egress throughput | +0.03 | [-0.41, +0.46] | 1 | Logs |
| ➖ | file_to_blackhole_0ms_latency | egress throughput | +0.01 | [-0.50, +0.53] | 1 | Logs |
| ➖ | ddot_metrics_sum_cumulative | memory utilization | +0.01 | [-0.13, +0.16] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api | ingress throughput | +0.01 | [-0.19, +0.20] | 1 | Logs |
| ➖ | file_to_blackhole_500ms_latency | egress throughput | +0.00 | [-0.39, +0.40] | 1 | Logs |
| ➖ | uds_dogstatsd_to_api_v3 | ingress throughput | +0.00 | [-0.19, +0.20] | 1 | Logs |
| ➖ | tcp_dd_logs_filter_exclude | ingress throughput | -0.00 | [-0.11, +0.11] | 1 | Logs |
| ➖ | otlp_ingest_metrics | memory utilization | -0.03 | [-0.20, +0.13] | 1 | Logs |
| ➖ | uds_dogstatsd_20mb_12k_contexts_20_senders | memory utilization | -0.07 | [-0.13, -0.01] | 1 | Logs |
| ➖ | ddot_metrics | memory utilization | -0.08 | [-0.25, +0.10] | 1 | Logs |
| ➖ | ddot_metrics_sum_cumulativetodelta_exporter | memory utilization | -0.09 | [-0.32, +0.13] | 1 | Logs |
| ➖ | quality_gate_idle_all_features | memory utilization | -0.10 | [-0.13, -0.06] | 1 | Logs bounds checks dashboard |
| ➖ | ddot_metrics_sum_delta | memory utilization | -0.15 | [-0.32, +0.01] | 1 | Logs |
| ➖ | docker_containers_cpu | % cpu utilization | -0.57 | [-3.66, +2.52] | 1 | Logs |
| ➖ | otlp_ingest_logs | memory utilization | -0.62 | [-0.71, -0.52] | 1 | Logs |
| ➖ | tcp_syslog_to_blackhole | ingress throughput | -0.75 | [-0.89, -0.61] | 1 | Logs |
Bounds Checks: ❌ Failed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | docker_containers_cpu | simple_check_run | 10/10 | 741 ≥ 26 | |
| ✅ | docker_containers_memory | memory_usage | 10/10 | 276.16MiB ≤ 370MiB | |
| ✅ | docker_containers_memory | simple_check_run | 10/10 | 685 ≥ 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.23GiB ≤ 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.22GiB ≤ 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 | 9/10 | 176.79MiB > 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.29MiB ≤ 550MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 3 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 208.73MiB ≤ 220MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 350.77 ≤ 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 | 403.78MiB ≤ 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
❌ Failed. Some Quality Gates were violated.
- quality_gate_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 9/10 replicas passed. Failed 1 which is > 0. Gate FAILED.
- quality_gate_metrics_logs, bounds check cpu_usage: 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 intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check missed_bytes: 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 memory_usage: 10/10 replicas passed. Gate passed.
Static quality checks✅ Please find below the results from static quality gates 31 successful checks with minimal change (< 2 KiB)
On-wire sizes (compressed)
|
### 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>
### What does this PR do? - add `@python_3_12//:python3` (current `rules_python`'s hermetic interpreter) to `python_win`'s srcs and expose it as [PYTHON_FOR_BUILD](https://github.com/python/cpython/blob/3.13/PCbuild/readme.txt#L236), - resolve it to an absolute path in `build_python.bat` before `build.bat` is called, so `MSBuild` can use it regardless of its working directory. ### Motivation CPython's `PCbuild/find_python.bat` looks for a Python interpreter to perform pre-compilation code generation (opcodes, grammar, token tables) and to orchestrate PGO training runs. When `PYTHON_FOR_BUILD` is not set, [find_python.bat](https://github.com/python/cpython/blob/3.13/PCbuild/find_python.bat) falls back through the following chain, each of which fails in a Bazel action running under `--incompatible_strict_action_env` (Bazel 9's default BTW): 1. `VIRTUAL_ENV`, 2. `externals` directory, 3. `HOST_PYTHON`, 4. `py.exe` launcher, 5. `NuGet` download: **failed for me**... which surfaced the hermeticity issue. `PYTHON_FOR_BUILD` is CPython's dedicated variable for the ([`3.6+`](https://github.com/python/cpython/blob/3.13/PCbuild/readme.txt#L6)) interpreter **used to build** CPython itself, as opposed to the one **being built** (i.e. `3.13.12` as of today). ### Describe how you validated your changes Confirmed on a local Windows VM that `find_python.bat` reports "found as PYTHON_FOR_BUILD" and proceeds past code generation. ### Additional Notes Found while making progress towards #48206; split out so it can be reviewed/merged/reverted independently. 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>
### What does this PR do? - add `@python_3_12//:python3` (current `rules_python`'s hermetic interpreter) to `python_win`'s srcs and expose it as [PYTHON_FOR_BUILD](https://github.com/python/cpython/blob/3.13/PCbuild/readme.txt#L236), - resolve it to an absolute path in `build_python.bat` before `build.bat` is called, so `MSBuild` can use it regardless of its working directory. ### Motivation CPython's `PCbuild/find_python.bat` looks for a Python interpreter to perform pre-compilation code generation (opcodes, grammar, token tables) and to orchestrate PGO training runs. When `PYTHON_FOR_BUILD` is not set, [find_python.bat](https://github.com/python/cpython/blob/3.13/PCbuild/find_python.bat) falls back through the following chain, each of which fails in a Bazel action running under `--incompatible_strict_action_env` (Bazel 9's default BTW): 1. `VIRTUAL_ENV`, 2. `externals` directory, 3. `HOST_PYTHON`, 4. `py.exe` launcher, 5. `NuGet` download: **failed for me**... which surfaced the hermeticity issue. `PYTHON_FOR_BUILD` is CPython's dedicated variable for the ([`3.6+`](https://github.com/python/cpython/blob/3.13/PCbuild/readme.txt#L6)) interpreter **used to build** CPython itself, as opposed to the one **being built** (i.e. `3.13.12` as of today). ### Describe how you validated your changes Confirmed on a local Windows VM that `find_python.bat` reports "found as PYTHON_FOR_BUILD" and proceeds past code generation. ### Additional Notes Found while making progress towards #48206; split out so it can be reviewed/merged/reverted independently. 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>
What does this PR do?
@python_3_12//:python3(currentrules_python's hermetic interpreter) topython_win's srcs and expose it as PYTHON_FOR_BUILD,build_python.batbeforebuild.batis called, soMSBuildcan use it regardless of its working directory.Motivation
CPython's
PCbuild/find_python.batlooks for a Python interpreter to perform pre-compilation code generation (opcodes, grammar, token tables) and to orchestrate PGO training runs.When
PYTHON_FOR_BUILDis not set, find_python.bat falls back through the following chain, each of which fails in a Bazel action running under--incompatible_strict_action_env(Bazel 9's default BTW):VIRTUAL_ENV,externalsdirectory,HOST_PYTHON,py.exelauncher,NuGetdownload: failed for me... which surfaced the hermeticity issue.PYTHON_FOR_BUILDis CPython's dedicated variable for the (3.6+) interpreter used to build CPython itself, as opposed to the one being built (i.e.3.13.12as of today).Describe how you validated your changes
Confirmed on a local Windows VM that
find_python.batreports "found as PYTHON_FOR_BUILD" and proceeds past code generation.Additional Notes
Found while making progress towards #48206; split out so it can be reviewed/merged/reverted independently.