Avoid double resolution during source builds (#656)

## Summary

This PR ensures that we re-use the resolution to install the build
dependencies when building a source distribution. Currently, we only
pass along the list of requirements, and then use the `Finder` to map
each requirement to a distribution. But we already determine the correct
distribution when resolving!

Closes https://github.com/astral-sh/puffin/issues/655.

crates/puffin-resolver/src/finder.rs

@@ -8,7 +8,7 @@ use anyhow::Result;
 use futures::StreamExt;
 use rustc_hash::FxHashMap;
 
-use distribution_types::{Dist, Metadata};
+use distribution_types::{Dist, Metadata, Resolution};
 use pep440_rs::Version;
 use pep508_rs::{Requirement, VersionOrUrl};
 use platform_tags::{TagPriority, Tags};
@@ -18,7 +18,6 @@ use puffin_normalize::PackageName;
 use pypi_types::IndexUrl;
 
 use crate::error::ResolveError;
-use crate::resolution::ResolutionManifest;
 
 pub struct DistFinder<'a> {
     tags: &'a Tags,
@@ -48,12 +47,9 @@ impl<'a> DistFinder<'a> {
     }
 
     /// Resolve a set of pinned packages into a set of wheels.
-    pub async fn resolve(
-        &self,
-        requirements: &[Requirement],
-    ) -> Result<ResolutionManifest, ResolveError> {
+    pub async fn resolve(&self, requirements: &[Requirement]) -> Result<Resolution, ResolveError> {
         if requirements.is_empty() {
-            return Ok(ResolutionManifest::default());
+            return Ok(Resolution::default());
         }
 
         // A channel to fetch package metadata (e.g., given `flask`, fetch all versions).
@@ -99,7 +95,7 @@ impl<'a> DistFinder<'a> {
             if let Some(reporter) = self.reporter.as_ref() {
                 reporter.on_complete();
             }
-            return Ok(ResolutionManifest::new(resolution));
+            return Ok(Resolution::new(resolution));
         }
 
         // Otherwise, wait for the package stream to complete.
@@ -133,7 +129,7 @@ impl<'a> DistFinder<'a> {
             reporter.on_complete();
         }
 
-        Ok(ResolutionManifest::new(resolution))
+        Ok(Resolution::new(resolution))
     }
 
     /// select a version that satisfies the requirement, preferring wheels to source distributions.

crates/puffin-resolver/src/lib.rs

@@ -3,7 +3,7 @@ pub use finder::{DistFinder, Reporter as FinderReporter};
 pub use manifest::Manifest;
 pub use prerelease_mode::PreReleaseMode;
 pub use pubgrub::PubGrubReportFormatter;
-pub use resolution::{ResolutionGraph, ResolutionManifest};
+pub use resolution::ResolutionGraph;
 pub use resolution_mode::ResolutionMode;
 pub use resolution_options::ResolutionOptions;
 pub use resolver::{

crates/puffin-resolver/src/resolution.rs

@@ -2,7 +2,6 @@ use std::hash::BuildHasherDefault;
 
 use anyhow::Result;
 use colored::Colorize;
-use itertools::Itertools;
 use petgraph::visit::EdgeRef;
 use petgraph::Direction;
 use pubgrub::range::Range;
@@ -11,9 +10,9 @@ use pubgrub::type_aliases::SelectedDependencies;
 use rustc_hash::FxHashMap;
 use url::Url;
 
-use distribution_types::{BuiltDist, Dist, Metadata, PackageId, SourceDist};
-use pep440_rs::{Version, VersionSpecifier, VersionSpecifiers};
-use pep508_rs::{Requirement, VerbatimUrl, VersionOrUrl};
+use distribution_types::{Dist, Metadata, PackageId};
+use pep440_rs::Version;
+use pep508_rs::{Requirement, VerbatimUrl};
 use puffin_normalize::{ExtraName, PackageName};
 use puffin_traits::OnceMap;
 use pypi_types::Metadata21;
@@ -22,51 +21,6 @@ use crate::pins::FilePins;
 use crate::pubgrub::{PubGrubDistribution, PubGrubPackage, PubGrubPriority, PubGrubVersion};
 use crate::ResolveError;
 
-/// A set of packages pinned at specific versions.
-#[derive(Debug, Default)]
-pub struct ResolutionManifest(FxHashMap<PackageName, Dist>);
-
-impl ResolutionManifest {
-    /// Create a new resolution from the given pinned packages.
-    pub(crate) fn new(packages: FxHashMap<PackageName, Dist>) -> Self {
-        Self(packages)
-    }
-
-    /// Return the distribution for the given package name, if it exists.
-    pub fn get(&self, package_name: &PackageName) -> Option<&Dist> {
-        self.0.get(package_name)
-    }
-
-    /// Iterate over the [`PackageName`] entities in this resolution.
-    pub fn packages(&self) -> impl Iterator<Item = &PackageName> {
-        self.0.keys()
-    }
-
-    /// Iterate over the [`Dist`] entities in this resolution.
-    pub fn into_distributions(self) -> impl Iterator<Item = Dist> {
-        self.0.into_values()
-    }
-
-    /// Return the number of distributions in this resolution.
-    pub fn len(&self) -> usize {
-        self.0.len()
-    }
-
-    /// Return `true` if there are no pinned packages in this resolution.
-    pub fn is_empty(&self) -> bool {
-        self.0.is_empty()
-    }
-
-    /// Return the set of [`Requirement`]s that this resolution represents.
-    pub fn requirements(&self) -> Vec<Requirement> {
-        self.0
-            .values()
-            .sorted_by_key(|package| package.name())
-            .map(as_requirement)
-            .collect()
-    }
-}
-
 /// A complete resolution graph in which every node represents a pinned package and every edge
 /// represents a dependency between two pinned packages.
 #[derive(Debug)]
@@ -224,7 +178,9 @@ impl ResolutionGraph {
         nodes.sort_unstable_by_key(|(_, package)| package.name());
         self.petgraph
             .node_indices()
-            .map(|node| as_requirement(&self.petgraph[node]))
+            .map(|node| &self.petgraph[node])
+            .cloned()
+            .map(Requirement::from)
             .collect()
     }
 
@@ -283,9 +239,9 @@ impl std::fmt::Display for ResolutionGraph {
     }
 }
 
-impl From<ResolutionGraph> for ResolutionManifest {
+impl From<ResolutionGraph> for distribution_types::Resolution {
     fn from(graph: ResolutionGraph) -> Self {
-        Self(
+        Self::new(
             graph
                 .petgraph
                 .node_indices()
@@ -300,58 +256,6 @@ impl From<ResolutionGraph> for ResolutionManifest {
     }
 }
 
-/// Create a [`Requirement`] from a [`Dist`].
-fn as_requirement(dist: &Dist) -> Requirement {
-    match dist {
-        Dist::Built(BuiltDist::Registry(wheel)) => Requirement {
-            name: wheel.name.clone(),
-            extras: None,
-            version_or_url: Some(VersionOrUrl::VersionSpecifier(VersionSpecifiers::from(
-                VersionSpecifier::equals_version(wheel.version.clone()),
-            ))),
-            marker: None,
-        },
-        Dist::Built(BuiltDist::DirectUrl(wheel)) => Requirement {
-            name: wheel.filename.name.clone(),
-            extras: None,
-            version_or_url: Some(VersionOrUrl::Url(wheel.url.clone())),
-            marker: None,
-        },
-        Dist::Built(BuiltDist::Path(wheel)) => Requirement {
-            name: wheel.filename.name.clone(),
-            extras: None,
-            version_or_url: Some(VersionOrUrl::Url(wheel.url.clone())),
-            marker: None,
-        },
-        Dist::Source(SourceDist::Registry(sdist)) => Requirement {
-            name: sdist.name.clone(),
-            extras: None,
-            version_or_url: Some(VersionOrUrl::VersionSpecifier(VersionSpecifiers::from(
-                VersionSpecifier::equals_version(sdist.version.clone()),
-            ))),
-            marker: None,
-        },
-        Dist::Source(SourceDist::DirectUrl(sdist)) => Requirement {
-            name: sdist.name.clone(),
-            extras: None,
-            version_or_url: Some(VersionOrUrl::Url(sdist.url.clone())),
-            marker: None,
-        },
-        Dist::Source(SourceDist::Git(sdist)) => Requirement {
-            name: sdist.name.clone(),
-            extras: None,
-            version_or_url: Some(VersionOrUrl::Url(sdist.url.clone())),
-            marker: None,
-        },
-        Dist::Source(SourceDist::Path(sdist)) => Requirement {
-            name: sdist.name.clone(),
-            extras: None,
-            version_or_url: Some(VersionOrUrl::Url(sdist.url.clone())),
-            marker: None,
-        },
-    }
-}
-
 #[derive(Debug)]
 pub enum Diagnostic {
     MissingExtra {

crates/puffin-resolver/tests/resolver.rs

@@ -12,6 +12,7 @@ use anyhow::Result;
 use chrono::{DateTime, Utc};
 use once_cell::sync::Lazy;
 
+use distribution_types::Resolution;
 use pep508_rs::{MarkerEnvironment, Requirement, StringVersion};
 use platform_host::{Arch, Os, Platform};
 use platform_tags::Tags;
@@ -53,13 +54,13 @@ impl BuildContext for DummyContext {
     fn resolve<'a>(
         &'a self,
         _requirements: &'a [Requirement],
-    ) -> Pin<Box<dyn Future<Output = Result<Vec<Requirement>>> + Send + 'a>> {
+    ) -> Pin<Box<dyn Future<Output = Result<Resolution>> + Send + 'a>> {
         panic!("The test should not need to build source distributions")
     }
 
     fn install<'a>(
         &'a self,
-        _requirements: &'a [Requirement],
+        _resolution: &'a Resolution,
         _venv: &'a Virtualenv,
     ) -> Pin<Box<dyn Future<Output = Result<()>> + Send + 'a>> {
         panic!("The test should not need to build source distributions")