refactor: shared build runner across platforms#2907
Conversation
|
I do like how the general run_builds function reads. And yes, centralisation of that logic is very sensible. Clever to side-step the Linux container complexity by just keeping that out of the refactor. (The Downsides in my head:
|
Adds cibuildwheel/platforms/runner.py: a Builder ABC (generic over the wheel path type, since Linux wheels live in the container as PurePosixPath) whose methods are the platform-specific build steps, and run_builds(), a single flat loop that owns everything identical across platforms: logging, compatible-wheel reuse, built/repaired wheel validation, the test gate, output bookkeeping, and cleanup. HostBuilder provides the step bodies shared by the five non-Linux platforms. run_before_all() and fatal_on_called_process_error() replace the per-platform copies of the before_all hook and the CalledProcessError-to-FatalError wrapper. No platform uses this yet; subsequent commits port them one at a time. Assisted-by: ClaudeCode:claude-fable-5
The per-wheel loop in build() is replaced by a PyodideBuilder driven by
runner.run_builds(). Behavior changes:
- a compatible-wheel (abi3) reuse no longer crashes with a NameError
(the old loop assigned built_wheel but read repaired_wheel; it was
unreachable in practice since pyodide wheels are never reused)
- a repair command producing zero wheels now raises
RepairStepProducedNoWheelError instead of a raw StopIteration, and
producing several is now an error, like on Linux
- before_test no longer receives the undocumented {wheel} placeholder;
test_command now does, matching the other platforms
- the per-identifier temp dir is now removed after each build
Assisted-by: ClaudeCode:claude-fable-5
The per-wheel loop becomes a MacOSBuilder; the per-architecture test loop (universal2 testing both halves, Rosetta emulation, arch-specific test-skip identifiers) stays inside test_wheel(), unchanged. Behavior changes: - a repair command producing several wheels is now an error, as on Linux - a missing uv now raises FatalError instead of AssertionError - the per-identifier temp dir is removed with ignore_errors, like Windows Assisted-by: ClaudeCode:claude-fable-5
The per-wheel loop becomes a WindowsBuilder. The ARM64-on-non-ARM64 test skip moves inside test_wheel(), so its warning now only appears when a test command is actually configured. A repair command producing several wheels is now an error, as on Linux. Assisted-by: ClaudeCode:claude-fable-5
The per-wheel loop becomes an IOSBuilder; the testbed-based test flow, the non-simulator/non-native skip steps, and the python-module test command validation stay inside test_wheel(). Behavior changes: - a test-suite failure now raises FatalError instead of calling sys.exit(1) directly (same exit code, consistent error reporting) - a repair command producing several wheels is now an error, as on Linux - the compatible-wheel reuse message now matches the other platforms - the per-identifier temp dir is removed with ignore_errors Assisted-by: ClaudeCode:claude-fable-5
The BuildState dataclass and its step functions fold into an AndroidBuilder; setup_target_python/setup_env and the other environment helpers are unchanged. Behavior changes: - test-environment (CIBW_TEST_ENVIRONMENT_ANDROID) is now applied to the test steps (before-test, wheel install, testbed invocation); it was documented but previously ignored on Android - a wheel name colliding with an earlier build now raises AlreadyBuiltWheelError, like every other platform - the redundant post-repair none-any.whl check is dropped (the built wheel is still checked, like every other platform) - the compatible-wheel reuse message now matches the other platforms, and the moved-elsewhere warning from move_file is now emitted - a missing uv now raises FatalError instead of AssertionError Assisted-by: ClaudeCode:claude-fable-5
The per-config loop in build_in_container() becomes a LinuxBuilder (generic over PurePosixPath — wheels stay container-side, and the existing copy-out and host-side audit behavior is preserved as overridden steps). build() keeps the container grouping, engine check, and OCIContainer lifecycle; the CalledProcessError wrapper keeps its per-container scope and troubleshoot() hook via fatal_on_called_process_error. build_in_container() keeps its signature, since tests mock it. Also fixes a latent bug: the host-side per-identifier temp dir was never created, so dependency-versions with packages: would have crashed writing constraints.txt on Linux. Assisted-by: ClaudeCode:claude-fable-5
39fdbba to
db751f8
Compare
Rework the builder design in response to review feedback: - Builder is now a Protocol rather than an ABC; platform builders are plain frozen dataclasses with no base class, checked structurally. - A builder is only constructed once its setup has completed. run_builds() takes BuildSpec(identifier, setup) pairs, where setup() prepares the environment and returns the ready-to-use builder, so every attribute on a builder is set from the moment it exists and the dataclass fields document exactly what state each platform carries. - The step bodies previously inherited from the HostBuilder base class are now module functions (runner.host_before_build() etc.) taking a small HostBuilder protocol; platforms delegate to them explicitly, so each platform file lists every step it performs. The two functions that run user commands take env explicitly, which also makes Android's dual-environment usage visible at the call sites. Assisted-by: ClaudeCode:claude-fable-5
|
I still need to review and see how well it did. 🤖 AI text below 🤖 Both downsides felt real, and @henryiii also wanted to move away from inheritance, so the latest commit reworks the design:
A small bonus: On |
Android checked the repaired wheel for none-any and its test suite asserts that; the other platforms only checked the built wheel, so a repair command emitting a pure wheel slipped through silently. Rather than dropping Android's check, extend it: run_builds() now validates the repaired wheel too, on all platforms. Assisted-by: ClaudeCode:claude-fable-5
Alternative to #2903, this time using Claude Fable 5 instead of Claude Opus 4.8. This is not just a one to one; this one doesn't have nearly as much restraining it, and I didn't tell it where I wanted to go later (
--stage). But otherwise it's also an unedited output of a plan.This is not mergable as it stands, but I think I can iterate on it to make something worth reviewing. I'll try to rebase it and work on it after 4.1.
🤖 AI text below 🤖
What
Extracts the per-wheel build loop that was duplicated across all six platform modules into a shared runner (
cibuildwheel/platforms/runner.py), driven by a per-platform builder class with clearly-named steps:Builder(Protocol[PathT])— one method per step, checked structurally; there is no base class. Each platform's builder is a frozen dataclass whose fields are exactly the state its steps need. Generic over the wheel-path type because Linux wheels live in the container asPurePosixPath; everywhere else they're hostPaths.BuildSpec(identifier, setup)— a builder is only constructed once its environment is fully set up:setup()does the work and returns the ready builder, so every attribute is set for the object's whole lifetime.run_builds()— a single flat loop that owns everything that was previously copy-pasted six times:log.build_start/build_end/step accounting, compatible-wheel (abi3) reuse, thenone-any.whlcheck, exactly-one-repaired-wheel validation,AlreadyBuiltWheelError, the test gate, output bookkeeping, and cleanup. The loop body reads top-to-bottom like the old per-platform loops — there's no step registry or hook indirection.host_*()functions — the step bodies shared by the five non-Linux platforms (before_build, default repair, audit, move-to-output, cleanup), taking a smallHostBuilderprotocol. Platforms delegate to them explicitly, so each platform module lists every step it performs.run_before_all()/fatal_on_called_process_error()— replace the per-platform copies of the before_all hook and theCalledProcessError → FatalErrorwrapper (Linux keeps its per-container scope andtroubleshoot()hook).Each platform's
build()shrinks to ~15 lines; platform-specific structure stays where it was and stays visible: macOS keeps its per-arch test loop (universal2/Rosetta), iOS its testbed flow, Android its dual build/android environments (the previousBuildStatepattern was the prototype for this design), pyodide itspyodide venvtesting, and Linux keepsget_build_steps()grouping and theOCIContainerlifecycle, invoking the runner once per container.build_in_container()keeps its signature (it's mocked inoption_prepare_test.py), and thePlatformModuleinterface is unchanged.Behavior changes
The duplication had let the platforms drift; this unifies the unjustified differences (one commit per platform, each itemized in its commit message):
Bugs fixed
NameError(assignedbuilt_wheel, readrepaired_wheel) — unreachable today only because pyodide wheels are never reusedStopIterationinstead ofRepairStepProducedNoWheelErrordependency-versionswithpackages:crashed because the host-side per-identifier temp dir was never createdtest-environment/CIBW_TEST_ENVIRONMENT_ANDROIDwas documented but silently ignored; it's now applied to before-test, the wheel install, and the testbed invocationUnified behavior
RepairStepProducedMultipleWheelsError, previously Linux/Android-only)AlreadyBuiltWheelErrorduplicate-name check; its post-repairnone-any.whlcheck is extended to every platform (previously the other five only checked the built wheel, so a repair command emitting a pure wheel slipped through)FatalErrorinstead of callingsys.exit(1)(same exit code)before_testno longer receives an undocumented{wheel}placeholder;test_commandnow receives{wheel}like Linux/macOS/WindowsAssertionError("uv not found")→FatalError(macOS, Android)log.step_end()accounting, host temp dirs removed withignore_errors=True(pyodide previously never removed them), Android gains the "was moved to … instead of" warningTesting
unit_test/runner_test.py(new): a recordingFakeBuildercovers step ordering, compatible-wheel reuse (build/repair/audit/move skipped, test still runs), 0/2-wheel repair errors, duplicate-name and none-any errors, test gating, and the error wrapper + troubleshoot hook.build()monkeypatch seams andoption_prepare_test.py), doctests, ruff, strict mypy (3.11 + 3.14), pylint.🤖 Generated with Claude Code