Skip to content

Commit af9e37e

Browse files
rd4398claude
andcommitted
refactor(resolver): rename and document resolve_from_provider semantics
Rename resolve_from_provider() to find_all_matching_from_provider() to better reflect its changed semantics after the resolver refactoring. Key behavioral changes: - Collects ALL matching candidates instead of single best one - Bypasses resolvelib's full dependency resolver - Passes empty incompatibilities={} dict (no version exclusion) - Only safe because BaseProvider.get_dependencies() returns [] Added comprehensive documentation explaining: - Why empty incompatibilities is safe (no transitive deps) - That this bypasses resolvelib's backtracking logic - Warning that extending get_dependencies() requires revisiting this This addresses the concern that the empty incompatibilities dict could silently produce incorrect results if dependency resolution logic changes. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Rohan Devasthale <rdevasth@redhat.com>
1 parent 9958129 commit af9e37e

7 files changed

Lines changed: 42 additions & 22 deletions

File tree

src/fromager/bootstrap_requirement_resolver.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,7 @@ def _resolve(
175175
req_type=req_type,
176176
ignore_platform=False,
177177
)
178-
results = resolver.resolve_from_provider(provider, req)
178+
results = resolver.find_all_matching_from_provider(provider, req)
179179
if results:
180180
return results
181181
except Exception:
@@ -201,7 +201,7 @@ def _resolve(
201201
req_type=req_type,
202202
ignore_platform=pbi.resolver_ignore_platform,
203203
)
204-
return resolver.resolve_from_provider(provider, req)
204+
return resolver.find_all_matching_from_provider(provider, req)
205205

