-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Fix handling of pre-releases in preferences #14498
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
Changes from 1 commit
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 |
|---|---|---|
|
|
@@ -34,6 +34,8 @@ pub struct Preference { | |
| /// is part of, otherwise `None`. | ||
| fork_markers: Vec<UniversalMarker>, | ||
| hashes: HashDigests, | ||
| /// The source of the preference. | ||
| source: PreferenceSource, | ||
| } | ||
|
|
||
| impl Preference { | ||
|
|
@@ -73,6 +75,7 @@ impl Preference { | |
| .map(String::as_str) | ||
| .map(HashDigest::from_str) | ||
| .collect::<Result<_, _>>()?, | ||
| source: PreferenceSource::RequirementsTxt, | ||
| })) | ||
| } | ||
|
|
||
|
|
@@ -91,6 +94,7 @@ impl Preference { | |
| index: PreferenceIndex::from(package.index(install_path)?), | ||
| fork_markers: package.fork_markers().to_vec(), | ||
| hashes: HashDigests::empty(), | ||
| source: PreferenceSource::Lock, | ||
| })) | ||
| } | ||
|
|
||
|
|
@@ -112,6 +116,7 @@ impl Preference { | |
| // `pylock.toml` doesn't have fork annotations. | ||
| fork_markers: vec![], | ||
| hashes: HashDigests::empty(), | ||
| source: PreferenceSource::Lock, | ||
| })) | ||
| } | ||
|
|
||
|
|
@@ -127,6 +132,7 @@ impl Preference { | |
| index: PreferenceIndex::Any, | ||
| fork_markers: vec![], | ||
| hashes: HashDigests::empty(), | ||
| source: PreferenceSource::Environment, | ||
| }) | ||
| } | ||
|
|
||
|
|
@@ -171,11 +177,26 @@ impl From<Option<IndexUrl>> for PreferenceIndex { | |
| } | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub(crate) enum PreferenceSource { | ||
| /// The preference is from an installed package in the environment. | ||
| Environment, | ||
| /// The preference is from a `uv.ock` file. | ||
| Lock, | ||
| /// The preference is from a `requirements.txt` file. | ||
| RequirementsTxt, | ||
| /// The preference is from the current solve, without a fork. | ||
| Resolve, | ||
| /// The preference is from a fork in the current solve. | ||
| Fork, | ||
| } | ||
|
Comment on lines
+180
to
+190
Member
Author
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. A little annoyed by the number of source variants, but propagating them from the original location seemed better than assuming one variant during |
||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub(crate) struct Entry { | ||
| marker: UniversalMarker, | ||
| index: PreferenceIndex, | ||
| pin: Pin, | ||
| source: PreferenceSource, | ||
| } | ||
|
|
||
| impl Entry { | ||
|
|
@@ -193,6 +214,11 @@ impl Entry { | |
| pub(crate) fn pin(&self) -> &Pin { | ||
| &self.pin | ||
| } | ||
|
|
||
| /// Return the source of the entry. | ||
| pub(crate) fn source(&self) -> PreferenceSource { | ||
| self.source | ||
| } | ||
| } | ||
|
|
||
| /// A set of pinned packages that should be preserved during resolution, if possible. | ||
|
|
@@ -245,6 +271,7 @@ impl Preferences { | |
| version: preference.version, | ||
| hashes: preference.hashes, | ||
| }, | ||
| source: preference.source, | ||
| }); | ||
| } else { | ||
| for fork_marker in preference.fork_markers { | ||
|
|
@@ -255,6 +282,7 @@ impl Preferences { | |
| version: preference.version.clone(), | ||
| hashes: preference.hashes.clone(), | ||
| }, | ||
| source: preference.source, | ||
| }); | ||
| } | ||
| } | ||
|
|
@@ -270,11 +298,13 @@ impl Preferences { | |
| index: Option<IndexUrl>, | ||
| markers: UniversalMarker, | ||
| pin: impl Into<Pin>, | ||
| source: PreferenceSource, | ||
| ) { | ||
| self.0.entry(package_name).or_default().push(Entry { | ||
| marker: markers, | ||
| index: PreferenceIndex::from(index), | ||
| pin: pin.into(), | ||
| source, | ||
| }); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1730,7 +1730,7 @@ pub(crate) async fn resolve_names( | |
| } | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub(crate) enum PreferenceSource<'lock> { | ||
| pub(crate) enum PreferenceLocation<'lock> { | ||
|
Member
Author
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. This renamed to disambiguate from
Member
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. Yeah that's good. |
||
| /// The preferences should be extracted from a lockfile. | ||
| Lock { | ||
| lock: &'lock Lock, | ||
|
|
@@ -1745,7 +1745,7 @@ pub(crate) struct EnvironmentSpecification<'lock> { | |
| /// The requirements to include in the environment. | ||
| requirements: RequirementsSpecification, | ||
| /// The preferences to respect when resolving. | ||
| preferences: Option<PreferenceSource<'lock>>, | ||
| preferences: Option<PreferenceLocation<'lock>>, | ||
| } | ||
|
|
||
| impl From<RequirementsSpecification> for EnvironmentSpecification<'_> { | ||
|
|
@@ -1758,9 +1758,9 @@ impl From<RequirementsSpecification> for EnvironmentSpecification<'_> { | |
| } | ||
|
|
||
| impl<'lock> EnvironmentSpecification<'lock> { | ||
| /// Set the [`PreferenceSource`] for the specification. | ||
| /// Set the [`PreferenceLocation`] for the specification. | ||
| #[must_use] | ||
| pub(crate) fn with_preferences(self, preferences: PreferenceSource<'lock>) -> Self { | ||
| pub(crate) fn with_preferences(self, preferences: PreferenceLocation<'lock>) -> Self { | ||
| Self { | ||
| preferences: Some(preferences), | ||
| ..self | ||
|
|
@@ -1869,7 +1869,7 @@ pub(crate) async fn resolve_environment( | |
|
|
||
| // If an existing lockfile exists, build up a set of preferences. | ||
| let preferences = match spec.preferences { | ||
| Some(PreferenceSource::Lock { lock, install_path }) => { | ||
| Some(PreferenceLocation::Lock { lock, install_path }) => { | ||
| let LockedRequirements { preferences, git } = | ||
| read_lock_requirements(lock, install_path, &upgrade)?; | ||
|
|
||
|
|
@@ -1881,7 +1881,7 @@ pub(crate) async fn resolve_environment( | |
|
|
||
| preferences | ||
| } | ||
| Some(PreferenceSource::Entries(entries)) => entries, | ||
| Some(PreferenceLocation::Entries(entries)) => entries, | ||
| None => vec![], | ||
| }; | ||
|
|
||
|
|
||
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.
This is the crux of the change. I believe relying on
marker.is_true()to determine if we should respect a preference was fallible for cases where the preference came an existing fork in the lockfile with markers attatched.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 don't think I remember / understand the "rather than a fork" / "comes from a solve without a fork" part, can you help me with that? In other words, why do we have both
forkandresolve?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.
Previously we used
marker.is_true()to capture the case whereresolution.env.try_universal_markers()returnsNone(which indicates we're not in a fork) and is unwrapped toUniversalMarker::TRUEand propagated here. Now, we just move that up to https://github.com/astral-sh/uv/pull/14498/files#diff-6be6d80fe4821c47b70a372260f55e73b8da8182b8dcad7525d5cd3eb584532bR448-R452.I don't actually know if we should do something different with
PreferenceSource::Resolve, but this is intended to match the existing behavior in case it was important — I don't actually know the original motivation either. I could do some digging.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.
It looks like it's been around for a long time: #5736. I guess this was a proxy for detecting whether the pre-release came from a lock file? I suspect you could collapse the fork and resolve cases, and make them both false...
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'm down for trying it.
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.
The PR suggests we have scenarios to test this already, so worth trying I think.