Allow conflicting prerelease strategies when forking #5150
Conversation
ibraheemdev
force-pushed
the
ibraheem/fork-prereleases
branch
from
5e6d993 to
70a64cc
Compare
| .map_ok(|dependency| PubGrubDependency { | ||
| local: locals::from_source(&requirement.source), | ||
| ..dependency | ||
| }) |
There was a problem hiding this comment.
Looks like there was a bug in #5104 because we only looked for locals on the root package, which ignored editables and path dependencies. We have to set these fields in PubGrubDependency::from_requirement, which is a little unfortunate because it means we parse the local from all dependencies, even though we only need to look at requirements that are directly in the manifest. Maybe we should move this step up to the Manifest directly.
There was a problem hiding this comment.
Hmm this might be incorrect, but I'm not sure how to represent what we want here.
| ) -> Option<Candidate<'a>> { | ||
| // If the package has a preference (e.g., an existing version from an existing lockfile), | ||
| // and the preference satisfies the current range, use that. | ||
| if let Some(version) = preferences.version(package_name) { | ||
| if range.contains(version) { | ||
| 'preference: { |
There was a problem hiding this comment.
nit: Should we move this block into its own function?
## Summary This fixes a few bugs introduced by #5104. I previously thought we could track conflicting locals the same way we track conflicting URLs in forks, but it turns out that ends up being very tricky. URL forks work because we prioritize directly URL requirements. We can't prioritize locals in the same way without conflicting with the URL prioritization (this may be possible but it's not trivial), so we run into issues where a correct resolution depends on the order in which dependencies are traversed. Instead, we track local versions across all forks in `Locals`. When applying a local version, we apply all locals with markers that intersect with the current fork. This way we end up applying some local versions without creating a fork. For example, given: ``` // pyproject.toml dependencies = [ "torch==2.0.0+cu118 ; platform_machine == 'x86_64'", ] // requirements.in torch==2.0.0 . ``` We choose `2.0.0+cu118` in all cases. However, if a disjoint fork is created based on local versions, the resolver will choose the most compatible local when it narrows to a specific fork. Thus we correctly respect local versions when forking: ``` // pyproject.toml dependencies = [ "torch==2.0.0+cu118 ; platform_machine == 'x86_64'", "torch==2.0.0+cpu ; platform_machine != 'x86_64'" ] // requirements.in torch==2.0.0 . ``` We should also be able to use a similar strategy for #5150. ## Test Plan This fixes #5220 locally for me, as well as a few other bugs that were not reported yet.
ibraheemdev
force-pushed
the
ibraheem/fork-prereleases
branch
from
e4df4df to
4ef5e0b
Compare
ibraheemdev
force-pushed
the
ibraheem/fork-prereleases
branch
from
4ef5e0b to
50bb5a6
Compare
| != AllowPreRelease::Yes | ||
| { | ||
| break 'preference; | ||
| } |
There was a problem hiding this comment.
Is this piece correct? I feel like we should still be respecting preferences here.
There was a problem hiding this comment.
I'm not sure, I think the issue comes from when we add preferences during resolution, because we can end up adding a preference for a prerelease from a different fork. Maybe the solution is to make Preferences a ForkMap?
There was a problem hiding this comment.
Actually I feel like this is correct. If pre-release versions are not allowed then the version range is not actually satisfied, right?
Summary
Similar to #5232, we should also track prerelease strategies per-fork, instead of globally per package. The common functionality for tracking locals and prerelease versions across forks is extracted into the
ForkMaptype.Resolves #4579. This doesn't quite solve #4959, as that issue relies on overlapping markers.