Azure / Azure/azure-sdk-for-python
[EngSys] Detect compiled dev requirements deterministically instead of regex-matching [tool.cibuildwheel]
- Dominant language
- Python
- Stars
- 5.6k
- Forks
- 3.4k
- Avg merge
- 1d 21h
- Merged PRs (30d)
- 193
Description
## Summary
[PR #48371](https://github.com/Azure/azure-sdk-for-python/pull/48371) added a `Check for compiled dev requirements` step to `eng/pipelines/templates/jobs/live.tests.yml` ([line 191](https://github.com/Azure/azure-sdk-for-python/blob/main/eng/pipelines/templates/jobs/live.tests.yml#L191)). It gates a set of `cibuildwheel` configuration steps on whether the service under test contains a package that needs them.
The gate works, but it is a **heuristic**: it recursively globs `sdk/` for `pyproject.toml` files and regex-matches `^\[tool\.cibuildwheel\]`.
```powershell
$found += Get-ChildItem -Path $root -Filter pyproject.toml -Recurse -ErrorAction SilentlyContinue `
| Where-Object { (Get-Content $_.FullName -Raw) -match '(?m)^\[tool\.cibuildwheel\]' }
```
### The problem
**`[tool.cibuildwheel]` is not what actually triggers cibuildwheel.** `create_package` branches on `setup_parsed.ext_modules` — see [`ci_tools/build.py:243`](https://github.com/Azure/azure-sdk-for-python/blob/main/eng/tools/azure-sdk-tools/ci_tools/build.py#L243) and [`:292`](https://github.com/Azure/azure-sdk-for-python/blob/main/eng/tools/azure-sdk-tools/ci_tools/build.py#L292). It never reads the `[tool.cibuildwheel]` section.
Worse, for the one package this currently matters for, **the file being grepped isn't where the truth lives**. `azure-storage-extensions` has no `[tool.setuptools.ext-modules]` in its `pyproject.toml` at all; `ParsedSetup` falls back to parsing the adjacent `setup.py` ([`parse_functions.py:713-722`](https://github.com/Azure/azure-sdk-for-python/blob/main/eng/tools/azure-sdk-tools/ci_tools/parsing/parse_functions.py#L713-L722)). The regex happens to match because that package also declares cibuildwheel config — the two are correlated today, not causally linked.
I scanned every package under `sdk/` and confirmed the sets are currently identical:
```
has ext_modules : ['sdk/storage/azure-storage-extensions']
has [tool.cibuildwheel]: ['sdk/storage/azure-storage-extensions']
ext_modules but NO cibuildwheel section (missed by current check): []
```
So there is **no active bug** — this is latent. A package that grows an extension without adding a `[tool.cibuildwheel]` section (relying on cibuildwheel defaults) would silently fail to be detected, and live tests for that service would start failing at dev-requirement install exactly the way storage did — with a root cause that is considerably harder to spot the second time.
### Secondary issue: the gate is scoped too broadly
The check answers *"does this **service directory** contain a compiled package?"* The condition that actually matters is *"does any package **under test in this job** have a relative dev requirement that resolves to a package with `ext_modules`?"* — which is what [`build_whl_for_req`](https://github.com/Azure/azure-sdk-for-python/blob/main/eng/tools/azure-sdk-tools/ci_tools/scenario/generation.py#L331-L337) actually evaluates at runtime. A job targeting a single storage package that has no such dev requirement still gets the variables set.
---
## Proposed solutions
Option A — Reuse package properties / ENABLE_EXTENSION_BUILD (not viable)
The natural instinct is to reuse what the build pipeline does. `steps/resolve-build-platforms.yml` sets `ENABLE_EXTENSION_BUILD` by reading the `PackageInfo` JSON folder.
Two independent blockers:
1. **The folder doesn't exist in this job.** `live.tests.yml` calls `resolve-package-targeting.yml` without `PackagePropertiesFolder`, which defaults to `''`. No `PackageInfo` folder is produced anywhere in the live test chain. `jobs/ci.tests.yml` does pass it; `jobs/live.tests.yml` does not.
2. **The schema has no field for this anyway.** `eng/scripts/get_package_properties.py` emits only `name`, `version`, `is_new_sdk`, folder, and `dependent_packages`. There is nothing about extensions or compilation.
That is precisely why `resolve-build-platforms.yml` resorts to:
```powershell
if ($packageProperties -contains "azure-storage-extensions") {
```
Adopting this pattern would trade a regex for a **hardcoded package name** — less deterministic, not more. Worth noting the build pipeline has the same latent problem, just expressed differently.
A variant would be to *add* an `ext_modules` / `is_compiled` field to the package properties schema and plumb `PackagePropertiesFolder` through `live.tests.yml`. That's a larger change touching cross-language tooling (`Package-Properties.ps1` consumers), and it makes the live test job depend on a `save-package-properties` step it doesn't currently run.
Option B1 — Query ParsedSetup directly (minimal change)
Keep the current shape and scope; just fix the predicate. Ask the same parser `create_package` uses:
```python
from ci_tools.parsing import ParsedSetup
ParsedSetup.from_path(package_dir).ext_modules
```
Walk the package directories under the service directory and check `.ext_modules` instead of regex-matching the file.
**Pros:** roughly a one-line semantic change; removes the proxy entirely; no parameter plumbing.
**Cons:** still service-directory-scoped rather than target-scoped; introduces a Python dependency into a step that is currently pure PowerShell (see the ordering constraint below).
Option B2 — Mirror build_whl_for_req exactly (most faithful)
Replicate what actually happens at runtime. For each package in `$(TargetingString)`, read its `dev_requirements.txt`, filter with `is_relative_install_path`, and check `.ext_modules` on the resolved target:
```python
for pkg in discover_targeted_packages(targeting_string, os.path.join("sdk", service_dir)):
for req in open(os.path.join(pkg, "dev_requirements.txt")):
if is_relative_install_path(req.strip(), pkg):
if ParsedSetup.from_path(os.path.abspath(os.path.join(pkg, req.strip()))).ext_modules:
# cibuildwheel will run
```
This is a line-for-line mirror of [`build_whl_for_req`](https://github.com/Azure/azure-sdk-for-python/blob/main/eng/tools/azure-sdk-tools/ci_tools/scenario/generation.py#L331-L337) — the function that invokes cibuildwheel. It uses the same `ParsedSetup`, the same `is_relative_install_path`, and the same dev-requirements traversal, so it cannot drift from the behavior it is predicting.
I prototyped this locally against the real repo:
| Input | Result |
|---|---|
| `azure-storage-blob` / `storage` | `true` — found `azure-storage-extensions`, 1 ext module |
| `azure-keyvault-keys` / `keyvault` | `false` |
It also tightens the gate from *service directory* to *packages actually under test*, addressing the secondary issue above.
**Pros:** correct predicate, correct scope, structurally incapable of diverging from runtime behavior.
**Cons:** most code; same ordering constraint as B1; ~1s per package parse cost (`ParsedSetup` execs `setup.py`).
Ordering constraint (applies to both B1 and B2)
`ci_tools` is **not importable where the step currently sits**. In `live.tests.yml` the new steps run *before* the `build-test.yml` template, and everything needed arrives inside it:
| What | Where |
|---|---|
| Interpreter selection | `steps/build-test.yml` → `use-python-version.yml` |
| Virtualenv | `steps/build-test.yml` → `use-venv.yml` |
| `azure-sdk-tools` install | `steps/build-test.yml` → `Prep Environment` (`pip install -r eng/ci_tools.txt`) |
`build-test.yml` exposes a `BeforeTestSteps` parameter that runs **after** `Prep Environment` and **before** `Run Tests` — exactly the window required. All four cibuildwheel steps would move into that hook.
**There is a latent bug in the way:** `live.tests.yml` declares `BeforeTestSteps` as a parameter (line 20) but **never forwards it** to `build-test.yml`. Live-test callers that set it have their steps silently dropped today. `jobs/ci.tests.yml` forwards correctly. `stages/archetype-sdk-tests.yml` does pass the parameter into `live.tests.yml`, so this is reachable from real callers — `stages/cosmos-sdk-client.yml` is an existing consumer of `BeforeTestSteps`.
Wiring it through is ~3 lines and fixes that bug as a side effect. It should arguably be fixed regardless of what happens to this issue.
Option C — Narrow what needs gating at all
Orthogonal simplification, combinable with any of the above.
The `CIBW_*` environment variables are **inert unless cibuildwheel actually runs**. Setting `CIBW_ARCHS`, `CIBW_SKIP`, `CIBW_TEST_SKIP`, and `CIBW_ENVIRONMENT_PASS_LINUX` in a job that never invokes cibuildwheel has no effect whatsoever.
So the `Configure cibuildwheel for dev requirement builds` step could be made **unconditional** at zero risk. Only the two Windows NuGet steps genuinely need gating, because those cost wall-clock time and can fail.
That shrinks the surface the detection logic has to protect from four steps to two, and correspondingly reduces the cost of the detection being wrong: a false negative would then only mean a Windows job fetching CPython from a blocked endpoint (loud, obvious) rather than a silent misconfiguration.
---
## Recommendation
> [!NOTE]
> The following recommendation was produced by an LLM agent (GitHub Copilot CLI) during the investigation that led to PR #48371. It reflects analysis of the code paths cited above and locally prototyped verification, but has not been reviewed by a human engineer. Treat it as a starting point rather than a decision.
**Option B2 (mirror `build_whl_for_req`) via `BeforeTestSteps`, combined with Option C.**
Rationale:
1. **C first, independently** — make the `Configure cibuildwheel` step unconditional. It is free, reduces the gated surface to the two Windows NuGet steps, and lowers the blast radius of any detection mistake.
2. **Fix the `BeforeTestSteps` forwarding bug in `live.tests.yml`** regardless. It's a genuine defect with a ~3-line fix, and it unblocks the ordering constraint.
3. **Then B2**, because it is the only option that cannot drift: it calls the same functions the runtime calls. B1 is a reasonable fallback if the parameter plumbing is unwanted — it is strictly better than the current regex for roughly one added line, and keeps every step where it is today.
Option A should be ruled out unless someone is separately motivated to add a compiled/extension field to the package properties schema, in which case `resolve-build-platforms.yml`'s hardcoded `azure-storage-extensions` string would be worth fixing at the same time.
**Priority: low.** There is no active bug — the heuristic and the truth agree on every package in the repo today. This is about preventing a confusing failure mode later, and the cost of that failure mode is a service's live tests breaking with a non-obvious root cause.
### Context
- PR: #48371
- Original failing build: [6634674](https://dev.azure.com/azure-sdk/internal/_build/results?buildId=6634674) (22 of 24 jobs failed at dev-requirement install)
- Validation build: [6636078](https://dev.azure.com/azure-sdk/internal/_build/results?buildId=6636078)
Contributor guide
Assessment
This issue has not been assessed yet.