From 19c6d655b5321e5168dbfdc50dc06733d3c3bc1b Mon Sep 17 00:00:00 2001
From: Charlie Marsh <charlie.r.marsh@gmail.com>
Date: Mon, 8 Jan 2024 16:19:54 -0500
Subject: [PATCH] Avoid duplicated source distribution handling in url (#841)

## Summary

Right now, both the callback _and_ the "We have no compatible wheel"
paths have a lot of repeated code. This PR changes the callback to
_just_ remove all the wheels and handle the download, and the rest of
the method following the callback is responsible for finding and
building any wheels.
---
 .../src/source/manifest.rs                    |  7 ---
 crates/puffin-distribution/src/source/mod.rs  | 58 +++++--------------
 2 files changed, 14 insertions(+), 51 deletions(-)

diff --git a/crates/puffin-distribution/src/source/manifest.rs b/crates/puffin-distribution/src/source/manifest.rs
index 667479500..03c01f933 100644
--- a/crates/puffin-distribution/src/source/manifest.rs
+++ b/crates/puffin-distribution/src/source/manifest.rs
@@ -9,13 +9,6 @@ use pypi_types::Metadata21;
 pub(crate) struct Manifest(FxHashMap<WheelFilename, DiskFilenameAndMetadata>);
 
 impl Manifest {
-    /// Initialize a [`Manifest`] from an iterator over entries.
-    pub(crate) fn from_iter(
-        iter: impl IntoIterator<Item = (WheelFilename, DiskFilenameAndMetadata)>,
-    ) -> Self {
-        Self(iter.into_iter().collect())
-    }
-
     /// Find a compatible wheel in the cache.
     pub(crate) fn find_compatible(
         &self,
diff --git a/crates/puffin-distribution/src/source/mod.rs b/crates/puffin-distribution/src/source/mod.rs
index e37a90457..885f4c641 100644
--- a/crates/puffin-distribution/src/source/mod.rs
+++ b/crates/puffin-distribution/src/source/mod.rs
@@ -141,53 +141,31 @@ impl<'a, T: BuildContext> SourceDistCachedBuilder<'a, T> {
     ) -> Result<BuiltWheelMetadata, SourceDistError> {
         let cache_entry = cache_shard.entry(METADATA);
 
-        let download_and_build = |response| {
+        let download = |response| {
             async {
                 // At this point, we're seeing a new or updated source distribution; delete all
-                // wheels, and rebuild.
+                // wheels, and redownload.
                 match fs::remove_dir_all(&cache_entry.dir()).await {
                     Ok(()) => debug!("Cleared built wheels and metadata for {source_dist}"),
                     Err(err) if err.kind() == std::io::ErrorKind::NotFound => (),
                     Err(err) => return Err(err.into()),
                 }
 
-                debug!("Downloading and building source distribution: {source_dist}");
-                let task = self
-                    .reporter
-                    .as_ref()
-                    .map(|reporter| reporter.on_build_start(source_dist));
+                debug!("Downloading source distribution: {source_dist}");
 
                 // Download the source distribution.
                 let source_dist_entry = cache_shard.entry(filename);
-                let cache_dir = self
-                    .persist_source_dist_url(response, source_dist, filename, &source_dist_entry)
+                self.persist_source_dist_url(response, source_dist, filename, &source_dist_entry)
                     .await?;
 
-                // Build the source distribution.
-                let (disk_filename, wheel_filename, metadata) = self
-                    .build_source_dist(source_dist, cache_dir, subdirectory, &cache_entry)
-                    .await?;
-
-                if let Some(task) = task {
-                    if let Some(reporter) = self.reporter.as_ref() {
-                        reporter.on_build_complete(source_dist, task);
-                    }
-                }
-
-                Ok(Manifest::from_iter([(
-                    wheel_filename,
-                    DiskFilenameAndMetadata {
-                        disk_filename,
-                        metadata,
-                    },
-                )]))
+                Ok(Manifest::default())
             }
-            .instrument(info_span!("download_and_build", source_dist = %source_dist))
+            .instrument(info_span!("download", source_dist = %source_dist))
         };
         let req = self.cached_client.uncached().get(url.clone()).build()?;
         let manifest = self
             .cached_client
-            .get_cached_with_callback(req, &cache_entry, download_and_build)
+            .get_cached_with_callback(req, &cache_entry, download)
             .await
             .map_err(|err| match err {
                 CachedClientError::Callback(err) => err,
@@ -208,23 +186,15 @@ impl<'a, T: BuildContext> SourceDistCachedBuilder<'a, T> {
             .as_ref()
             .map(|reporter| reporter.on_build_start(source_dist));
 
-        // Start by downloading the source distribution.
-        let response = self
-            .cached_client
-            .uncached()
-            .get(url.clone())
-            .send()
-            .await
-            .map_err(puffin_client::Error::RequestMiddlewareError)?;
-
-        let source_dist_entry = cache_shard.entry(filename);
-        let cache_dir = self
-            .persist_source_dist_url(response, source_dist, filename, &source_dist_entry)
-            .await?;
-
         // Build the source distribution.
+        let source_dist_entry = cache_shard.entry(filename);
         let (disk_filename, wheel_filename, metadata) = self
-            .build_source_dist(source_dist, cache_dir, subdirectory, &cache_entry)
+            .build_source_dist(
+                source_dist,
+                source_dist_entry.path(),
+                subdirectory,
+                &cache_entry,
+            )
             .await?;
 
         if let Some(task) = task {