-
Notifications
You must be signed in to change notification settings - Fork 959
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Visit proxy packages eagerly #10441
Visit proxy packages eagerly #10441
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,13 @@ | ||
use pubgrub::Range; | ||
use rustc_hash::FxHashMap; | ||
use std::cmp::Reverse; | ||
use std::collections::hash_map::OccupiedEntry; | ||
|
||
use crate::fork_urls::ForkUrls; | ||
use pubgrub::Range; | ||
use rustc_hash::FxHashMap; | ||
|
||
use uv_normalize::PackageName; | ||
use uv_pep440::Version; | ||
|
||
use crate::fork_urls::ForkUrls; | ||
use crate::pubgrub::package::PubGrubPackage; | ||
use crate::pubgrub::PubGrubPackageInner; | ||
|
||
|
@@ -57,6 +58,7 @@ impl PubGrubPriorities { | |
let priority = if urls.get(name).is_some() { | ||
PubGrubPriority::DirectUrl(Reverse(index)) | ||
} else if version.as_singleton().is_some() { | ||
// TODO(charlie): Take local version ranges into account (e.g., `[2.0, 2.0+[max])`). | ||
PubGrubPriority::Singleton(Reverse(index)) | ||
} else { | ||
// Keep the conflict-causing packages to avoid loops where we seesaw between | ||
|
@@ -80,6 +82,7 @@ impl PubGrubPriorities { | |
let priority = if urls.get(name).is_some() { | ||
PubGrubPriority::DirectUrl(Reverse(next)) | ||
} else if version.as_singleton().is_some() { | ||
// TODO(charlie): Take local version ranges into account (e.g., `[2.0, 2.0+[max])`). | ||
PubGrubPriority::Singleton(Reverse(next)) | ||
} else { | ||
PubGrubPriority::Unspecified(Reverse(next)) | ||
|
@@ -98,7 +101,7 @@ impl PubGrubPriorities { | |
| PubGrubPriority::ConflictEarly(Reverse(index)) | ||
| PubGrubPriority::Singleton(Reverse(index)) | ||
| PubGrubPriority::DirectUrl(Reverse(index)) => Some(*index), | ||
PubGrubPriority::Root => None, | ||
PubGrubPriority::Proxy | PubGrubPriority::Root => None, | ||
} | ||
} | ||
|
||
|
@@ -107,9 +110,9 @@ impl PubGrubPriorities { | |
let package_priority = match &**package { | ||
PubGrubPackageInner::Root(_) => Some(PubGrubPriority::Root), | ||
PubGrubPackageInner::Python(_) => Some(PubGrubPriority::Root), | ||
PubGrubPackageInner::Marker { name, .. } => self.package_priority.get(name).copied(), | ||
PubGrubPackageInner::Extra { name, .. } => self.package_priority.get(name).copied(), | ||
PubGrubPackageInner::Dev { name, .. } => self.package_priority.get(name).copied(), | ||
PubGrubPackageInner::Marker { .. } => Some(PubGrubPriority::Proxy), | ||
PubGrubPackageInner::Extra { .. } => Some(PubGrubPriority::Proxy), | ||
PubGrubPackageInner::Dev { .. } => Some(PubGrubPriority::Proxy), | ||
PubGrubPackageInner::Package { name, .. } => self.package_priority.get(name).copied(), | ||
}; | ||
let virtual_package_tiebreaker = self | ||
|
@@ -224,6 +227,13 @@ pub(crate) enum PubGrubPriority { | |
/// [`ForkUrls`]. | ||
DirectUrl(Reverse<usize>), | ||
|
||
/// The package is a proxy package. | ||
/// | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you expand this by saying something like:
It took me a while to get why this was safe to do. |
||
/// We process proxy packages eagerly since each proxy package expands into two "regular" | ||
/// [`PubGrubPackage`] packages, which gives us additional constraints while not affecting the | ||
/// priorities (since the expanded dependencies are all linked to the same package name). | ||
Proxy, | ||
|
||
/// The package is the root package. | ||
Root, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix this separately, but this fix alone does not resolve the bug in the linked issue.