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
…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>
📝 WalkthroughWalkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
| dep_name = canonicalize_name(dep.name) | ||
| if dep_name in resolved_build_sys: | ||
| version, download_url = resolved_build_sys[dep_name] | ||
| if version in dep.specifier: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file structure and read the relevant code section
wc -l src/fromager/bootstrapper.pyRepository: python-wheel-build/fromager
Length of output: 105
🏁 Script executed:
# Read the code around lines 1631-1634
sed -n '1620,1650p' src/fromager/bootstrapper.pyRepository: python-wheel-build/fromager
Length of output: 1450
🏁 Script executed:
# Read the entire _filter_deps_satisfied_by_build_system method
sed -n '1610,1670p' src/fromager/bootstrapper.pyRepository: python-wheel-build/fromager
Length of output: 2668
🏁 Script executed:
# Check how resolved_build_sys is constructed and used
rg "resolved_build_sys" src/fromager/bootstrapper.py -B 2 -A 2Repository: python-wheel-build/fromager
Length of output: 1961
🏁 Script executed:
# Look for extras handling in the codebase, particularly around lines 1237-1240
sed -n '1230,1250p' src/fromager/bootstrapper.pyRepository: python-wheel-build/fromager
Length of output: 690
🏁 Script executed:
# Check Requirement class and how extras are used
rg "class Requirement|\.extras" src/fromager/bootstrapper.py -B 1 -A 1 | head -50Repository: python-wheel-build/fromager
Length of output: 177
🏁 Script executed:
# Verify imports and exception handling patterns in the file
head -100 src/fromager/bootstrapper.pyRepository: python-wheel-build/fromager
Length of output: 2752
🏁 Script executed:
# Examine how resolved_build_sys is constructed
sed -n '1500,1580p' src/fromager/bootstrapper.pyRepository: python-wheel-build/fromager
Length of output: 3066
🏁 Script executed:
# Search for _resolve_build_system_versions_by_name to see how it builds the dict
rg "_resolve_build_system_versions_by_name|_get_resolved_build_system_versions" src/fromager/bootstrapper.py -A 20 | head -80Repository: python-wheel-build/fromager
Length of output: 3185
🏁 Script executed:
# Check if extras are ever considered in the filtering logic
rg "dep.extras|\.extras" src/fromager/bootstrapper.py -B 3 -A 3Repository: python-wheel-build/fromager
Length of output: 239
🏁 Script executed:
# Verify how Requirements with extras are handled elsewhere in the build-system path
rg "build.system|BuildSystem" src/fromager/bootstrapper.py -i | head -20Repository: python-wheel-build/fromager
Length of output: 1318
🏁 Script executed:
# Check the Requirement type to understand extras field
rg "from packaging" src/fromager/bootstrapper.pyRepository: python-wheel-build/fromager
Length of output: 218
🏁 Script executed:
# Check if build-system dependencies can have extras
rg "get_build_system_dependencies" src/fromager/ -A 10 | head -40Repository: python-wheel-build/fromager
Length of output: 2214
🏁 Script executed:
# Look for any extras tracking in the graph or elsewhere
rg "extras" src/fromager/bootstrapper.py -B 2 -A 2Repository: python-wheel-build/fromager
Length of output: 679
🏁 Script executed:
# Check test files to see if extras scenarios are tested
fd "test.*\.py" src/fromager/ -o tests/ | xargs rg "extras.*build.system|build.system.*extras" 2>/dev/null | head -20Repository: python-wheel-build/fromager
Length of output: 203
🏁 Script executed:
# Verify what type Requirement is and whether it has extras attribute
rg "class Requirement|`@property`.*extras" --type py -B 1 -A 2 | grep -A 5 "class Requirement\|extras"Repository: python-wheel-build/fromager
Length of output: 468
🏁 Script executed:
# Check if there are existing tests for extras handling in build dependencies
find . -name "*.py" -type f | xargs grep -l "extras.*build" | head -10Repository: python-wheel-build/fromager
Length of output: 105
🏁 Script executed:
# Look at the dependencies module to see if extras are handled there
sed -n '1,100p' src/fromager/dependencies.pyRepository: python-wheel-build/fromager
Length of output: 2963
🏁 Script executed:
# Search for any conditional logic based on extras in bootstrapper
rg "if.*extras|extras.*if" src/fromager/bootstrapper.pyRepository: python-wheel-build/fromager
Length of output: 53
🏁 Script executed:
# Check how packaging.Requirement extras work
python3 << 'PY'
from packaging.requirements import Requirement
# Test cases
req1 = Requirement("foo>=1")
req2 = Requirement("foo[bar]>=1")
req3 = Requirement("foo[bar,baz]>=1")
print(f"req1: name={req1.name}, extras={req1.extras}")
print(f"req2: name={req2.name}, extras={req2.extras}")
print(f"req3: name={req3.name}, extras={req3.extras}")
print(f"\nAre req1 and req2 equivalent? {req1 == req2}")
PYRepository: python-wheel-build/fromager
Length of output: 206
🏁 Script executed:
# Check if build-system deps can even have extras
grep -r "build.system.*extras\|extras.*build.system" src/fromager/ --include="*.py" | head -20Repository: python-wheel-build/fromager
Length of output: 53
Requirement extras are ignored in build-system reuse filtering.
When a build-backend or build-sdist dependency includes extras (e.g., foo[bar]>=1), the current filter treats it as satisfied by a build-system foo==1, which lacks those extras. The extras may pull in additional dependencies that won't be installed, causing incomplete builds.
Keep deps with extras unresolved until the reuse map tracks them:
Proposed fix
if dep_name in resolved_build_sys:
version, download_url = resolved_build_sys[dep_name]
+ if dep.extras:
+ logger.info(
+ "%s dependency %s requests extras not tracked by "
+ "the build-system reuse map; resolving independently",
+ dep_req_type,
+ dep,
+ )
+ unsatisfied.add(dep)
+ continue
if version in dep.specifier:🤖 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/bootstrapper.py` around lines 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.
| unresolved_backend = self._filter_deps_satisfied_by_build_system( | ||
| item.build_backend_deps, | ||
| resolved_build_sys, | ||
| RequirementType.BUILD_BACKEND, | ||
| parent, | ||
| ) | ||
| unresolved_sdist = self._filter_deps_satisfied_by_build_system( | ||
| item.build_sdist_deps, | ||
| resolved_build_sys, | ||
| RequirementType.BUILD_SDIST, | ||
| parent, | ||
| ) | ||
|
|
||
| backend_items = self._create_unresolved_work_items( | ||
| unresolved_backend, | ||
| RequirementType.BUILD_BACKEND, | ||
| item.req, | ||
| item.resolved_version, | ||
| ) | ||
| sdist_items = self._create_unresolved_work_items( | ||
| item.build_sdist_deps, | ||
| unresolved_sdist, |
There was a problem hiding this comment.
Persist the filtered deps before BUILD.
unresolved_backend/unresolved_sdist only affect work-item creation. _phase_build() still installs item.build_backend_deps | item.build_sdist_deps, which remain unfiltered, so an unpinned backend/sdist dep can still be installed independently and drift from the graph.
🐛 Proposed fix
- unresolved_backend = self._filter_deps_satisfied_by_build_system(
+ item.build_backend_deps = self._filter_deps_satisfied_by_build_system(
item.build_backend_deps,
resolved_build_sys,
RequirementType.BUILD_BACKEND,
parent,
)
- unresolved_sdist = self._filter_deps_satisfied_by_build_system(
+ item.build_sdist_deps = self._filter_deps_satisfied_by_build_system(
item.build_sdist_deps,
resolved_build_sys,
RequirementType.BUILD_SDIST,
parent,
)
backend_items = self._create_unresolved_work_items(
- unresolved_backend,
+ item.build_backend_deps,
RequirementType.BUILD_BACKEND,
item.req,
item.resolved_version,
)
sdist_items = self._create_unresolved_work_items(
- unresolved_sdist,
+ item.build_sdist_deps,
RequirementType.BUILD_SDIST,
item.req,
item.resolved_version,
)🤖 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/bootstrapper.py` around lines 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.
| calls = mock_create.call_args_list | ||
| assert calls[0][0][0] == set() | ||
| assert calls[1][0][0] == set() | ||
| assert len(result) == 1 |
There was a problem hiding this comment.
Verify the returned item is the input item.
The test checks len(result) == 1 but doesn't verify what that item is. According to _phase_prepare_build's contract, it should return [item] when no dependencies need resolution.
Suggested fix
assert item.phase == BootstrapPhase.BUILD
calls = mock_create.call_args_list
assert calls[0][0][0] == set()
assert calls[1][0][0] == set()
-assert len(result) == 1
+assert result == [item]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert len(result) == 1 | |
| assert result == [item] |
🤖 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` at 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.
| "%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
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