Skip to content

Commit

Permalink
uv-resolver: support project-level conflicts
Browse files Browse the repository at this point in the history 
Supporting project level conflicts ends up being pretty tricky, mostly
because depenedency groups are represented as dependencies of the
project you're trying to declare a conflict for. So by filtering out the
project in the fork for the conflicting group, you end up filtering out
the group itself too.

To work-around this, we add a `parent` field to `PubGrubDependency`, and
use this to filter based on project conflicts. This lets us do "delayed"
filtering by one level.

The rest of the changes in this commit are for reporting errors
when you try to activate the group without disabling the project.
  • Loading branch information
@BurntSushi
BurntSushi committed Nov 14, 2024
1 parent 391ee0f commit 112eb32
Show file tree
Hide file tree
Showing 6 changed files with 263 additions and 27 deletions.
54 changes: 49 additions & 5 deletions crates/uv-resolver/src/pubgrub/dependencies.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ use tracing::warn;
use uv_normalize::{ExtraName, PackageName};
use uv_pep440::{Version, VersionSpecifiers};
use uv_pypi_types::{
ParsedArchiveUrl, ParsedDirectoryUrl, ParsedGitUrl, ParsedPathUrl, ParsedUrl, Requirement,
RequirementSource, VerbatimParsedUrl,
ConflictItemRef, ParsedArchiveUrl, ParsedDirectoryUrl, ParsedGitUrl, ParsedPathUrl, ParsedUrl,
Requirement, RequirementSource, VerbatimParsedUrl,
};

use crate::pubgrub::{PubGrubPackage, PubGrubPackageInner};
Expand All @@ -17,6 +17,21 @@ pub(crate) struct PubGrubDependency {
pub(crate) package: PubGrubPackage,
pub(crate) version: Ranges<Version>,

/// When the parent that created this dependency is a "normal" package
/// (non-extra non-group), this corresponds to its name.
///
/// This is used to create project-level `ConflictItemRef` for a specific
/// package. In effect, this lets us "delay" filtering of project
/// dependencies when a conflict is declared between the project and a
/// group.
///
/// The main problem with deal with project level conflicts is that if you
/// declare a conflict between a package and a group, we represent that
/// group as a dependency of that package. So if you filter out the package
/// in a fork due to a conflict, you also filter out the group. Therefore,
/// we introduce this parent field to enable "delayed" filtering.
pub(crate) parent: Option<PackageName>,

/// The original version specifiers from the requirement.
pub(crate) specifier: Option<VersionSpecifiers>,

Expand All @@ -29,8 +44,12 @@ pub(crate) struct PubGrubDependency {
impl PubGrubDependency {
pub(crate) fn from_requirement<'a>(
requirement: &'a Requirement,
source_name: Option<&'a PackageName>,
parent_package: Option<&'a PubGrubPackage>,
) -> impl Iterator<Item = Self> + 'a {
let parent_name = parent_package.and_then(|package| package.name_no_root());
let is_normal_parent = parent_package
.map(|pp| pp.extra().is_none() && pp.dev().is_none())
.unwrap_or(false);
// Add the package, plus any extra variants.
iter::once(None)
.chain(requirement.extras.clone().into_iter().map(Some))
Expand All @@ -45,32 +64,43 @@ impl PubGrubDependency {
match &*package {
PubGrubPackageInner::Package { name, .. } => {
// Detect self-dependencies.
if source_name.is_some_and(|source_name| source_name == name) {
if parent_name.is_some_and(|parent_name| parent_name == name) {
warn!("{name} has a dependency on itself");
return None;
}

Some(PubGrubDependency {
package: package.clone(),
version: version.clone(),
parent: if is_normal_parent {
parent_name.cloned()
} else {
None
},
specifier,
url,
})
}
PubGrubPackageInner::Marker { .. } => Some(PubGrubDependency {
package: package.clone(),
version: version.clone(),
parent: if is_normal_parent {
parent_name.cloned()
} else {
None
},
specifier,
url,
}),
PubGrubPackageInner::Extra { name, .. } => {
debug_assert!(
!source_name.is_some_and(|source_name| source_name == name),
!parent_name.is_some_and(|parent_name| parent_name == name),
"extras not flattened for {name}"
);
Some(PubGrubDependency {
package: package.clone(),
version: version.clone(),
parent: None,
specifier,
url,
})
Expand All @@ -79,6 +109,20 @@ impl PubGrubDependency {
}
})
}

/// Extracts a possible conflicting item from this dependency.
///
/// If this package can't possibly be classified as conflicting, then this
/// returns `None`.
pub(crate) fn conflicting_item(&self) -> Option<ConflictItemRef<'_>> {
if let Some(conflict) = self.package.conflicting_item() {
return Some(conflict);
}
if let Some(ref parent) = self.parent {
return Some(ConflictItemRef::from(parent));
}
None
}
}

/// A PubGrub-compatible package and version range.
Expand Down
7 changes: 4 additions & 3 deletions crates/uv-resolver/src/pubgrub/package.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,13 +208,14 @@ impl PubGrubPackage {
}
}

