Skip to content
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

uv-resolver: add new UniversalMarker type #9334

Merged
merged 6 commits into from
Nov 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions crates/uv-pep508/src/marker/tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2451,6 +2451,39 @@ mod test {
assert!(m("sys_platform == 'win32' and sys_platform != 'win32'").is_false());
}

/// This tests the difference between simplifying extras and simplifying
/// other stringly-valued marker attributes.
///
/// In particular, this demonstrates that `extra != "foo"` would actually
/// be more clearly written as `"foo" not in extras`. And similarly, `extra
/// == "foo"` would be written as `"foo" in extras`. This is different from
/// other attributes, like `sys_platform`, where the test is just string
/// equality. That is, `extra` is a set where as `sys_platform` is just a
/// single value.
#[test]
fn test_simplification_extra_versus_other() {
// Here, the `extra != 'foo'` cannot be simplified out, because
// `extra == 'foo'` can be true even when `extra == 'bar`' is true.
assert_simplifies(
r#"extra != "foo" and (extra == "bar" or extra == "baz")"#,
"(extra == 'bar' and extra != 'foo') or (extra == 'baz' and extra != 'foo')",
);
// But here, the `sys_platform != 'foo'` can be simplified out, because
// it is strictly disjoint with
// `sys_platform == "bar" or sys_platform == "baz"`.
assert_simplifies(
r#"sys_platform != "foo" and (sys_platform == "bar" or sys_platform == "baz")"#,
"sys_platform == 'bar' or sys_platform == 'baz'",
);

// Another case I experimented with and wanted to verify.
assert_simplifies(
r#"(extra != "bar" and (extra == "foo" or extra == "baz"))
or ((extra != "foo" and extra != "bar") and extra == "baz")"#,
"(extra != 'bar' and extra == 'baz') or (extra != 'bar' and extra == 'foo')",
);
}

