Skip to content

Commit

Permalink
Remove URL from PubGrubPackage
Browse files Browse the repository at this point in the history
  • Loading branch information
@konstin
konstin committed Jun 24, 2024
1 parent 0f7a056 commit b1c9cfd
Show file tree
Hide file tree
Showing 9 changed files with 232 additions and 366 deletions.
8 changes: 6 additions & 2 deletions crates/uv-resolver/src/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use uv_normalize::PackageName;

use crate::candidate_selector::CandidateSelector;
use crate::dependency_provider::UvDependencyProvider;
use crate::fork_urls::ForkUrls;
use crate::pubgrub::{
PubGrubPackage, PubGrubPackageInner, PubGrubReportFormatter, PubGrubSpecifierError,
};
Expand All @@ -36,8 +37,8 @@ pub enum ResolveError {
#[error(transparent)]
Join(#[from] tokio::task::JoinError),

#[error("Attempted to wait on an unregistered task")]
Unregistered,
#[error("Attempted to wait on an unregistered task: `{_0}`")]
Unregistered(String),

#[error("Package metadata name `{metadata}` does not match given name `{given}`")]
NameMismatch {
Expand Down Expand Up @@ -185,6 +186,7 @@ impl From<pubgrub::error::PubGrubError<UvDependencyProvider>> for ResolveError {
index_locations: None,
unavailable_packages: FxHashMap::default(),
incomplete_packages: FxHashMap::default(),
fork_urls: ForkUrls::default(),
})
}
pubgrub::error::PubGrubError::SelfDependency { package, version } => {
Expand All @@ -207,6 +209,7 @@ pub struct NoSolutionError {
index_locations: Option<IndexLocations>,
unavailable_packages: FxHashMap<PackageName, UnavailablePackage>,
incomplete_packages: FxHashMap<PackageName, BTreeMap<Version, IncompletePackage>>,
fork_urls: ForkUrls,
}

impl std::error::Error for NoSolutionError {}
Expand All @@ -229,6 +232,7 @@ impl std::fmt::Display for NoSolutionError {
&self.index_locations,
&self.unavailable_packages,
&self.incomplete_packages,
&self.fork_urls,
) {
write!(f, "\n\n{hint}")?;
}
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-resolver/src/fork_urls.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use uv_normalize::PackageName;
use crate::ResolveError;

/// See [`crate::resolver::SolveState`].
#[derive(Default, Clone)]
#[derive(Default, Debug, Clone)]
pub(crate) struct ForkUrls(FxHashMap<PackageName, VerbatimParsedUrl>);

impl ForkUrls {
Expand Down
41 changes: 24 additions & 17 deletions crates/uv-resolver/src/pubgrub/dependencies.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use tracing::{debug, warn};
use pep440_rs::{Version, VersionSpecifiers};
use pypi_types::{
ParsedArchiveUrl, ParsedDirectoryUrl, ParsedGitUrl, ParsedPathUrl, ParsedUrl, Requirement,
RequirementSource,
RequirementSource, VerbatimParsedUrl,
};
use uv_git::GitResolver;
use uv_normalize::{ExtraName, PackageName};
Expand All @@ -19,7 +19,7 @@ use crate::resolver::{Locals, Urls};
use crate::{PubGrubSpecifier, ResolveError};

#[derive(Debug)]
pub struct PubGrubDependencies(Vec<(PubGrubPackage, Range<Version>)>);
pub struct PubGrubDependencies(Vec<(PubGrubPackage, Range<Version>, Option<VerbatimParsedUrl>)>);

impl PubGrubDependencies {
/// Generate a set of PubGrub dependencies from a set of requirements.
Expand Down Expand Up @@ -55,7 +55,11 @@ impl PubGrubDependencies {
git,
)
})) {
let PubGrubRequirement { package, version } = result?;
let PubGrubRequirement {
package,
version,
url,
} = result?;

match &*package {
PubGrubPackageInner::Package { name, .. } => {
Expand All @@ -65,17 +69,17 @@ impl PubGrubDependencies {
continue;
}

dependencies.push((package.clone(), version.clone()));
dependencies.push((package.clone(), version.clone(), url));
}
PubGrubPackageInner::Marker { .. } => {
dependencies.push((package.clone(), version.clone()));
dependencies.push((package.clone(), version.clone(), url));
}
PubGrubPackageInner::Extra { name, .. } => {
debug_assert!(
!source_name.is_some_and(|source_name| source_name == name),
"extras not flattened for {name}"
);
dependencies.push((package.clone(), version.clone()));
dependencies.push((package.clone(), version.clone(), url));
}
_ => {}
}
Expand All @@ -85,18 +89,20 @@ impl PubGrubDependencies {
}

/// Add a [`PubGrubPackage`] and [`PubGrubVersion`] range into the dependencies.
pub(crate) fn push(&mut self, package: PubGrubPackage, version: Range<Version>) {
self.0.push((package, version));
}

/// Iterate over the dependencies.
pub(crate) fn iter(&self) -> impl Iterator<Item = &(PubGrubPackage, Range<Version>)> {
self.0.iter()
pub(crate) fn push(
&mut self,
package: PubGrubPackage,
version: Range<Version>,
url: Option<VerbatimParsedUrl>,
) {
self.0.push((package, version, url));
}
}

/// Convert a [`PubGrubDependencies`] to a [`DependencyConstraints`].
impl From<PubGrubDependencies> for Vec<(PubGrubPackage, Range<Version>)> {
impl From<PubGrubDependencies>
for Vec<(PubGrubPackage, Range<Version>, Option<VerbatimParsedUrl>)>
{
fn from(dependencies: PubGrubDependencies) -> Self {
dependencies.0
}
Expand All @@ -107,6 +113,7 @@ impl From<PubGrubDependencies> for Vec<(PubGrubPackage, Range<Version>)> {
pub(crate) struct PubGrubRequirement {
pub(crate) package: PubGrubPackage,
pub(crate) version: Range<Version>,
pub(crate) url: Option<VerbatimParsedUrl>,
}

impl PubGrubRequirement {
Expand Down Expand Up @@ -232,13 +239,13 @@ impl PubGrubRequirement {
urls.canonicalize_allowed_url(&requirement.name, git, verbatim_url, &parsed_url)?
};
Ok(Self {
package: PubGrubPackage::from_url(
package: PubGrubPackage::from_package(
requirement.name.clone(),
extra,
requirement.marker.clone(),
url.clone(),
),
version: Range::full(),
url: Some(url.clone()),
})
}

Expand Down Expand Up @@ -318,9 +325,9 @@ impl PubGrubRequirement {
requirement.name.clone(),
extra,
requirement.marker.clone(),
url,
),
version,
url,
};
Ok(requirement)
}
Expand Down
83 changes: 1 addition & 82 deletions crates/uv-resolver/src/pubgrub/package.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ use std::ops::Deref;
use std::sync::Arc;

use pep508_rs::MarkerTree;
use pypi_types::VerbatimParsedUrl;
use uv_normalize::{ExtraName, GroupName, PackageName};

/// [`Arc`] wrapper around [`PubGrubPackageInner`] to make cloning (inside PubGrub) cheap.
Expand Down Expand Up @@ -49,43 +48,6 @@ pub enum PubGrubPackageInner {
extra: Option<ExtraName>,
dev: Option<GroupName>,
marker: Option<MarkerTree>,
/// The URL of the package, if it was specified in the requirement.
///
/// There are a few challenges that come with URL-based packages, and how they map to
/// PubGrub.
///
/// If the user declares a direct URL dependency, and then a transitive dependency
/// appears for the same package, we need to ensure that the direct URL dependency can
/// "satisfy" that requirement. So if the user declares a URL dependency on Werkzeug, and a
/// registry dependency on Flask, we need to ensure that Flask's dependency on Werkzeug
/// is resolved by the URL dependency. This means: (1) we need to avoid adding a second
/// Werkzeug variant from PyPI; and (2) we need to error if the Werkzeug version requested
/// by Flask doesn't match that of the URL dependency.
///
/// Additionally, we need to ensure that we disallow multiple versions of the same package,
/// even if requested from different URLs.
///
/// To enforce this requirement, we require that all possible URL dependencies are
/// defined upfront, as `requirements.txt` or `constraints.txt` or similar. Otherwise,
/// solving these graphs becomes far more complicated -- and the "right" behavior isn't
/// even clear. For example, imagine that you define a direct dependency on Werkzeug, and
/// then one of your other direct dependencies declares a dependency on Werkzeug at some
/// URL. Which is correct? By requiring direct dependencies, the semantics are at least
/// clear.
///
/// With the list of known URLs available upfront, we then only need to do two things:
///
/// 1. When iterating over the dependencies for a single package, ensure that we respect
/// URL variants over registry variants, if the package declares a dependency on both
/// `Werkzeug==2.0.0` _and_ `Werkzeug @ https://...` , which is strange but possible.
/// This is enforced by [`crate::pubgrub::dependencies::PubGrubDependencies`].
/// 2. Reject any URL dependencies that aren't known ahead-of-time.
///
/// Eventually, we could relax this constraint, in favor of something more lenient, e.g., if
/// we're going to have a dependency that's provided as a URL, we _need_ to visit the URL
/// version before the registry version. So we could just error if we visit a URL variant
/// _after_ a registry variant.
url: Option<VerbatimParsedUrl>,
},
/// A proxy package to represent a dependency with an extra (e.g., `black[colorama]`).
///
Expand All @@ -104,7 +66,6 @@ pub enum PubGrubPackageInner {
name: PackageName,
extra: ExtraName,
marker: Option<MarkerTree>,
url: Option<VerbatimParsedUrl>,
},
/// A proxy package to represent an enabled "dependency group" (e.g., development dependencies).
///
Expand All @@ -115,7 +76,6 @@ pub enum PubGrubPackageInner {
name: PackageName,
dev: GroupName,
marker: Option<MarkerTree>,
url: Option<VerbatimParsedUrl>,
},
/// A proxy package for a base package with a marker (e.g., `black; python_version >= "3.6"`).
///
Expand All @@ -124,7 +84,6 @@ pub enum PubGrubPackageInner {
Marker {
name: PackageName,
marker: MarkerTree,
url: Option<VerbatimParsedUrl>,
},
}

Expand All @@ -134,7 +93,6 @@ impl PubGrubPackage {
name: PackageName,
extra: Option<ExtraName>,
mut marker: Option<MarkerTree>,
url: Option<VerbatimParsedUrl>,
) -> Self {
// Remove all extra expressions from the marker, since we track extras
// separately. This also avoids an issue where packages added via
Expand All @@ -147,54 +105,15 @@ impl PubGrubPackage {
name,
extra,
marker,
url,
}))
} else if let Some(marker) = marker {
Self(Arc::new(PubGrubPackageInner::Marker { name, marker, url }))
Self(Arc::new(PubGrubPackageInner::Marker { name, marker }))
} else {
Self(Arc::new(PubGrubPackageInner::Package {
name,
extra,
dev: None,
marker,
url,
}))
}
}

/// Create a [`PubGrubPackage`] from a package name and URL.
pub(crate) fn from_url(
name: PackageName,
extra: Option<ExtraName>,
mut marker: Option<MarkerTree>,
url: VerbatimParsedUrl,
) -> Self {
// Remove all extra expressions from the marker, since we track extras
// separately. This also avoids an issue where packages added via
// extras end up having two distinct marker expressions, which in turn
// makes them two distinct packages. This results in PubGrub being
// unable to unify version constraints across such packages.
marker = marker.and_then(|m| m.simplify_extras_with(|_| true));
if let Some(extra) = extra {
Self(Arc::new(PubGrubPackageInner::Extra {
name,
extra,
marker,
url: Some(url),
}))
} else if let Some(marker) = marker {
Self(Arc::new(PubGrubPackageInner::Marker {
name,
marker,
url: Some(url),
}))
} else {
Self(Arc::new(PubGrubPackageInner::Package {
name,
extra,
dev: None,
marker,
url: Some(url),
}))
}
}
Expand Down
Loading

0 comments on commit b1c9cfd

Please sign in to comment.