From 4d82de95d097266bfc8357b976803ba4925c81cf Mon Sep 17 00:00:00 2001 From: Doug Hellmann Date: Mon, 22 Jun 2026 13:59:10 -0400 Subject: [PATCH] refactor(bootstrapper): simplify top-level requirement interface Add public `bootstrap(requirements: list[Requirement])` that combines the resolution loop (previously `resolve_and_add_top_level()`) and the DFS processing loop into a single method. Callers no longer need to manage `requirement_ctxvar` context or track which requirements resolved successfully. Rename the old `bootstrap(req, req_type)` to `_bootstrap_one()` (private) since it is only used by the git URL resolution path in `_handle_build_requirements()`. Rename `resolve_and_add_top_level()` to `_resolve_and_add_top_level()` (private) as it is now only called inside `bootstrap()`. `bootstrap()` and `_bootstrap_one()` shared an identical DFS while-loop. Extract it into `_run_bootstrap_loop(stack)` so there is a single implementation to maintain. Remove the per-requirement failed-version reporting block from `_bootstrap_one()` since `finalize()` already produces a complete summary. Update `commands/bootstrap.py` to replace the two-loop pattern with a single `bt.bootstrap(to_build)` call, and remove the now-unused `requirement_ctxvar` import. Update all affected tests to use the renamed private methods or the new public interface. Closes: #1211 Co-Authored-By: Claude Signed-off-by: Doug Hellmann --- src/fromager/bootstrapper.py | 128 ++++++++++++++++--------- src/fromager/commands/bootstrap.py | 28 +----- tests/test_bootstrap.py | 4 - tests/test_bootstrap_test_mode.py | 5 +- tests/test_bootstrapper.py | 138 ++++++++++++++++++++------- tests/test_bootstrapper_iterative.py | 16 ++-- 6 files changed, 199 insertions(+), 120 deletions(-) diff --git a/src/fromager/bootstrapper.py b/src/fromager/bootstrapper.py index 1b334f07b..c2de0c6d9 100644 --- a/src/fromager/bootstrapper.py +++ b/src/fromager/bootstrapper.py @@ -34,7 +34,7 @@ wheels, ) from .dependency_graph import DependencyGraph -from .log import req_ctxvar_context +from .log import req_ctxvar_context, requirement_ctxvar from .requirements_file import RequirementType, SourceType if typing.TYPE_CHECKING: @@ -199,12 +199,14 @@ def __init__( # Track failed versions in multiple_versions mode self._failed_versions: dict[tuple[str, str], Exception] = {} - def resolve_and_add_top_level( + def _resolve_and_add_top_level( self, req: Requirement, ) -> tuple[str, Version] | None: """Resolve a top-level requirement and add it to the dependency graph. + Private method called only by ``bootstrap()``. + This is the pre-resolution phase before recursive bootstrapping begins. In test mode, catches resolution errors and records them as failures. @@ -324,6 +326,76 @@ def resolve_versions( return_all_versions=return_all_versions, ) + def bootstrap(self, requirements: list[Requirement]) -> None: + """Bootstrap all top-level requirements and their transitive dependencies. + + .. versionadded:: 0.89 + Replaces the former ``bootstrap(req, req_type)`` signature. + + Resolves each requirement, adds it to the dependency graph, and processes + the full dependency tree using an iterative DFS loop. Handles + ``requirement_ctxvar`` context internally; callers do not need to manage it. + + In test mode, records failures and continues instead of raising. In + ``multiple_versions`` mode, processes all matching versions per requirement. + + Args: + requirements: Top-level requirements to resolve and bootstrap. + """ + # Resolve all top-level reqs and build initial stack. + # Use the token pattern (no try/finally) so that if resolution raises + # in normal mode, the context var stays set for the top-level error + # handler in __main__.py to include the package name in its log message. + stack: list[WorkItem] = [] + for req in requirements: + token = requirement_ctxvar.set(req) + result = self._resolve_and_add_top_level(req) + requirement_ctxvar.reset(token) + if result is not None: + stack.append( + WorkItem( + req=req, + req_type=RequirementType.TOP_LEVEL, + phase=BootstrapPhase.RESOLVE, + why_snapshot=[], + parent=None, + ) + ) + + self._run_bootstrap_loop(stack) + + def _run_bootstrap_loop(self, stack: list[WorkItem]) -> None: + """Run the iterative DFS bootstrap loop over a pre-built work stack. + + Pops items one at a time, dispatches each phase, and pushes any + follow-on items (continuations and new dependencies) back onto the + stack. Updates the progress bar as items complete. + + Args: + stack: Initial list of ``WorkItem`` objects to process. Modified + in-place; empty on return. + """ + while stack: + self._record_stack_state(stack) + item = stack.pop() + self.why = list(item.why_snapshot) + + with req_ctxvar_context(item.req), self._track_why(item): + try: + new_items = self._dispatch_phase(item) + except Exception as err: + new_items = self._handle_phase_error(item, err) + + new_dep_count = sum( + 1 for it in new_items if it.phase == BootstrapPhase.RESOLVE + ) + if new_dep_count > 0: + self.progressbar.update_total(new_dep_count) + if not new_items: + self.progressbar.update() + + stack.extend(new_items) + def _processing_build_requirement(self, current_req_type: RequirementType) -> bool: """Are we currently processing a build requirement? @@ -350,8 +422,12 @@ def _processing_build_requirement(self, current_req_type: RequirementType) -> bo logger.debug("is not a build requirement") return False - def bootstrap(self, req: Requirement, req_type: RequirementType) -> None: - """Bootstrap a package and its dependencies using an iterative loop. + def _bootstrap_one(self, req: Requirement, req_type: RequirementType) -> None: + """Bootstrap a single requirement using an iterative DFS loop. + + Internal method used only by the git URL resolution path + (``_handle_build_requirements``). All other callers should use + ``bootstrap(requirements)`` instead. Uses an explicit LIFO stack instead of recursion to handle arbitrarily deep dependency graphs without hitting Python's recursion limit. @@ -386,49 +462,11 @@ def bootstrap(self, req: Requirement, req_type: RequirementType) -> None: ) ] - # Main iterative DFS loop - while stack: - self._record_stack_state(stack) - item = stack.pop() - self.why = list(item.why_snapshot) - - with req_ctxvar_context(item.req), self._track_why(item): - try: - new_items = self._dispatch_phase(item) - except Exception as err: - new_items = self._handle_phase_error(item, err) - - # Progress bar: count new RESOLVE-phase items as new dependencies - new_dep_count = sum( - 1 for it in new_items if it.phase == BootstrapPhase.RESOLVE - ) - if new_dep_count > 0: - self.progressbar.update_total(new_dep_count) - if not new_items: - self.progressbar.update() - - # Phase handlers return [continuation, *new_deps] so extend() - # naturally puts new deps on top of the stack (processed first). - stack.extend(new_items) + self._run_bootstrap_loop(stack) # Restore why stack for the caller self.why = saved_why - # In multiple versions mode, report any failures for this requirement - if self.multiple_versions and self._failed_versions: - req_name = canonicalize_name(req.name) - failed_for_req = { - (name, ver): exc - for (name, ver), exc in self._failed_versions.items() - if name == req_name - } - if failed_for_req: - logger.warning( - f"{req.name}: {len(failed_for_req)} version(s) failed to bootstrap" - ) - for (name, ver), exc in failed_for_req.items(): - logger.warning(f" - {name}=={ver}: {type(exc).__name__}: {exc}") - @contextlib.contextmanager def _track_why( self, @@ -626,7 +664,7 @@ def _handle_build_requirements( # Save/restore self.why because the iterative bootstrap() # modifies it internally for each work item. saved_why = list(self.why) - self.bootstrap(req=dep, req_type=build_type) + self._bootstrap_one(req=dep, req_type=build_type) self.why = saved_why self.progressbar.update() @@ -1437,7 +1475,7 @@ def _phase_start(self, item: WorkItem) -> list[WorkItem]: assert item.resolved_version is not None assert item.source_url is not None - # Add to graph (skip TOP_LEVEL, already added in resolve_and_add_top_level) + # Add to graph (skip TOP_LEVEL, already added in _resolve_and_add_top_level) if item.req_type != RequirementType.TOP_LEVEL: self._add_to_graph( item.req, diff --git a/src/fromager/commands/bootstrap.py b/src/fromager/commands/bootstrap.py index 9baeb36ce..08a5bea8a 100644 --- a/src/fromager/commands/bootstrap.py +++ b/src/fromager/commands/bootstrap.py @@ -20,7 +20,6 @@ resolver, server, ) -from ..log import requirement_ctxvar from .build import build_parallel from .graph import find_why, show_explain_duplicates @@ -200,28 +199,11 @@ def bootstrap( multiple_versions=multiple_versions, ) - # Pre-resolution phase: Resolve all top-level dependencies before recursive - # bootstrapping begins. Test-mode error handling is in Bootstrapper. - # Note: We don't use try/finally here because: - # - In test-mode: exceptions are caught inside resolve_and_add_top_level() - # - In normal mode: exceptions should propagate with context preserved for logging - logger.info("resolving top-level dependencies before building") - resolved_reqs: list[Requirement] = [] - for req in to_build: - token = requirement_ctxvar.set(req) - result = bt.resolve_and_add_top_level(req) - if result is not None: - resolved_reqs.append(req) - # If result is None, test_mode or multiple_versions recorded the failure - requirement_ctxvar.reset(token) - - # Bootstrap only packages that were successfully resolved - # Note: Same pattern - no try/finally to preserve context for error logging - for req in resolved_reqs: - token = requirement_ctxvar.set(req) - bt.bootstrap(req, requirements_file.RequirementType.TOP_LEVEL) - progressbar.update() - requirement_ctxvar.reset(token) + # Resolve and bootstrap all top-level dependencies and their transitive + # dependencies. Context management and error handling are handled internally + # by Bootstrapper.bootstrap(). + logger.info("resolving and bootstrapping top-level dependencies") + bt.bootstrap(list(to_build)) # Finalize test mode and check for failures exit_code = bt.finalize() diff --git a/tests/test_bootstrap.py b/tests/test_bootstrap.py index 944926492..94b98c0e8 100644 --- a/tests/test_bootstrap.py +++ b/tests/test_bootstrap.py @@ -560,7 +560,6 @@ def test_multiple_versions_auto_disables_constraints( mock_progress.return_value.__enter__.return_value = Mock() mock_progress.return_value.__exit__.return_value = None mock_bt_instance = Mock() - mock_bt_instance.resolve_and_add_top_level.return_value = ("url", Version("1.0")) mock_bt_instance.finalize.return_value = 0 mock_bootstrapper.return_value = mock_bt_instance @@ -608,7 +607,6 @@ def test_multiple_versions_with_skip_constraints_no_duplicate_log( mock_progress.return_value.__enter__.return_value = Mock() mock_progress.return_value.__exit__.return_value = None mock_bt_instance = Mock() - mock_bt_instance.resolve_and_add_top_level.return_value = ("url", Version("1.0")) mock_bt_instance.finalize.return_value = 0 mock_bootstrapper.return_value = mock_bt_instance @@ -657,7 +655,6 @@ def test_without_multiple_versions_constraints_not_disabled( mock_progress.return_value.__enter__.return_value = Mock() mock_progress.return_value.__exit__.return_value = None mock_bt_instance = Mock() - mock_bt_instance.resolve_and_add_top_level.return_value = ("url", Version("1.0")) mock_bt_instance.finalize.return_value = 0 mock_bootstrapper.return_value = mock_bt_instance mock_write_constraints.return_value = True @@ -720,7 +717,6 @@ def test_max_release_age_sets_context( mock_progress.return_value.__enter__.return_value = Mock() mock_progress.return_value.__exit__.return_value = None mock_bt_instance = Mock() - mock_bt_instance.resolve_and_add_top_level.return_value = ("url", Version("1.0")) mock_bt_instance.finalize.return_value = 0 mock_bootstrapper.return_value = mock_bt_instance diff --git a/tests/test_bootstrap_test_mode.py b/tests/test_bootstrap_test_mode.py index 150261f56..607a5a204 100644 --- a/tests/test_bootstrap_test_mode.py +++ b/tests/test_bootstrap_test_mode.py @@ -16,7 +16,6 @@ from packaging.requirements import Requirement from fromager import bootstrapper, context -from fromager.requirements_file import RequirementType class TestBootstrapperInitialization: @@ -267,7 +266,7 @@ def test_resolution_failure_recorded_in_test_mode( side_effect=RuntimeError("Version resolution failed"), ): # Should not raise in test mode - bt.bootstrap(req=req, req_type=RequirementType.TOP_LEVEL) + bt.bootstrap([req]) # Verify failure was recorded assert len(bt.failed_packages) == 1 @@ -293,4 +292,4 @@ def test_resolution_failure_raises_in_normal_mode( side_effect=RuntimeError("Version resolution failed"), ): with pytest.raises(RuntimeError, match="Version resolution failed"): - bt.bootstrap(req=req, req_type=RequirementType.TOP_LEVEL) + bt.bootstrap([req]) diff --git a/tests/test_bootstrapper.py b/tests/test_bootstrapper.py index ee00f658f..7de6ce38d 100644 --- a/tests/test_bootstrapper.py +++ b/tests/test_bootstrapper.py @@ -337,44 +337,36 @@ def mock_dispatch(item: bootstrapper.WorkItem) -> list[bootstrapper.WorkItem]: raise ValueError("Simulated failure for version 1.5") return [] + req = Requirement("testpkg>=1.0") + with patch.object(bt, "_dispatch_phase", side_effect=mock_dispatch): with patch.object(bt, "_has_been_seen", return_value=False): - with patch("fromager.bootstrapper.logger") as mock_logger: - req = Requirement("testpkg>=1.0") - - bt.bootstrap( - req=req, - req_type=RequirementType.INSTALL, - ) - - # All 3 versions should reach build phases - assert build_phase_count["count"] == 3 - - # Verify that version 1.5 is in failed_versions - assert len(bt._failed_versions) == 1 - key = (canonicalize_name("testpkg"), "1.5") - assert key in bt._failed_versions - exc = bt._failed_versions[key] - assert isinstance(exc, ValueError) - assert str(exc) == "Simulated failure for version 1.5" - - # Verify that a warning was logged for the failed version - warning_calls = [ - call - for call in mock_logger.warning.call_args_list - if "failed to bootstrap" in str(call) - ] - assert len(warning_calls) >= 1 - - # Verify that failed version 1.5 is NOT in the dependency graph - failed_key = f"{canonicalize_name('testpkg')}==1.5" - assert failed_key not in tmp_context.dependency_graph.nodes - - # Verify that successful versions ARE in the dependency graph - success_key_20 = f"{canonicalize_name('testpkg')}==2.0" - success_key_10 = f"{canonicalize_name('testpkg')}==1.0" - assert success_key_20 in tmp_context.dependency_graph.nodes - assert success_key_10 in tmp_context.dependency_graph.nodes + bt._bootstrap_one( + req=req, + req_type=RequirementType.INSTALL, + ) + + # All 3 versions should reach build phases + assert build_phase_count["count"] == 3 + + # Verify that version 1.5 is in failed_versions + assert len(bt._failed_versions) == 1 + pkg_name = canonicalize_name("testpkg") + version_str = "1.5" + assert (pkg_name, version_str) in bt._failed_versions + exc = bt._failed_versions[(pkg_name, version_str)] + assert isinstance(exc, ValueError) + assert str(exc) == "Simulated failure for version 1.5" + + # Verify that failed version 1.5 is NOT in the dependency graph + failed_key = f"{canonicalize_name('testpkg')}==1.5" + assert failed_key not in tmp_context.dependency_graph.nodes + + # Verify that successful versions ARE in the dependency graph + success_key_20 = f"{canonicalize_name('testpkg')}==2.0" + success_key_10 = f"{canonicalize_name('testpkg')}==1.0" + assert success_key_20 in tmp_context.dependency_graph.nodes + assert success_key_10 in tmp_context.dependency_graph.nodes @patch("fromager.resolver.find_all_matching_from_provider") @@ -718,6 +710,78 @@ def counting_record(stack: list[bootstrapper.WorkItem]) -> None: ), patch.object(bt, "_phase_start", return_value=[]), ): - bt.bootstrap(req=req, req_type=RequirementType.TOP_LEVEL) + bt._bootstrap_one(req=req, req_type=RequirementType.TOP_LEVEL) assert call_count["n"] >= 1 + + +def test_bootstrap_with_empty_list(tmp_context: WorkContext) -> None: + """bootstrap([]) completes without error and runs no phases.""" + bt = bootstrapper.Bootstrapper(tmp_context) + with patch.object(bt, "_dispatch_phase") as mock_dispatch: + bt.bootstrap([]) + mock_dispatch.assert_not_called() + + +def test_bootstrap_with_single_requirement(tmp_context: WorkContext) -> None: + """bootstrap([req]) resolves and processes the requirement.""" + bt = bootstrapper.Bootstrapper(tmp_context) + req = Requirement("testpkg==1.0") + + with ( + patch.object( + bt, + "_resolve_and_add_top_level", + return_value=("http://example.test/testpkg-1.0.tar.gz", Version("1.0")), + ), + patch.object(bt, "_dispatch_phase", return_value=[]) as mock_dispatch, + patch.object(bt, "_record_stack_state"), + ): + bt.bootstrap([req]) + + mock_dispatch.assert_called_once() + item = mock_dispatch.call_args[0][0] + assert item.req == req + assert item.req_type == RequirementType.TOP_LEVEL + assert item.phase == bootstrapper.BootstrapPhase.RESOLVE + + +def test_bootstrap_skips_failed_resolution(tmp_context: WorkContext) -> None: + """bootstrap() skips requirements whose resolution returns None.""" + bt = bootstrapper.Bootstrapper(tmp_context) + req = Requirement("badpkg") + + with ( + patch.object(bt, "_resolve_and_add_top_level", return_value=None), + patch.object(bt, "_dispatch_phase") as mock_dispatch, + patch.object(bt, "_record_stack_state"), + ): + bt.bootstrap([req]) + + mock_dispatch.assert_not_called() + + +def test_bootstrap_two_requirements_both_processed(tmp_context: WorkContext) -> None: + """bootstrap() processes all requirements in the list.""" + bt = bootstrapper.Bootstrapper(tmp_context) + req1 = Requirement("pkg1==1.0") + req2 = Requirement("pkg2==2.0") + + dispatch_calls: list = [] + + def fake_dispatch(item: bootstrapper.WorkItem) -> list: + dispatch_calls.append(item.req.name) + return [] + + with ( + patch.object( + bt, + "_resolve_and_add_top_level", + return_value=("http://example.test/pkg-1.0.tar.gz", Version("1.0")), + ), + patch.object(bt, "_dispatch_phase", side_effect=fake_dispatch), + patch.object(bt, "_record_stack_state"), + ): + bt.bootstrap([req1, req2]) + + assert sorted(dispatch_calls) == ["pkg1", "pkg2"] diff --git a/tests/test_bootstrapper_iterative.py b/tests/test_bootstrapper_iterative.py index 14de344eb..9c6a4f474 100644 --- a/tests/test_bootstrapper_iterative.py +++ b/tests/test_bootstrapper_iterative.py @@ -833,7 +833,7 @@ def tracking_dispatch(item: WorkItem) -> list[WorkItem]: return_value=[("https://pypi.org/pkg-1.0.tar.gz", Version("1.0"))], ), ): - bt.bootstrap(Requirement("pkg"), RequirementType.TOP_LEVEL) + bt._bootstrap_one(Requirement("pkg"), RequirementType.TOP_LEVEL) assert phases_visited == [ BootstrapPhase.RESOLVE, @@ -898,7 +898,7 @@ def tracking_dispatch(item: WorkItem) -> list[WorkItem]: return_value=[("https://pypi.org/pkg-1.0.tar.gz", Version("1.0"))], ), ): - bt.bootstrap(Requirement("parent"), RequirementType.TOP_LEVEL) + bt._bootstrap_one(Requirement("parent"), RequirementType.TOP_LEVEL) # child's RESOLVE and START must appear before parent's COMPLETE req_phase_pairs = [ @@ -945,7 +945,7 @@ def mock_dispatch(item: WorkItem) -> list[WorkItem]: ), patch.object(bt, "_has_been_seen", return_value=False), ): - bt.bootstrap(Requirement("pkg"), RequirementType.INSTALL) + bt._bootstrap_one(Requirement("pkg"), RequirementType.INSTALL) assert len(bt._failed_versions) == 1 assert (canonicalize_name("pkg"), "1.5") in bt._failed_versions @@ -981,9 +981,9 @@ def mock_dispatch(item: WorkItem) -> list[WorkItem]: ), patch.object(bt, "_has_been_seen", return_value=False), ): - bt.bootstrap(Requirement("good-pkg"), RequirementType.INSTALL) - bt.bootstrap(Requirement("bad-dep"), RequirementType.INSTALL) - bt.bootstrap(Requirement("another-good"), RequirementType.INSTALL) + bt._bootstrap_one(Requirement("good-pkg"), RequirementType.INSTALL) + bt._bootstrap_one(Requirement("bad-dep"), RequirementType.INSTALL) + bt._bootstrap_one(Requirement("another-good"), RequirementType.INSTALL) assert "good-pkg" in completed assert "another-good" in completed @@ -1014,9 +1014,9 @@ def mock_dispatch(item: WorkItem) -> list[WorkItem]: ), ): # Bootstrap a package that will fail - bt.bootstrap(Requirement("fail-pkg"), RequirementType.TOP_LEVEL) + bt._bootstrap_one(Requirement("fail-pkg"), RequirementType.TOP_LEVEL) # Bootstrap another that will succeed - bt.bootstrap(Requirement("ok-pkg"), RequirementType.TOP_LEVEL) + bt._bootstrap_one(Requirement("ok-pkg"), RequirementType.TOP_LEVEL) assert len(bt.failed_packages) == 1 assert bt.failed_packages[0]["package"] == "fail-pkg"