Skip to content

Commit

Permalink
Show a dedicated PubGrub hint for --unsafe-best-match (#7645)
Browse files Browse the repository at this point in the history
## Summary

Closes #5510.
  • Loading branch information
charliermarsh authored Sep 23, 2024
1 parent 47eeef5 commit b14696c
Show file tree
Hide file tree
Showing 6 changed files with 103 additions and 6 deletions.
11 changes: 8 additions & 3 deletions crates/distribution-types/src/index_url.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ static DEFAULT_INDEX_URL: LazyLock<IndexUrl> =
LazyLock::new(|| IndexUrl::Pypi(VerbatimUrl::from_url(PYPI_URL.clone())));

/// The URL of an index to use for fetching packages (e.g., PyPI).
#[derive(Debug, Clone, Hash, Eq, PartialEq)]
#[derive(Debug, Clone, Hash, Eq, PartialEq, Ord, PartialOrd)]
pub enum IndexUrl {
Pypi(VerbatimUrl),
Url(VerbatimUrl),
Expand Down Expand Up @@ -384,9 +384,14 @@ impl<'a> IndexLocations {
}
}

/// Return an iterator over all [`IndexUrl`] entries.
/// Return an iterator over all [`IndexUrl`] entries in order.
///
/// Prioritizes the extra indexes over the main index.
///
/// If `no_index` was enabled, then this always returns an empty
/// iterator.
pub fn indexes(&'a self) -> impl Iterator<Item = &'a IndexUrl> + 'a {
self.index().into_iter().chain(self.extra_index())
self.extra_index().chain(self.index())
}

/// Return an iterator over the [`FlatIndexLocation`] entries.
Expand Down
6 changes: 5 additions & 1 deletion crates/uv-resolver/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use indexmap::IndexSet;
use pubgrub::{DefaultStringReporter, DerivationTree, Derived, External, Range, Reporter};
use rustc_hash::FxHashMap;

