Skip to content

Commit

Permalink
Add package name to PubGrubPackage::Url and only create dummy depen…
Browse files Browse the repository at this point in the history
…dencies for the root package

Attaching dummy dependencies to child packages does not help us because child packages with distinct urls are not considered unique by pubgrub
  • Loading branch information
zanieb committed Nov 9, 2023
1 parent e99af01 commit 5d62b28
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 22 deletions.
10 changes: 8 additions & 2 deletions crates/puffin-resolver/src/pubgrub/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,13 @@ pub enum PubGrubPackage {
#[derivative(Hash = "ignore")]
Option<Url>,
),
Url(Url),
Url(
PackageName,
#[derivative(PartialEq = "ignore")]
#[derivative(PartialOrd = "ignore")]
#[derivative(Hash = "ignore")]
Url,
),
}

impl std::fmt::Display for PubGrubPackage {
Expand All @@ -85,7 +91,7 @@ impl std::fmt::Display for PubGrubPackage {
PubGrubPackage::Package(name, Some(extra), ..) => {
write!(f, "{name}[{extra}]")
}
PubGrubPackage::Url(url) => write!(f, "{url}"),
PubGrubPackage::Url(name, url) => write!(f, "{name} @ {url}"),
}
}
}
4 changes: 2 additions & 2 deletions crates/puffin-resolver/src/pubgrub/priority.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::{cmp::Reverse};
use std::cmp::Reverse;

use fxhash::FxHashMap;

Expand Down Expand Up @@ -26,7 +26,7 @@ impl PubGrubPriorities {
.copied()
.map(|priority| priority + 1)
.map(Reverse),
PubGrubPackage::Url(_) => None,
PubGrubPackage::Url(..) => None,
}
}
}
Expand Down
42 changes: 24 additions & 18 deletions crates/puffin-resolver/src/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
}
}
}
PubGrubPackage::Url(_) => {}
PubGrubPackage::Url(..) => {}
}
Ok(())
}
Expand Down Expand Up @@ -465,7 +465,7 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
Ok(Some(version))
}

PubGrubPackage::Url(_) => {
PubGrubPackage::Url(..) => {
// TODO(zanieb): This should always be true but it's not enforced by the types
let Some((Bound::Included(v), Bound::Included(_))) = range.bounding_range() else {
return Ok(None);
Expand All @@ -489,21 +489,40 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
match package {
PubGrubPackage::Root(_) => {
// Add the root requirements.
let constraints = PubGrubDependencies::try_from_requirements(
let mut constraints = PubGrubDependencies::try_from_requirements(
&self.requirements,
&self.constraints,
None,
None,
self.markers,
)?;

let mut urls = Vec::new();
for (package, version) in constraints.iter() {
debug!("Adding direct dependency: {package:?} {version:?}");

if let PubGrubPackage::Package(package_name, _, Some(url)) = package {
let package = PubGrubPackage::Url(package_name.clone(), url.clone());
let length = url_versions.len();
let version = url_versions
.entry(url.clone())
.or_insert(Version::from_release(vec![length]));

debug!(
"Adding dummy dependency to root for {package} with unique key {version}"
);
urls.push((package, Range::singleton(version.clone())));
}

// Emit a request to fetch the metadata for this package.
Self::visit_package(package, priorities, in_flight, request_sink)?;
}

for item in urls {
let (package, version) = item;
constraints.push(package, version);
}

Ok(Dependencies::Known(constraints.into()))
}

Expand Down Expand Up @@ -534,19 +553,6 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
Self::visit_package(package, priorities, in_flight, request_sink)?;
}

if let Some(url) = url {
let package = PubGrubPackage::Url(url.clone());
let length = url_versions.len();
let version = url_versions
.entry(url.clone())
.or_insert(Version::from_release(vec![length]));

debug!(
"Adding dummy dependency to {package_name} @ {package} with url-version{version}"
);
constraints.push(package, Range::singleton(version.clone()));
}

if let Some(extra) = extra {
if !metadata
.provides_extras
Expand All @@ -565,7 +571,7 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
}
// TODO(zanieb): Improve documentation here
// Return an empty set for dummy url packages
PubGrubPackage::Url(_) => Ok(Dependencies::Known(Vec::new())),
PubGrubPackage::Url(..) => Ok(Dependencies::Known(Vec::new())),
}
}

Expand Down Expand Up @@ -851,7 +857,7 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> {
);
}
// TODO(zanieb): Determine if progress should be reported here
PubGrubPackage::Url(_) => {}
PubGrubPackage::Url(..) => {}
}
}
}
Expand Down

0 comments on commit 5d62b28

Please sign in to comment.