Conversation
|
Any feedback on the documentation welcome! |
|
Thanks very much for writing this @joerick! I read the documentation you added and it all looks good to me on a high level. Would you like that I close my PR? I admit that I have been stretched thin for time to take a look at it again after the review comments, and was planning to get to it at some point in April – so if it is the case that you would like to take over, please let me know! |
|
Thanks @agriyakhetarpal. Yes let's close out #2745 and continue work here. Happy for you to contribute too if you want as well! I've added a todo list to the PR description. |
|
Awesome, thank you! Please let me know when you are done with your changes, and I'd be happy to create some PRs against this branch – so as to save ourselves from the trouble of mutual merge conflicts :D I had a fun time writing the tests out in #2745, so I'm happy to continue in that direction. |
|
I really like the fact that running both twine check on all wheels but abi3audit only on abi3 wheels is already being thought of ! |
|
It took me a while to get to this (seven days more than expected :D), but I've added a ton of tests in the last bunch of commits above, adding to our existing test suite!
I did not fully understand what the
item meant, and how we could implement it. @joerick, I assume you have some clearer ideas about this! Once again, thanks for letting me continue and build on top of your changes here! So, while it may not be complete, I'll mark it as ready for review so that others can chime in :) |
There was a problem hiding this comment.
Pull request overview
Adds a configurable “audit” step to cibuildwheel, with an abi3audit-based default that runs after wheel repair and can be customized via new options.
Changes:
- Introduces
audit-command/audit-requiresoptions (defaults + schema) and wires them intoBuildOptions. - Implements a new audit runner (
cibuildwheel/audit.py) and calls it after repair across platforms (including Linux-outside-container handling). - Adds unit + integration tests and updates docs/README/diagram to document the new auditing behavior.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| unit_test/utils_test.py | Adds unit tests for abi3 wheel filename detection. |
| unit_test/options_toml_test.py | Adds unit tests for new TOML/env options parsing. |
| unit_test/main_tests/main_options_test.py | Verifies default/override behavior for audit-requires. |
| unit_test/audit_test.py | Unit tests for needs_audit and run_audit behaviors. |
| test/test_abi3audit.py | Integration tests for default abi3audit behavior and customization. |
| README.md | Documents new auditing options in the options table. |
| docs/options.md | Adds “Auditing” section and updates related dependency-versions note. |
| docs/faq.md | Updates ABI3 guidance to reflect automatic abi3audit. |
| docs/diagram.html | Adds audit step node and adjusts grid layout. |
| cibuildwheel/venv.py | Refactors venv activation into activate_virtualenv(). |
| cibuildwheel/util/packaging.py | Adds is_abi3_wheel() helper. |
| cibuildwheel/resources/defaults.toml | Sets defaults for audit options; disables audit-command on pyodide. |
| cibuildwheel/resources/constraints.in | Adds abi3audit to constraints inputs. |
| cibuildwheel/resources/cibuildwheel.schema.json | Adds schema entries for audit options and override inheritance. |
| cibuildwheel/platforms/windows.py | Runs audit after repair on Windows. |
| cibuildwheel/platforms/pyodide.py | Runs audit after repair on Pyodide builds (default command disabled). |
| cibuildwheel/platforms/macos.py | Runs audit after repair on macOS. |
| cibuildwheel/platforms/linux.py | Runs audit outside the container (copies wheel out) when needed. |
| cibuildwheel/platforms/ios.py | Runs audit after repair on iOS. |
| cibuildwheel/platforms/android.py | Runs audit after repair on Android. |
| cibuildwheel/options.py | Parses/stores new audit options into BuildOptions and GlobalOptions. |
| cibuildwheel/errors.py | Adds AuditCommandFailedError. |
| cibuildwheel/audit.py | New module implementing audit venv setup, dependency install, and command execution. |
| bin/generate_schema.py | Updates schema generator inputs for new options. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| audit_venv_dir = tmp_dir / "audit_venv" | ||
| if not audit_venv_dir.exists(): | ||
| audit_venv_dir.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| env = virtualenv( | ||
| version, | ||
| Path(sys.executable), | ||
| audit_venv_dir, | ||
| dependency_constraint=dependency_constraint, | ||
| use_uv=use_uv, | ||
| ) | ||
| else: | ||
| env = activate_virtualenv(audit_venv_dir) |
There was a problem hiding this comment.
run_audit pre-creates audit_venv_dir with mkdir() before calling virtualenv(...). Both virtualenv and uv venv commonly expect the target directory to not already exist, so this can cause venv creation to fail (and if it partially fails, later runs will try to activate_virtualenv a non-venv directory). Consider not creating audit_venv_dir ahead of time (only ensure the parent exists), and/or validating the venv by checking for pyvenv.cfg/bin/python before reusing it.
| log.step("Auditing wheel...") | ||
|
|
||
| use_uv = build_options.build_frontend.name in {"uv", "build[uv]"} | ||
| version = f"{sys.version_info.major}.{sys.version_info.minor}.{sys.version_info.micro}" | ||
| dependency_constraint = build_options.dependency_constraints.get_for_python_version( |
There was a problem hiding this comment.
use_uv for the audit venv is derived from build_options.build_frontend. On Linux, wheels may be built with uv inside the container, but run_audit executes on the host where uv may not be installed, causing an assertion failure in venv.virtualenv(use_uv=True). The audit environment should likely be created with the standard virtualenv flow regardless of the build frontend, or at least fall back to virtualenv when uv isn’t available on the host.
| for command_template in audit_command: | ||
| if "{abi3_wheel}" in command_template and "{wheel}" in command_template: | ||
| msg = ( | ||
| f"Invalid audit command {command_template!r}: cannot contain both {{abi3_wheel}} " | ||
| "and {{wheel}} placeholders" | ||
| ) | ||
| raise errors.ConfigurationError(msg) | ||
|
|
||
| if "{abi3_wheel}" in command_template and not is_abi3_wheel(wheel.name): | ||
| continue |
There was a problem hiding this comment.
Audit commands that contain neither {wheel} nor {abi3_wheel} are currently silently ignored: needs_audit returns False, so run_audit exits without ever surfacing the misconfiguration. Since the docs say each command must include one placeholder, this should raise a ConfigurationError (either while parsing options or when validating audit_command here) so users don’t think auditing ran when it didn’t.
| dependency versions on Linux, use the [`manylinux-*` / `musllinux-*`](#linux-image) | ||
| options. | ||
|
|
||
| There is one exception to this rule - if `audit-requires` specifies `abi3audit`, its version is govenered by this option, because audits take place outside of the build container. |
There was a problem hiding this comment.
Typo: govenered should be governed.
| There is one exception to this rule - if `audit-requires` specifies `abi3audit`, its version is govenered by this option, because audits take place outside of the build container. | |
| There is one exception to this rule - if `audit-requires` specifies `abi3audit`, its version is governed by this option, because audits take place outside of the build container. |
|
|
||
| If no audit command is specified, or no audit is required (i.e. your project builds non-abi3 wheels and the command refers only to abi3 wheels), then the audit environment won't be created and this option is ignored. | ||
|
|
||
| If you leave this as the default, the versions of abit3audit and libraries are pinned according to [`dependency-versions`](#dependency-versions), even on Linux. |
There was a problem hiding this comment.
Typo: abit3audit should be abi3audit.
| If you leave this as the default, the versions of abit3audit and libraries are pinned according to [`dependency-versions`](#dependency-versions), even on Linux. | |
| If you leave this as the default, the versions of abi3audit and libraries are pinned according to [`dependency-versions`](#dependency-versions), even on Linux. |
| "type": "object", | ||
| "properties": { | ||
| "audit-command": { | ||
| "description": "Execute a shell command to audit each wheel after all builds complete. Use {wheel} for each wheel path, or {abi3_wheel} to only audit abi3 wheels.", |
There was a problem hiding this comment.
The schema description says audit runs "after all builds complete", but the implementation runs auditing per wheel right after the repair step. Please update this description to match the actual behavior so config UIs and users aren’t misled.
| "description": "Execute a shell command to audit each wheel after all builds complete. Use {wheel} for each wheel path, or {abi3_wheel} to only audit abi3 wheels.", | |
| "description": "Execute a shell command to audit each wheel after it is repaired. Use {wheel} for each wheel path, or {abi3_wheel} to only audit abi3 wheels.", |
| build | ||
| delocate | ||
| virtualenv | ||
| abi3audit |
There was a problem hiding this comment.
abi3audit was added to constraints.in, but the checked-in compiled constraints files (e.g. constraints.txt / constraints-python*.txt) don’t appear to include a pinned abi3audit version yet. Since audit-requires defaults to abi3audit and the code conditionally uses the pinned constraints for determinism, please regenerate and commit the updated constraints outputs (via the repo’s constraints update workflow) so pinned mode is actually reproducible.
| abi3audit | |
| abi3audit==0.0.24 |
Following on from #2745.
I've only sketched out the functionality in documentation form so far - implementation to come.The idea is that audit commands can specify
{abi3wheel}or{wheel}placeholders, which determine whether they're run for a particular wheel.I think the best approach here would be to run the audit on each wheel as it's built, similar to @agriyakhetarpal's approach. As I've been thinking about it, it turns out there's nothing stopping that as far as I'm aware and it prevents the frustration of leaving the failure to the end of the build.
Todos
audit-requiresinheritto work outside of overrides, making it easier for audit configs to extend the default