use distribution_types::{BuiltDist, IndexLocations, InstalledDist, SourceDist};
use distribution_types::{BuiltDist, IndexLocations, IndexUrl, InstalledDist, SourceDist};
use pep440_rs::Version;
use pep508_rs::MarkerTree;
use tracing::trace;
Expand Down Expand Up @@ -122,6 +122,7 @@ pub(crate) type ErrorTree = DerivationTree<PubGrubPackage, Range<Version>, Unava
pub struct NoSolutionError {
error: pubgrub::NoSolutionError<UvDependencyProvider>,
available_versions: FxHashMap<PackageName, BTreeSet<Version>>,
available_indexes: FxHashMap<PackageName, BTreeSet<IndexUrl>>,
selector: CandidateSelector,
python_requirement: PythonRequirement,
index_locations: IndexLocations,
Expand All @@ -137,6 +138,7 @@ impl NoSolutionError {
pub(crate) fn new(
error: pubgrub::NoSolutionError<UvDependencyProvider>,
available_versions: FxHashMap<PackageName, BTreeSet<Version>>,
available_indexes: FxHashMap<PackageName, BTreeSet<IndexUrl>>,
selector: CandidateSelector,
python_requirement: PythonRequirement,
index_locations: IndexLocations,
Expand All @@ -149,6 +151,7 @@ impl NoSolutionError {
Self {
error,
available_versions,
available_indexes,
selector,
python_requirement,
index_locations,
Expand Down Expand Up @@ -254,6 +257,7 @@ impl std::fmt::Display for NoSolutionError {
&tree,
&self.selector,
&self.index_locations,
&self.available_indexes,
&self.unavailable_packages,
&self.incomplete_packages,
&self.fork_urls,
Expand Down
69 changes: 67 additions & 2 deletions crates/uv-resolver/src/pubgrub/report.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ use owo_colors::OwoColorize;
use pubgrub::{DerivationTree, Derived, External, Map, Range, ReportFormatter, Term};
use rustc_hash::FxHashMap;

use distribution_types::IndexLocations;
use distribution_types::{IndexLocations, IndexUrl};
use pep440_rs::Version;
use uv_configuration::IndexStrategy;
use uv_normalize::PackageName;

use crate::candidate_selector::CandidateSelector;
Expand Down Expand Up @@ -504,6 +505,7 @@ impl PubGrubReportFormatter<'_> {
derivation_tree: &ErrorTree,
selector: &CandidateSelector,
index_locations: &IndexLocations,
available_indexes: &FxHashMap<PackageName, BTreeSet<IndexUrl>>,
unavailable_packages: &FxHashMap<PackageName, UnavailablePackage>,
incomplete_packages: &FxHashMap<PackageName, BTreeMap<Version, IncompletePackage>>,
fork_urls: &ForkUrls,
Expand All @@ -528,12 +530,14 @@ impl PubGrubReportFormatter<'_> {
);
}

// Check for no versions due to no `--find-links` flat index
// Check for no versions due to no `--find-links` flat index.
Self::index_hints(
package,
name,
set,
selector,
index_locations,
available_indexes,
unavailable_packages,
incomplete_packages,
output_hints,
Expand Down Expand Up @@ -582,6 +586,7 @@ impl PubGrubReportFormatter<'_> {
&derived.cause1,
selector,
index_locations,
available_indexes,
unavailable_packages,
incomplete_packages,
fork_urls,
Expand All @@ -593,6 +598,7 @@ impl PubGrubReportFormatter<'_> {
&derived.cause2,
selector,
index_locations,
available_indexes,
unavailable_packages,
incomplete_packages,
fork_urls,
Expand All @@ -608,7 +614,9 @@ impl PubGrubReportFormatter<'_> {
package: &PubGrubPackage,
name: &PackageName,
set: &Range<Version>,
selector: &CandidateSelector,
index_locations: &IndexLocations,
available_indexes: &FxHashMap<PackageName, BTreeSet<IndexUrl>>,
unavailable_packages: &FxHashMap<PackageName, UnavailablePackage>,
incomplete_packages: &FxHashMap<PackageName, BTreeMap<Version, IncompletePackage>>,
hints: &mut IndexSet<PubGrubHint>,
Expand Down Expand Up @@ -686,6 +694,27 @@ impl PubGrubReportFormatter<'_> {
}
}
}

// Add hints due to the package being available on an index, but not at the correct version,
// with subsequent indexes that were _not_ queried.
if matches!(selector.index_strategy(), IndexStrategy::FirstIndex) {
if let Some(found_index) = available_indexes.get(name).and_then(BTreeSet::first) {
// Determine whether the index is the last-available index. If not, then some
// indexes were not queried, and could contain a compatible version.
if let Some(next_index) = index_locations
.indexes()
.skip_while(|url| *url != found_index)
.nth(1)
{
hints.insert(PubGrubHint::UncheckedIndex {
package: package.clone(),
range: set.clone(),
found_index: found_index.clone(),
next_index: next_index.clone(),
});
}
}
}
}

fn prerelease_available_hint(
Expand Down Expand Up @@ -819,11 +848,25 @@ pub(crate) enum PubGrubHint {
// excluded from `PartialEq` and `Hash`
package_requires_python: Range<Version>,
},
/// A non-workspace package depends on a workspace package, which is likely shadowing a
/// transitive dependency.
DependsOnWorkspacePackage {
package: PubGrubPackage,
dependency: PubGrubPackage,
workspace: bool,
},
/// A package was available on an index, but not at the correct version, and at least one
/// subsequent index was not queried. As such, a compatible version may be available on an
/// one of the remaining indexes.
UncheckedIndex {
package: PubGrubPackage,
// excluded from `PartialEq` and `Hash`
range: Range<Version>,
// excluded from `PartialEq` and `Hash`
found_index: IndexUrl,
// excluded from `PartialEq` and `Hash`
next_index: IndexUrl,
},
}

/// This private enum mirrors [`PubGrubHint`] but only includes fields that should be
Expand Down Expand Up @@ -869,6 +912,9 @@ enum PubGrubHintCore {
dependency: PubGrubPackage,
workspace: bool,
},
UncheckedIndex {
package: PubGrubPackage,
},
}

