Exclude stale native build artifacts from PEP 517 build copy#1682
Exclude stale native build artifacts from PEP 517 build copy#1682aiolibsbot wants to merge 10 commits into
Conversation
|
@aiolibsbot rr |
PR Review — Exclude stale native build artifacts from PEP 517 build copyThe core fix is correct and well-placed. Filtering at the 🟡 Important1. Apply @webknjaz's :pep:`517` role suggestion (`CHANGES/1583.packaging.rst`, L2)@webknjaz requested the from the isolated :pep:`517` build directory, preventing wheels fromThis renders a hyperlink in the published release notes and matches house style. Note for spelling: Checklist
SummaryThe core fix is correct and well-placed. Filtering at the Automated review by Kōan1bb84c6 |
Rebase with requested adjustmentsBranch StatsActions performed
CI statusCI will be checked asynchronously. Automated by Kōan |
1e8c2fc to
15935ba
Compare
|
change looks ok but could use a simple unit test on _exclude_dir_path |
|
no advertising please, remove |
|
@aiolibsbot rr |
Agreed —
The backend is not on the default test |
Will remove. Also noting for the agent's future runs: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1682 +/- ##
=======================================
Coverage 99.81% 99.81%
=======================================
Files 21 22 +1
Lines 4249 4287 +38
Branches 250 250
=======================================
+ Hits 4241 4279 +38
Misses 5 5
Partials 3 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Rebase with requested adjustmentsBranch Changes applied
StatsActions performed
CI statusCI will be checked asynchronously. Automated by Kōan |
b2b4926 to
4e6022d
Compare
|
add a simple unit test on _exclude_dir_path |
|
@aiolibsbot rr |
Confirming this is still the outstanding blocker on the code side. |
Add it to this branch |
Rebase with requested adjustmentsBranch Changes applied
StatsActions performed
CI statusCI will be checked asynchronously. Automated by Kōan |
|
mypy fail |
|
Also don't run the backend tests in the wheel build |
|
@aiolibsbot rr |
Agreed, and it's worse than just "shouldn't run" — as written the new file will fail in the cibuildwheel leg, not skip. The fixture does The cleanest fix is at the test-command level — adding test-command = 'pytest -v -m "not hypothesis" --no-cov --ignore={project}/tests/test_pep517_backend.py {project}/tests'That way the wheel test venv never tries to import the backend at all, and we don't need to add a custom marker (the |
Confirmed, will strip the |
The current revision should clear both errors — |
Rebase with requested adjustmentsBranch Changes applied
StatsActions performed
CI statusCI will be checked asynchronously. Automated by Kōan |
| @@ -0,0 +1,3 @@ | |||
| Excluded stale native build artifacts (``*.so``, ``*.pyd``, ``*.dylib``) | |||
| from the isolated PEP 517 build directory, preventing wheels from | |||
|
|
||
| # Skip stale native build artifacts so cross-interpreter `.so`/`.pyd`/`.dylib` | ||
| # from prior in-place builds don't leak into the wheel. See #1583. | ||
| _STALE_BUILD_ARTIFACT_SUFFIXES = ('.so', '.pyd', '.dylib') |
There was a problem hiding this comment.
I wonder if we could somehow get a list of Git-tracked files and build this list dynamically.
There was a problem hiding this comment.
Considered, but a git-tracked allowlist has a hard edge case here: PEP 517 wheel builds frequently run from an unpacked sdist that has no .git directory, so git ls-files returns nothing (or errors). At that point the ignore callback would either exclude everything or fall back to a heuristic, so a suffix-style denylist would have to come back as the no-git path anyway. There's also a mechanical wrinkle: copytree invokes the ignore callback per directory with names relative to that directory, so we'd have to reconstruct each entry's repo-relative path to query git, which adds real complexity. For #1583 specifically the three-suffix denylist is sufficient and self-contained. A tracked-file allowlist is appealing because it would also subsume the broader ".venv/ and other untracked junk gets copied into the build tree" problem, but that's a larger change and probably better scoped as its own PR with an explicit no-git fallback rather than folded in here.
|
|
||
| import pytest | ||
|
|
||
| _PACKAGING_DIR = Path(__file__).resolve().parents[1] / "packaging" |
There was a problem hiding this comment.
Have you tried pytest's pythonpath config setting?
There was a problem hiding this comment.
pythonpath (the pytest 7+ ini option) would work, but it puts packaging/ on sys.path for the entire session rather than just the one test that needs it. That's broader than necessary: it would be active during collection of every test module, and the ini setting still applies under the cibuildwheel run even though this file is --ignored there, so packaging/ and its build-only imports (expandvars, setuptools.build_meta) would be importable in the wheel test venv where they shouldn't be reachable. Scoping the path manipulation to the fixture keeps it confined to the test that actually imports the backend. If we did move to pythonpath, the manual insert/remove (or the MonkeyPatch.context() from the sibling comment) would go away entirely.
|
|
||
| yield _backend | ||
| finally: | ||
| sys.path.remove(str(_PACKAGING_DIR)) |
There was a problem hiding this comment.
FYI, the built-in monkeypatch fixture can handle this more elegantly: https://docs.pytest.org/en/stable/reference/reference.html#pytest.MonkeyPatch.syspath_prepend
There was a problem hiding this comment.
Agreed it's cleaner and the auto-revert removes the hand-rolled try/finally. One wrinkle worth flagging: this fixture is scope="module", and the built-in monkeypatch fixture is function-scoped, so requesting it here raises ScopeMismatch. The module-scoped equivalent is the context-manager form you linked:
@pytest.fixture(scope="module")
def backend() -> Iterator[ModuleType]:
pytest.importorskip("setuptools")
pytest.importorskip("expandvars")
with pytest.MonkeyPatch.context() as mp:
mp.syspath_prepend(str(_PACKAGING_DIR))
from pep517_backend import _backend # noqa: PLC0415
yield _backendAlternatively drop the fixture to function scope and use the plain monkeypatch fixture; the import is cached in sys.modules after the first call, so re-resolving it per test is effectively free and the scope="module" optimization isn't buying much. Either form removes the manual sys.path.insert/remove block.
There was a problem hiding this comment.
Any chance to use the built-in pytester fixture to run this stuff e2e in true isolation?
There was a problem hiding this comment.
pytester would let us stand up a throwaway source tree and drive _in_temporary_directory -> copytree end-to-end in true isolation, and it would additionally exercise the symlinks=True + is_file() interaction that the pure unit tests can't reach, for example a symlinked .so (which is_file() follows, so it gets excluded) versus a dangling symlink (which stays). The trade-off is that it's heavier and couples the test to setuptools/cythonize being runnable in the test environment, whereas _exclude_dir_path is a pure function whose only I/O is the is_file() probe, so the tmp_path tests already pin the filter logic and both print branches. I'd treat a pytester e2e as a worthwhile follow-up specifically if we want coverage of the copytree wiring and the symlink behavior, rather than as a requirement for validating this particular fix.
|
@aiolibsbot rr |
Pre-built ``*.so``/``*.pyd``/``*.dylib`` files left in the source tree from a previous build under a different Python interpreter were being copied into the isolated PEP 517 build directory by ``copytree``, which caused them to be packaged into the wheel for the current interpreter ABI. This is the scenario behind aio-libs#1583: ``cibuildwheel`` does not clear out previously built extensions between Python versions, so each subsequent build was inheriting stale shared libs from earlier runs in the same checkout. The fix extends ``_exclude_dir_path`` so that ``copytree``'s ``ignore`` callback also drops any file ending in ``.so``, ``.pyd``, or ``.dylib`` at copy time. The current build still produces a fresh extension inside the isolated build tree as usual. Verified end-to-end by planting a fake ``yarl/_quoting_c.cpython-399-x86_64-linux-gnu.so`` and running ``python -m build --wheel`` -- the resulting wheel contained only the real ``cpython-312`` extension. Existing test suite (1386 tests) still passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rebase with requested adjustmentsBranch Changes applied
StatsActions performed
CI statusCI will be checked asynchronously. Automated by Kōan |
747e91d to
cd63b57
Compare
What do these changes do?
Filter out pre-built C-extension shared libraries (
*.so,*.pyd,*.dylib)from the source tree at PEP 517
copytreetime, so they cannot leak intothe isolated build directory and end up bundled in the resulting wheel.
The change is localized to
_exclude_dir_pathinpackaging/pep517_backend/_backend.py, which is already theignorecallback passed to
shutil.copytreefor the temporary build directory.It now also drops any file ending in one of the three native-extension
suffixes, alongside the existing tempdir-parent self-exclusion.
Are there changes in behavior for the user?
Yes, but only on the build side. Previously, when
cibuildwheelranmultiple Python interpreters in the same checkout (or when a developer
had done an in-place build before invoking
python -m build), stale.sofiles from earlier builds would be copied into the isolated buildtree and packaged into the wheel — yielding wheels that contained shared
libs for the wrong ABI. With this change, those stale artifacts are
skipped at copy time and the wheel only contains the extension freshly
compiled for the current interpreter.
The PEP 517 backend now also logs a short note for each directory where
stale artifacts were skipped, so the behavior is observable in build
output.
Is it a substantial burden for the maintainer to support this?
No. The change adds ~12 lines to a single function in the PEP 517
backend, gated by a small module-level tuple of three well-known
native-extension suffixes. There is no new dependency, no new public
API, and no new configuration surface. Future maintenance amounts to
updating the suffix tuple if a new platform extension appears, which
has not happened in years.
Related issue number
Fixes #1583.
Checklist
(verified end-to-end by planting a fake
yarl/_quoting_c.cpython-399-x86_64-linux-gnu.soand runningpython -m build --wheel; the resulting wheel contained only thereal
cpython-312extension. The existingtests/suite continuesto pass — 1386 tests. The PEP 517 backend itself has no dedicated
pytest suite in this repo, so a unit test was not added here.)
(news fragment added:
CHANGES/1583.packaging.rst)CHANGES/folder1583.packaging.rstpackaging🤖 Generated with Claude Code