Split install plan into builder and struct (#955)

The `InstallPlan` does a lot of work in the constructor, which I tend to
feel is an anti-pattern. With cache refresh, it's also going to need to
be made `async`, so it really feels like it should be a clearer method
rather than an async, fallible constructor that does a bunch of IO. This
PR splits into a `Planner` (with a `build` method) and a `Plan`.

crates/puffin-cli/src/commands/pip_install.rs

@@ -21,7 +21,7 @@ use puffin_cache::Cache;
 use puffin_client::{FlatIndex, FlatIndexClient, RegistryClient, RegistryClientBuilder};
 use puffin_dispatch::BuildDispatch;
 use puffin_installer::{
-    BuiltEditable, Downloader, InstallPlan, Reinstall, ResolvedEditable, SitePackages,
+    BuiltEditable, Downloader, Plan, Planner, Reinstall, ResolvedEditable, SitePackages,
 };
 use puffin_interpreter::{Interpreter, Virtualenv};
 use puffin_normalize::PackageName;
@@ -470,22 +470,16 @@ async fn install(
         .into_iter()
         .map(ResolvedEditable::Built)
         .collect::<Vec<_>>();
-    let InstallPlan {
+
+    let Plan {
         local,
         remote,
         reinstalls,
         extraneous: _,
-    } = InstallPlan::from_requirements(
-        &requirements,
-        editables,
-        site_packages,
-        reinstall,
-        index_urls,
-        cache,
-        venv,
-        tags,
-    )
-    .context("Failed to determine installation plan")?;
+    } = Planner::with_requirements(&requirements)
+        .with_editable_requirements(editables)
+        .build(site_packages, reinstall, index_urls, cache, venv, tags)
+        .context("Failed to determine installation plan")?;
 
     // Nothing to do.
     if remote.is_empty() && local.is_empty() && reinstalls.is_empty() {
@@ -524,7 +518,6 @@ async fn install(
         let downloader = Downloader::new(cache, tags, client, build_dispatch)
             .with_reporter(DownloadReporter::from(printer).with_length(remote.len() as u64));
 
-        // STOPSHIP(charlie): This needs to be shared!
         let wheels = downloader
             .download(remote, in_flight)
             .await

crates/puffin-cli/src/commands/pip_sync.rs

@@ -12,7 +12,7 @@ use platform_tags::Tags;
 use puffin_cache::Cache;
 use puffin_client::{FlatIndex, FlatIndexClient, RegistryClient, RegistryClientBuilder};
 use puffin_dispatch::BuildDispatch;
-use puffin_installer::{Downloader, InstallPlan, Reinstall, ResolvedEditable, SitePackages};
+use puffin_installer::{Downloader, Plan, Planner, Reinstall, ResolvedEditable, SitePackages};
 use puffin_interpreter::Virtualenv;
 use puffin_resolver::InMemoryIndex;
 use puffin_traits::{InFlight, SetupPyStrategy};
@@ -111,22 +111,22 @@ pub(crate) async fn pip_sync(
 
     // Partition into those that should be linked from the cache (`local`), those that need to be
     // downloaded (`remote`), and those that should be removed (`extraneous`).
-    let InstallPlan {
+    let Plan {
         local,
         remote,
         reinstalls,
         extraneous,
-    } = InstallPlan::from_requirements(
-        &requirements,
-        resolved_editables.editables,
-        site_packages,
-        reinstall,
-        &index_locations,
-        &cache,
-        &venv,
-        tags,
-    )
-    .context("Failed to determine installation plan")?;
+    } = Planner::with_requirements(&requirements)
+        .with_editable_requirements(resolved_editables.editables)
+        .build(
+            site_packages,
+            reinstall,
+            &index_locations,
+            &cache,
+            &venv,
+            tags,
+        )
+        .context("Failed to determine installation plan")?;
 
     // Nothing to do.
     if remote.is_empty() && local.is_empty() && reinstalls.is_empty() && extraneous.is_empty() {

crates/puffin-dispatch/src/lib.rs

@@ -14,7 +14,7 @@ use pep508_rs::Requirement;
 use puffin_build::{SourceBuild, SourceBuildContext};
 use puffin_cache::Cache;
 use puffin_client::{FlatIndex, RegistryClient};
-use puffin_installer::{Downloader, InstallPlan, Installer, Reinstall, SitePackages};
+use puffin_installer::{Downloader, Installer, Plan, Planner, Reinstall, SitePackages};
 use puffin_interpreter::{Interpreter, Virtualenv};
 use puffin_resolver::{InMemoryIndex, Manifest, ResolutionOptions, Resolver};
 use puffin_traits::{BuildContext, BuildKind, InFlight, SetupPyStrategy};
@@ -149,14 +149,12 @@ impl<'a> BuildContext for BuildDispatch<'a> {
             let site_packages =
                 SitePackages::from_executable(venv).context("Failed to list installed packages")?;
 
-            let InstallPlan {
+            let Plan {
                 local,
                 remote,
                 reinstalls,
                 extraneous,
-            } = InstallPlan::from_requirements(
-                &resolution.requirements(),
-                Vec::new(),
+            } = Planner::with_requirements(&resolution.requirements()).build(
                 site_packages,
                 &Reinstall::None,
                 self.index_locations,

crates/puffin-installer/src/lib.rs

@@ -1,7 +1,7 @@
 pub use downloader::{Downloader, Reporter as DownloadReporter};
 pub use editable::{BuiltEditable, ResolvedEditable};
 pub use installer::{Installer, Reporter as InstallReporter};
-pub use plan::{InstallPlan, Reinstall};
+pub use plan::{Plan, Planner, Reinstall};
 pub use site_packages::SitePackages;
 pub use uninstall::uninstall;
 

crates/puffin-installer/src/plan.rs

@@ -17,51 +17,57 @@ use puffin_normalize::PackageName;
 
 use crate::{ResolvedEditable, SitePackages};
 
-#[derive(Debug, Default)]
-pub struct InstallPlan {
-    /// The distributions that are not already installed in the current environment, but are
-    /// available in the local cache.
-    pub local: Vec<CachedDist>,
-
-    /// The distributions that are not already installed in the current environment, and are
-    /// not available in the local cache.
-    pub remote: Vec<Requirement>,
+/// A planner to generate an [`Plan`] based on a set of requirements.
+#[derive(Debug)]
+pub struct Planner<'a> {
+    requirements: &'a [Requirement],
+    editable_requirements: Vec<ResolvedEditable>,
+}
 
-    /// Any distributions that are already installed in the current environment, but will be
-    /// re-installed (including upgraded) to satisfy the requirements.
-    pub reinstalls: Vec<InstalledDist>,
+impl<'a> Planner<'a> {
+    /// Set the requirements use in the [`Plan`].
+    #[must_use]
+    pub fn with_requirements(requirements: &'a [Requirement]) -> Self {
+        Self {
+            requirements,
+            editable_requirements: Vec::new(),
+        }
+    }
 
-    /// Any distributions that are already installed in the current environment, and are
-    /// _not_ necessary to satisfy the requirements.
-    pub extraneous: Vec<InstalledDist>,
-}
+    /// Set the editable requirements use in the [`Plan`].
+    #[must_use]
+    pub fn with_editable_requirements(self, editable_requirements: Vec<ResolvedEditable>) -> Self {
+        Self {
+            editable_requirements,
+            ..self
+        }
+    }
 
-impl InstallPlan {
     /// Partition a set of requirements into those that should be linked from the cache, those that
     /// need to be downloaded, and those that should be removed.
-    #[allow(clippy::too_many_arguments)]
-    pub fn from_requirements(
-        requirements: &[Requirement],
-        editable_requirements: Vec<ResolvedEditable>,
+    pub fn build(
+        self,
         mut site_packages: SitePackages,
         reinstall: &Reinstall,
         index_locations: &IndexLocations,
         cache: &Cache,
         venv: &Virtualenv,
         tags: &Tags,
-    ) -> Result<Self> {
+    ) -> Result<Plan> {
         // Index all the already-downloaded wheels in the cache.
         let mut registry_index = RegistryWheelIndex::new(cache, tags, index_locations);
 
         let mut local = vec![];
         let mut remote = vec![];
         let mut reinstalls = vec![];
         let mut extraneous = vec![];
-        let mut seen =
-            FxHashSet::with_capacity_and_hasher(requirements.len(), BuildHasherDefault::default());
+        let mut seen = FxHashSet::with_capacity_and_hasher(
+            self.requirements.len(),
+            BuildHasherDefault::default(),
+        );
 
         // Remove any editable requirements.
-        for requirement in editable_requirements {
+        for requirement in self.editable_requirements {
             // If we see the same requirement twice, then we have a conflict.
             if !seen.insert(requirement.name().clone()) {
                 bail!(
@@ -99,7 +105,7 @@ impl InstallPlan {
             }
         }
 
-        for requirement in requirements {
+        for requirement in self.requirements {
             // Filter out incompatible requirements.
             if !requirement.evaluate_markers(venv.interpreter().markers(), &[]) {
                 continue;
@@ -322,7 +328,7 @@ impl InstallPlan {
             }
         }
 
-        Ok(InstallPlan {
+        Ok(Plan {
             local,
             remote,
             reinstalls,
@@ -331,6 +337,25 @@ impl InstallPlan {
     }
 }
 
+#[derive(Debug, Default)]
+pub struct Plan {
+    /// The distributions that are not already installed in the current environment, but are
+    /// available in the local cache.
+    pub local: Vec<CachedDist>,
+
+    /// The distributions that are not already installed in the current environment, and are
+    /// not available in the local cache.
+    pub remote: Vec<Requirement>,
+
+    /// Any distributions that are already installed in the current environment, but will be
+    /// re-installed (including upgraded) to satisfy the requirements.
+    pub reinstalls: Vec<InstalledDist>,
+
+    /// Any distributions that are already installed in the current environment, and are
+    /// _not_ necessary to satisfy the requirements.
+    pub extraneous: Vec<InstalledDist>,
+}
+
 #[derive(Debug)]
 pub enum Reinstall {
     /// Don't reinstall any packages; respect the existing installation.