fix(bootstrapper): reuse build-system dep versions for build-backend/sdist#1201
fix(bootstrapper): reuse build-system dep versions for build-backend/sdist#1201shifa-khan 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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
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: 3
🧹 Nitpick comments (1)
tests/test_bootstrapper_iterative.py (1)
1803-1823: ⚡ Quick winConsider testing warning presence without checking message content.
Per project learnings, asserting on log message text is discouraged as it's a brittle implementation detail. The functional behavior (dep passes through,
_add_to_graphnot called) is already verified intest_incompatible_specifier_passes_through.Consider refactoring to verify a WARNING was logged without checking exact content, or remove this test entirely.
Alternative approach
def test_incompatible_specifier_logs_warning( self, tmp_context: WorkContext, caplog: pytest.LogCaptureFixture ) -> None: """A conflicting dep logs a warning about the build config conflict.""" + import logging bt = bootstrapper.Bootstrapper(tmp_context) resolved_build_sys = { canonicalize_name("foo"): (Version("1.0"), "https://pypi.test/simple/") } parent = (Requirement("testpkg==1.0"), Version("1.0")) with patch.object(bt, "_add_to_graph"): bt._filter_deps_satisfied_by_build_system( {Requirement("foo>=2.0")}, resolved_build_sys, RequirementType.BUILD_BACKEND, parent, ) - assert "conflicts with" in caplog.text - assert "foo>=2.0" in caplog.text - assert "foo==1.0" in caplog.text + warnings = [r for r in caplog.records if r.levelno == logging.WARNING] + assert len(warnings) == 1Based on learnings: "avoid asserting on exact log output strings since they are brittle implementation details. Prefer verifying functional behavior instead."
🤖 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_bootstrapper_iterative.py` around lines 1803 - 1823, The test_incompatible_specifier_logs_warning method asserts on specific log message content strings like "conflicts with", "foo>=2.0", and "foo==1.0", which are brittle implementation details. Either refactor the assertions to verify that a WARNING level log was recorded without checking the exact text content (for example, by checking caplog.records and the log level), or remove this test entirely if the functional behavior of incompatible specifiers is already verified by test_incompatible_specifier_passes_through.Source: Learnings
🤖 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/bootstrapper.py`:
- Around line 1704-1724: The filtered dependencies unresolved_backend and
unresolved_sdist are only used for work-item creation, but the _phase_build()
method still installs the original unfiltered item.build_backend_deps and
item.build_sdist_deps, allowing unfiltered dependencies to be installed
independently and drift from the resolved dependency graph. After filtering
these dependencies in this section, persist the filtered results back to the
item object by updating item.build_backend_deps and item.build_sdist_deps with
the filtered versions so that _phase_build() will use the filtered dependencies
instead of the unfiltered originals.
- Around line 1631-1634: The build-system reuse filtering in the block starting
with the canonicalize_name assignment and the check for dep_name in
resolved_build_sys is not accounting for dependency extras. When a dependency
includes extras (e.g., foo[bar]>=1), the code incorrectly treats it as satisfied
by a resolved build-system dependency that lacks those extras. Add a check after
verifying dep_name is in resolved_build_sys to ensure the dependency does not
have extras specified; if dep.extras is non-empty, skip the reuse logic for that
dependency to keep it unresolved until the reuse map can properly track extras.
In `@tests/test_bootstrapper_iterative.py`:
- Line 1995: The test assertion on the line with `len(result) == 1` only
verifies the length of the result but not the content. According to the contract
of `_phase_prepare_build`, when no dependencies need resolution, it should
return the input item wrapped in a list. Add an additional assertion after the
length check to verify that `result[0]` equals the input item that was passed to
the function, ensuring the function returns the expected item.
---
Nitpick comments:
In `@tests/test_bootstrapper_iterative.py`:
- Around line 1803-1823: The test_incompatible_specifier_logs_warning method
asserts on specific log message content strings like "conflicts with",
"foo>=2.0", and "foo==1.0", which are brittle implementation details. Either
refactor the assertions to verify that a WARNING level log was recorded without
checking the exact text content (for example, by checking caplog.records and the
log level), or remove this test entirely if the functional behavior of
incompatible specifiers is already verified by
test_incompatible_specifier_passes_through.
🪄 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: 916f5e4b-0177-47ef-889a-5e96b57e5056
📒 Files selected for processing (2)
src/fromager/bootstrapper.pytests/test_bootstrapper_iterative.py
| "%s dependency %s conflicts with " | ||
| "build-system dependency %s==%s; " | ||
| "resolving independently", | ||
| dep_req_type, |
There was a problem hiding this comment.
Added a warning for when build-system and build-backend have incompatible specifiers for the same package. Seemed like this would be helpful
…sdist Build-backend and build-sdist deps that are already satisfied by a resolved build-system dep now reuse that version instead of resolving independently from PyPI. Previously, an unpinned build-backend dep like `hatch-cython` would resolve to the latest version (0.6.0) even when build-system already had `hatch-cython==0.5` pinned and resolved. This produced a wrong graph with two conflicting versions. Also adds a warning when build-backend and build-system have the same package but with incompatible version specifiers. Closes: python-wheel-build#1194 Co-Authored-By: Claude <claude@anthropic.com> Signed-off-by: shifa-khan <shikhan@redhat.com>
1 participant
Build-backend and build-sdist deps that are already satisfied by a resolved build-system dep now reuse that version instead of resolving independently from PyPI.
Previously, an unpinned build-backend dep like
hatch-cythonwould resolve to the latest version (0.6.0) even when build-system already hadhatch-cython==0.5pinned and resolved. This produced a wrong graph with two conflicting versions.Also adds a warning when build-backend and build-system have the same package but with incompatible version specifiers.
Closes: #1194