Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use dynamic dispatch to simplify reporters #10086

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions crates/uv-distribution/src/distribution_database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,10 @@ impl<'a, Context: BuildContext> DistributionDatabase<'a, Context> {

/// Set the [`Reporter`] to use for this source distribution fetcher.
#[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
}
}
Expand Down
24 changes: 20 additions & 4 deletions crates/uv-distribution/src/source/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1319,7 +1319,11 @@ 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(Facade::from)
.map(Arc::new)
.map(|reporter| reporter as _),
)
.await?;

Expand Down Expand Up @@ -1416,7 +1420,11 @@ 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(Facade::from)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really annoyed by this whole Facade setup... I want to be able to do something like:

impl <T: Reporter> uv_distribution::Reporter for Arc<dyn T> {
    fn on_build_start(&self, source: &BuildableSource) -> usize {
        self.on_build_start(source)
    }

    fn on_build_complete(&self, source: &BuildableSource, id: usize) {
        self.on_build_complete(source, id);
    }

    fn on_checkout_start(&self, url: &Url, rev: &str) -> usize {
        self.on_checkout_start(url, rev)
    }

    fn on_checkout_complete(&self, url: &Url, rev: &str, id: usize) {
        self.on_checkout_complete(url, rev, id);
    }
}

But I run into orphan trait problems.

The general problem here is that I have multiple Reporter traits. And if you implement one of them, then you should get some others "for free". But for now, I have to do this:

/// A facade for converting from [`Reporter`] to [`uv_git::Reporter`].
pub(crate) struct Facade {
    reporter: Arc<dyn Reporter>,
}

impl From<Arc<dyn Reporter>> for Facade {
    fn from(reporter: Arc<dyn Reporter>) -> Self {
        Self { reporter }
    }
}

impl uv_git::Reporter for Facade {
    fn on_checkout_start(&self, url: &Url, rev: &str) -> usize {
        self.reporter.on_checkout_start(url, rev)
    }

    fn on_checkout_complete(&self, url: &Url, rev: &str, id: usize) {
        self.reporter.on_checkout_complete(url, rev, id);
    }
}

.map(Arc::new)
.map(|reporter| reporter as _),
)
.await?;

Expand Down Expand Up @@ -1592,7 +1600,11 @@ 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(Facade::from)
.map(Arc::new)
.map(|reporter| reporter as _),
)
.await?;
}
Expand All @@ -1603,7 +1615,11 @@ 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(Facade::from)
.map(Arc::new)
.map(|reporter| reporter as _),
)
.await?;
}
Expand Down
6 changes: 3 additions & 3 deletions crates/uv-git/src/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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}");

Expand Down Expand Up @@ -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}");

Expand Down Expand Up @@ -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.
///
Expand Down
11 changes: 6 additions & 5 deletions crates/uv-git/src/source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
use std::borrow::Cow;
use std::path::{Path, PathBuf};
use std::sync::Arc;

use anyhow::Result;
use reqwest_middleware::ClientWithMiddleware;
Expand All @@ -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>,
Expand All @@ -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
}
}
Expand Down
17 changes: 9 additions & 8 deletions crates/uv-installer/src/installer.rs
Original file line number Diff line number Diff line change
@@ -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,
}
Expand Down Expand Up @@ -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
}
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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>> {
Expand Down
11 changes: 6 additions & 5 deletions crates/uv-installer/src/preparer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(Arc::new(Facade::from(reporter.clone()))),
reporter: Some(reporter),
}
}

Expand Down Expand Up @@ -269,7 +270,7 @@ 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`].
/// A facade for converting from [`Reporter`] to [`uv_distribution::Reporter`].
struct Facade {
reporter: Arc<dyn Reporter>,
}
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-requirements/src/extras.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-requirements/src/lookahead.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-requirements/src/source_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-requirements/src/unnamed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions crates/uv-resolver/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -245,15 +245,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(Arc::new(Facade::from(reporter))),
}
}

Expand Down
10 changes: 5 additions & 5 deletions crates/uv-resolver/src/resolver/provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
use uv_normalize::PackageName;
use uv_pep440::{Version, VersionSpecifiers};
Expand Down Expand Up @@ -91,9 +91,9 @@ pub trait ResolverProvider {
dist: &'io Dist,
) -> 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
Expand Down Expand Up @@ -253,9 +253,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
Expand Down
8 changes: 7 additions & 1 deletion crates/uv-resolver/src/resolver/reporter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,13 @@ pub trait Reporter: Send + Sync {

/// A facade for converting from [`Reporter`] to [`uv_distribution::Reporter`].
pub(crate) struct Facade {
pub(crate) reporter: Arc<dyn Reporter>,
reporter: Arc<dyn Reporter>,
}

impl From<Arc<dyn Reporter>> for Facade {
fn from(reporter: Arc<dyn Reporter>) -> Self {
Self { reporter }
}
}

impl uv_distribution::Reporter for Facade {
Expand Down
19 changes: 12 additions & 7 deletions crates/uv/src/commands/pip/operations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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?,
);
Expand All @@ -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?;

Expand Down Expand Up @@ -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?,
);
Expand Down Expand Up @@ -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?
}
Expand Down Expand Up @@ -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?
};
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand Down
Loading
Loading