fix(sources): PEP 625 sdist name by default

[tiran]

Pull Request Description

What

default_download_source now uses PEP 625-compatible sdist name instead of relying on remote URL file name. This solves a file conflict when multiple packages use the same remote sources. The file name is the normalized, lower-case package name with - replaced by _, e.g. flit_core instead of flit-core or zope_interface instead of zope.interface.

Zip file support is incompatible with PEP 625. The package setting download_source.destination_filename can also define a PEP 625-incompatible file name. We should tackle these issues in the near future.

• https://packaging.python.org/en/latest/specifications/source-distribution-format/
• https://peps.python.org/pep-0625/

Why

The current default causes problems when several packages have the same upstream source tar ball. In downstream we build several packages from the mlserver-{version}.tar.gz source tar ball.

See: #1142

• PR follows CONTRIBUTING.md guidelines

[coderabbitai]

📝 Walkthrough

Walkthrough

The change adds fallback logic to default_download_source that derives a sensible local filename when destination_filename is absent from the package build info. The function extracts the remote filename from the resolved URL, determines whether to use .zip or .tar.gz format based on the URL extension, incorporates override_module_name and version into the generated name, and emits a debug log of the chosen default.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: implementing PEP 625-compatible sdist naming in the default_download_source function.
Description check	✅ Passed	The description clearly explains what changed (PEP 625-compatible sdist naming), why it matters (solves file conflicts when multiple packages share sources), and references the relevant issue and specifications.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

src/fromager/sources.py (1)

221-221: ⚡ Quick win

Wrap per-requirement debug logging in request context

Line 221 adds requirement-scoped logging without req_ctxvar_context(), which breaks the project’s logging contract and makes correlation harder.

Suggested change

+from .context import req_ctxvar_context
...
-        logger.debug("config has no destination_filename, use default %r", destination_filename)
+        with req_ctxvar_context(req):
+            logger.debug(
+                "config has no destination_filename, use default %r",
+                destination_filename,
+            )

As per coding guidelines, "Use req_ctxvar_context() for per-requirement logging".

🤖 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 `@src/fromager/sources.py` at line 221, The debug log call for
destination_filename must be executed inside the requirement request context;
wrap the logger.debug("config has no destination_filename, use default %r",
destination_filename) call with req_ctxvar_context() so the per-requirement
context is active (e.g., use a with req_ctxvar_context(): block around that
logger.debug in the function that contains it), and ensure req_ctxvar_context is
imported where needed so correlation fields are present in logs.

🤖 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.

Nitpick comments:
In `@src/fromager/sources.py`:
- Line 221: The debug log call for destination_filename must be executed inside
the requirement request context; wrap the logger.debug("config has no
destination_filename, use default %r", destination_filename) call with
req_ctxvar_context() so the per-requirement context is active (e.g., use a with
req_ctxvar_context(): block around that logger.debug in the function that
contains it), and ensure req_ctxvar_context is imported where needed so
correlation fields are present in logs.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0f997965-1041-407e-8da7-78f700645557

📥 Commits

Reviewing files that changed from the base of the PR and between 2132ef3 and 527964f.

📒 Files selected for processing (1)

• src/fromager/sources.py

[smoparth]

LGTM. I checked a few places where the new filename could conflict with consumers but the fuzzy matchings and fallbacks already cover everything.

We may want to add a couple of test cases to validate this new behavior, like asserting that two packages sharing the same upstream URL (like the mlserver monorepo) get distinct local filenames.

Also, CodeRabbit left a nitpick about wrapping the logger.debug with req_ctxvar_context() for per-requirement log correlation. PTAL.

[tiran]

Also, CodeRabbit left a nitpick about wrapping the logger.debug with req_ctxvar_context() for per-requirement log correlation. PTAL.

That is wrong. req_ctxvar_context() should not be called here. The context vars are set up in the core loops of bootstrap, builder, and CLI entry points.

[LalatenduMohanty]

We atleast need tests covering following

1. When no destination_filename is configured, the downloaded sdist gets a normalized {name}-{version}.tar.gz name instead of the remote URL's
   filename.
2. ZIP extension preserved : Same as above, but when the source URL points to a .zip file, the default filename keeps the .zip extension.

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@tests/test_sources.py`:
- Around line 169-184: The parametrized tests under pytest.mark.parametrize
(parameters "pkg,version_str,url,expected_filename") currently only cover hyphen
normalization; add two cases to verify dot and uppercase normalization: one with
"zope.interface==X.Y" expecting the filename to use "zope_interface-X.Y" (dots
-> underscores) and one with an uppercase package like "Cython==A.B" expecting
the filename to be lowercase "cython-A.B" (uppercase -> lowercase per PEP 625);
add these two tuples to the existing list so the test exercising the filename
normalization logic covers dots and case normalization as well.

🪄 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: 8abd82c6-6f08-431d-948c-3b08158011a3

📥 Commits

Reviewing files that changed from the base of the PR and between 527964f and 9e4b6e1.

📒 Files selected for processing (2)

• src/fromager/sources.py
• tests/test_sources.py

🚧 Files skipped from review as they are similar to previous changes (1)

• src/fromager/sources.py