refactor(resolver): inline _resolve() into resolve() and clarify fallback logic

Inline `_resolve()` into `resolve()` to remove unnecessary indirection.
Restructure the empty-results handling as explicit early returns with
per-case log messages. Guard against IndexError on empty results.

Co-Authored-By: Claude <claude@anthropic.com>
Signed-off-by: Rohan Devasthale <rdevasth@redhat.com>

Author: rd4398

src/fromager/bootstrap_requirement_resolver.py

@@ -121,42 +121,7 @@ def resolve(
             logger.debug(f"resolved {req} from cache")
             return list(cached_result) if return_all_versions else [cached_result[0]]
 
-        # Resolve using strategies
-        results = self._resolve(req, req_type, parent_req, pre_built)
-
-        # Only cache non-empty results.
-        if results:
-            self.cache_resolution(req, pre_built, results)
-
-        if not results:
-            return []
-        return results if return_all_versions else [results[0]]
-
-    def _resolve(
-        self,
-        req: Requirement,
-        req_type: RequirementType,
-        parent_req: Requirement | None,
-        pre_built: bool,
-    ) -> list[tuple[str, Version]]:
-        """Internal resolution logic without caching.
-
-        Tries resolution strategies in order:
-        1. Previous dependency graph
-        2. PyPI resolution (source or prebuilt)
-        3. Remote wheel cache server (multi-version mode only, when age
-           filtering produced no source candidates)
-
-        Args:
-            req: Package requirement
-            req_type: Type of requirement
-            parent_req: Parent requirement from dependency chain
-            pre_built: Whether to resolve prebuilt (True) or source (False)
-
-        Returns:
-            List of (url, version) tuples sorted by version (highest first)
-        """
-        # Try graph
+        # Try previous dependency graph
         cached_resolution = self._resolve_from_graph(
             req=req,
             req_type=req_type,
@@ -168,49 +133,63 @@ def _resolve(
             logger.debug(
                 f"resolved from previous bootstrap: {len(cached_resolution)} version(s)"
             )
-            return cached_resolution
-
-        # Fallback to PyPI using provider pattern
-        if pre_built:
+            results = cached_resolution
+        elif pre_built:
             # Resolve prebuilt wheel
-            # Get wheel server URLs (use PyPI as cache/fallback server)
             wheel_server_urls = wheels.get_wheel_server_urls(
                 self.ctx, req, cache_wheel_server_url=resolver.PYPI_SERVER_URL
             )
-            # Use shared retry loop logic from wheels module
-            return wheels.resolve_all_prebuilt_wheels(
+            results = wheels.resolve_all_prebuilt_wheels(
                 ctx=self.ctx,
                 req=req,
                 wheel_server_urls=wheel_server_urls,
                 req_type=req_type,
             )
+        else:
+            # Resolve source (sdist)
+            provider = sources.get_source_provider(
+                ctx=self.ctx,
+                req=req,
+                sdist_server_url=resolver.PYPI_SERVER_URL,
+                req_type=req_type,
+            )
+            max_age_cutoff = resolver._compute_max_age_cutoff(self.ctx)
+            results = resolver.find_all_matching_from_provider(
+                provider,
+                req,
+                max_age_cutoff=max_age_cutoff,
+                fallback_on_empty_age_filter=not self.multiple_versions,
+            )
 
-        # Resolve source (sdist)
-        provider = sources.get_source_provider(
-            ctx=self.ctx,
-            req=req,
-            sdist_server_url=resolver.PYPI_SERVER_URL,
-            req_type=req_type,
-        )
-        max_age_cutoff = resolver._compute_max_age_cutoff(self.ctx)
-        results = resolver.find_all_matching_from_provider(
-            provider,
-            req,
-            max_age_cutoff=max_age_cutoff,
-            fallback_on_empty_age_filter=not self.multiple_versions,
-        )
+            if results:
+                pass
+            elif not self.multiple_versions:
+                logger.warning(
+                    "%s: resolver returned no results",
+                    req.name,
+                )
+            elif not self.cache_wheel_server_url:
+                logger.warning(
+                    "%s: resolver returned no results and we have no "
+                    "wheel cache server URL to fall back on",
+                    req.name,
+                )
+            else:
+                logger.info(
+                    "%s: no results found with normal resolution, "
+                    "falling back to the cache server %s",
+                    req.name,
+                    self.cache_wheel_server_url,
+                )
+                results = self._resolve_from_cache_server(req)
 
-        if not results and self.multiple_versions and self.cache_wheel_server_url:
-            results = self._resolve_from_cache_server(req)
+        # Only cache non-empty results.
+        if results:
+            self.cache_resolution(req, pre_built, results)
 
         if not results:
-            logger.warning(
-                "%s: could not find any versions (all filtered by "
-                "max-release-age and no cached wheel on server)",
-                req.name,
-            )
-
-        return results
+            return []
+        return results if return_all_versions else [results[0]]
 
     def _resolve_from_cache_server(self, req: Requirement) -> list[tuple[str, Version]]:
         """Fall back to the remote wheel cache server for a cached version.

tests/test_bootstrap_requirement_resolver.py

@@ -762,7 +762,7 @@ def test_returns_empty_on_exception(self, tmp_context: WorkContext) -> None:
     def test_resolve_uses_cache_fallback_when_age_filter_empties(
         self, tmp_context: WorkContext
     ) -> None:
-        """_resolve falls back to cache server when age filter produces empty result."""
+        """resolve() falls back to cache server when age filter produces empty result."""
         brr = BootstrapRequirementResolver(
             tmp_context,
             multiple_versions=True,
@@ -786,11 +786,12 @@ def test_resolve_uses_cache_fallback_when_age_filter_empties(
                 return_value=[("http://cache.test/testpkg-1.0.whl", Version("1.0"))],
             ) as mock_cache,
         ):
-            result = brr._resolve(
+            result = brr.resolve(
                 req,
                 RequirementType.INSTALL,
                 parent_req=None,
                 pre_built=False,
+                return_all_versions=True,
             )
 
         mock_pypi.assert_called_once()
@@ -801,7 +802,7 @@ def test_resolve_uses_cache_fallback_when_age_filter_empties(
     def test_resolve_skips_cache_fallback_in_single_version_mode(
         self, tmp_context: WorkContext
     ) -> None:
-        """_resolve does not fall back to cache server in single-version mode."""
+        """resolve() does not fall back to cache server in single-version mode."""
         brr = BootstrapRequirementResolver(
             tmp_context,
             multiple_versions=False,
@@ -821,7 +822,7 @@ def test_resolve_skips_cache_fallback_in_single_version_mode(
             ),
             patch.object(brr, "_resolve_from_cache_server") as mock_cache,
         ):
-            brr._resolve(
+            brr.resolve(
                 req,
                 RequirementType.INSTALL,
                 parent_req=None,
@@ -833,7 +834,7 @@ def test_resolve_skips_cache_fallback_in_single_version_mode(
     def test_resolve_skips_cache_fallback_when_no_server_url(
         self, tmp_context: WorkContext
     ) -> None:
-        """_resolve does not fall back when no cache_wheel_server_url is set."""
+        """resolve() does not fall back when no cache_wheel_server_url is set."""
         brr = BootstrapRequirementResolver(
             tmp_context,
             multiple_versions=True,
@@ -853,11 +854,12 @@ def test_resolve_skips_cache_fallback_when_no_server_url(
             ),
             patch.object(brr, "_resolve_from_cache_server") as mock_cache,
         ):
-            brr._resolve(
+            brr.resolve(
                 req,
                 RequirementType.INSTALL,
                 parent_req=None,
                 pre_built=False,
+                return_all_versions=True,
             )
 
         mock_cache.assert_not_called()

tests/test_bootstrap_requirement_resolver_multiple.py

@@ -30,83 +30,93 @@ def tmp_context(tmp_path: Path) -> WorkContext:
     pbi.resolver_ignore_platform = False
     pbi.resolver_sdist_server_url.return_value = "https://pypi.org/simple/"
     ctx.package_build_info.return_value = pbi
+    ctx.max_release_age = None
     return ctx
 
 
+_MOCK_VERSIONS = [
+    ("https://pypi.org/testpkg-2.0.tar.gz", Version("2.0")),
+    ("https://pypi.org/testpkg-1.5.tar.gz", Version("1.5")),
+    ("https://pypi.org/testpkg-1.0.tar.gz", Version("1.0")),
+]
+
+
 def test_resolve_return_all_versions_true(tmp_context: WorkContext) -> None:
     """resolve() with return_all_versions=True returns all matching versions."""
-    resolver = BootstrapRequirementResolver(tmp_context)
-
-    # Mock the _resolve method to return multiple versions
-    with patch.object(
-        resolver,
-        "_resolve",
-        return_value=[
-            ("https://pypi.org/testpkg-2.0.tar.gz", Version("2.0")),
-            ("https://pypi.org/testpkg-1.5.tar.gz", Version("1.5")),
-            ("https://pypi.org/testpkg-1.0.tar.gz", Version("1.0")),
-        ],
+    brr = BootstrapRequirementResolver(tmp_context)
+
+    with (
+        patch.object(brr, "_resolve_from_graph", return_value=None),
+        patch(
+            "fromager.bootstrap_requirement_resolver.sources.get_source_provider",
+        ),
+        patch(
+            "fromager.bootstrap_requirement_resolver.resolver"
+            ".find_all_matching_from_provider",
+            return_value=list(_MOCK_VERSIONS),
+        ),
     ):
         req = Requirement("testpkg>=1.0")
-        results = resolver.resolve(
+        results = brr.resolve(
             req=req,
             req_type=RequirementType.INSTALL,
             parent_req=None,
             return_all_versions=True,
         )
 
-        # Should return all 3 versions
         assert len(results) == 3
-        assert results[0] == ("https://pypi.org/testpkg-2.0.tar.gz", Version("2.0"))
-        assert results[1] == ("https://pypi.org/testpkg-1.5.tar.gz", Version("1.5"))
-        assert results[2] == ("https://pypi.org/testpkg-1.0.tar.gz", Version("1.0"))
+        assert results[0] == _MOCK_VERSIONS[0]
+        assert results[1] == _MOCK_VERSIONS[1]
+        assert results[2] == _MOCK_VERSIONS[2]
 
 
 def test_resolve_return_all_versions_false_default(tmp_context: WorkContext) -> None:
     """resolve() with return_all_versions=False (default) returns list with only highest version."""
-    resolver = BootstrapRequirementResolver(tmp_context)
-
-    # Mock the _resolve method to return multiple versions
-    with patch.object(
-        resolver,
-        "_resolve",
-        return_value=[
-            ("https://pypi.org/testpkg-2.0.tar.gz", Version("2.0")),
-            ("https://pypi.org/testpkg-1.5.tar.gz", Version("1.5")),
-            ("https://pypi.org/testpkg-1.0.tar.gz", Version("1.0")),
-        ],
+    brr = BootstrapRequirementResolver(tmp_context)
+
+    with (
+        patch.object(brr, "_resolve_from_graph", return_value=None),
+        patch(
+            "fromager.bootstrap_requirement_resolver.sources.get_source_provider",
+        ),
+        patch(
+            "fromager.bootstrap_requirement_resolver.resolver"
+            ".find_all_matching_from_provider",
+            return_value=list(_MOCK_VERSIONS),
+        ),
     ):
         req = Requirement("testpkg>=1.0")
-
-        # Call without return_all_versions (default False)
-        results = resolver.resolve(
+        results = brr.resolve(
             req=req,
             req_type=RequirementType.INSTALL,
             parent_req=None,
         )
 
-        # Should return list with only the highest version
         assert len(results) == 1
-        assert results[0] == ("https://pypi.org/testpkg-2.0.tar.gz", Version("2.0"))
+        assert results[0] == _MOCK_VERSIONS[0]
 
 
 def test_resolve_return_all_versions_uses_cache(tmp_context: WorkContext) -> None:
     """resolve() with return_all_versions=True uses cache correctly."""
-    resolver = BootstrapRequirementResolver(tmp_context)
-
-    # First call - will populate cache
-    with patch.object(
-        resolver,
-        "_resolve",
-        return_value=[
-            ("https://pypi.org/testpkg-2.0.tar.gz", Version("2.0")),
-            ("https://pypi.org/testpkg-1.0.tar.gz", Version("1.0")),
-        ],
-    ) as mock_resolve:
+    brr = BootstrapRequirementResolver(tmp_context)
+
+    with (
+        patch.object(brr, "_resolve_from_graph", return_value=None),
+        patch(
+            "fromager.bootstrap_requirement_resolver.sources.get_source_provider",
+        ),
+        patch(
+            "fromager.bootstrap_requirement_resolver.resolver"
+            ".find_all_matching_from_provider",
+            return_value=[
+                ("https://pypi.org/testpkg-2.0.tar.gz", Version("2.0")),
+                ("https://pypi.org/testpkg-1.0.tar.gz", Version("1.0")),
+            ],
+        ) as mock_resolve,
+    ):
         req = Requirement("testpkg>=1.0")
 
-        # First call
-        results1 = resolver.resolve(
+        results1 = brr.resolve(
             req=req,
             req_type=RequirementType.INSTALL,
             parent_req=None,
@@ -115,25 +125,22 @@ def test_resolve_return_all_versions_uses_cache(tmp_context: WorkContext) -> Non
         assert len(results1) == 2
         assert mock_resolve.call_count == 1
 
-        # Second call - should use cache
-        results2 = resolver.resolve(
+        results2 = brr.resolve(
             req=req,
             req_type=RequirementType.INSTALL,
             parent_req=None,
             return_all_versions=True,
         )
 
-        # Should not call _resolve again
+        # Should not call resolution again — cache hit
         assert mock_resolve.call_count == 1
-        # Should return same results
         assert results2 == results1
 
 
 def test_resolve_return_all_versions_with_previous_graph(
     tmp_context: WorkContext,
 ) -> None:
     """resolve() with return_all_versions=True works with previous graph."""
-    # Create graph with multiple versions of the same package
     prev_graph = DependencyGraph()
     prev_graph.add_dependency(
         parent_name=None,
@@ -150,23 +157,19 @@ def test_resolve_return_all_versions_with_previous_graph(
         req_version=Version("1.0"),
     )
 
-    # Mock dependency_graph in context
     tmp_context.dependency_graph = prev_graph
 
-    resolver = BootstrapRequirementResolver(tmp_context, prev_graph)
+    brr = BootstrapRequirementResolver(tmp_context, prev_graph)
 
-    # Request with version spec that matches both
     req = Requirement("testpkg>=1.0")
-    results = resolver.resolve(
+    results = brr.resolve(
         req=req,
         req_type=RequirementType.TOP_LEVEL,
         parent_req=None,
         return_all_versions=True,
     )
 
-    # Should return both versions from graph
     assert len(results) == 2
-    # Verify versions (should be sorted highest first)
     versions = [v for _, v in results]
     assert Version("2.0") in versions
     assert Version("1.0") in versions