Azure / Azure/azure-sdk-for-python

[EngSys] Detect compiled dev requirements deterministically instead of regex-matching [tool.cibuildwheel]

Abierto
#48,372 0 comentarios 1 reacción 1 asignado Reclamado por @danieljurek Ver en GitHub
EngSys
Lenguaje dominante
Python
Estrellas
5.6k
Forks
3.4k
Merge medio
2 d 2 h
PR fusionados (30 d)
213

Descripción

## 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)

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.