#[test]
fn test_marker_negation() {
assert_eq!(
Expand Down
8 changes: 4 additions & 4 deletions crates/uv-resolver/src/candidate_selector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ use uv_distribution_types::{CompatibleDist, IncompatibleDist, IncompatibleSource
use uv_distribution_types::{DistributionMetadata, IncompatibleWheel, Name, PrioritizedDist};
use uv_normalize::PackageName;
use uv_pep440::Version;
use uv_pep508::MarkerTree;
use uv_types::InstalledPackagesProvider;

use crate::preferences::Preferences;
use crate::prerelease::{AllowPrerelease, PrereleaseStrategy};
use crate::resolution_mode::ResolutionStrategy;
use crate::universal_marker::UniversalMarker;
use crate::version_map::{VersionMap, VersionMapDistHandle};
use crate::{Exclusions, Manifest, Options, ResolverEnvironment};

Expand Down Expand Up @@ -140,10 +140,10 @@ impl CandidateSelector {
// first has the matching half and then the mismatching half.
let preferences_match = preferences
.get(package_name)
.filter(|(marker, _index, _version)| env.included_by_marker(marker));
.filter(|(marker, _index, _version)| env.included_by_marker(marker.pep508()));
let preferences_mismatch = preferences
.get(package_name)
.filter(|(marker, _index, _version)| !env.included_by_marker(marker));
.filter(|(marker, _index, _version)| !env.included_by_marker(marker.pep508()));
let preferences = preferences_match.chain(preferences_mismatch).filter_map(
|(marker, source, version)| {
// If the package is mapped to an explicit index, only consider preferences that
Expand All @@ -167,7 +167,7 @@ impl CandidateSelector {
/// Return the first preference that satisfies the current range and is allowed.
fn get_preferred_from_iter<'a, InstalledPackages: InstalledPackagesProvider>(
&'a self,
preferences: impl Iterator<Item = (&'a MarkerTree, &'a Version)>,
preferences: impl Iterator<Item = (&'a UniversalMarker, &'a Version)>,
package_name: &'a PackageName,
range: &Range<Version>,
version_maps: &'a [VersionMap],
Expand Down
15 changes: 8 additions & 7 deletions crates/uv-resolver/src/graph_ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ use petgraph::visit::EdgeRef;
use petgraph::{Direction, Graph};
use rustc_hash::{FxBuildHasher, FxHashMap};
use std::collections::hash_map::Entry;
use uv_pep508::MarkerTree;

use crate::universal_marker::UniversalMarker;

/// Determine the markers under which a package is reachable in the dependency tree.
///
Expand All @@ -13,9 +14,9 @@ use uv_pep508::MarkerTree;
/// whenever we re-reach a node through a cycle the marker we have is a more
/// specific marker/longer path, so we don't update the node and don't re-queue it.
pub(crate) fn marker_reachability<T>(
graph: &Graph<T, MarkerTree>,
fork_markers: &[MarkerTree],
) -> FxHashMap<NodeIndex, MarkerTree> {
graph: &Graph<T, UniversalMarker>,
fork_markers: &[UniversalMarker],
) -> FxHashMap<NodeIndex, UniversalMarker> {
// Note that we build including the virtual packages due to how we propagate markers through
// the graph, even though we then only read the markers for base packages.
let mut reachability = FxHashMap::with_capacity_and_hasher(graph.node_count(), FxBuildHasher);
Expand All @@ -36,12 +37,12 @@ pub(crate) fn marker_reachability<T>(

// The root nodes are always applicable, unless the user has restricted resolver
// environments with `tool.uv.environments`.
let root_markers: MarkerTree = if fork_markers.is_empty() {
MarkerTree::TRUE
let root_markers = if fork_markers.is_empty() {
UniversalMarker::TRUE
} else {
fork_markers
.iter()
.fold(MarkerTree::FALSE, |mut acc, marker| {
.fold(UniversalMarker::FALSE, |mut acc, marker| {
acc.or(marker.clone());
acc
})
Expand Down
2 changes: 2 additions & 0 deletions crates/uv-resolver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ pub use resolver::{
PackageVersionsResult, Reporter as ResolverReporter, Resolver, ResolverEnvironment,
ResolverProvider, VersionsResponse, WheelMetadataResult,
};
pub use universal_marker::UniversalMarker;
pub use version_map::VersionMap;
pub use yanks::AllowedYanks;

Expand Down Expand Up @@ -56,5 +57,6 @@ mod requires_python;
mod resolution;
mod resolution_mode;
mod resolver;
mod universal_marker;
mod version_map;
mod yanks;
92 changes: 62 additions & 30 deletions crates/uv-resolver/src/lock/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ pub use crate::lock::target::InstallTarget;
pub use crate::lock::tree::TreeDisplay;
use crate::requires_python::SimplifiedMarkerTree;
use crate::resolution::{AnnotatedDist, ResolutionGraphNode};
use crate::universal_marker::UniversalMarker;
use crate::{
ExcludeNewer, InMemoryIndex, MetadataResponse, PrereleaseMode, RequiresPython, ResolutionMode,
ResolverOutput,
Expand Down Expand Up @@ -55,23 +56,26 @@ mod tree;
/// The current version of the lockfile format.
pub const VERSION: u32 = 1;

static LINUX_MARKERS: LazyLock<MarkerTree> = LazyLock::new(|| {
MarkerTree::from_str(
static LINUX_MARKERS: LazyLock<UniversalMarker> = LazyLock::new(|| {
let pep508 = MarkerTree::from_str(
"platform_system == 'Linux' and os_name == 'posix' and sys_platform == 'linux'",
)
.unwrap()
.unwrap();
UniversalMarker::new(pep508, MarkerTree::TRUE)
});
static WINDOWS_MARKERS: LazyLock<MarkerTree> = LazyLock::new(|| {
MarkerTree::from_str(
static WINDOWS_MARKERS: LazyLock<UniversalMarker> = LazyLock::new(|| {
let pep508 = MarkerTree::from_str(
"platform_system == 'Windows' and os_name == 'nt' and sys_platform == 'win32'",
)
.unwrap()
.unwrap();
UniversalMarker::new(pep508, MarkerTree::TRUE)
});
static MAC_MARKERS: LazyLock<MarkerTree> = LazyLock::new(|| {
MarkerTree::from_str(
static MAC_MARKERS: LazyLock<UniversalMarker> = LazyLock::new(|| {
let pep508 = MarkerTree::from_str(
"platform_system == 'Darwin' and os_name == 'posix' and sys_platform == 'darwin'",
)
.unwrap()
.unwrap();
UniversalMarker::new(pep508, MarkerTree::TRUE)
});

#[derive(Clone, Debug, serde::Deserialize)]
Expand All @@ -80,7 +84,7 @@ pub struct Lock {
version: u32,
/// If this lockfile was built from a forking resolution with non-identical forks, store the
/// forks in the lockfile so we can recreate them in subsequent resolutions.
fork_markers: Vec<MarkerTree>,
fork_markers: Vec<UniversalMarker>,
/// The conflicting groups/extras specified by the user.
conflicts: Conflicts,
/// The list of supported environments specified by the user.
Expand Down Expand Up @@ -315,7 +319,7 @@ impl Lock {
manifest: ResolverManifest,
conflicts: Conflicts,
supported_environments: Vec<MarkerTree>,
fork_markers: Vec<MarkerTree>,
fork_markers: Vec<UniversalMarker>,
) -> Result<Self, LockError> {
// Put all dependencies for each package in a canonical order and
// check for duplicates.
Expand Down Expand Up @@ -597,7 +601,7 @@ impl Lock {

/// If this lockfile was built from a forking resolution with non-identical forks, return the
/// markers of those forks, otherwise `None`.
pub fn fork_markers(&self) -> &[MarkerTree] {
pub fn fork_markers(&self) -> &[UniversalMarker] {
self.fork_markers.as_slice()
}

Expand All @@ -614,7 +618,13 @@ impl Lock {
let fork_markers = each_element_on_its_line_array(
self.fork_markers
.iter()
.map(|marker| SimplifiedMarkerTree::new(&self.requires_python, marker.clone()))
.map(|marker| {
// TODO(ag): Consider whether `resolution-markers` should actually
// include conflicting marker info. In which case, we should serialize
// the entire `UniversalMarker` (taking care to still make the PEP 508
// simplified).
SimplifiedMarkerTree::new(&self.requires_python, marker.pep508().clone())
})
.filter_map(|marker| marker.try_to_string()),
);
doc.insert("resolution-markers", value(fork_markers));
Expand Down Expand Up @@ -1434,6 +1444,9 @@ impl TryFrom<LockWire> for Lock {
.fork_markers
.into_iter()
.map(|simplified_marker| simplified_marker.into_marker(&wire.requires_python))
// TODO(ag): Consider whether this should also deserialize a conflict marker.
// We currently aren't serializing. Dropping it completely is likely to be wrong.
.map(|complexified_marker| UniversalMarker::new(complexified_marker, MarkerTree::TRUE))
.collect();
let lock = Lock::new(
wire.version,
Expand Down Expand Up @@ -1475,7 +1488,7 @@ pub struct Package {
/// the next resolution.
///
/// Named `resolution-markers` in `uv.lock`.
fork_markers: Vec<MarkerTree>,
fork_markers: Vec<UniversalMarker>,
/// The resolved dependencies of the package.
dependencies: Vec<Dependency>,
/// The resolved optional dependencies of the package.
Expand All @@ -1489,7 +1502,7 @@ pub struct Package {
impl Package {
fn from_annotated_dist(
annotated_dist: &AnnotatedDist,
fork_markers: Vec<MarkerTree>,
fork_markers: Vec<UniversalMarker>,
root: &Path,
) -> Result<Self, LockError> {
let id = PackageId::from_annotated_dist(annotated_dist, root)?;
Expand Down Expand Up @@ -1549,7 +1562,7 @@ impl Package {
&mut self,
requires_python: &RequiresPython,
annotated_dist: &AnnotatedDist,
marker: MarkerTree,
marker: UniversalMarker,
root: &Path,
) -> Result<(), LockError> {
let new_dep =
Expand Down Expand Up @@ -1595,7 +1608,7 @@ impl Package {
requires_python: &RequiresPython,
extra: ExtraName,
annotated_dist: &AnnotatedDist,
marker: MarkerTree,
marker: UniversalMarker,
root: &Path,
) -> Result<(), LockError> {
let dep = Dependency::from_annotated_dist(requires_python, annotated_dist, marker, root)?;
Expand All @@ -1621,7 +1634,7 @@ impl Package {
requires_python: &RequiresPython,
group: GroupName,
annotated_dist: &AnnotatedDist,
marker: MarkerTree,
marker: UniversalMarker,
root: &Path,
) -> Result<(), LockError> {
let dep = Dependency::from_annotated_dist(requires_python, annotated_dist, marker, root)?;
Expand Down Expand Up @@ -1957,7 +1970,13 @@ impl Package {
let wheels = each_element_on_its_line_array(
self.fork_markers
.iter()
.map(|marker| SimplifiedMarkerTree::new(requires_python, marker.clone()))
// TODO(ag): Consider whether `resolution-markers` should actually
// include conflicting marker info. In which case, we should serialize
// the entire `UniversalMarker` (taking care to still make the PEP 508
// simplified).
.map(|marker| {
SimplifiedMarkerTree::new(requires_python, marker.pep508().clone())
})
.filter_map(|marker| marker.try_to_string()),
);
table.insert("resolution-markers", value(wheels));
Expand Down Expand Up @@ -2112,7 +2131,7 @@ impl Package {
}

/// Return the fork markers for this package, if any.
pub fn fork_markers(&self) -> &[MarkerTree] {
pub fn fork_markers(&self) -> &[UniversalMarker] {
self.fork_markers.as_slice()
}

Expand Down Expand Up @@ -2223,6 +2242,11 @@ impl PackageWire {
.fork_markers
.into_iter()
.map(|simplified_marker| simplified_marker.into_marker(requires_python))
// TODO(ag): Consider whether this should also deserialize a conflict marker.
// We currently aren't serializing. Dropping it completely is likely to be wrong.
.map(|complexified_marker| {
UniversalMarker::new(complexified_marker, MarkerTree::TRUE)
})
.collect(),
dependencies: unwire_deps(self.dependencies)?,
optional_dependencies: self
Expand Down Expand Up @@ -3477,8 +3501,9 @@ impl TryFrom<WheelWire> for Wheel {
struct Dependency {
package_id: PackageId,
extra: BTreeSet<ExtraName>,
/// A marker simplified by assuming `requires-python` is satisfied.
/// So if `requires-python = '>=3.8'`, then
/// A marker simplified from the PEP 508 marker in `complexified_marker`
/// by assuming `requires-python` is satisfied. So if
/// `requires-python = '>=3.8'`, then
/// `python_version >= '3.8' and python_version < '3.12'`
/// gets simplfiied to `python_version < '3.12'`.
///
Expand All @@ -3496,21 +3521,21 @@ struct Dependency {
/// `requires-python` applies to the entire lock file, it's
/// acceptable to do comparisons on the simplified form.
simplified_marker: SimplifiedMarkerTree,
/// The "complexified" marker is a marker that can stand on its
/// own independent of `requires-python`. It can be safely used
/// for any kind of marker algebra.
complexified_marker: MarkerTree,
/// The "complexified" marker is a universal marker whose PEP 508
/// marker can stand on its own independent of `requires-python`.
/// It can be safely used for any kind of marker algebra.
complexified_marker: UniversalMarker,
}

impl Dependency {
fn new(
requires_python: &RequiresPython,
package_id: PackageId,
extra: BTreeSet<ExtraName>,
complexified_marker: MarkerTree,
complexified_marker: UniversalMarker,
) -> Dependency {
let simplified_marker =
SimplifiedMarkerTree::new(requires_python, complexified_marker.clone());
SimplifiedMarkerTree::new(requires_python, complexified_marker.pep508().clone());
Dependency {
package_id,
extra,
Expand All @@ -3522,7 +3547,7 @@ impl Dependency {
fn from_annotated_dist(
requires_python: &RequiresPython,
annotated_dist: &AnnotatedDist,
complexified_marker: MarkerTree,
complexified_marker: UniversalMarker,
root: &Path,
) -> Result<Dependency, LockError> {
let package_id = PackageId::from_annotated_dist(annotated_dist, root)?;
Expand Down Expand Up @@ -3590,6 +3615,7 @@ struct DependencyWire {
extra: BTreeSet<ExtraName>,
#[serde(default)]
marker: SimplifiedMarkerTree,
// FIXME: Add support for representing conflict markers.
}

impl DependencyWire {
Expand All @@ -3603,7 +3629,8 @@ impl DependencyWire {
package_id: self.package_id.unwire(unambiguous_package_ids)?,
extra: self.extra,
simplified_marker: self.marker,
complexified_marker,
// FIXME: Support reading conflict markers.
complexified_marker: UniversalMarker::new(complexified_marker, MarkerTree::TRUE),
})
}
}
Expand Down Expand Up @@ -4103,6 +4130,11 @@ enum LockErrorKind {
#[source]
err: DependencyGroupError,
},
/// An error that occurs when trying to export a `uv.lock` with
/// conflicting extras/groups specified to `requirements.txt`.
/// (Because `requirements.txt` cannot encode them.)
#[error("Cannot represent `uv.lock` with conflicting extras or groups as `requirements.txt`")]
ConflictsNotAllowedInRequirementsTxt,
}

/// An error that occurs when a source string could not be parsed.
Expand Down
Loading
Loading