feat(sdist): add helpers to normalize git clones and tarballs into va…#1169
feat(sdist): add helpers to normalize git clones and tarballs into va…#1169jlarkin09 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds src/fromager/sdist.py with PKG_INFO_TEMPLATE, _write_pkg_info(), make_sdist_directory(), and repack_as_sdist() to normalize source dirs to {name}-{version}, ensure PKG-INFO (including optional build_dir placement), rebase build_dir when renames occur, and create deterministic {name}-{version}.tar.gz archives. Integrates these helpers into sources.py (prepare_source and default_build_sdist) and refactors ensure_pkg_info to delegate stub creation. A new tests/test_sdist.py verifies directory normalization, PKG-INFO behavior, build_dir rebasing, tar contents, filename normalization, and archive replacement. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_sdist.py (1)
66-82: ⚡ Quick winAdd regression test for
rename + build_dirpath rebasingCurrent tests validate
build_dironly when no rename happens. Please add a case wheresource_dirname is changed andbuild_diris inside it, then assert PKG-INFO/tar creation still succeeds from the rebased path.As per coding guidelines
tests/**: Verify test actually tests the intended behavior. Check for missing edge cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_sdist.py` around lines 66 - 82, Add a regression test that exercises the rename + build_dir rebasing path: create a source directory whose basename differs from the package name passed to sdist.make_sdist_directory (e.g., src = tmp_path / "pkg-1.0" but call make_sdist_directory(..., "pkg-renamed", Version("1.0"), build_dir=build_dir)), ensure build_dir is inside the source (build_dir = src / "src"), call sdist.make_sdist_directory and then assert that PKG-INFO exists both in the returned result path and inside the rebased build_dir (and that a tar/sdist was created if your test harness checks archives), so the test covers the case where source_dir is renamed and build_dir must be rebased correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/fromager/sdist.py`:
- Around line 81-100: After renaming source_dir (the shutil.move call) rebase
any build_dir that points into the old source_dir so it isn't a stale path:
capture the old_source_dir before the move, then after setting source_dir =
desired compute if build_dir is not None and build_dir is within old_source_dir
(use build_dir.relative_to(old_source_dir] in a try/except) and if so set
build_dir = desired / relative; keep existing behavior for _write_pkg_info and
the later tar logic; apply the same rebasing fix to the analogous block around
lines 132-143.
---
Nitpick comments:
In `@tests/test_sdist.py`:
- Around line 66-82: Add a regression test that exercises the rename + build_dir
rebasing path: create a source directory whose basename differs from the package
name passed to sdist.make_sdist_directory (e.g., src = tmp_path / "pkg-1.0" but
call make_sdist_directory(..., "pkg-renamed", Version("1.0"),
build_dir=build_dir)), ensure build_dir is inside the source (build_dir = src /
"src"), call sdist.make_sdist_directory and then assert that PKG-INFO exists
both in the returned result path and inside the rebased build_dir (and that a
tar/sdist was created if your test harness checks archives), so the test covers
the case where source_dir is renamed and build_dir must be rebased correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dbc0f19f-c545-48f8-bd7c-35ff1b31a9c9
📒 Files selected for processing (3)
src/fromager/sdist.pysrc/fromager/sources.pytests/test_sdist.py
tiran
left a comment
tiran
left a comment
There was a problem hiding this comment.
Unfortunately this approach is not going to work. We have several packages that have a non-standard package layout and do not support Python sdist format.
| logger = logging.getLogger(__name__) | ||
|
|
||
| PKG_INFO_TEMPLATE = """\ | ||
| Metadata-Version: 2.2 |
There was a problem hiding this comment.
Is there any reason to bump the metadata version?
There was a problem hiding this comment.
@LalatenduMohanty mentioned it in the issue #554 (comment)
There was a problem hiding this comment.
Which packages are affected and do they use custom build_sdist plugin overrides?
There was a problem hiding this comment.
@jlarkin09 you should be able to run fromager+ with this PR against downstream bootstrap and build. We should atleast run an analysis to see if this will break some packages.
There was a problem hiding this comment.
@tiran You asked to bump to 2.2 meta data version in you original issue description.
There was a problem hiding this comment.
@LalatenduMohanty @tiran ran the non-accelerated/cpu-ubi9 collection on walkerpass against thisbranch. All packages bootstrapped and built successfully
jlarkin09
force-pushed
the
feat/sdist-helpers-554
branch
from
4c2965f to
845a282
Compare
| req.name, | ||
| version, | ||
| build_dir=build_dir, | ||
| ) |
There was a problem hiding this comment.
We are discarding return value. If the directory is renamed (source directory name doesn't match
{normalized_name}-{version}), both sdist_root_dir and build_dir still point to the old, now-nonexistent paths.
…lid sdists Git clones and non-standard tarballs (e.g. GitHub release assets) lack PKG-INFO and standardized directory naming that PEP 517 build backends expect. This causes setuptools-scm failures during the PREPARE_BUILD phase when get_requires_for_build_wheel is invoked. Add a new sdist module with make_sdist_directory() and repack_as_sdist() that normalize source directories before build dependency resolution. Bump stub PKG-INFO from Metadata-Version 1.0 to 2.2 and integrate into the git clone prepare_source path and default_build_sdist. Closes: python-wheel-build#554 Signed-off-by: James Larkin <jlarkin@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com>
jlarkin09
force-pushed
the
feat/sdist-helpers-554
branch
from
845a282 to
a1060b7
Compare
3 participants
Add sdist.make_sdist_directory() and sdist.repack_as_sdist() helpers that normalize git clones and non-standard tarballs into valid sdist layouts. Bump stub PKG-INFO from Metadata-Version 1.0 to 2.2 and integrate into the git clone prepare_source path and default_build_sdist.
Git clones and GitHub release tarballs lack PKG-INFO, causing setuptools-scm failures when PEP 517 hooks run during PREPARE_BUILD. Today ensure_pkg_info only runs during build_sdist -- one step too late. These helpers create a valid-enough sdist layout earlier in the pipeline using only (name, version).
Closes #554