Use hermetic MSVC and msbuild to build cpython on Windows - #54718
Conversation
Files inventory check summaryFile checks results against ancestor d263e531: Results for datadog-agent_7.84.0~devel.git.125.59d659b.pipeline.130418778-1_amd64.deb:No change detected Results for datadog-iot-agent_7.84.0~devel.git.125.59d659b.pipeline.130418778-1_amd64.deb:No change detected |
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 8b7574e Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_metrics_logs | memory utilization | +1.03 | [+0.78, +1.27] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | +0.29 | [+0.14, +0.43] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | +0.20 | [+0.08, +0.32] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_idle | memory utilization | +0.14 | [+0.04, +0.24] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | +0.13 | [+0.06, +0.21] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | -0.12 | [-0.16, -0.08] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_private_action_runner | memory utilization | -0.32 | [-0.44, -0.20] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_logs | % cpu utilization | -3.64 | [-4.48, -2.80] | 1 | Logs bounds checks dashboard |
Bounds Checks: ❌ Failed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | quality_gate_idle | intake_connections | 10/10 | 4 = 4 | bounds checks dashboard |
| ❌ | quality_gate_idle | memory_usage | 0/10 | 174.23MiB > 154MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 729.21KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 4 = 4 | bounds checks dashboard |
| ❌ | quality_gate_idle_all_features | memory_usage | 8/10 | 516.65MiB > 512MiB | 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 | 17 ≤ 40 | bounds checks dashboard |
| ❌ | quality_gate_logs | memory_usage | 2/10 | 202.92MiB > 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.19MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 416.26 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 18 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 391.53MiB ≤ 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 |
| ✅ | quality_gate_private_action_runner | memory_usage | 10/10 | 71.54MiB ≤ 75MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 25.99 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 325.19MiB ≤ 330MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 70.08 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 302.28MiB ≤ 310MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 23.92 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 308.69MiB ≤ 320MiB | 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".
Replicate Execution Details
We run multiple replicates for each experiment/variant. However, we allow replicates to be automatically retried if there are any failures, up to 8 times, at which point the replicate is marked dead and we are unable to run analysis for the entire experiment. We call each of these attempts at running replicates a replicate execution. This section lists all replicate executions that failed due to the target crashing or being oom killed.
Note: In the below tables we bucket failures by experiment, variant, and failure type. For each of these buckets we list out the replicate indexes that failed with an annotation signifying how many times said replicate failed with the given failure mode. In the below example the baseline variant of the experiment named experiment_with_failures had two replicates that failed by oom kills. Replicate 0, which failed 8 executions, and replicate 1 which failed 6 executions, all with the same failure mode.
| Experiment | Variant | Replicates | Failure | Logs | Debug Dashboard |
|---|---|---|---|---|---|
| experiment_with_failures | baseline | 0 (x8) 1 (x6) | Oom killed | Debug Dashboard |
The debug dashboard links will take you to a debugging dashboard specifically designed to investigate replicate execution failures.
❌ Retried Profiling Replicate Execution Failures (ddprof)
Note: Profiling replicas may still be executing. See the debug dashboard for up to date status.
| Experiment | Variant | Replicates | Failure | Debug Dashboard |
|---|---|---|---|---|
| quality_gate_logs | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_metrics_logs | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_security_idle | baseline | 10 | Crashed (exit code: 134) | Debug Dashboard |
| quality_gate_security_no_fs_load | comparison | 10 | Crashed (exit code: 134) | Debug Dashboard |
CI Pass/Fail Decision
❌ Failed. Some Quality Gates were violated.
- quality_gate_private_action_runner, 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.
- 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 cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_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_logs, bounds check memory_usage: 2/10 replicas passed. Failed 8 which is > 0. Gate FAILED.
- quality_gate_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 0/10 replicas passed. Failed 10 which is > 0. Gate FAILED.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check cpu_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_idle_all_features, bounds check memory_usage: 8/10 replicas passed. Failed 2 which is > 0. Gate FAILED.
Static quality checks✅ Please find below the results from static quality gates 33 successful checks with minimal change (< 2 KiB)
|
| visual_studio( | ||
| name = "visual_studio", | ||
| path_variable = "VSTUDIO_ROOT", | ||
| version = "17.14.36717.8", # 18.4.11612.150 |
There was a problem hiding this comment.
This comment just to remind this silently disables the version guard for the remaining host-dependent consumer, which you'll address in a subsequent PR, right?
There was a problem hiding this comment.
visual_studio rule will be completely gone in the follow up PR, yes
| ) | ||
|
|
||
| if ctx.attr.version and ctx.attr.version != vs_version: | ||
| fail("Version '%s' doesn't match expected version '%s'" % (vs_version, ctx.attr.version)) |
There was a problem hiding this comment.
i.e. this statement is now unreachable.
There was a problem hiding this comment.
It doesn't matter, I will remove visual_studio.bzl entirely
| + if not toolset_dir: | ||
| + fail("Visual Studio manifest has no Microsoft.VisualStudio.VC.MSBuild.<toolset>.Base package") | ||
| + | ||
| + target_prefix = "{}{}.{}.".format(prefix, toolset_dir, targets[0].lower()) |
There was a problem hiding this comment.
platform_toolset is derived from targets[0] only: harmless today since only "x64" is configured but would be potentially wrong (unlikely to be accepted upstream) if this ever needs multi-arch.
There was a problem hiding this comment.
Fair but I would keep it as is for now. I asked the maintainers if they would be willing to accept this upstream. If so I will definitely rework this piece.
|
🎯 Code Coverage (details) 🔗 Commit SHA: 59d659b | Docs | Datadog PR Page | Give us feedback! |
<!--Please give us some feedback on your experience writing this PR ! https://app.datadoghq.com/forms/43db4c02-6837-400c-8083-692e141b1b88 !--> ### What does this PR do? This PR fully eliminates usage of `visual_studio.bzl` that was just pinning version of a local MSVC installation. #54718 introduced a fully hermetic MSVC toolchain, as well as MSBuild tool and migrated cpython build process to it. This PR handles datadog-interop.dll, the last consumer of `msbuild` that was relying on `visual_studio` repository rule. - Remove `visual_studio.bzl` - Introduce `pkg/inventory/software/main_windows_test.go` to properly wire interop DLL into tests under Bazel. - Remove `-AsArray` from `pkg/inventory/software/integration_windows_test.go` as it is only supported in PowerShell 6+, for instance, my DD issued Windows laptop has 5.1, so this simply doesn't `work. - Enable `software_test` in CI. From now on it will be executed under `//...` Co-authored-by: joseph.gette <joseph.gette@datadoghq.com>
<!--Please give us some feedback on your experience writing this PR ! https://app.datadoghq.com/forms/43db4c02-6837-400c-8083-692e141b1b88 !--> ### What does this PR do? This PR fully eliminates usage of `visual_studio.bzl` that was just pinning version of a local MSVC installation. #54718 introduced a fully hermetic MSVC toolchain, as well as MSBuild tool and migrated cpython build process to it. This PR handles datadog-interop.dll, the last consumer of `msbuild` that was relying on `visual_studio` repository rule. - Remove `visual_studio.bzl` - Introduce `pkg/inventory/software/main_windows_test.go` to properly wire interop DLL into tests under Bazel. - Remove `-AsArray` from `pkg/inventory/software/integration_windows_test.go` as it is only supported in PowerShell 6+, for instance, my DD issued Windows laptop has 5.1, so this simply doesn't `work. - Enable `software_test` in CI. From now on it will be executed under `//...` Co-authored-by: joseph.gette <joseph.gette@datadoghq.com> 268c717
What does this PR do?
Previous implementation relied on the local installation of MSVC and msbuild to build cpython. A custom repository rule would verify version of the installed MSVC that worked perfectly fine inside of build images. Unfortunately, Windows Updates do bump MSVC as well, so at some point the build started to fail on the Windows host (not inside of the container) due to msbuild version mismatch. One of the potential solutions was to relax version check but that would inevitably introduce caching issues when we can't efficiently share cache across machines unable to ensure that everyone is using the same MSVC. Thus, this PR targets to introduce a fully hermetic MSVC and msbuild distribution that is fully managed by Bazel. We use toolchains_msvc as basis applying a patch on top of that to, first, stop filtering out msbuild and, second, expose additional headers and dlls that are required to make this whole enterprise work.
There is one more consumer for msbuild -
libdatadog-interop.dll. This will be handled in a follow up PR.