/// Extracts a possible conflicting group from this package.
/// Extracts a possible conflicting item from this package.
///
/// If this package can't possibly be classified as a conflicting group,
/// then this returns `None`.
/// If this package can't possibly be classified as a conflicting, then
/// this returns `None`.
pub(crate) fn conflicting_item(&self) -> Option<ConflictItemRef<'_>> {
let package = self.name_no_root()?;
match (self.extra(), self.dev()) {
// (None, None) => Some(ConflictItemRef::from(package)),
(None, None) => None,
(Some(extra), None) => Some(ConflictItemRef::from((package, extra))),
(None, Some(group)) => Some(ConflictItemRef::from((package, group))),
Expand Down
17 changes: 12 additions & 5 deletions crates/uv-resolver/src/resolver/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -547,6 +547,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
let PubGrubDependency {
package,
version: _,
parent: _,
specifier: _,
url: _,
} = dependency;
Expand Down Expand Up @@ -715,6 +716,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
let PubGrubDependency {
package,
version: _,
parent: _,
specifier: _,
url: _,
} = dependency;
Expand Down Expand Up @@ -1409,7 +1411,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
let mut dependencies: Vec<_> = requirements
.iter()
.flat_map(|requirement| {
PubGrubDependency::from_requirement(requirement, Some(name))
PubGrubDependency::from_requirement(requirement, Some(package))
})
.collect();

Expand All @@ -1428,6 +1430,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
marker: marker.clone(),
}),
version: Range::singleton(version.clone()),
parent: None,
specifier: None,
url: None,
});
Expand All @@ -1451,6 +1454,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
marker: marker.and_then(MarkerTree::contents),
}),
version: Range::singleton(version.clone()),
parent: None,
specifier: None,
url: None,
})
Expand Down Expand Up @@ -1479,6 +1483,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
marker: marker.cloned(),
}),
version: Range::singleton(version.clone()),
parent: None,
specifier: None,
url: None,
})
Expand All @@ -1505,6 +1510,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
marker: marker.cloned(),
}),
version: Range::singleton(version.clone()),
parent: None,
specifier: None,
url: None,
})
Expand Down Expand Up @@ -2259,6 +2265,7 @@ impl ForkState {
let PubGrubDependency {
package,
version,
parent: _,
specifier: _,
url: _,
} = dependency;
Expand Down Expand Up @@ -2978,7 +2985,7 @@ impl Fork {

/// Add a dependency to this fork.
fn add_dependency(&mut self, dep: PubGrubDependency) {
if let Some(conflicting_item) = dep.package.conflicting_item() {
if let Some(conflicting_item) = dep.conflicting_item() {
self.conflicts.insert(conflicting_item.to_owned());
}
self.dependencies.push(dep);
Expand All @@ -2997,7 +3004,7 @@ impl Fork {
if self.env.included_by_marker(markers) {
return true;
}
if let Some(conflicting_item) = dep.package.conflicting_item() {
if let Some(conflicting_item) = dep.conflicting_item() {
self.conflicts.remove(&conflicting_item);
}
false
Expand All @@ -3016,13 +3023,13 @@ impl Fork {
fn exclude(mut self, groups: impl IntoIterator<Item = ConflictItem>) -> Fork {
self.env = self.env.exclude_by_group(groups);
self.dependencies.retain(|dep| {
let Some(conflicting_item) = dep.package.conflicting_item() else {
let Some(conflicting_item) = dep.conflicting_item() else {
return true;
};
if self.env.included_by_group(conflicting_item) {
return true;
}
if let Some(conflicting_item) = dep.package.conflicting_item() {
if let Some(conflicting_item) = dep.conflicting_item() {
self.conflicts.remove(&conflicting_item);
}
false
Expand Down
4 changes: 2 additions & 2 deletions crates/uv/src/commands/project/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ pub(crate) enum ProjectError {
"{} are incompatible with the declared conflicts: {{{}}}",
_1.iter().map(|conflict| {
match conflict {
ConflictKind::Project => format!("project"),
ConflictKind::Project => format!("the project"),
ConflictKind::Extra(ref extra) => format!("extra `{extra}`"),
ConflictKind::Group(ref group) => format!("group `{group}`"),
}
Expand All @@ -94,7 +94,7 @@ pub(crate) enum ProjectError {
.iter()
.map(|item| {
match item.kind() {
ConflictKind::Project => format!("project"),
ConflictKind::Project => format!("the project"),
ConflictKind::Extra(ref extra) => format!("`{}[{}]`", item.package(), extra),
ConflictKind::Group(ref group) => format!("`{}:{}`", item.package(), group),
}
Expand Down
18 changes: 6 additions & 12 deletions crates/uv/src/commands/project/sync.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -291,18 +291,12 @@ pub(super) async fn do_sync(
for set in conflicts.iter() {
let mut conflicts: Vec<ConflictKind> = vec![];
for item in set.iter() {
if item
.extra()
.map(|extra| extras.contains(extra))
.unwrap_or(false)
{
conflicts.push(item.kind().clone());
}
if item
.group()
.map(|group1| dev.iter().any(|group2| group1 == group2))
.unwrap_or(false)
{
let is_conflicting = match *item.kind() {
ConflictKind::Project => dev.prod(),
ConflictKind::Extra(ref extra) => extras.contains(extra),
ConflictKind::Group(ref group1) => dev.iter().any(|group2| group1 == group2),
};
if is_conflicting {
conflicts.push(item.kind().clone());
}
}
Expand Down
Loading

0 comments on commit 112eb32

Please sign in to comment.