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

Add an UnusableDependencies incompatibility kind and use for conflicting versions #424

Merged
merged 6 commits into from
Nov 16, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
18 changes: 10 additions & 8 deletions crates/puffin-cli/tests/pip_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -666,14 +666,16 @@ fn compile_numpy_py38() -> Result<()> {
.arg("--no-build")
.env("VIRTUAL_ENV", venv.as_os_str())
.current_dir(&temp_dir), @r###"
success: false
exit_code: 2
----- stdout -----

----- stderr -----
error: Failed to build distribution: numpy-1.24.4.tar.gz
Caused by: Building source distributions is disabled
"###);
success: true
exit_code: 0
----- stdout -----
# This file was autogenerated by Puffin v0.0.1 via the following command:
# [BIN_PATH] pip-compile requirements.in --python-version py38 --cache-dir [CACHE_DIR]
numpy==1.24.4

----- stderr -----
Resolved 1 package in [TIME]
"###);
});

Ok(())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,16 @@ info:
- pip-compile
- pyproject.toml
- "--cache-dir"
- /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmpN531dN
- /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmpvGljPp
env:
VIRTUAL_ENV: /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmp99w9dK/.venv
VIRTUAL_ENV: /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmp2rBXR2/.venv
---
success: false
exit_code: 1
----- stdout -----

----- stderr -----
× No solution found when resolving dependencies:
╰─▶ my-project depends on django∅
╰─▶ my-project dependencies are unusable: `django==5.0b1` does not intersect
with `django==5.0a1`

