Skip to content

Commit 2f0096e

Browse files
rd4398claude
andcommitted
fix(bootstrap): resolve all versions in multiple-versions mode and prevent duplicate edges
Address architect and engineer feedback on multiple-versions implementation: 1. **Process all versions in resolve_and_add_top_level()** - Pass return_all_versions=self.multiple_versions to resolve_versions() - Loop through all resolved versions and add each to the graph - Previously only processed results[0], losing other versions - Maintain backward compatibility by returning first version 2. **Prevent duplicate TOP_LEVEL edges in graph** - Skip _add_to_graph() call for TOP_LEVEL in _bootstrap_single_version() - TOP_LEVEL nodes are already added in resolve_and_add_top_level() - Calling _add_to_graph() again would create duplicate edges 3. **Handle empty resolution results** - Check if resolved_versions is empty after resolution - Raise RuntimeError to prevent silently skipping dependencies - Existing exception handler routes to test_mode or fail-fast 4. **Fix test: move import to top of file** - Move canonicalize_name import from local to top-level - Change test to use RequirementType.INSTALL instead of TOP_LEVEL - TOP_LEVEL graph addition now happens in resolve_and_add_top_level() All 447 unit tests pass. All 20 bootstrap/build e2e tests pass. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Rohan Devasthale <rdevasth@redhat.com>
1 parent 889df21 commit 2f0096e

2 files changed

Lines changed: 35 additions & 19 deletions

File tree

src/fromager/bootstrapper.py

Lines changed: 31 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,10 @@ def resolve_and_add_top_level(
141141
This is the pre-resolution phase before recursive bootstrapping begins.
142142
In test mode, catches resolution errors and records them as failures.
143143
144+
When multiple_versions is enabled, resolves and adds all matching versions
145+
to the graph, but still returns only the first (highest) version for
146+
backward compatibility.
147+
144148
Args:
145149
req: The top-level requirement to resolve.
146150
@@ -156,20 +160,27 @@ def resolve_and_add_top_level(
156160
results = self.resolve_versions(
157161
req=req,
158162
req_type=RequirementType.TOP_LEVEL,
163+
return_all_versions=self.multiple_versions,
159164
)
160-
source_url, version = results[0]
161-
logger.info("%s resolves to %s", req, version)
162-
self.ctx.dependency_graph.add_dependency(
163-
parent_name=None,
164-
parent_version=None,
165-
req_type=RequirementType.TOP_LEVEL,
166-
req=req,
167-
req_version=version,
168-
download_url=source_url,
169-
pre_built=pbi.pre_built,
170-
constraint=self.ctx.constraints.get_constraint(req.name),
171-
)
172-
return (source_url, version)
165+
if len(results) > 1:
166+
logger.info(f"resolved {len(results)} version(s) for {req}")
167+
168+
# Add all resolved versions to the graph
169+
for source_url, version in results:
170+
logger.info("%s resolves to %s", req, version)
171+
self.ctx.dependency_graph.add_dependency(
172+
parent_name=None,
173+
parent_version=None,
174+
req_type=RequirementType.TOP_LEVEL,
175+
req=req,
176+
req_version=version,
177+
download_url=source_url,
178+
pre_built=pbi.pre_built,
179+
constraint=self.ctx.constraints.get_constraint(req.name),
180+
)
181+
182+
# Return first result for backward compatibility
183+
return results[0]
173184
except Exception as err:
174185
if not self.test_mode:
175186
raise
@@ -286,6 +297,10 @@ def bootstrap(self, req: Requirement, req_type: RequirementType) -> None:
286297
self._record_test_mode_failure(req, None, err, "resolution")
287298
return
288299

300+
# Check if resolution returned no versions
301+
if not resolved_versions:
302+
raise RuntimeError(f"Could not resolve any versions for {req}")
303+
289304
# Bootstrap each resolved version
290305
for source_url, resolved_version in resolved_versions:
291306
self._bootstrap_single_version(req, req_type, source_url, resolved_version)
@@ -322,7 +337,9 @@ def _bootstrap_single_version(
322337
parent = (parent_req, parent_version)
323338

324339
# Update dependency graph unconditionally (before seen check to capture all edges)
325-
self._add_to_graph(req, req_type, resolved_version, source_url, parent)
340+
# Skip for TOP_LEVEL as they were already added in resolve_and_add_top_level()
341+
if req_type != RequirementType.TOP_LEVEL:
342+
self._add_to_graph(req, req_type, resolved_version, source_url, parent)
326343

327344
# Build sdist-only (no wheel) if flag is set, unless this is a build
328345
# requirement which always needs a full wheel.

tests/test_bootstrapper.py

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
from unittest.mock import Mock, patch
44

55
from packaging.requirements import Requirement
6+
from packaging.utils import canonicalize_name
67
from packaging.version import Version
78

89
from fromager import bootstrapper, requirements_file
@@ -297,9 +298,6 @@ def test_build_from_source_returns_dataclass(tmp_context: WorkContext) -> None:
297298

298299
def test_multiple_versions_continues_on_error(tmp_context: WorkContext) -> None:
299300
"""Test that multiple versions mode continues when one version fails."""
300-
301-
from packaging.utils import canonicalize_name
302-
303301
# Enable multiple versions mode
304302
bt = bootstrapper.Bootstrapper(tmp_context, multiple_versions=True)
305303

@@ -336,10 +334,11 @@ def mock_bootstrap_impl(
336334
with patch("fromager.bootstrapper.logger") as mock_logger:
337335
req = Requirement("testpkg>=1.0")
338336

339-
# Call bootstrap
337+
# Call bootstrap with INSTALL type (not TOP_LEVEL, since TOP_LEVEL
338+
# nodes are added in resolve_and_add_top_level())
340339
bt.bootstrap(
341340
req=req,
342-
req_type=RequirementType.TOP_LEVEL,
341+
req_type=RequirementType.INSTALL,
343342
)
344343

345344
# Verify _bootstrap_impl was called 3 times (all versions attempted)

0 commit comments

Comments
 (0)