feat(sources): generate .git_archival.txt for setuptools-scm builds#962
Conversation
LalatenduMohanty
force-pushed
the
961_git-archival-for-setuptools-scm
branch
2 times, most recently
from
779259f to
87d777b
Compare
LalatenduMohanty
force-pushed
the
961_git-archival-for-setuptools-scm
branch
4 times, most recently
from
545b0a5 to
8287e3c
Compare
LalatenduMohanty
force-pushed
the
961_git-archival-for-setuptools-scm
branch
2 times, most recently
from
d7d9adf to
811b9e9
Compare
LalatenduMohanty
force-pushed
the
961_git-archival-for-setuptools-scm
branch
from
811b9e9 to
23827f7
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ensure_git_archival(version, target_dir) to src/fromager/sources.py and invokes it from default_build_sdist() when creating an sdist and no Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
♻️ Duplicate comments (1)
src/fromager/sources.py (1)
693-697:⚠️ Potential issue | 🟠 MajorCheck the source root for git metadata before synthesizing archival data.
This guard only looks at
build_dir/.git. Packages with a nestedbuild_dirkeep git metadata atsdist_root_dir, so this path will still write a synthetic.git_archival.txtfor a real checkout.Proposed fix
- if not build_dir.joinpath(".git").exists(): + has_git_metadata = ( + build_dir.joinpath(".git").exists() + or sdist_root_dir.joinpath(".git").exists() + ) + if not has_git_metadata: ensure_git_archival( version=version, target_dir=build_dir, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fromager/sources.py` around lines 693 - 697, The current guard only checks build_dir/.git before calling ensure_git_archival, which can synthesize .git_archival.txt for projects where the real git metadata lives at sdist_root_dir; update the condition to check for git metadata in the source root as well (e.g., verify sdist_root_dir.joinpath(".git").exists() or similar) and only call ensure_git_archival(version=version, target_dir=build_dir) when neither build_dir nor sdist_root_dir contains a .git directory; reference the build_dir and sdist_root_dir variables and the ensure_git_archival function to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fromager/sources.py`:
- Around line 818-823: The code returns True for any existing .git_archival.txt
that lacks the unprocessed marker, but doesn't validate the archival payload, so
empty/truncated files can be treated as valid; update the check in the block
that reads archival_file to validate the content contains the required
setuptools-scm fields (e.g., "node" and/or "describe-name" entries or a
non-empty node value) before returning True: if those keys/values are missing,
log a warning and proceed as if the file is unprocessed (so replacement
happens). Refer to archival_file, _UNPROCESSED_ARCHIVAL_MARKER and target_dir
when locating and updating the logic.
---
Duplicate comments:
In `@src/fromager/sources.py`:
- Around line 693-697: The current guard only checks build_dir/.git before
calling ensure_git_archival, which can synthesize .git_archival.txt for projects
where the real git metadata lives at sdist_root_dir; update the condition to
check for git metadata in the source root as well (e.g., verify
sdist_root_dir.joinpath(".git").exists() or similar) and only call
ensure_git_archival(version=version, target_dir=build_dir) when neither
build_dir nor sdist_root_dir contains a .git directory; reference the build_dir
and sdist_root_dir variables and the ensure_git_archival function to locate the
change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4b2969c1-cb4c-4d05-a87a-8882f2a5bd6e
📒 Files selected for processing (2)
src/fromager/sources.pytests/test_sources.py
LalatenduMohanty
force-pushed
the
961_git-archival-for-setuptools-scm
branch
from
23827f7 to
efe230a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fromager/sources.py`:
- Around line 823-831: The current check builds a set "fields" from keys only,
so entries like "node:" or "describe-name:" with empty values still pass; change
the parsing to build a dict (e.g., parse "content" into something like parsed =
{key.strip(): value.strip() for line in content.splitlines() if ":" in line for
key, _, value in [line.partition(':')]}) and then replace the membership check
with a validation that _REQUIRED_ARCHIVAL_FIELDS is a subset of parsed.keys()
and that all parsed[field] are non-empty (truthy) for each required field; keep
the existing check for _UNPROCESSED_ARCHIVAL_MARKER in content and only treat
the file as archival when the marker is absent and all required fields have
non-empty values.
🪄 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: c784ffc9-48c4-48a0-af58-90fe2889bb07
📒 Files selected for processing (2)
src/fromager/sources.pytests/test_sources.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_sources.py
LalatenduMohanty
force-pushed
the
961_git-archival-for-setuptools-scm
branch
2 times, most recently
from
96ffb9a to
a300456
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_sources.py (1)
320-331: Add explicit assertions fornode-datein generated/repaired archival files.Current tests won’t catch regressions where
node-dateis missing from synthesized output. Add assertions in creation/replacement paths to verify the full archival contract.Suggested test additions
content = archival.read_text() assert "describe-name: 1.2.3\n" in content assert "node: " in content + assert "node-date: " in content assert "$Format:" not in content @@ content = archival.read_text() assert "describe-name: 4.5.6\n" in content + assert "node-date: " in content assert "$Format:" not in content @@ content = archival.read_text() assert "describe-name: 3.0.0\n" in content + assert "node-date: " in content @@ content = archival.read_text() assert "describe-name: 5.0.0\n" in content + assert "node-date: " in contentAs per coding guidelines, "tests/**: Verify test actually tests the intended behavior. Check for missing edge cases."
Also applies to: 344-347, 371-374, 382-384
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_sources.py` around lines 320 - 331, Update the tests that validate generated/rewritten .git_archival.txt (e.g., test_creates_file_when_missing and the similar tests around lines 344-347, 371-374, 382-384) to assert that the "node-date: " line is present and correctly populated in the file content; locate the archival file read (variable archival or calls to sources.ensure_git_archival) and add an assertion like `assert "node-date: " in content` (and where applicable assert the date format or non-empty value) so missing node-date regressions are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fromager/sources.py`:
- Around line 793-799: Add "node-date" to the required archival fields set and
include a corresponding node-date line in the synthesized archival template;
update the symbols _REQUIRED_ARCHIVAL_FIELDS and _GIT_ARCHIVAL_CONTENT (and the
duplicate occurrence later in the file) so the archival content emits
"node-date: {node_date}" and the required-fields set contains "node-date" to
match the new .git_archival.txt contract.
---
Nitpick comments:
In `@tests/test_sources.py`:
- Around line 320-331: Update the tests that validate generated/rewritten
.git_archival.txt (e.g., test_creates_file_when_missing and the similar tests
around lines 344-347, 371-374, 382-384) to assert that the "node-date: " line is
present and correctly populated in the file content; locate the archival file
read (variable archival or calls to sources.ensure_git_archival) and add an
assertion like `assert "node-date: " in content` (and where applicable assert
the date format or non-empty value) so missing node-date regressions are caught.
🪄 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: 0eff024c-b129-4cc8-9b79-1a7ab4c18088
📒 Files selected for processing (2)
src/fromager/sources.pytests/test_sources.py
LalatenduMohanty
force-pushed
the
961_git-archival-for-setuptools-scm
branch
from
a300456 to
173746e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fromager/sources.py`:
- Around line 798-828: ensure_git_archival currently returns None when
.git_archival.txt is missing; instead, create a synthetic archival file using
the provided version and return False to indicate a new file was written. In
ensure_git_archival(), when archival_file.is_file() is False, write a minimal
setuptools-scm archival payload to archival_file (for example include a
"describe-name: {version}" line using the Version value passed in) with
appropriate file permissions, flush/close it, and return False; keep the
existing behavior for cases where the file exists and only replace if
placeholders are present (preserve the existing logic in ensure_git_archival
that handles existing files).
🪄 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: e1a1de89-1ee1-471e-8169-4f4299268a93
📒 Files selected for processing (2)
src/fromager/sources.pytests/test_sources.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_sources.py
LalatenduMohanty
force-pushed
the
961_git-archival-for-setuptools-scm
branch
from
173746e to
16a06e4
Compare
|
@tiran Looks like my approach had a drawback and it was failing for the packages who needs a custom tag_regex, so we need to rethink about how we want to achieve this. I added a comment #961 (comment) for direction we should take. PTAL |
LalatenduMohanty
force-pushed
the
961_git-archival-for-setuptools-scm
branch
2 times, most recently
from
2bdd4c1 to
02e3d25
Compare
LalatenduMohanty
force-pushed
the
961_git-archival-for-setuptools-scm
branch
from
02e3d25 to
16e6406
Compare
LalatenduMohanty
force-pushed
the
961_git-archival-for-setuptools-scm
branch
from
16e6406 to
6254b53
Compare
|
Traced through setuptools-scm's source to check the setuptools-scm uses a The current code only creates
That last case is narrow — most sources are either PyPI sdists or git clones. We also can't easily fix it since we'd need to know the project's tag prefix to synthesize a matching |
When building from an sdist that lacks .git metadata, setuptools-scm
cannot determine the package version. This adds a .git_archival.txt
file with the correct version so setuptools-scm can resolve it.
- Skips packages with .git directories (no fix needed)
- Replaces existing files that have unprocessed placeholders or
missing fields
- Creates a new file only when PKG-INFO is also absent (git clones
or custom downloads, not PyPI sdists)
Closes: python-wheel-build#961
Co-Authored-By: Claude <claude@anthropic.com>
Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>
LalatenduMohanty
force-pushed
the
961_git-archival-for-setuptools-scm
branch
from
6254b53 to
4920bd6
Compare
3 participants
When building from an sdist that lacks .git metadata, setuptools-scm
cannot determine the package version. This adds a .git_archival.txt
file with the correct version so setuptools-scm can resolve it.
missing fields
or custom downloads, not PyPI sdists)
Closes: #961