Skip to content

Commit

Permalink
Report yanks for cached and resolved packages (#3772)
Browse files Browse the repository at this point in the history
## Summary

We now show yanks as part of the resolution diagnostics, so they now
appear for `sync`, `install`, `compile`, and any other operations.
Further, they'll also appear for cached packages (but not packages that
are _already_ installed).

Closes #3768.

Closes #3766.
  • Loading branch information
charliermarsh committed May 22, 2024
1 parent 1fc71ea commit 74c494d
Show file tree
Hide file tree
Showing 17 changed files with 153 additions and 117 deletions.
1 change: 0 additions & 1 deletion Cargo.lock

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

79 changes: 68 additions & 11 deletions crates/distribution-types/src/resolution.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::collections::BTreeMap;

use pep508_rs::VerbatimUrl;
use uv_normalize::PackageName;
use uv_normalize::{ExtraName, PackageName};

use crate::{
BuiltDist, DirectorySourceDist, Dist, InstalledDirectUrlDist, InstalledDist, LocalEditable,
Expand All @@ -10,17 +10,26 @@ use crate::{

/// A set of packages pinned at specific versions.
#[derive(Debug, Default, Clone)]
pub struct Resolution(BTreeMap<PackageName, ResolvedDist>);
pub struct Resolution {
packages: BTreeMap<PackageName, ResolvedDist>,
diagnostics: Vec<Diagnostic>,
}

impl Resolution {
/// Create a new resolution from the given pinned packages.
pub fn new(packages: BTreeMap<PackageName, ResolvedDist>) -> Self {
Self(packages)
pub fn new(
packages: BTreeMap<PackageName, ResolvedDist>,
diagnostics: Vec<Diagnostic>,
) -> Self {
Self {
packages,
diagnostics,
}
}

/// Return the remote distribution for the given package name, if it exists.
pub fn get_remote(&self, package_name: &PackageName) -> Option<&Dist> {
match self.0.get(package_name) {
match self.packages.get(package_name) {
Some(dist) => match dist {
ResolvedDist::Installable(dist) => Some(dist),
ResolvedDist::Installed(_) => None,
Expand All @@ -31,32 +40,37 @@ impl Resolution {

/// Iterate over the [`PackageName`] entities in this resolution.
pub fn packages(&self) -> impl Iterator<Item = &PackageName> {
self.0.keys()
self.packages.keys()
}

/// Iterate over the [`ResolvedDist`] entities in this resolution.
pub fn distributions(&self) -> impl Iterator<Item = &ResolvedDist> {
self.0.values()
self.packages.values()
}

/// Return the number of distributions in this resolution.
pub fn len(&self) -> usize {
self.0.len()
self.packages.len()
}

/// Return `true` if there are no pinned packages in this resolution.
pub fn is_empty(&self) -> bool {
self.0.is_empty()
self.packages.is_empty()
}

/// Return the set of [`Requirement`]s that this resolution represents.
pub fn requirements(&self) -> impl Iterator<Item = Requirement> + '_ {
self.0.values().map(Requirement::from)
self.packages.values().map(Requirement::from)
}

/// Return the [`Diagnostic`]s that were produced during resolution.
pub fn diagnostics(&self) -> &[Diagnostic] {
&self.diagnostics
}

/// Return an iterator over the [`LocalEditable`] entities in this resolution.
pub fn editables(&self) -> impl Iterator<Item = LocalEditable> + '_ {
self.0.values().filter_map(|dist| match dist {
self.packages.values().filter_map(|dist| match dist {
ResolvedDist::Installable(Dist::Source(SourceDist::Directory(
DirectorySourceDist {
path,
Expand Down Expand Up @@ -84,6 +98,49 @@ impl Resolution {
}
}

#[derive(Debug, Clone)]
pub enum Diagnostic {
MissingExtra {
/// The distribution that was requested with a non-existent extra. For example,
/// `black==23.10.0`.
dist: ResolvedDist,
/// The extra that was requested. For example, `colorama` in `black[colorama]`.
extra: ExtraName,
},
YankedVersion {
/// The package that was requested with a yanked version. For example, `black==23.10.0`.
dist: ResolvedDist,
/// The reason that the version was yanked, if any.
reason: Option<String>,
},
}

impl Diagnostic {
/// Convert the diagnostic into a user-facing message.
pub fn message(&self) -> String {
match self {
Self::MissingExtra { dist, extra } => {
format!("The package `{dist}` does not have an extra named `{extra}`.")
}
Self::YankedVersion { dist, reason } => {
if let Some(reason) = reason {
format!("`{dist}` is yanked (reason: \"{reason}\").")
} else {
format!("`{dist}` is yanked.")
}
}
}
}

/// Returns `true` if the [`PackageName`] is involved in this diagnostic.
pub fn includes(&self, name: &PackageName) -> bool {
match self {
Self::MissingExtra { dist, .. } => name == dist.name(),
Self::YankedVersion { dist, .. } => name == dist.name(),
}
}
}

impl From<&ResolvedDist> for Requirement {
fn from(resolved_dist: &ResolvedDist) -> Self {
let source = match resolved_dist {
Expand Down
13 changes: 13 additions & 0 deletions crates/distribution-types/src/resolved.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::fmt::{Display, Formatter};

use pep508_rs::PackageName;
use pypi_types::Yanked;

use crate::{
BuiltDist, Dist, DistributionId, DistributionMetadata, Identifier, IndexUrl, InstalledDist,
Expand Down Expand Up @@ -51,6 +52,18 @@ impl ResolvedDist {
Self::Installed(_) => None,
}
}

/// Returns the [`Yanked`] status of the distribution, if available.
pub fn yanked(&self) -> Option<&Yanked> {
match self {
Self::Installable(dist) => match dist {
Dist::Source(SourceDist::Registry(sdist)) => sdist.file.yanked.as_ref(),
Dist::Built(BuiltDist::Registry(wheel)) => wheel.best_wheel().file.yanked.as_ref(),
_ => None,
},
Self::Installed(_) => None,
}
}
}

impl ResolvedDistRef<'_> {
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-resolver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ pub use options::{Options, OptionsBuilder};
pub use preferences::{Preference, PreferenceError};
pub use prerelease_mode::PreReleaseMode;
pub use python_requirement::PythonRequirement;
pub use resolution::{AnnotationStyle, Diagnostic, DisplayResolutionGraph, ResolutionGraph};
pub use resolution::{AnnotationStyle, DisplayResolutionGraph, ResolutionGraph};
pub use resolution_mode::ResolutionMode;
pub use resolver::{
BuildId, DefaultResolverProvider, InMemoryIndex, MetadataResponse, PackageVersionsResult,
Expand Down
3 changes: 2 additions & 1 deletion crates/uv-resolver/src/lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ impl Lock {
let resolved_dist = ResolvedDist::Installable(dist.to_dist(marker_env, tags));
map.insert(name, resolved_dist);
}
Resolution::new(map)
let diagnostics = vec![];
Resolution::new(map, diagnostics)
}

/// Returns the distribution with the given name. If there are multiple
Expand Down
54 changes: 22 additions & 32 deletions crates/uv-resolver/src/resolution/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@ use pubgrub::type_aliases::SelectedDependencies;
use rustc_hash::{FxHashMap, FxHashSet};

use distribution_types::{
Dist, DistributionMetadata, Name, ParsedUrlError, Requirement, ResolvedDist, VersionId,
VersionOrUrlRef,
Diagnostic, Dist, DistributionMetadata, Name, ParsedUrlError, Requirement, ResolvedDist,
VersionId, VersionOrUrlRef,
};
use pep440_rs::{Version, VersionSpecifier};
use pep508_rs::MarkerEnvironment;
use uv_normalize::{ExtraName, PackageName};
use pypi_types::Yanked;
use uv_normalize::PackageName;

use crate::dependency_provider::UvDependencyProvider;
use crate::editables::Editables;
Expand Down Expand Up @@ -172,6 +173,23 @@ impl ResolutionGraph {
.expect("Every package should be pinned")
.clone();

// Track yanks for any registry distributions.
match dist.yanked() {
None | Some(Yanked::Bool(false)) => {}
Some(Yanked::Bool(true)) => {
diagnostics.push(Diagnostic::YankedVersion {
dist: dist.clone(),
reason: None,
});
}
Some(Yanked::Reason(reason)) => {
diagnostics.push(Diagnostic::YankedVersion {
dist: dist.clone(),
reason: Some(reason.clone()),
});
}
}

// Extract the hashes, preserving those that were already present in the
// lockfile if necessary.
let hashes = if let Some(digests) = preferences
Expand Down Expand Up @@ -573,35 +591,7 @@ impl From<ResolutionGraph> for distribution_types::Resolution {
)
})
.collect(),
graph.diagnostics,
)
}
}

#[derive(Debug)]
pub enum Diagnostic {
MissingExtra {
/// The distribution that was requested with an non-existent extra. For example,
/// `black==23.10.0`.
dist: ResolvedDist,
/// The extra that was requested. For example, `colorama` in `black[colorama]`.
extra: ExtraName,
},
}

impl Diagnostic {
/// Convert the diagnostic into a user-facing message.
pub fn message(&self) -> String {
match self {
Self::MissingExtra { dist, extra } => {
format!("The package `{dist}` does not have an extra named `{extra}`.")
}
}
}

/// Returns `true` if the [`PackageName`] is involved in this diagnostic.
pub fn includes(&self, name: &PackageName) -> bool {
match self {
Self::MissingExtra { dist, .. } => name == dist.name(),
}
}
}
2 changes: 1 addition & 1 deletion crates/uv-resolver/src/resolution/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use pypi_types::{HashDigest, Metadata23};
use uv_normalize::{ExtraName, PackageName};

pub use crate::resolution::display::{AnnotationStyle, DisplayResolutionGraph};
pub use crate::resolution::graph::{Diagnostic, ResolutionGraph};
pub use crate::resolution::graph::ResolutionGraph;

mod display;
mod graph;
Expand Down
1 change: 0 additions & 1 deletion crates/uv/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ install-wheel-rs = { workspace = true, features = ["clap"], default-features = f
pep440_rs = { workspace = true }
pep508_rs = { workspace = true }
platform-tags = { workspace = true }
pypi-types = { workspace = true }
requirements-txt = { workspace = true, features = ["http"] }
uv-auth = { workspace = true }
uv-cache = { workspace = true, features = ["clap"] }
Expand Down
15 changes: 4 additions & 11 deletions crates/uv/src/commands/pip/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ use uv_resolver::{
use uv_types::{BuildIsolation, EmptyInstalledPackages, HashStrategy, InFlight};
use uv_warnings::warn_user;

use crate::commands::pip::operations;
use crate::commands::reporters::{DownloadReporter, ResolverReporter};
use crate::commands::{elapsed, ExitStatus};
use crate::printer::Printer;
Expand Down Expand Up @@ -589,17 +590,6 @@ pub(crate) async fn pip_compile(
.dimmed()
)?;

// Notify the user of any diagnostics.
for diagnostic in resolution.diagnostics() {
writeln!(
printer.stderr(),
"{}{} {}",
"warning".yellow().bold(),
":".bold(),
diagnostic.message().bold()
)?;
}

// Write the resolved dependencies to the output channel.
let mut writer = OutputWriter::new(!quiet || output_file.is_none(), output_file)?;

Expand Down Expand Up @@ -700,6 +690,9 @@ pub(crate) async fn pip_compile(
}
}

// Notify the user of any resolution diagnostics.
operations::diagnose_resolution(resolution.diagnostics(), printer)?;

Ok(ExitStatus::Success)
}

Expand Down
9 changes: 6 additions & 3 deletions crates/uv/src/commands/pip/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -444,9 +444,12 @@ pub(crate) async fn pip_install(
)
.await?;

// Validate the environment.
if strict {
operations::report_diagnostics(&resolution, &venv, printer)?;
// Notify the user of any resolution diagnostics.
operations::diagnose_resolution(resolution.diagnostics(), printer)?;

// Notify the user of any environment diagnostics.
if strict && !dry_run {
operations::diagnose_environment(&resolution, &venv, printer)?;
}

Ok(ExitStatus::Success)
Expand Down
Loading

0 comments on commit 74c494d

Please sign in to comment.