206206
def get_cached_resolution(
207207
self,
@@ -369,8 +369,8 @@ def _resolve_from_version_source(
369369
constraints=self.ctx.constraints,
370370
use_resolver_cache=False,
371371
)
372-
# resolve_from_provider now returns all matching candidates
373-
return resolver.resolve_from_provider(provider, req)
372+
# find_all_matching_from_provider now returns all matching candidates
373+
return resolver.find_all_matching_from_provider(provider, req)
374374
except Exception as err:
375375
logger.debug(f"could not resolve {req} from {version_source}: {err}")
376376
return None

src/fromager/resolver.py

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ def resolve(
103103
req_type=req_type,
104104
ignore_platform=ignore_platform,
105105
)
106-
results = resolve_from_provider(provider, req)
106+
results = find_all_matching_from_provider(provider, req)
107107
return results[0]
108108

109109

@@ -168,21 +168,37 @@ def ending(self, state: typing.Any) -> None:
168168
self._report("successfully resolved %r", self.req)
169169

170170

171-
def resolve_from_provider(
171+
def find_all_matching_from_provider(
172172
provider: BaseProvider, req: Requirement
173173
) -> list[tuple[str, Version]]:
174-
"""Resolve requirement and return all matching candidates.
174+
"""Find all matching candidates from provider without full dependency resolution.
175+
176+
This function collects ALL candidates that match the requirement, rather than
177+
performing full dependency resolution to find a single best candidate.
175178
176179
Returns list of (url, version) tuples sorted by version (highest first).
180+
181+
IMPORTANT: This bypasses resolvelib's full resolver to collect all matching
182+
candidates. This is safe ONLY because BaseProvider.get_dependencies() returns
183+
an empty list (no transitive dependencies to resolve). The empty incompatibilities
184+
dict means no version is ever excluded based on conflicts.
185+
186+
If get_dependencies() is ever extended to return actual dependencies, this
187+
function must be revisited to use resolvelib's full resolution algorithm
188+
(Resolver.resolve()) to properly handle dependency conflicts and backtracking.
177189
"""
178190
# Get all matching candidates directly from provider
179191
# instead of using resolvelib's resolver which picks just one
180192
identifier = provider.identify(req)
181193
try:
194+
# Bypass resolvelib's resolver to collect all matching candidates rather than
195+
# just the single best one. This is safe because get_dependencies() returns []
196+
# (no transitive deps to resolve). If get_dependencies() is ever extended,
197+
# this must be revisited to use resolvelib's full resolution.
182198
candidates = provider.find_matches(
183199
identifier=identifier,
184200
requirements={identifier: [req]},
185-
incompatibilities={},
201+
incompatibilities={}, # Empty - safe only because no transitive deps
186202
)
187203
except resolvelib.resolvers.ResolverException as err:
188204
constraint = provider.constraints.get_constraint(req.name)

src/fromager/sources.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -160,7 +160,7 @@ def resolve_source(
160160
)
161161

162162
# Get all matching candidates from provider
163-
results = resolver.resolve_from_provider(provider, req)
163+
results = resolver.find_all_matching_from_provider(provider, req)
164164

165165
# Return highest version (first in sorted list)
166166
url, version = results[0]

src/fromager/wheels.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -487,7 +487,7 @@ def resolve_prebuilt_wheel(
487487
)
488488

489489
# Get all matching candidates from provider
490-
results = resolver.resolve_from_provider(provider, req)
490+
results = resolver.find_all_matching_from_provider(provider, req)
491491

492492
if results:
493493
# Return highest version (first in sorted list)

tests/test_bootstrap_requirement_resolver.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -448,7 +448,7 @@ def test_resolve_rejects_git_urls_for_source(tmp_context: WorkContext) -> None:
448448
)
449449

450450

451-
@patch("fromager.resolver.resolve_from_provider")
451+
@patch("fromager.resolver.find_all_matching_from_provider")
452452
def test_resolve_allows_git_urls_for_prebuilt(
453453
mock_resolve: MagicMock,
454454
tmp_context: WorkContext,
@@ -476,7 +476,7 @@ def test_resolve_allows_git_urls_for_prebuilt(
476476
assert version == Version("1.0")
477477

478478

479-
@patch("fromager.resolver.resolve_from_provider")
479+
@patch("fromager.resolver.find_all_matching_from_provider")
480480
def test_resolve_auto_routes_to_prebuilt(
481481
mock_resolve: MagicMock,
482482
tmp_context: WorkContext,
@@ -514,7 +514,7 @@ def test_resolve_auto_routes_to_prebuilt(
514514
assert version == Version("1.0")
515515

516516

517-
@patch("fromager.resolver.resolve_from_provider")
517+
@patch("fromager.resolver.find_all_matching_from_provider")
518518
def test_resolve_auto_routes_to_source(
519519
mock_resolve: MagicMock,
520520
tmp_context: WorkContext,
@@ -554,7 +554,7 @@ def test_resolve_auto_routes_to_source(
554554
assert version == Version("2.0")
555555

556556

557-
@patch("fromager.resolver.resolve_from_provider")
557+
@patch("fromager.resolver.find_all_matching_from_provider")
558558
def test_resolve_prebuilt_after_source_uses_separate_cache(
559559
mock_resolve: MagicMock,
560560
tmp_context: WorkContext,

tests/test_resolver.py

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1213,7 +1213,9 @@ def test_custom_resolver_error_message_missing_tag() -> None:
12131213
provider = resolver.GitHubTagProvider(organization="test-org", repo="test-repo")
12141214

12151215
with pytest.raises(resolvelib.resolvers.ResolverException) as exc_info:
1216-
resolver.resolve_from_provider(provider, Requirement("test-package==1.0.0"))
1216+
resolver.find_all_matching_from_provider(
1217+
provider, Requirement("test-package==1.0.0")
1218+
)
12171219

12181220
error_message = str(exc_info.value)
12191221
assert (
@@ -1249,7 +1251,9 @@ def custom_resolver_provider(
12491251
provider = custom_resolver_provider()
12501252

12511253
with pytest.raises(resolvelib.resolvers.ResolverException) as exc_info:
1252-
resolver.resolve_from_provider(provider, Requirement("test-package==1.0.0"))
1254+
resolver.find_all_matching_from_provider(
1255+
provider, Requirement("test-package==1.0.0")
1256+
)
12531257

12541258
error_message = str(exc_info.value)
12551259
# After fix for issue #858, the error message should indicate that a GitHub resolver was used

tests/test_sources.py

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,14 @@ def test_invalid_tarfile(mock_download_url: typing.Any, tmp_path: pathlib.Path)
2323
sources._download_source_check(req=req, destination_dir=fake_dir, url=fake_url)
2424

2525

26-
@patch("fromager.resolver.resolve_from_provider")
26+
@patch("fromager.resolver.find_all_matching_from_provider")
2727
@patch("fromager.sources._download_source_check")
2828
def test_resolve_source_from_settings(
2929
download_source_check: Mock,
30-
resolve_from_provider: Mock,
30+
find_all_matching_from_provider: Mock,
3131
testdata_context: context.WorkContext,
3232
) -> None:
33-
resolve_from_provider.return_value = [("url", Version("42.1.2"))]
33+
find_all_matching_from_provider.return_value = [("url", Version("42.1.2"))]
3434
download_source_check.return_value = pathlib.Path("filename.zip")
3535
req = Requirement("test_pkg==42.1.2")
3636
sdist_server_url = "https://sdist.test/egg"
@@ -59,7 +59,7 @@ def test_resolve_source_from_settings(
5959
)
6060

6161

62-
@patch("fromager.resolver.resolve_from_provider")
62+
@patch("fromager.resolver.find_all_matching_from_provider")
6363
@patch("fromager.sources._download_source_check")
6464
@patch.multiple(
6565
packagesettings.PackageBuildInfo,
@@ -69,10 +69,10 @@ def test_resolve_source_from_settings(
6969
)
7070
def test_resolve_source_with_predefined_resolve_dist(
7171
download_source_check: Mock,
72-
resolve_from_provider: Mock,
72+
find_all_matching_from_provider: Mock,
7373
tmp_context: context.WorkContext,
7474
) -> None:
75-
resolve_from_provider.return_value = [("url", Version("1.0"))]
75+
find_all_matching_from_provider.return_value = [("url", Version("1.0"))]
7676
download_source_check.return_value = pathlib.Path("filename")
7777
req = Requirement("foo==1.0")
7878

0 commit comments

Comments
 (0)