From 66634288f381367961c1730828248508338f645f Mon Sep 17 00:00:00 2001 From: Lalatendu Mohanty Date: Sat, 14 Mar 2026 10:50:57 -0400 Subject: [PATCH] refactor(bootstrapper): replace _handle_test_mode_failure with force_prebuilt flag Move test-mode prebuilt fallback from _build_from_source into bootstrap(). On build failure, bootstrap() now records the error and retries via _bootstrap_impl(force_prebuilt=True), which resolves and downloads a pre-built wheel internally. This removes _handle_test_mode_failure and the req_type parameter from _build_from_source. Closes: #892 Co-Authored-By: Claude Signed-off-by: Lalatendu Mohanty --- src/fromager/bootstrapper.py | 293 +++++++++++++---------------- src/fromager/commands/bootstrap.py | 8 +- tests/test_bootstrap_test_mode.py | 163 ++++++++++++++++ tests/test_bootstrapper.py | 3 +- 4 files changed, 304 insertions(+), 163 deletions(-) diff --git a/src/fromager/bootstrapper.py b/src/fromager/bootstrapper.py index 11440b5d..f970175e 100644 --- a/src/fromager/bootstrapper.py +++ b/src/fromager/bootstrapper.py @@ -294,13 +294,47 @@ def bootstrap(self, req: Requirement, req_type: RequirementType) -> None: self._bootstrap_impl( req, req_type, source_url, resolved_version, build_sdist_only ) - except Exception as err: + except Exception as build_error: if not self.test_mode: raise + + # Test mode: record the build failure unconditionally self._record_test_mode_failure( - req, str(resolved_version), err, "bootstrap" + req, str(resolved_version), build_error, "bootstrap" ) + # Attempt pre-built fallback + logger.warning( + "test mode: build failed for %s==%s, attempting pre-built fallback: %s", + req.name, + resolved_version, + build_error, + ) + try: + actual_version = self._bootstrap_impl( + req, + req_type, + source_url, + resolved_version, + build_sdist_only, + force_prebuilt=True, + ) + logger.info( + "test mode: successfully used pre-built wheel for %s==%s", + req.name, + actual_version, + ) + except Exception as fallback_error: + logger.error( + "test mode: pre-built fallback also failed for %s: %s", + req.name, + fallback_error, + exc_info=True, + ) + # Build failure already recorded above. Re-raise original + # error so callers know the package wasn't installed. + raise build_error from fallback_error + def _bootstrap_impl( self, req: Requirement, @@ -308,7 +342,8 @@ def _bootstrap_impl( source_url: str, resolved_version: Version, build_sdist_only: bool, - ) -> None: + force_prebuilt: bool = False, + ) -> Version: """Internal implementation - performs the actual bootstrap work. Called by bootstrap() after setup, validation, and seen-checking. @@ -316,13 +351,19 @@ def _bootstrap_impl( Args: req: The requirement to bootstrap. req_type: The type of requirement. - source_url: The resolved source URL. + source_url: The resolved source URL (sdist or wheel URL). resolved_version: The resolved version. build_sdist_only: Whether to build only sdist (no wheel). + force_prebuilt: If True, resolve a pre-built wheel URL and skip + source build. Used for test-mode fallback after build failure. + + Returns: + The version that was actually installed. This may differ from + resolved_version when force_prebuilt=True and the prebuilt + resolver returns a different version. Error Handling: - Fatal errors (source build, prebuilt download) raise exceptions - for bootstrap() to catch and record. + Fatal errors (source build, prebuilt download) raise exceptions. Non-fatal errors (post-hook, dependency extraction) are recorded locally and processing continues. These are recorded here because @@ -339,12 +380,35 @@ def _bootstrap_impl( cached_wheel_filename: pathlib.Path | None = None unpacked_cached_wheel: pathlib.Path | None = None - if pbi.pre_built: + if pbi.pre_built or force_prebuilt: + if force_prebuilt: + # Resolve prebuilt wheel URL (source_url is the sdist URL) + parent_req = self.why[-1][1] if self.why else None + wheel_url, fallback_version = self._resolver.resolve( + req=req, + req_type=req_type, + parent_req=parent_req, + pre_built=True, + ) + if fallback_version != resolved_version: + logger.warning( + "test mode: version mismatch for %s" + " - requested %s, fallback %s", + req.name, + resolved_version, + fallback_version, + ) + resolved_version = fallback_version + # Update source_url so _add_to_build_order records + # the wheel URL, not the original sdist URL. + source_url = wheel_url + else: + wheel_url = source_url wheel_filename, unpack_dir = self._download_prebuilt( req=req, req_type=req_type, resolved_version=resolved_version, - wheel_url=source_url, + wheel_url=wheel_url, ) build_result = SourceBuildResult( wheel_filename=wheel_filename, @@ -360,12 +424,11 @@ def _bootstrap_impl( req, resolved_version ) - # Build from source (handles test-mode fallback internally) + # Build from source build_result = self._build_from_source( req=req, resolved_version=resolved_version, source_url=source_url, - req_type=req_type, build_sdist_only=build_sdist_only, cached_wheel_filename=cached_wheel_filename, unpacked_cached_wheel=unpacked_cached_wheel, @@ -428,14 +491,19 @@ def _bootstrap_impl( self.progressbar.update_total(len(install_dependencies)) for dep in self._sort_requirements(install_dependencies): with req_ctxvar_context(dep): - # In test mode, bootstrap() catches and records failures internally. - # In normal mode, it raises immediately which we propagate. - self.bootstrap(req=dep, req_type=RequirementType.INSTALL) + try: + self.bootstrap(req=dep, req_type=RequirementType.INSTALL) + except Exception: + if not self.test_mode: + raise + # Failure already recorded in failed_packages; continue. self.progressbar.update() # Clean up build directories self.ctx.clean_build_dirs(build_result.sdist_root_dir, build_result.build_env) + return resolved_version + @contextlib.contextmanager def _track_why( self, @@ -615,9 +683,12 @@ def _handle_build_requirements( for dep in self._sort_requirements(build_dependencies): with req_ctxvar_context(dep): - # In test mode, bootstrap() catches and records failures internally. - # In normal mode, it raises immediately which we propagate. - self.bootstrap(req=dep, req_type=build_type) + try: + self.bootstrap(req=dep, req_type=build_type) + except Exception: + if not self.test_mode: + raise + # Failure already recorded in failed_packages; continue. self.progressbar.update() def _download_prebuilt( @@ -800,7 +871,6 @@ def _build_from_source( req: Requirement, resolved_version: Version, source_url: str, - req_type: RequirementType, build_sdist_only: bool, cached_wheel_filename: pathlib.Path | None, unpacked_cached_wheel: pathlib.Path | None, @@ -808,166 +878,69 @@ def _build_from_source( """Build package from source. Orchestrates download, preparation, build environment setup, and build. - In test mode, attempts pre-built fallback on failure. Raises: - Exception: In normal mode, if build fails. - In test mode, only if build fails AND fallback also fails. + Exception: If any step (download, prepare, build) fails. + Callers are responsible for fallback handling. """ - try: - # Download and prepare source (if no cached wheel) - if not unpacked_cached_wheel: - logger.debug("no cached wheel, downloading sources") - source_filename = self._download_source( - req=req, - resolved_version=resolved_version, - source_url=source_url, - ) - sdist_root_dir = self._prepare_source( - req=req, - resolved_version=resolved_version, - source_filename=source_filename, - ) - else: - logger.debug(f"have cached wheel in {unpacked_cached_wheel}") - sdist_root_dir = unpacked_cached_wheel / unpacked_cached_wheel.stem - - assert sdist_root_dir is not None - - if sdist_root_dir.parent.parent != self.ctx.work_dir: - raise ValueError( - f"'{sdist_root_dir}/../..' should be {self.ctx.work_dir}" - ) - unpack_dir = sdist_root_dir.parent - - build_env = self._create_build_env( - req=req, - resolved_version=resolved_version, - parent_dir=sdist_root_dir.parent, - ) - - # Prepare build dependencies (always needed) - # Note: This may recursively call bootstrap() for build deps, - # which has its own error handling. - self._prepare_build_dependencies( + # Download and prepare source (if no cached wheel) + if not unpacked_cached_wheel: + logger.debug("no cached wheel, downloading sources") + source_filename = self._download_source( req=req, resolved_version=resolved_version, - sdist_root_dir=sdist_root_dir, - build_env=build_env, - ) - - # Build wheel or sdist - wheel_filename, sdist_filename = self._do_build( - req=req, - resolved_version=resolved_version, - sdist_root_dir=sdist_root_dir, - build_env=build_env, - build_sdist_only=build_sdist_only, - cached_wheel_filename=cached_wheel_filename, - ) - - source_type = sources.get_source_type(self.ctx, req) - - return SourceBuildResult( - wheel_filename=wheel_filename, - sdist_filename=sdist_filename, - unpack_dir=unpack_dir, - sdist_root_dir=sdist_root_dir, - build_env=build_env, - source_type=source_type, + source_url=source_url, ) - - except Exception as build_error: - if not self.test_mode: - raise - - # Test mode: attempt pre-built fallback - fallback_result = self._handle_test_mode_failure( + sdist_root_dir = self._prepare_source( req=req, resolved_version=resolved_version, - req_type=req_type, - build_error=build_error, + source_filename=source_filename, ) - if fallback_result is None: - # Fallback failed, re-raise for bootstrap() to catch - raise + else: + logger.debug(f"have cached wheel in {unpacked_cached_wheel}") + sdist_root_dir = unpacked_cached_wheel / unpacked_cached_wheel.stem - return fallback_result + assert sdist_root_dir is not None - def _handle_test_mode_failure( - self, - req: Requirement, - resolved_version: Version, - req_type: RequirementType, - build_error: Exception, - ) -> SourceBuildResult | None: - """Handle build failure in test mode by attempting pre-built fallback. - - Args: - req: The requirement that failed to build. - resolved_version: The version that was attempted. - req_type: The type of requirement (for fallback resolution). - build_error: The original exception from the build attempt. + if sdist_root_dir.parent.parent != self.ctx.work_dir: + raise ValueError(f"'{sdist_root_dir}/../..' should be {self.ctx.work_dir}") + unpack_dir = sdist_root_dir.parent - Returns: - SourceBuildResult if fallback succeeded, None if fallback also failed. - """ - logger.warning( - "test mode: build failed for %s==%s, attempting pre-built fallback: %s", - req.name, - resolved_version, - build_error, + build_env = self._create_build_env( + req=req, + resolved_version=resolved_version, + parent_dir=sdist_root_dir.parent, ) - try: - parent_req = self.why[-1][1] if self.why else None - wheel_url, fallback_version = self._resolver.resolve( - req=req, - req_type=req_type, - parent_req=parent_req, - pre_built=True, # Force prebuilt for test mode fallback - ) - - if fallback_version != resolved_version: - logger.warning( - "test mode: version mismatch for %s - requested %s, fallback %s", - req.name, - resolved_version, - fallback_version, - ) - - wheel_filename, unpack_dir = self._download_prebuilt( - req=req, - req_type=req_type, - resolved_version=fallback_version, - wheel_url=wheel_url, - ) + # Prepare build dependencies. Recursive bootstrap() calls handle + # their own error recovery in test mode. + self._prepare_build_dependencies( + req=req, + resolved_version=resolved_version, + sdist_root_dir=sdist_root_dir, + build_env=build_env, + ) - logger.info( - "test mode: successfully used pre-built wheel for %s==%s", - req.name, - fallback_version, - ) - # Package succeeded via fallback - no failure to record + # Build wheel or sdist + wheel_filename, sdist_filename = self._do_build( + req=req, + resolved_version=resolved_version, + sdist_root_dir=sdist_root_dir, + build_env=build_env, + build_sdist_only=build_sdist_only, + cached_wheel_filename=cached_wheel_filename, + ) - return SourceBuildResult( - wheel_filename=wheel_filename, - sdist_filename=None, - unpack_dir=unpack_dir, - sdist_root_dir=None, - build_env=None, - source_type=SourceType.PREBUILT, - ) + source_type = sources.get_source_type(self.ctx, req) - except Exception as fallback_error: - logger.error( - "test mode: pre-built fallback also failed for %s: %s", - req.name, - fallback_error, - exc_info=True, - ) - # Return None to signal failure; bootstrap() will record via re-raised exception - return None + return SourceBuildResult( + wheel_filename=wheel_filename, + sdist_filename=sdist_filename, + unpack_dir=unpack_dir, + sdist_root_dir=sdist_root_dir, + build_env=build_env, + source_type=source_type, + ) def _look_for_existing_wheel( self, diff --git a/src/fromager/commands/bootstrap.py b/src/fromager/commands/bootstrap.py index f42cfb71..25c9fe5d 100644 --- a/src/fromager/commands/bootstrap.py +++ b/src/fromager/commands/bootstrap.py @@ -182,7 +182,13 @@ def bootstrap( # 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) + try: + bt.bootstrap(req, requirements_file.RequirementType.TOP_LEVEL) + except Exception: + if not test_mode: + raise + # In test mode, failure is already recorded in + # bt.failed_packages; continue to next requirement. progressbar.update() requirement_ctxvar.reset(token) diff --git a/tests/test_bootstrap_test_mode.py b/tests/test_bootstrap_test_mode.py index 84b8bc60..777c13ad 100644 --- a/tests/test_bootstrap_test_mode.py +++ b/tests/test_bootstrap_test_mode.py @@ -14,6 +14,7 @@ import pytest from packaging.requirements import Requirement +from packaging.version import Version from fromager import bootstrapper, context from fromager.requirements_file import RequirementType @@ -290,3 +291,165 @@ def test_resolution_failure_raises_in_normal_mode( ): with pytest.raises(RuntimeError, match="Version resolution failed"): bt.bootstrap(req=req, req_type=RequirementType.TOP_LEVEL) + + +class TestPrebuiltFallback: + """Test prebuilt fallback behavior in test mode when build fails.""" + + def test_fallback_succeeds_records_build_failure( + self, tmp_context: context.WorkContext, caplog: pytest.LogCaptureFixture + ) -> None: + """Test that build failure is always recorded, even when fallback succeeds.""" + bt = bootstrapper.Bootstrapper(ctx=tmp_context, test_mode=True) + req = Requirement("test-package>=1.0") + + with ( + mock.patch.object( + bt, + "resolve_version", + return_value=("https://sdist.url", Version("1.0")), + ), + mock.patch.object(bt, "_add_to_graph"), + mock.patch.object(bt, "_has_been_seen", return_value=False), + mock.patch.object(bt, "_mark_as_seen"), + # First call fails (build), second succeeds (fallback with force_prebuilt) + mock.patch.object( + bt, + "_bootstrap_impl", + side_effect=[RuntimeError("Build failed"), None], + ), + ): + bt.bootstrap(req=req, req_type=RequirementType.TOP_LEVEL) + + # Build failure is always recorded + assert len(bt.failed_packages) == 1 + assert "Build failed" in bt.failed_packages[0]["exception_message"] + assert "successfully used pre-built wheel" in caplog.text + + def test_fallback_also_fails_raises_original_error( + self, tmp_context: context.WorkContext, caplog: pytest.LogCaptureFixture + ) -> None: + """Test that when fallback also fails, original build error is re-raised.""" + bt = bootstrapper.Bootstrapper(ctx=tmp_context, test_mode=True) + req = Requirement("test-package>=1.0") + + with ( + mock.patch.object( + bt, + "resolve_version", + return_value=("https://sdist.url", Version("1.0")), + ), + mock.patch.object(bt, "_add_to_graph"), + mock.patch.object(bt, "_has_been_seen", return_value=False), + mock.patch.object(bt, "_mark_as_seen"), + # Both build and fallback fail + mock.patch.object( + bt, + "_bootstrap_impl", + side_effect=[ + RuntimeError("Original build failed"), + RuntimeError("Fallback also failed"), + ], + ), + ): + with pytest.raises(RuntimeError, match="Original build failed") as exc_info: + bt.bootstrap(req=req, req_type=RequirementType.TOP_LEVEL) + + # Original error is raised, chained from fallback error + assert exc_info.value.__cause__ is not None + assert "Fallback also failed" in str(exc_info.value.__cause__) + + # Failure recorded with ORIGINAL error before fallback was attempted + assert len(bt.failed_packages) == 1 + failure = bt.failed_packages[0] + assert failure["package"] == "test-package" + assert failure["version"] == "1.0" + assert failure["failure_type"] == "bootstrap" + assert "Original build failed" in failure["exception_message"] + assert "pre-built fallback also failed" in caplog.text + + def test_build_failure_raises_in_normal_mode( + self, tmp_context: context.WorkContext + ) -> None: + """Test that build failures raise immediately in normal mode (no fallback).""" + bt = bootstrapper.Bootstrapper(ctx=tmp_context, test_mode=False) + req = Requirement("test-package>=1.0") + + with ( + mock.patch.object( + bt, + "resolve_version", + return_value=("https://sdist.url", Version("1.0")), + ), + mock.patch.object(bt, "_add_to_graph"), + mock.patch.object(bt, "_has_been_seen", return_value=False), + mock.patch.object(bt, "_mark_as_seen"), + mock.patch.object( + bt, + "_bootstrap_impl", + side_effect=RuntimeError("Build failed"), + ), + ): + with pytest.raises(RuntimeError, match="Build failed"): + bt.bootstrap(req=req, req_type=RequirementType.TOP_LEVEL) + + # No fallback attempted in normal mode + assert len(bt.failed_packages) == 0 + + def test_force_prebuilt_version_mismatch_logs_warning( + self, tmp_context: context.WorkContext, caplog: pytest.LogCaptureFixture + ) -> None: + """Test that version mismatch in force_prebuilt path logs a warning. + + When the prebuilt resolver returns a different version than the + originally resolved source version, a warning is logged and the + fallback version propagates correctly to downstream calls. + """ + bt = bootstrapper.Bootstrapper(ctx=tmp_context, test_mode=True) + req = Requirement("test-package>=1.0") + original_version = Version("1.0") + fallback_version = Version("1.2") + fallback_wheel_url = "https://wheel.url/test_package-1.2-py3-none-any.whl" + mock_wheel = tmp_context.work_dir / "test_package-1.2-py3-none-any.whl" + + with ( + mock.patch.object( + bt, + "resolve_version", + return_value=("https://sdist.url", original_version), + ), + mock.patch.object(bt, "_add_to_graph"), + mock.patch.object(bt, "_has_been_seen", return_value=False), + mock.patch.object(bt, "_mark_as_seen"), + mock.patch.object( + bt._resolver, + "resolve", + return_value=(fallback_wheel_url, fallback_version), + ), + mock.patch.object( + bt, + "_download_prebuilt", + return_value=(mock_wheel, tmp_context.work_dir), + ) as mock_download, + mock.patch("fromager.hooks.run_post_bootstrap_hooks"), + mock.patch.object(bt, "_get_install_dependencies", return_value=[]), + mock.patch.object(bt, "_add_to_build_order") as mock_build_order, + mock.patch.object( + bt, "_build_from_source", side_effect=RuntimeError("Build failed") + ), + ): + bt.bootstrap(req=req, req_type=RequirementType.TOP_LEVEL) + + # Version mismatch warning is logged + assert "version mismatch" in caplog.text + assert "1.0" in caplog.text + assert "1.2" in caplog.text + + # Fallback version propagated to downstream calls + mock_download.assert_called_once() + assert mock_download.call_args.kwargs["resolved_version"] == fallback_version + + # Build order records fallback wheel URL, not original sdist URL + mock_build_order.assert_called_once() + assert mock_build_order.call_args.kwargs["version"] == fallback_version + assert mock_build_order.call_args.kwargs["source_url"] == fallback_wheel_url diff --git a/tests/test_bootstrapper.py b/tests/test_bootstrapper.py index 46b78c79..5a594fa0 100644 --- a/tests/test_bootstrapper.py +++ b/tests/test_bootstrapper.py @@ -5,7 +5,7 @@ from packaging.requirements import Requirement from packaging.version import Version -from fromager import bootstrapper, requirements_file +from fromager import bootstrapper from fromager.context import WorkContext from fromager.requirements_file import RequirementType, SourceType @@ -277,7 +277,6 @@ def test_build_from_source_returns_dataclass(tmp_context: WorkContext) -> None: req=Requirement("test-package"), resolved_version=Version("1.0.0"), source_url="https://pypi.org/simple/test-package", - req_type=requirements_file.RequirementType.TOP_LEVEL, build_sdist_only=False, cached_wheel_filename=None, unpacked_cached_wheel=None,