Use dynamic dispatch to simplify reporters (#10086)

## Summary

Sort of undecided on this. These are already stored as `dyn Reporter` in
each struct, so we're already using dynamic dispatch in that sense. But
all the methods take `impl Reporter`. This is sometimes nice (the
callsites are simpler?), but it also means that in practice, you often
_can't_ pass `None` to these methods that accept `Option<impl
Reporter>`, because Rust can't infer the generic type.

Anyway, this adds more consistency and simplifies the setup by using
`Arc<dyn Reporter>` everywhere.
This commit is contained in:
Charlie Marsh 2025-01-06 12:04:00 -05:00 committed by GitHub
parent 54b9e8ff82
commit 66a603b6c4
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
19 changed files with 117 additions and 74 deletions

View file

@ -82,11 +82,10 @@ impl<'a, Context: BuildContext> DistributionDatabase<'a, Context> {
/// Set the [`Reporter`] to use for the [`DistributionDatabase`].
#[must_use]
pub fn with_reporter(self, reporter: impl Reporter + 'static) -> Self {
let reporter = Arc::new(reporter);
pub fn with_reporter(self, reporter: Arc<dyn Reporter>) -> Self {
Self {
reporter: Some(reporter.clone()),
builder: self.builder.with_reporter(reporter),
builder: self.builder.with_reporter(reporter.clone()),
reporter: Some(reporter),
..self
}
}

View file

@ -29,15 +29,18 @@ pub trait Reporter: Send + Sync {
fn on_download_complete(&self, name: &PackageName, id: usize);
}
/// A facade for converting from [`Reporter`] to [`uv_git::Reporter`].
pub(crate) struct Facade {
reporter: Arc<dyn Reporter>,
impl dyn Reporter {
/// Converts this reporter to a [`uv_git::Reporter`].
pub(crate) fn into_git_reporter(self: Arc<dyn Reporter>) -> Arc<dyn uv_git::Reporter> {
Arc::new(Facade {
reporter: self.clone(),
})
}
}
impl From<Arc<dyn Reporter>> for Facade {
fn from(reporter: Arc<dyn Reporter>) -> Self {
Self { reporter }
}
/// A facade for converting from [`Reporter`] to [`uv_git::Reporter`].
struct Facade {
reporter: Arc<dyn Reporter>,
}
impl uv_git::Reporter for Facade {

View file

@ -1,5 +1,13 @@
//! Fetch and build source distributions from remote sources.
// This is to squash warnings about `|r| r.into_git_reporter()`. Clippy wants
// me to eta-reduce that and write it as
// `<(dyn reporter::Reporter + 'static)>::into_git_reporter`
// instead. But that's a monster. On the other hand, applying this suppression
// instruction more granularly is annoying. So we just slap it on the module
// for now. ---AG
#![allow(clippy::redundant_closure_for_method_calls)]
use std::borrow::Cow;
use std::ops::Bound;
use std::path::{Path, PathBuf};
@ -9,7 +17,6 @@ use std::sync::Arc;
use crate::distribution_database::ManagedClient;
use crate::error::Error;
use crate::metadata::{ArchiveMetadata, GitWorkspaceMember, Metadata};
use crate::reporter::Facade;
use crate::source::built_wheel_metadata::BuiltWheelMetadata;
use crate::source::revision::Revision;
use crate::{Reporter, RequiresDist};
@ -1330,7 +1337,9 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {
resource.git,
client.unmanaged.uncached_client(resource.url).clone(),
self.build_context.cache().bucket(CacheBucket::Git),
self.reporter.clone().map(Facade::from),
self.reporter
.clone()
.map(|reporter| reporter.into_git_reporter()),
)
.await?;
@ -1427,7 +1436,9 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {
resource.git,
client.unmanaged.uncached_client(resource.url).clone(),
self.build_context.cache().bucket(CacheBucket::Git),
self.reporter.clone().map(Facade::from),
self.reporter
.clone()
.map(|reporter| reporter.into_git_reporter()),
)
.await?;
@ -1603,7 +1614,9 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {
&source.git,
client.unmanaged.uncached_client(&source.url).clone(),
self.build_context.cache().bucket(CacheBucket::Git),
self.reporter.clone().map(Facade::from),
self.reporter
.clone()
.map(|reporter| reporter.into_git_reporter()),
)
.await?;
}
@ -1614,7 +1627,9 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {
source.git,
client.unmanaged.uncached_client(source.url).clone(),
self.build_context.cache().bucket(CacheBucket::Git),
self.reporter.clone().map(Facade::from),
self.reporter
.clone()
.map(|reporter| reporter.into_git_reporter()),
)
.await?;
}

