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 ! |
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.
freakboy3742
left a comment
There was a problem hiding this comment.
Seems reasonable from the iOS perspective; there aren't any auditing tools at present, but this would allow a project to define one if they wanted to.
|
Alright – I think I have made all the changes I wanted to make here, but will self-review a little later once others have a chance to look at them. I'm all done, so I'll unassign myself now :D I've retriggered the CI for the Azure Windows timeout, and all other CI tests look green. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 33 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mhsmith
left a comment
There was a problem hiding this comment.
Looks fine as far as Android is concerned. I haven't used abi3audit myself, but if it works for Linux then in principle it should work for Android too.
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