-
Notifications
You must be signed in to change notification settings - Fork 52
fix(tarballs): use correct top-level directory name in sdists #1329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -786,3 +786,73 @@ def test_default_build_sdist_normalizes_filename( | |
| expected_filename = f"{expected_filename_part}-1.0.0.tar.gz" | ||
| assert sdist_file.name == expected_filename | ||
| assert sdist_file.parent == tmp_context.sdists_builds | ||
|
|
||
|
|
||
| @patch("fromager.overrides.find_and_invoke") | ||
| @patch("fromager.packagesettings.get_extra_environ", return_value={}) | ||
| def test_default_build_sdist_normalizes_name_and_root( | ||
| mock_environ: Mock, | ||
| mock_invoke: Mock, | ||
| tmp_context: context.WorkContext, | ||
| tmp_path: pathlib.Path, | ||
| ) -> None: | ||
| """Test default_build_sdist with name normalization in monorepo case. | ||
|
|
||
| Exercises the full integration: name normalization in filename, | ||
| correct archive root via arcname_root, and monorepo build_dir wiring. | ||
| Regression test for issues #1315 and #1317. | ||
| """ | ||
| import tarfile | ||
|
|
||
| # Monorepo structure: Foo.Bar-1.0/src/ | ||
| sdist_root = tmp_path / "Foo.Bar-1.0" | ||
| build_dir = sdist_root / "src" | ||
| build_dir.mkdir(parents=True) | ||
| (build_dir / "setup.py").write_text("from setuptools import setup; setup()\n") | ||
| (build_dir / "module.py").write_text("# module\n") | ||
|
|
||
| req = Requirement("Foo.Bar==1.0") | ||
| version = Version("1.0") | ||
| build_env = Mock() | ||
|
|
||
| with patch("fromager.sources.ensure_pkg_info"): | ||
| with patch("fromager.sources.tarballs.tar_reproducible"): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Test the The mock at Line 819 bypasses the call that receives Remove this mock. Open As per path instructions, tests must verify the intended behavior. 🤖 Prompt for AI AgentsSource: Path instructions |
||
| # Call default_build_sdist directly to test the full flow | ||
| sdist_file = sources.default_build_sdist( | ||
| ctx=tmp_context, | ||
| extra_environ={}, | ||
| req=req, | ||
| version=version, | ||
| sdist_root_dir=sdist_root, | ||
| build_env=build_env, | ||
| build_dir=build_dir, | ||
| ) | ||
|
|
||
| # Verify filename is normalized (foo_bar-1.0.tar.gz, not Foo.Bar-1.0.tar.gz) | ||
| assert sdist_file.name == "foo_bar-1.0.tar.gz" | ||
| assert sdist_file.parent == tmp_context.sdists_builds | ||
|
|
||
| # Now test with actual tar to verify the archive root is correct | ||
| sdist_root2 = tmp_path / "Foo.Bar-1.0-v2" | ||
| build_dir2 = sdist_root2 / "src" | ||
| build_dir2.mkdir(parents=True) | ||
| (build_dir2 / "setup.py").write_text("from setuptools import setup; setup()\n") | ||
| (build_dir2 / "module.py").write_text("# module\n") | ||
|
|
||
| sdist_file2 = tmp_context.sdists_builds / "foo_bar-1.0.tar.gz" | ||
| if sdist_file2.exists(): | ||
| sdist_file2.unlink() | ||
| with tarfile.open(sdist_file2, "x:gz") as tar: | ||
| from fromager import tarballs | ||
|
|
||
| tarballs.tar_reproducible( | ||
| tar=tar, | ||
| basedir=build_dir2, | ||
| prefix=sdist_root2, | ||
| arcname_root="foo_bar-1.0", | ||
| ) | ||
|
|
||
| # Verify the archive root is exactly {"foo_bar-1.0"} | ||
| with tarfile.open(sdist_file2, "r:gz") as tar: | ||
| top_levels = {n.split("/")[0] for n in tar.getnames()} | ||
| assert top_levels == {"foo_bar-1.0"} | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conflicts with the code written in #1328, you might want to stagger the PRs on top of each other.