Skip to content

Commit

Permalink
Move conflicting dependencies into PubGrub (#1796)
Browse files Browse the repository at this point in the history
## Summary

This revives a PR from long ago
(#383 and
astral-sh/pubgrub#24) that modifies how we deal
with dependencies that are declared multiple times within a single
package.

To quote from the originating PR:

> Uses an experimental pubgrub branch (#370) that allows us to handle
multiple version ranges for a single dependency to the solver which
results in better error messages because the derivation tree contains
all of the relevant versions. Previously, the version ranges were merged
(by us) in the resolver before handing them to pubgrub since only one
range could be provided per package. Since we don't merge the versions
anymore, we no longer give the solver an empty range for conflicting
requirements; instead the solver comes to that conclusion from the
provided versions. You can see the improved error message for direct
dependencies in [this
snapshot](https://github.com/astral-sh/puffin/pull/383/files#diff-a0437f2c20cde5e2f15199a3bf81a102b92580063268417847ec9c793a115bd0).

The main issue with that PR was around its handling of URL dependencies,
so this PR _also_ refactors how we handle those. Previously, we stored
URL dependencies on `PubGrubPackage`, but they were omitted from the
hash and equality implementations of `PubGrubPackage`. This led to some
really careful codepaths wherein we had to ensure that we always visited
URLs before non-URL packages, so that the URL-inclusive versions were
included in any hashmaps, etc. I considered preserving this approach,
but it would require us to rely on lots of internal details of PubGrub
(since we'd now be relying on PubGrub to merge those packages in the
"right" order).

So, instead, we now _always_ set the URL on a given package, whenever
that package was _given_ a URL upfront. I think this is easier to reason
about: if the user provided a URL for `flask`, then we should just
always add the URL for `flask`. If we see some other URL for `flask`, we
error, like before. If we see some unknown URL for `flask`, we error,
like before.

Closes #1522.

Closes #1821.

Closes #1615.
  • Loading branch information
charliermarsh committed Feb 22, 2024
1 parent b3be53b commit 7eaed07
Show file tree
Hide file tree
Showing 20 changed files with 703 additions and 367 deletions.
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Expand Up @@ -66,7 +66,7 @@ owo-colors = { version = "4.0.0" }
petgraph = { version = "0.6.4" }
platform-info = { version = "2.0.2" }
plist = { version = "1.6.0" }
pubgrub = { git = "https://github.com/zanieb/pubgrub", rev = "9b6d89cb8a0c7902815c8b2ae99106ba322ffb14" }
pubgrub = { git = "https://github.com/zanieb/pubgrub", rev = "aab132a3d4d444dd8dd41d8c4e605abd69dacfe1" }
pyo3 = { version = "0.20.2" }
pyo3-log = { version = "0.9.0"}
pyproject-toml = { version = "0.10.0" }
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-dev/src/resolve_cli.rs
Expand Up @@ -105,7 +105,7 @@ pub(crate) async fn resolve_cli(args: ResolveCliArgs) -> Result<()> {
&flat_index,
&index,
&build_dispatch,
);
)?;
let resolution_graph = resolver.resolve().await.with_context(|| {
format!(
"No solution found when resolving: {}",
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-dispatch/src/lib.rs
Expand Up @@ -121,7 +121,7 @@ impl<'a> BuildContext for BuildDispatch<'a> {
self.flat_index,
self.index,
self,
);
)?;
let graph = resolver.resolve().await.with_context(|| {
format!(
"No solution found when resolving: {}",
Expand Down
30 changes: 30 additions & 0 deletions crates/uv-resolver/src/constraints.rs
@@ -0,0 +1,30 @@
use std::hash::BuildHasherDefault;

use rustc_hash::FxHashMap;

use pep508_rs::Requirement;
use uv_normalize::PackageName;

/// A set of constraints for a set of requirements.
#[derive(Debug, Default, Clone)]
pub(crate) struct Constraints(FxHashMap<PackageName, Vec<Requirement>>);

impl Constraints {
/// Create a new set of constraints from a set of requirements.
pub(crate) fn from_requirements(requirements: Vec<Requirement>) -> Self {
let mut constraints: FxHashMap<PackageName, Vec<Requirement>> =
FxHashMap::with_capacity_and_hasher(requirements.len(), BuildHasherDefault::default());
for requirement in requirements {
constraints
.entry(requirement.name.clone())
.or_default()
.push(requirement);
}
Self(constraints)
}

/// Get the constraints for a package.
pub(crate) fn get(&self, name: &PackageName) -> Option<&Vec<Requirement>> {
self.0.get(name)
}
}
33 changes: 33 additions & 0 deletions crates/uv-resolver/src/editables.rs
@@ -0,0 +1,33 @@
use std::hash::BuildHasherDefault;

use rustc_hash::FxHashMap;

use distribution_types::LocalEditable;
use pypi_types::Metadata21;
use uv_normalize::PackageName;

/// A set of editable packages, indexed by package name.
#[derive(Debug, Default, Clone)]
pub(crate) struct Editables(FxHashMap<PackageName, (LocalEditable, Metadata21)>);

impl Editables {
/// Create a new set of editables from a set of requirements.
pub(crate) fn from_requirements(requirements: Vec<(LocalEditable, Metadata21)>) -> Self {
let mut editables =
FxHashMap::with_capacity_and_hasher(requirements.len(), BuildHasherDefault::default());
for (editable_requirement, metadata) in requirements {
editables.insert(metadata.name.clone(), (editable_requirement, metadata));
}
Self(editables)
}

/// Get the editable for a package.
pub(crate) fn get(&self, name: &PackageName) -> Option<&(LocalEditable, Metadata21)> {
self.0.get(name)
}

/// Iterate over all editables.
pub(crate) fn iter(&self) -> impl Iterator<Item = &(LocalEditable, Metadata21)> {
self.0.values()
}
}
11 changes: 8 additions & 3 deletions crates/uv-resolver/src/error.rs
Expand Up @@ -7,7 +7,6 @@ use indexmap::IndexMap;
use pubgrub::range::Range;
use pubgrub::report::{DefaultStringReporter, DerivationTree, Reporter};
use rustc_hash::FxHashMap;
use url::Url;

use distribution_types::{BuiltDist, IndexLocations, PathBuiltDist, PathSourceDist, SourceDist};
use once_map::OnceMap;
Expand Down Expand Up @@ -46,14 +45,20 @@ pub enum ResolveError {
#[error("~= operator requires at least two release segments: {0}")]
InvalidTildeEquals(pep440_rs::VersionSpecifier),

#[error("Requirements contain conflicting URLs for package `{0}`:\n- {1}\n- {2}")]
ConflictingUrlsDirect(PackageName, String, String),

#[error("There are conflicting URLs for package `{0}`:\n- {1}\n- {2}")]
ConflictingUrls(PackageName, String, String),
ConflictingUrlsTransitive(PackageName, String, String),

#[error("There are conflicting versions for `{0}`: {1}")]
ConflictingVersions(String, String),

#[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),
DisallowedUrl(PackageName, String),

#[error("There are conflicting editable requirements for package `{0}`:\n- {1}\n- {2}")]
ConflictingEditables(PackageName, String, String),

#[error(transparent)]
DistributionType(#[from] distribution_types::Error),
Expand Down
2 changes: 2 additions & 0 deletions crates/uv-resolver/src/lib.rs
Expand Up @@ -12,7 +12,9 @@ pub use resolver::{
};

mod candidate_selector;
mod constraints;
mod dependency_mode;
mod editables;
mod error;
mod finder;
mod manifest;
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-resolver/src/overrides.rs
@@ -1,6 +1,6 @@
use itertools::Either;
use std::hash::BuildHasherDefault;

use itertools::Either;
use rustc_hash::FxHashMap;

use pep508_rs::Requirement;
Expand Down

0 comments on commit 7eaed07

Please sign in to comment.