View file

@ -44,7 +44,7 @@ impl GitResolver {
url: &GitUrl,
client: ClientWithMiddleware,
cache: PathBuf,
reporter: Option<impl Reporter + 'static>,
reporter: Option<Arc<dyn Reporter>>,
) -> Result<GitSha, GitResolverError> {
debug!("Resolving source distribution from Git: {url}");
@ -88,7 +88,7 @@ impl GitResolver {
url: &GitUrl,
client: ClientWithMiddleware,
cache: PathBuf,
reporter: Option<impl Reporter + 'static>,
reporter: Option<Arc<dyn Reporter>>,
) -> Result<Fetch, GitResolverError> {
debug!("Fetching source distribution from Git: {url}");
@ -138,7 +138,7 @@ impl GitResolver {
/// For example, given a Git dependency with a reference to a branch or tag, return a URL
/// with a precise reference to the current commit of that branch or tag.
///
/// This method takes into account various normalizations that are independent from the Git
/// This method takes into account various normalizations that are independent of the Git
/// layer. For example: removing `#subdirectory=pkg_dir`-like fragments, and removing `git+`
/// prefix kinds.
///

View file

@ -4,6 +4,7 @@
use std::borrow::Cow;
use std::path::{Path, PathBuf};
use std::sync::Arc;
use anyhow::Result;
use reqwest_middleware::ClientWithMiddleware;
@ -24,11 +25,11 @@ pub struct GitSource {
/// The path to the Git source database.
cache: PathBuf,
/// The reporter to use for this source.
reporter: Option<Box<dyn Reporter>>,
reporter: Option<Arc<dyn Reporter>>,
}
impl GitSource {
/// Initialize a new Git source.
/// Initialize a [`GitSource`] with the given Git URL, HTTP client, and cache path.
pub fn new(
git: GitUrl,
client: impl Into<ClientWithMiddleware>,
@ -42,11 +43,11 @@ impl GitSource {
}
}
/// Set the [`Reporter`] to use for this `GIt` source.
/// Set the [`Reporter`] to use for the [`GitSource`].
#[must_use]
pub fn with_reporter(self, reporter: impl Reporter + 'static) -> Self {
pub fn with_reporter(self, reporter: Arc<dyn Reporter>) -> Self {
Self {
reporter: Some(Box::new(reporter)),
reporter: Some(reporter),
..self
}
}

View file

@ -1,21 +1,22 @@
use std::convert;
use std::sync::{Arc, LazyLock};
use anyhow::{Context, Error, Result};
use rayon::iter::{IntoParallelRefIterator, ParallelIterator};
use std::convert;
use std::sync::LazyLock;
use tokio::sync::oneshot;
use tracing::instrument;
use uv_install_wheel::{linker::LinkMode, Layout};
use uv_cache::Cache;
use uv_configuration::RAYON_INITIALIZE;
use uv_distribution_types::CachedDist;
use uv_install_wheel::{linker::LinkMode, Layout};
use uv_python::PythonEnvironment;
pub struct Installer<'a> {
venv: &'a PythonEnvironment,
link_mode: LinkMode,
cache: Option<&'a Cache>,
reporter: Option<Box<dyn Reporter>>,
reporter: Option<Arc<dyn Reporter>>,
installer_name: Option<String>,
installer_metadata: bool,
}
@ -50,9 +51,9 @@ impl<'a> Installer<'a> {
/// Set the [`Reporter`] to use for this installer.
#[must_use]
pub fn with_reporter(self, reporter: impl Reporter + 'static) -> Self {
pub fn with_reporter(self, reporter: Arc<dyn Reporter>) -> Self {
Self {
reporter: Some(Box::new(reporter)),
reporter: Some(reporter),
..self
}
}
@ -66,7 +67,7 @@ impl<'a> Installer<'a> {
}
}
/// Set the whether to link Uv specific files in dist-info
/// Set whether to install uv-specifier files in the dist-info directory.
#[must_use]
pub fn with_installer_metadata(self, installer_metadata: bool) -> Self {
Self {
@ -151,7 +152,7 @@ fn install(
layout: Layout,
installer_name: Option<String>,
link_mode: LinkMode,
reporter: Option<Box<dyn Reporter>>,
reporter: Option<Arc<dyn Reporter>>,
relocatable: bool,
installer_metadata: bool,
) -> Result<Vec<CachedDist>> {

View file

@ -48,15 +48,16 @@ impl<'a, Context: BuildContext> Preparer<'a, Context> {
/// Set the [`Reporter`] to use for operations.
#[must_use]
pub fn with_reporter(self, reporter: impl Reporter + 'static) -> Self {
let reporter: Arc<dyn Reporter> = Arc::new(reporter);
pub fn with_reporter(self, reporter: Arc<dyn Reporter>) -> Self {
Self {
tags: self.tags,
cache: self.cache,
hashes: self.hashes,
build_options: self.build_options,
database: self.database.with_reporter(Facade::from(reporter.clone())),
reporter: Some(reporter.clone()),
database: self
.database
.with_reporter(reporter.clone().into_distribution_reporter()),
reporter: Some(reporter),
}
}
@ -271,15 +272,20 @@ pub trait Reporter: Send + Sync {
fn on_checkout_complete(&self, url: &Url, rev: &str, index: usize);
}
/// A facade for converting from [`Reporter`] to [`uv_git::Reporter`].
struct Facade {
reporter: Arc<dyn Reporter>,
impl dyn Reporter {
/// Converts this reporter to a [`uv_distribution::Reporter`].
pub(crate) fn into_distribution_reporter(
self: Arc<dyn Reporter>,
) -> Arc<dyn uv_distribution::Reporter> {
Arc::new(Facade {
reporter: self.clone(),
})
}
}
impl From<Arc<dyn Reporter>> for Facade {
fn from(reporter: Arc<dyn Reporter>) -> Self {
Self { reporter }
}
/// A facade for converting from [`Reporter`] to [`uv_distribution::Reporter`].
struct Facade {
reporter: Arc<dyn Reporter>,
}
impl uv_distribution::Reporter for Facade {

View file

@ -37,7 +37,7 @@ impl<'a, Context: BuildContext> ExtrasResolver<'a, Context> {
/// Set the [`Reporter`] to use for this resolver.
#[must_use]
pub fn with_reporter(self, reporter: impl Reporter + 'static) -> Self {
pub fn with_reporter(self, reporter: Arc<dyn Reporter>) -> Self {
Self {
database: self.database.with_reporter(reporter),
..self

View file

@ -67,7 +67,7 @@ impl<'a, Context: BuildContext> LookaheadResolver<'a, Context> {
/// Set the [`Reporter`] to use for this resolver.
#[must_use]
pub fn with_reporter(self, reporter: impl Reporter + 'static) -> Self {
pub fn with_reporter(self, reporter: Arc<dyn Reporter>) -> Self {
Self {
database: self.database.with_reporter(reporter),
..self

View file

@ -65,7 +65,7 @@ impl<'a, Context: BuildContext> SourceTreeResolver<'a, Context> {
/// Set the [`Reporter`] to use for this resolver.
#[must_use]
pub fn with_reporter(self, reporter: impl Reporter + 'static) -> Self {
pub fn with_reporter(self, reporter: Arc<dyn Reporter>) -> Self {
Self {
database: self.database.with_reporter(reporter),
..self

View file

@ -49,7 +49,7 @@ impl<'a, Context: BuildContext> NamedRequirementsResolver<'a, Context> {
/// Set the [`Reporter`] to use for this resolver.
#[must_use]
pub fn with_reporter(self, reporter: impl Reporter + 'static) -> Self {
pub fn with_reporter(self, reporter: Arc<dyn Reporter>) -> Self {
Self {
database: self.database.with_reporter(reporter),
..self

View file

@ -73,7 +73,6 @@ pub use crate::resolver::provider::{
DefaultResolverProvider, MetadataResponse, PackageVersionsResult, ResolverProvider,
VersionsResponse, WheelMetadataResult,
};
use crate::resolver::reporter::Facade;
pub use crate::resolver::reporter::{BuildId, Reporter};
use crate::yanks::AllowedYanks;
use crate::{marker, DependencyMode, Exclusions, FlatIndex, Options, ResolutionMode, VersionMap};
@ -243,15 +242,15 @@ impl<Provider: ResolverProvider, InstalledPackages: InstalledPackagesProvider>
/// Set the [`Reporter`] to use for this installer.
#[must_use]
pub fn with_reporter(self, reporter: impl Reporter + 'static) -> Self {
let reporter = Arc::new(reporter);
pub fn with_reporter(self, reporter: Arc<dyn Reporter>) -> Self {
Self {
state: ResolverState {
reporter: Some(reporter.clone()),
..self.state
},
provider: self.provider.with_reporter(Facade { reporter }),
provider: self
.provider
.with_reporter(reporter.into_distribution_reporter()),
}
}

View file

@ -2,7 +2,7 @@ use std::future::Future;
use std::sync::Arc;
use uv_configuration::BuildOptions;
use uv_distribution::{ArchiveMetadata, DistributionDatabase};
use uv_distribution::{ArchiveMetadata, DistributionDatabase, Reporter};
use uv_distribution_types::{Dist, IndexCapabilities, IndexUrl, InstalledDist, RequestedDist};
use uv_normalize::PackageName;
use uv_pep440::{Version, VersionSpecifiers};
@ -97,9 +97,9 @@ pub trait ResolverProvider {
dist: &'io InstalledDist,
) -> impl Future<Output = WheelMetadataResult> + 'io;
/// Set the [`uv_distribution::Reporter`] to use for this installer.
/// Set the [`Reporter`] to use for this installer.
#[must_use]
fn with_reporter(self, reporter: impl uv_distribution::Reporter + 'static) -> Self;
fn with_reporter(self, reporter: Arc<dyn Reporter>) -> Self;
}
/// The main IO backend for the resolver, which does cached requests network requests using the
@ -273,9 +273,9 @@ impl<'a, Context: BuildContext> ResolverProvider for DefaultResolverProvider<'a,
}
}
/// Set the [`uv_distribution::Reporter`] to use for this installer.
/// Set the [`Reporter`] to use for this installer.
#[must_use]
fn with_reporter(self, reporter: impl uv_distribution::Reporter + 'static) -> Self {
fn with_reporter(self, reporter: Arc<dyn Reporter>) -> Self {
Self {
fetcher: self.fetcher.with_reporter(reporter),
..self

View file

@ -37,9 +37,20 @@ pub trait Reporter: Send + Sync {
fn on_checkout_complete(&self, url: &Url, rev: &str, id: usize);
}
impl dyn Reporter {
/// Converts this reporter to a [`uv_distribution::Reporter`].
pub(crate) fn into_distribution_reporter(
self: Arc<dyn Reporter>,
) -> Arc<dyn uv_distribution::Reporter> {
Arc::new(Facade {
reporter: self.clone(),
})
}
}
/// A facade for converting from [`Reporter`] to [`uv_distribution::Reporter`].
pub(crate) struct Facade {
pub(crate) reporter: Arc<dyn Reporter>,
struct Facade {
reporter: Arc<dyn Reporter>,
}
impl uv_distribution::Reporter for Facade {

View file

@ -6,6 +6,7 @@ use owo_colors::OwoColorize;
use std::collections::{BTreeSet, HashSet};
use std::fmt::Write;
use std::path::PathBuf;
use std::sync::Arc;
use tracing::debug;
use uv_tool::InstalledTools;
@ -138,7 +139,7 @@ pub(crate) async fn resolve<InstalledPackages: InstalledPackagesProvider>(
index,
DistributionDatabase::new(client, build_dispatch, concurrency.downloads),
)
.with_reporter(ResolverReporter::from(printer))
.with_reporter(Arc::new(ResolverReporter::from(printer)))
.resolve(unnamed.into_iter())
.await?,
);
@ -152,7 +153,7 @@ pub(crate) async fn resolve<InstalledPackages: InstalledPackagesProvider>(
index,
DistributionDatabase::new(client, build_dispatch, concurrency.downloads),
)
.with_reporter(ResolverReporter::from(printer))
.with_reporter(Arc::new(ResolverReporter::from(printer)))
.resolve(source_trees.iter().map(PathBuf::as_path))
.await?;
@ -221,7 +222,7 @@ pub(crate) async fn resolve<InstalledPackages: InstalledPackagesProvider>(
index,
DistributionDatabase::new(client, build_dispatch, concurrency.downloads),
)
.with_reporter(ResolverReporter::from(printer))
.with_reporter(Arc::new(ResolverReporter::from(printer)))
.resolve(unnamed.into_iter())
.await?,
);
@ -251,7 +252,7 @@ pub(crate) async fn resolve<InstalledPackages: InstalledPackagesProvider>(
index,
DistributionDatabase::new(client, build_dispatch, concurrency.downloads),
)
.with_reporter(ResolverReporter::from(printer))
.with_reporter(Arc::new(ResolverReporter::from(printer)))
.resolve(&resolver_env)
.await?
}
@ -297,7 +298,7 @@ pub(crate) async fn resolve<InstalledPackages: InstalledPackagesProvider>(
installed_packages,
DistributionDatabase::new(client, build_dispatch, concurrency.downloads),
)?
.with_reporter(reporter);
.with_reporter(Arc::new(reporter));
resolver.resolve().await?
};
@ -460,7 +461,9 @@ pub(crate) async fn install(
build_options,
DistributionDatabase::new(client, build_dispatch, concurrency.downloads),
)
.with_reporter(PrepareReporter::from(printer).with_length(remote.len() as u64));
.with_reporter(Arc::new(
PrepareReporter::from(printer).with_length(remote.len() as u64),
));
let wheels = preparer
.prepare(remote.clone(), in_flight, resolution)
@ -519,7 +522,9 @@ pub(crate) async fn install(
.with_link_mode(link_mode)
.with_cache(cache)
.with_installer_metadata(installer_metadata)
.with_reporter(InstallReporter::from(printer).with_length(installs.len() as u64))
.with_reporter(Arc::new(
InstallReporter::from(printer).with_length(installs.len() as u64),
))
// This technically can block the runtime, but we are on the main thread and
// have no other running tasks at this point, so this lets us avoid spawning a blocking
// task.

View file

@ -1,12 +1,12 @@
use std::collections::hash_map::Entry;
use std::fmt::Write;
use std::io;
use std::path::{Path, PathBuf};
use anyhow::{bail, Context, Result};
use itertools::Itertools;
use owo_colors::OwoColorize;
use rustc_hash::{FxBuildHasher, FxHashMap};
use std::collections::hash_map::Entry;
use std::fmt::Write;
use std::io;
use std::path::{Path, PathBuf};
use std::sync::Arc;
use tracing::debug;
use url::Url;
@ -350,7 +350,7 @@ pub(crate) async fn add(
state.index(),
DistributionDatabase::new(&client, &build_dispatch, concurrency.downloads),
)
.with_reporter(ResolverReporter::from(printer))
.with_reporter(Arc::new(ResolverReporter::from(printer)))
.resolve(unnamed.into_iter())
.await?,
);

View file

@ -3,6 +3,7 @@
use std::collections::{BTreeMap, BTreeSet};
use std::fmt::Write;
use std::path::Path;
use std::sync::Arc;
use owo_colors::OwoColorize;
use rustc_hash::{FxBuildHasher, FxHashMap};
@ -625,7 +626,7 @@ async fn do_lock(
// Resolve the requirements.
let resolution = pip::operations::resolve(
ExtrasResolver::new(&hasher, state.index(), database)
.with_reporter(ResolverReporter::from(printer))
.with_reporter(Arc::new(ResolverReporter::from(printer)))
.resolve(target.members_requirements())
.await
.map_err(|err| ProjectError::Operation(err.into()))?

View file

@ -1,6 +1,7 @@
use std::collections::BTreeSet;
use std::fmt::Write;
use std::path::{Path, PathBuf};
use std::sync::Arc;
use itertools::Itertools;
use owo_colors::OwoColorize;
@ -1077,7 +1078,7 @@ pub(crate) async fn resolve_names(
state.index(),
DistributionDatabase::new(&client, &build_dispatch, concurrency.downloads),
)
.with_reporter(ResolverReporter::from(printer))
.with_reporter(Arc::new(ResolverReporter::from(printer)))
.resolve(unnamed.into_iter())
.await?,
);

View file

@ -6,6 +6,7 @@ use indicatif::{MultiProgress, ProgressBar, ProgressStyle};
use owo_colors::OwoColorize;
use rustc_hash::FxHashMap;
use url::Url;
use uv_cache::Removal;
use uv_distribution_types::{
BuildableSource, CachedDist, DistributionMetadata, Name, SourceDist, VersionOrUrlRef,