diff --git a/crates/puffin-resolver/src/pubgrub/dependencies.rs b/crates/puffin-resolver/src/pubgrub/dependencies.rs index 2538021df..4b1ed9bb5 100644 --- a/crates/puffin-resolver/src/pubgrub/dependencies.rs +++ b/crates/puffin-resolver/src/pubgrub/dependencies.rs @@ -1,10 +1,9 @@ -use fxhash::FxHashMap; use itertools::Itertools; use pubgrub::range::Range; use tracing::warn; use pep508_rs::{MarkerEnvironment, Requirement, VersionOrUrl}; -use puffin_cache::CanonicalUrl; + use puffin_normalize::{ExtraName, PackageName}; use crate::pubgrub::specifier::PubGrubSpecifier; @@ -57,21 +56,6 @@ impl PubGrubDependencies { let (package, version) = result?; dependencies.push((package.clone(), version.clone())); - - // if let Some(entry) = dependencies.get_key_value(&package) { - // // Merge the versions. - // let version = merge_versions(entry.1, &version); - - // // Merge the package. - // if let Some(package) = merge_package(entry.0, &package)? { - // dependencies.remove(&package); - // dependencies.insert(package, version); - // } else { - // dependencies.insert(package, version); - // } - // } else { - // dependencies.insert(package.clone(), version.clone()); - // } } } @@ -107,18 +91,6 @@ impl PubGrubDependencies { let (package, version) = result?; dependencies.push((package.clone(), version.clone())); - - // if let Some(entry) = dependencies.get_key_value(&package) { - // // Merge the versions. - // let version = merge_versions(entry.1, &version); - - // // Merge the package. - // if let Some(package) = merge_package(entry.0, &package)? { - // dependencies.insert(package, version); - // } else { - // dependencies.insert(package, version); - // } - // } } } @@ -151,12 +123,12 @@ fn to_pubgrub( match requirement.version_or_url.as_ref() { // The requirement has no specifier (e.g., `flask`). None => Ok(( - PubGrubPackage::Package(requirement.name.clone(), extra, None), + PubGrubPackage::Package(requirement.name.clone(), extra), Range::full(), )), // The requirement has a URL (e.g., `flask @ file:///path/to/flask`). Some(VersionOrUrl::Url(url)) => Ok(( - PubGrubPackage::Package(requirement.name.clone(), extra, Some(url.clone())), + PubGrubPackage::UrlPackage(requirement.name.clone(), extra, url.clone()), Range::full(), )), // The requirement has a specifier (e.g., `flask>=1.0`). @@ -168,67 +140,9 @@ fn to_pubgrub( range.intersection(&specifier.into()) })?; Ok(( - PubGrubPackage::Package(requirement.name.clone(), extra, None), + PubGrubPackage::Package(requirement.name.clone(), extra), version, )) } } } - -/// Merge two [`PubGrubVersion`] ranges. -fn merge_versions( - left: &Range, - right: &Range, -) -> Range { - left.intersection(right) -} - -/// Merge two [`PubGrubPackage`] instances. -fn merge_package( - left: &PubGrubPackage, - right: &PubGrubPackage, -) -> Result, ResolveError> { - match (left, right) { - // Either package is `root`. - (PubGrubPackage::Root(_), _) | (_, PubGrubPackage::Root(_)) => Ok(None), - - // Left package has a URL. Propagate the URL. - (PubGrubPackage::Package(name, extra, Some(url)), PubGrubPackage::Package(.., None)) => { - Ok(Some(PubGrubPackage::Package( - name.clone(), - extra.clone(), - Some(url.clone()), - ))) - } - - // Right package has a URL. - (PubGrubPackage::Package(.., None), PubGrubPackage::Package(name, extra, Some(url))) => { - Ok(Some(PubGrubPackage::Package( - name.clone(), - extra.clone(), - Some(url.clone()), - ))) - } - - // Neither package has a URL. - (PubGrubPackage::Package(_name, _extra, None), PubGrubPackage::Package(.., None)) => { - Ok(None) - } - - // Both packages have a URL. - ( - PubGrubPackage::Package(name, _extra, Some(left)), - PubGrubPackage::Package(.., Some(right)), - ) => { - if CanonicalUrl::new(left) == CanonicalUrl::new(right) { - Ok(None) - } else { - Err(ResolveError::ConflictingUrls( - name.clone(), - left.to_string(), - right.to_string(), - )) - } - } - } -} diff --git a/crates/puffin-resolver/src/pubgrub/package.rs b/crates/puffin-resolver/src/pubgrub/package.rs index 0e08936fa..ca3404629 100644 --- a/crates/puffin-resolver/src/pubgrub/package.rs +++ b/crates/puffin-resolver/src/pubgrub/package.rs @@ -14,7 +14,8 @@ use puffin_normalize::{ExtraName, PackageName}; #[derivative(PartialEq, Hash)] pub enum PubGrubPackage { Root(Option), - Package( + Package(PackageName, Option), + UrlPackage( PackageName, Option, /// The URL of the package, if it was specified in the requirement. @@ -66,7 +67,7 @@ pub enum PubGrubPackage { #[derivative(PartialEq = "ignore")] #[derivative(PartialOrd = "ignore")] #[derivative(Hash = "ignore")] - Option, + Url, ), } @@ -80,8 +81,13 @@ impl std::fmt::Display for PubGrubPackage { write!(f, "root") } } - PubGrubPackage::Package(name, None, ..) => write!(f, "{name}"), - PubGrubPackage::Package(name, Some(extra), ..) => { + PubGrubPackage::Package(name, None) => write!(f, "{name}"), + PubGrubPackage::Package(name, Some(extra)) => { + write!(f, "{name}[{extra}]") + } + + PubGrubPackage::UrlPackage(name, None, ..) => write!(f, "{name}"), + PubGrubPackage::UrlPackage(name, Some(extra), ..) => { write!(f, "{name}[{extra}]") } } diff --git a/crates/puffin-resolver/src/pubgrub/priority.rs b/crates/puffin-resolver/src/pubgrub/priority.rs index dc8213bcc..d7b2af048 100644 --- a/crates/puffin-resolver/src/pubgrub/priority.rs +++ b/crates/puffin-resolver/src/pubgrub/priority.rs @@ -20,7 +20,7 @@ impl PubGrubPriorities { pub(crate) fn get(&self, package: &PubGrubPackage) -> Option { match package { PubGrubPackage::Root(_) => Some(Reverse(0)), - PubGrubPackage::Package(name, _, _) => self + PubGrubPackage::Package(name, ..) | PubGrubPackage::UrlPackage(name, ..) => self .0 .get(name) .copied() diff --git a/crates/puffin-resolver/src/pubgrub/report.rs b/crates/puffin-resolver/src/pubgrub/report.rs index 3565205a2..e4693e351 100644 --- a/crates/puffin-resolver/src/pubgrub/report.rs +++ b/crates/puffin-resolver/src/pubgrub/report.rs @@ -269,10 +269,16 @@ impl ResolutionFailureReporter { let terms_vec: Vec<_> = terms.iter().collect(); match terms_vec.as_slice() { [] | [(PubGrubPackage::Root(_), _)] => "version solving failed".into(), - [(package @ PubGrubPackage::Package(..), Term::Positive(range))] => { + [( + package @ (PubGrubPackage::Package(..) | PubGrubPackage::UrlPackage(..)), + Term::Positive(range), + )] => { format!("{package}{range} is forbidden") } - [(package @ PubGrubPackage::Package(..), Term::Negative(range))] => { + [( + package @ (PubGrubPackage::Package(..) | PubGrubPackage::UrlPackage(..)), + Term::Negative(range), + )] => { format!("{package}{range} is mandatory") } [(p1, Term::Positive(r1)), (p2, Term::Negative(r2))] => { diff --git a/crates/puffin-resolver/src/resolution.rs b/crates/puffin-resolver/src/resolution.rs index 947c83ad2..316357d1f 100644 --- a/crates/puffin-resolver/src/resolution.rs +++ b/crates/puffin-resolver/src/resolution.rs @@ -70,7 +70,7 @@ impl Graph { FxHashMap::with_capacity_and_hasher(selection.len(), BuildHasherDefault::default()); for (package, version) in selection { match package { - PubGrubPackage::Package(package_name, None, None) => { + PubGrubPackage::Package(package_name, None) => { let version = Version::from(version.clone()); let file = pins .get(package_name) @@ -83,7 +83,7 @@ impl Graph { let index = graph.add_node(pinned_package); inverse.insert(package_name, index); } - PubGrubPackage::Package(package_name, None, Some(url)) => { + PubGrubPackage::UrlPackage(package_name, None, url) => { let url = redirects .get(url) .map_or_else(|| url.clone(), |url| url.value().clone()); @@ -103,10 +103,13 @@ impl Graph { if let Kind::FromDependencyOf(self_package, self_version, dependency_package, _) = &state.incompatibility_store[*id].kind { - let PubGrubPackage::Package(self_package, None, _) = self_package else { + let (PubGrubPackage::Package(self_package, None) + | PubGrubPackage::UrlPackage(self_package, None, _)) = self_package + else { continue; }; - let PubGrubPackage::Package(dependency_package, None, _) = dependency_package + let (PubGrubPackage::Package(dependency_package, None) + | PubGrubPackage::UrlPackage(dependency_package, None, _)) = dependency_package else { continue; }; diff --git a/crates/puffin-resolver/src/resolver.rs b/crates/puffin-resolver/src/resolver.rs index 90fe5e172..83154d565 100644 --- a/crates/puffin-resolver/src/resolver.rs +++ b/crates/puffin-resolver/src/resolver.rs @@ -9,6 +9,7 @@ use futures::channel::mpsc::UnboundedReceiver; use futures::{pin_mut, FutureExt, StreamExt, TryFutureExt}; use fxhash::{FxHashMap, FxHashSet}; use pubgrub::error::PubGrubError; + use pubgrub::range::Range; use pubgrub::solver::{Incompatibility, State}; use tokio::select; @@ -281,14 +282,14 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> { ) -> Result<(), ResolveError> { match package { PubGrubPackage::Root(_) => {} - PubGrubPackage::Package(package_name, _extra, None) => { + PubGrubPackage::Package(package_name, _extra) => { // Emit a request to fetch the metadata for this package. if in_flight.insert_package(package_name) { priorities.add(package_name.clone()); request_sink.unbounded_send(Request::Package(package_name.clone()))?; } } - PubGrubPackage::Package(package_name, _extra, Some(url)) => { + PubGrubPackage::UrlPackage(package_name, _extra, url) => { // Emit a request to fetch the metadata for this package. if in_flight.insert_url(url) { priorities.add(package_name.clone()); @@ -318,7 +319,7 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> { // Iterate over the potential packages, and fetch file metadata for any of them. These // represent our current best guesses for the versions that we _might_ select. for (package, range) in packages { - let PubGrubPackage::Package(package_name, _extra, None) = package else { + let PubGrubPackage::Package(package_name, _extra) = package else { continue; }; @@ -372,7 +373,7 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> { return match package { PubGrubPackage::Root(_) => Ok(Some(MIN_VERSION.clone())), - PubGrubPackage::Package(package_name, _extra, Some(url)) => { + PubGrubPackage::UrlPackage(package_name, _extra, url) => { debug!("Searching for a compatible version of {package_name} @ {url} ({range})",); // If the URL wasn't declared in the direct dependencies or constraints, reject it. @@ -404,7 +405,7 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> { } } - PubGrubPackage::Package(package_name, _extra, None) => { + PubGrubPackage::Package(package_name, _extra) => { // Wait for the metadata to be available. let entry = self.index.packages.wait(package_name).await.unwrap(); let version_map = entry.value(); @@ -470,7 +471,7 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> { in_flight: &mut InFlight, request_sink: &futures::channel::mpsc::UnboundedSender, ) -> Result { - match package { + let (package_name, extra, entry) = match package { PubGrubPackage::Root(_) => { // Add the root requirements. let constraints = PubGrubDependencies::try_from_requirements( @@ -488,53 +489,53 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> { Self::visit_package(package, priorities, in_flight, request_sink)?; } - Ok(Dependencies::Known(constraints.into())) + return Ok(Dependencies::Known(constraints.into())); } - - PubGrubPackage::Package(package_name, extra, url) => { + PubGrubPackage::Package(package_name, extra) => { // Wait for the metadata to be available. - let entry = match url { - Some(url) => self.index.versions.wait(url.as_str()).await.unwrap(), - None => { - let versions = pins.get(package_name).unwrap(); - let file = versions.get(version.into()).unwrap(); - self.index.versions.wait(&file.hashes.sha256).await.unwrap() - } - }; - let metadata = entry.value(); - - let mut constraints = PubGrubDependencies::try_from_requirements( - &metadata.requires_dist, - &self.constraints, - extra.as_ref(), - Some(package_name), - self.markers, - )?; - - for (package, version) in constraints.iter() { - debug!("Adding transitive dependency: {package} {version}"); - - // Emit a request to fetch the metadata for this package. - Self::visit_package(package, priorities, in_flight, request_sink)?; - } - - if let Some(extra) = extra { - if !metadata - .provides_extras - .iter() - .any(|provided_extra| provided_extra == extra) - { - return Ok(Dependencies::Unknown); - } - constraints.insert( - PubGrubPackage::Package(package_name.clone(), None, None), - Range::singleton(version.clone()), - ); - } - - Ok(Dependencies::Known(constraints.into())) + let versions = pins.get(package_name).unwrap(); + let file = versions.get(version.into()).unwrap(); + let entry = self.index.versions.wait(&file.hashes.sha256).await.unwrap(); + (package_name, extra, entry) } + PubGrubPackage::UrlPackage(package_name, extra, url) => { + let entry = self.index.versions.wait(url.as_str()).await.unwrap(); + (package_name, extra, entry) + } + }; + + let metadata = entry.value(); + + let mut constraints = PubGrubDependencies::try_from_requirements( + &metadata.requires_dist, + &self.constraints, + extra.as_ref(), + Some(package_name), + self.markers, + )?; + + for (package, version) in constraints.iter() { + debug!("Adding transitive dependency: {package} {version}"); + + // Emit a request to fetch the metadata for this package. + Self::visit_package(package, priorities, in_flight, request_sink)?; } + + if let Some(extra) = extra { + if !metadata + .provides_extras + .iter() + .any(|provided_extra| provided_extra == extra) + { + return Ok(Dependencies::Unknown); + } + constraints.insert( + PubGrubPackage::Package(package_name.clone(), None), + Range::singleton(version.clone()), + ); + } + + Ok(Dependencies::Known(constraints.into())) } /// Fetch the metadata for a stream of packages and versions. @@ -808,10 +809,10 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> { if let Some(reporter) = self.reporter.as_ref() { match package { PubGrubPackage::Root(_) => {} - PubGrubPackage::Package(package_name, extra, Some(url)) => { + PubGrubPackage::UrlPackage(package_name, extra, url) => { reporter.on_progress(package_name, extra.as_ref(), VersionOrUrl::Url(url)); } - PubGrubPackage::Package(package_name, extra, None) => { + PubGrubPackage::Package(package_name, extra) => { reporter.on_progress( package_name, extra.as_ref(),