impl From<PubGrubHint> for PubGrubHintCore {
Expand Down Expand Up @@ -921,6 +967,7 @@ impl From<PubGrubHint> for PubGrubHintCore {
dependency,
workspace,
},
PubGrubHint::UncheckedIndex { package, .. } => Self::UncheckedIndex { package },
}
}
}
Expand Down Expand Up @@ -1140,6 +1187,24 @@ impl std::fmt::Display for PubGrubHint {
dependency,
)
}
Self::UncheckedIndex {
package,
range,
found_index,
next_index,
} => {
write!(
f,
"{}{} `{}` was found on {}, but not at the requested version ({}). A compatible version may be available on a subsequent index (e.g., {}). By default, uv will only consider versions that are published on the first index that contains a given package, to avoid dependency confusion attacks. If all indexes are equally trusted, use `{}` to consider all versions from all indexes, regardless of the order in which they were defined.",
"hint".bold().cyan(),
":".bold(),
package,
found_index.cyan(),
PackageRange::compatibility(package, range, None).cyan(),
next_index.cyan(),
"--index-strategy unsafe-best-match".green(),
)
}
}
}
}
Expand Down
13 changes: 13 additions & 0 deletions crates/uv-resolver/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1944,6 +1944,7 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
}
}

let mut available_indexes = FxHashMap::default();
let mut available_versions = FxHashMap::default();
for package in err.packages() {
let Some(name) = package.name() else { continue };
Expand All @@ -1956,19 +1957,31 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
}
if let Some(response) = self.index.packages().get(name) {
if let VersionsResponse::Found(ref version_maps) = *response {
// Track the available versions, across all indexes.
for version_map in version_maps {
available_versions
.entry(name.clone())
.or_insert_with(BTreeSet::new)
.extend(version_map.versions().cloned());
}

// Track the indexes in which the package is available.
available_indexes
.entry(name.clone())
.or_insert(BTreeSet::new())
.extend(
version_maps
.iter()
.filter_map(|version_map| version_map.index().cloned()),
);
}
}
}

ResolveError::NoSolution(NoSolutionError::new(
err,
available_versions,
available_indexes,
self.selector.clone(),
self.python_requirement.clone(),
index_locations.clone(),
Expand Down
8 changes: 8 additions & 0 deletions crates/uv-resolver/src/version_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,14 @@ impl VersionMap {
}
}

/// Return the index URL where this package came from.
pub(crate) fn index(&self) -> Option<&IndexUrl> {
match &self.inner {
VersionMapInner::Eager(_) => None,
VersionMapInner::Lazy(lazy) => Some(&lazy.index),
}
}

/// Return an iterator over the versions and distributions.
///
/// Note that the value returned in this iterator is a [`VersionMapDist`],
Expand Down
2 changes: 2 additions & 0 deletions crates/uv/tests/pip_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10682,6 +10682,8 @@ fn compile_index_url_first_match() -> Result<()> {
----- stderr -----
× No solution found when resolving dependencies:
╰─▶ Because there is no version of jinja2==3.1.0 and you require jinja2==3.1.0, we can conclude that your requirements are unsatisfiable.
hint: `jinja2` was found on https://download.pytorch.org/whl/cpu, but not at the requested version (jinja2==3.1.0). A compatible version may be available on a subsequent index (e.g., https://pypi.org/simple). By default, uv will only consider versions that are published on the first index that contains a given package, to avoid dependency confusion attacks. If all indexes are equally trusted, use `--index-strategy unsafe-best-match` to consider all versions from all indexes, regardless of the order in which they were defined.
"###
);

Expand Down

0 comments on commit b14696c

Please sign in to comment.