3 changes: 3 additions & 0 deletions crates/puffin-resolver/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ pub enum ResolveError {
#[error("Conflicting URLs for package `{0}`: {1} and {2}")]
ConflictingUrls(PackageName, String, String),

#[error("`{0}{1}` does not intersect with `{0}{2}`")]
ConflictingVersions(PubGrubPackage, Range<PubGrubVersion>, Range<PubGrubVersion>),

#[error("Package `{0}` attempted to resolve via URL: {1}. URL dependencies must be expressed as direct requirements or constraints. Consider adding `{0} @ {1}` to your dependencies or constraints file.")]
DisallowedUrl(PackageName, Url),

Expand Down
18 changes: 14 additions & 4 deletions crates/puffin-resolver/src/pubgrub/dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ impl PubGrubDependencies {

if let Some(entry) = dependencies.get_key_value(&package) {
// Merge the versions.
let version = merge_versions(entry.1, &version);
let version = merge_versions(&package, entry.1, &version)?;

// Merge the package.
if let Some(package) = merge_package(entry.0, &package)? {
Expand Down Expand Up @@ -107,7 +107,7 @@ impl PubGrubDependencies {

if let Some(entry) = dependencies.get_key_value(&package) {
// Merge the versions.
let version = merge_versions(entry.1, &version);
let version = merge_versions(&package, entry.1, &version)?;

// Merge the package.
if let Some(package) = merge_package(entry.0, &package)? {
Expand Down Expand Up @@ -178,10 +178,20 @@ fn to_pubgrub(

/// Merge two [`PubGrubVersion`] ranges.
fn merge_versions(
package: &PubGrubPackage,
left: &Range<PubGrubVersion>,
right: &Range<PubGrubVersion>,
) -> Range<PubGrubVersion> {
left.intersection(right)
) -> Result<Range<PubGrubVersion>, ResolveError> {
let result = left.intersection(right);
if result.is_empty() {
Err(ResolveError::ConflictingVersions(
package.clone(),
left.clone(),
right.clone(),
))
} else {
Ok(result)
}
}

/// Merge two [`PubGrubPackage`] instances.
Expand Down
25 changes: 25 additions & 0 deletions crates/puffin-resolver/src/pubgrub/report.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::fmt;

use pubgrub::package::Package;
use pubgrub::range::Range;
use pubgrub::report::{DerivationTree, Derived, External, Reporter};
use pubgrub::term::Term;
Expand Down Expand Up @@ -345,6 +346,8 @@ enum PuffinExternal {
NoVersions(PubGrubPackage, Range<PubGrubVersion>),
/// Dependencies of the package are unavailable for versions in that set.
UnavailableDependencies(PubGrubPackage, Range<PubGrubVersion>),
/// Dependencies of the package are unusable for versions in that set.
UnusableDependencies(PubGrubPackage, Range<PubGrubVersion>, Option<String>),
/// Incompatibility coming from the dependencies of a given package.
FromDependencyOf(
PubGrubPackage,
Expand All @@ -362,6 +365,9 @@ impl PuffinExternal {
External::UnavailableDependencies(p, vs) => {
PuffinExternal::UnavailableDependencies(p, vs)
}
External::UnusableDependencies(p, vs, reason) => {
PuffinExternal::UnusableDependencies(p, vs, reason)
}
External::FromDependencyOf(p, vs, p_dep, vs_dep) => {
PuffinExternal::FromDependencyOf(p, vs, p_dep, vs_dep)
}
Expand Down Expand Up @@ -395,6 +401,25 @@ impl fmt::Display for PuffinExternal {
)
}
}
Self::UnusableDependencies(package, set, reason) => {
if let Some(reason) = reason {
if matches!(package, PubGrubPackage::Root(_)) {
write!(f, "{package} dependencies are unusable: {reason}")
} else {
if set == &Range::full() {
write!(f, "dependencies of {package} are unusable: {reason}")
} else {
write!(f, "dependencies of {package}{set} are unusable: {reason}",)
}
}
} else {
if set == &Range::full() {
write!(f, "dependencies of {package} are unusable")
} else {
write!(f, "dependencies of {package}{set} are unusable")
}
}
}
Self::FromDependencyOf(package, package_set, dependency, dependency_set) => {
if package_set == &Range::full() && dependency_set == &Range::full() {
write!(f, "{package} depends on {dependency}")
Expand Down
16 changes: 15 additions & 1 deletion crates/puffin-resolver/src/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,14 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
));
continue;
}
Dependencies::Unusable(reason) => {
state.add_incompatibility(Incompatibility::unusable_dependencies(
package.clone(),
version.clone(),
reason.clone(),
));
continue;
}
Dependencies::Known(constraints) if constraints.contains_key(package) => {
return Err(PubGrubError::SelfDependency {
package: package.clone(),
Expand Down Expand Up @@ -455,7 +463,11 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
None,
None,
self.markers,
)?;
);
if let Err(err @ ResolveError::ConflictingVersions(..)) = constraints {
return Ok(Dependencies::Unusable(Some(format!("{err}"))));
}
let constraints = constraints?;

for (package, version) in constraints.iter() {
debug!("Adding direct dependency: {package}{version}");
Expand Down Expand Up @@ -910,6 +922,8 @@ impl<'a> FromIterator<&'a Url> for AllowedUrls {
enum Dependencies {
/// Package dependencies are unavailable.
Unknown,
/// Package dependencies are not usable
Unusable(Option<String>),
/// Container for all available package versions.
Known(DependencyConstraints<PubGrubPackage, Range<PubGrubVersion>>),
}
16 changes: 16 additions & 0 deletions vendor/pubgrub/src/internal/incompatibility.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ pub enum Kind<P: Package, VS: VersionSet> {
NoVersions(P, VS),
/// Dependencies of the package are unavailable for versions in that range.
UnavailableDependencies(P, VS),
/// Dependencies of the package are unusable for versions in that range.
UnusableDependencies(P, VS, Option<String>),
/// Incompatibility coming from the dependencies of a given package.
FromDependencyOf(P, VS, P, VS),
/// Derived from two causes. Stores cause ids.
Expand Down Expand Up @@ -104,6 +106,17 @@ impl<P: Package, VS: VersionSet> Incompatibility<P, VS> {
}
}

/// Create an incompatibility to remember
/// that a package version is not selectable
/// because its dependencies are not usable.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line breaks here make me read this like a haiku 😂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me too haha I just kept the existing style but it's funny

pub fn unusable_dependencies(package: P, version: VS::V, reason: Option<String>) -> Self {
let set = VS::singleton(version);
Self {
package_terms: SmallMap::One([(package.clone(), Term::Positive(set.clone()))]),
kind: Kind::UnusableDependencies(package, set, reason),
}
}

/// Build an incompatibility from a given dependency.
pub fn from_dependency(package: P, version: VS::V, dep: (&P, &VS)) -> Self {
let set1 = VS::singleton(version);
Expand Down Expand Up @@ -206,6 +219,9 @@ impl<P: Package, VS: VersionSet> Incompatibility<P, VS> {
Kind::UnavailableDependencies(package, set) => DerivationTree::External(
External::UnavailableDependencies(package.clone(), set.clone()),
),
Kind::UnusableDependencies(package, set, reason) => DerivationTree::External(
External::UnusableDependencies(package.clone(), set.clone(), reason.clone()),
),
Kind::FromDependencyOf(package, set, dep_package, dep_set) => {
DerivationTree::External(External::FromDependencyOf(
package.clone(),
Expand Down
4 changes: 4 additions & 0 deletions vendor/pubgrub/src/range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,10 @@ impl<V> Range<V> {
segments: SmallVec::one((Included(v1.into()), Excluded(v2.into()))),
}
}

pub fn is_empty(&self) -> bool {
self.segments.is_empty()
}
}

impl<V: Clone> Range<V> {
Expand Down
32 changes: 32 additions & 0 deletions vendor/pubgrub/src/report.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ pub enum External<P: Package, VS: VersionSet> {
NoVersions(P, VS),
/// Dependencies of the package are unavailable for versions in that set.
UnavailableDependencies(P, VS),
/// Dependencies of the package are unusable for versions in that set.
UnusableDependencies(P, VS, Option<String>),
/// Incompatibility coming from the dependencies of a given package.
FromDependencyOf(P, VS, P, VS),
}
Expand Down Expand Up @@ -113,6 +115,13 @@ impl<P: Package, VS: VersionSet> DerivationTree<P, VS> {
DerivationTree::External(External::UnavailableDependencies(_, r)) => Some(
DerivationTree::External(External::UnavailableDependencies(package, set.union(&r))),
),
DerivationTree::External(External::UnusableDependencies(_, r, reason)) => {
Some(DerivationTree::External(External::UnusableDependencies(
package,
set.union(&r),
reason,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 this is probably not quite right because we shouldn't merge the reasons here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you say a bit more about this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it matters, but this merges the two incompatibilities by taking the union of their version ranges and one of the reasons but if the reasons are different we should not merge them because they are distinct incompatibilities. Or.. we could merge them and say "{reason} and {reason}" but that seems unclear later on since you lose the reason for each range.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we could make the reason it's own enum with its own merge semantics, but that requires encoding the set of reasons in the PubGrub crate.

)))
}
DerivationTree::External(External::FromDependencyOf(p1, r1, p2, r2)) => {
if p1 == package {
Some(DerivationTree::External(External::FromDependencyOf(
Expand Down Expand Up @@ -158,6 +167,29 @@ impl<P: Package, VS: VersionSet> fmt::Display for External<P, VS> {
)
}
}
Self::UnusableDependencies(package, set, reason) => {
if let Some(reason) = reason {
if set == &VS::full() {
write!(f, "dependencies of {} are unusable: {reason}", package)
} else {
write!(
f,
"dependencies of {} at version {} are unusable: {reason}",
package, set
)
}
} else {
if set == &VS::full() {
write!(f, "dependencies of {} are unusable", package)
} else {
write!(
f,
"dependencies of {} at version {} are unusable",
package, set
)
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems generic enough that it could feasibly be upstreamed 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self::FromDependencyOf(p, set_p, dep, set_dep) => {
if set_p == &VS::full() && set_dep == &VS::full() {
write!(f, "{} depends on {}", p, dep)
Expand Down
Loading