fix(install): re-setup bin entries after running lifecycle scripts (#26752)

Fixes #26677

Some packages (like supabase) declare bin entries that don't exist until
lifecycle scripts are run. For instance, the lifecycle script downloads
a binary file which serves as a bin entrypoint.

Unfortunately you can't just defer setting up the bin entries until
after lifecycle scripts have run, because the scripts may rely on them.

I looked into this, and PNPM just re-links bin entries after running
lifecycle scripts. I think that's about the best we can do as well.

Note that we'll only re-setup bin entries for packages whose lifecycle
scripts we run. This should limit the performance cost, as typically a
given project will not have many lifecycle scripts (and of those, many
of them probably don't have bin entries to set up).

cli/npm/managed/resolvers/common/bin_entries.rs

@@ -18,6 +18,7 @@ pub struct BinEntries<'a> {
   seen_names: HashMap<&'a str, &'a NpmPackageId>,
   /// The bin entries
   entries: Vec<(&'a NpmResolutionPackage, PathBuf)>,
+  sorted: bool,
 }
 
 /// Returns the name of the default binary for the given package.
@@ -31,6 +32,20 @@ fn default_bin_name(package: &NpmResolutionPackage) -> &str {
     .map_or(package.id.nv.name.as_str(), |(_, name)| name)
 }
 
+pub fn warn_missing_entrypoint(
+  bin_name: &str,
+  package_path: &Path,
+  entrypoint: &Path,
+) {
+  log::warn!(
+    "{} Trying to set up '{}' bin for \"{}\", but the entry point \"{}\" doesn't exist.",
+    deno_terminal::colors::yellow("Warning"),
+    bin_name,
+    package_path.display(),
+    entrypoint.display()
+  );
+}
+
 impl<'a> BinEntries<'a> {
   pub fn new() -> Self {
     Self::default()
@@ -42,6 +57,7 @@ impl<'a> BinEntries<'a> {
     package: &'a NpmResolutionPackage,
     package_path: PathBuf,
   ) {
+    self.sorted = false;
     // check for a new collision, if we haven't already
     // found one
     match package.bin.as_ref().unwrap() {
@@ -79,16 +95,21 @@ impl<'a> BinEntries<'a> {
       &str, // bin name
       &str, // bin script
     ) -> Result<(), AnyError>,
+    mut filter: impl FnMut(&NpmResolutionPackage) -> bool,
   ) -> Result<(), AnyError> {
-    if !self.collisions.is_empty() {
+    if !self.collisions.is_empty() && !self.sorted {
       // walking the dependency tree to find out the depth of each package
       // is sort of expensive, so we only do it if there's a collision
       sort_by_depth(snapshot, &mut self.entries, &mut self.collisions);
+      self.sorted = true;
     }
 
     let mut seen = HashSet::new();
 
     for (package, package_path) in &self.entries {
+      if !filter(package) {
+        continue;
+      }
       if let Some(bin_entries) = &package.bin {
         match bin_entries {
           deno_npm::registry::NpmPackageVersionBinEntry::String(script) => {
@@ -118,8 +139,8 @@ impl<'a> BinEntries<'a> {
   }
 
   /// Collect the bin entries into a vec of (name, script path)
-  pub fn into_bin_files(
+  pub fn collect_bin_files(
-    mut self,
+    &mut self,
     snapshot: &NpmResolutionSnapshot,
   ) -> Vec<(String, PathBuf)> {
     let mut bins = Vec::new();
@@ -131,17 +152,18 @@ impl<'a> BinEntries<'a> {
           bins.push((name.to_string(), package_path.join(script)));
           Ok(())
         },
+        |_| true,
       )
       .unwrap();
     bins
   }
 
-  /// Finish setting up the bin entries, writing the necessary files
+  fn set_up_entries_filtered(
-  /// to disk.
-  pub fn finish(
     mut self,
     snapshot: &NpmResolutionSnapshot,
     bin_node_modules_dir_path: &Path,
+    filter: impl FnMut(&NpmResolutionPackage) -> bool,
+    mut handler: impl FnMut(&EntrySetupOutcome<'_>),
   ) -> Result<(), AnyError> {
     if !self.entries.is_empty() && !bin_node_modules_dir_path.exists() {
       std::fs::create_dir_all(bin_node_modules_dir_path).with_context(
@@ -160,18 +182,54 @@ impl<'a> BinEntries<'a> {
         Ok(())
       },
       |package, package_path, name, script| {
-        set_up_bin_entry(
+        let outcome = set_up_bin_entry(
           package,
           name,
           script,
           package_path,
           bin_node_modules_dir_path,
-        )
+        )?;
+        handler(&outcome);
+        Ok(())
       },
+      filter,
     )?;
 
     Ok(())
   }
+
+  /// Finish setting up the bin entries, writing the necessary files
+  /// to disk.
+  pub fn finish(
+    self,
+    snapshot: &NpmResolutionSnapshot,
+    bin_node_modules_dir_path: &Path,
+    handler: impl FnMut(&EntrySetupOutcome<'_>),
+  ) -> Result<(), AnyError> {
+    self.set_up_entries_filtered(
+      snapshot,
+      bin_node_modules_dir_path,
+      |_| true,
+      handler,
+    )
+  }
+
+  /// Finish setting up the bin entries, writing the necessary files
+  /// to disk.
+  pub fn finish_only(
+    self,
+    snapshot: &NpmResolutionSnapshot,
+    bin_node_modules_dir_path: &Path,
+    handler: impl FnMut(&EntrySetupOutcome<'_>),
+    only: &HashSet<&NpmPackageId>,
+  ) -> Result<(), AnyError> {
+    self.set_up_entries_filtered(
+      snapshot,
+      bin_node_modules_dir_path,
+      |package| only.contains(&package.id),
+      handler,
+    )
+  }
 }
 
 // walk the dependency tree to find out the depth of each package
@@ -233,16 +291,17 @@ fn sort_by_depth(
   });
 }
 
-pub fn set_up_bin_entry(
+pub fn set_up_bin_entry<'a>(
-  package: &NpmResolutionPackage,
+  package: &'a NpmResolutionPackage,
-  bin_name: &str,
+  bin_name: &'a str,
   #[allow(unused_variables)] bin_script: &str,
-  #[allow(unused_variables)] package_path: &Path,
+  #[allow(unused_variables)] package_path: &'a Path,
   bin_node_modules_dir_path: &Path,
-) -> Result<(), AnyError> {
+) -> Result<EntrySetupOutcome<'a>, AnyError> {
   #[cfg(windows)]
   {
     set_up_bin_shim(package, bin_name, bin_node_modules_dir_path)?;
+    Ok(EntrySetupOutcome::Success)
   }
   #[cfg(unix)]
   {
@@ -252,9 +311,8 @@ pub fn set_up_bin_entry(
       bin_script,
       package_path,
       bin_node_modules_dir_path,
-    )?;
+    )
   }
-  Ok(())
 }
 
 #[cfg(windows)]
@@ -301,14 +359,39 @@ fn make_executable_if_exists(path: &Path) -> Result<bool, AnyError> {
   Ok(true)
 }
 
+pub enum EntrySetupOutcome<'a> {
+  #[cfg_attr(windows, allow(dead_code))]
+  MissingEntrypoint {
+    bin_name: &'a str,
+    package_path: &'a Path,
+    entrypoint: PathBuf,
+    package: &'a NpmResolutionPackage,
+  },
+  Success,
+}
+
+impl<'a> EntrySetupOutcome<'a> {
+  pub fn warn_if_failed(&self) {
+    match self {
+      EntrySetupOutcome::MissingEntrypoint {
+        bin_name,
+        package_path,
+        entrypoint,
+        ..
+      } => warn_missing_entrypoint(bin_name, package_path, entrypoint),
+      EntrySetupOutcome::Success => {}
+    }
+  }
+}
+
 #[cfg(unix)]
-fn symlink_bin_entry(
+fn symlink_bin_entry<'a>(
-  _package: &NpmResolutionPackage,
+  package: &'a NpmResolutionPackage,
-  bin_name: &str,
+  bin_name: &'a str,
   bin_script: &str,
-  package_path: &Path,
+  package_path: &'a Path,
   bin_node_modules_dir_path: &Path,
-) -> Result<(), AnyError> {
+) -> Result<EntrySetupOutcome<'a>, AnyError> {
   use std::io;
   use std::os::unix::fs::symlink;
   let link = bin_node_modules_dir_path.join(bin_name);
@@ -318,14 +401,12 @@ fn symlink_bin_entry(
     format!("Can't set up '{}' bin at {}", bin_name, original.display())
   })?;
   if !found {
-    log::warn!(
+    return Ok(EntrySetupOutcome::MissingEntrypoint {
-      "{} Trying to set up '{}' bin for \"{}\", but the entry point \"{}\" doesn't exist.",
-      deno_terminal::colors::yellow("Warning"),
       bin_name,
-      package_path.display(),
+      package_path,
-      original.display()
+      entrypoint: original,
-    );
+      package,
-    return Ok(());
+    });
   }
 
   let original_relative =
@@ -348,7 +429,7 @@ fn symlink_bin_entry(
           original_relative.display()
         )
       })?;
-      return Ok(());
+      return Ok(EntrySetupOutcome::Success);
     }
     return Err(err).with_context(|| {
       format!(
@@ -359,5 +440,5 @@ fn symlink_bin_entry(
     });
   }
 
-  Ok(())
+  Ok(EntrySetupOutcome::Success)
 }

cli/npm/managed/resolvers/common/lifecycle_scripts.rs

@@ -10,6 +10,7 @@ use deno_runtime::deno_io::FromRawIoHandle;
 use deno_semver::package::PackageNv;
 use deno_semver::Version;
 use std::borrow::Cow;
+use std::collections::HashSet;
 use std::rc::Rc;
 
 use std::path::Path;
@@ -61,7 +62,7 @@ impl<'a> LifecycleScripts<'a> {
   }
 }
 
-fn has_lifecycle_scripts(
+pub fn has_lifecycle_scripts(
   package: &NpmResolutionPackage,
   package_path: &Path,
 ) -> bool {
@@ -83,7 +84,7 @@ fn is_broken_default_install_script(script: &str, package_path: &Path) -> bool {
 }
 
 impl<'a> LifecycleScripts<'a> {
-  fn can_run_scripts(&self, package_nv: &PackageNv) -> bool {
+  pub fn can_run_scripts(&self, package_nv: &PackageNv) -> bool {
     if !self.strategy.can_run_scripts() {
       return false;
     }
@@ -98,6 +99,9 @@ impl<'a> LifecycleScripts<'a> {
       PackagesAllowedScripts::None => false,
     }
   }
+  pub fn has_run_scripts(&self, package: &NpmResolutionPackage) -> bool {
+    self.strategy.has_run(package)
+  }
   /// Register a package for running lifecycle scripts, if applicable.
   ///
   /// `package_path` is the path containing the package's code (its root dir).
@@ -110,12 +114,12 @@ impl<'a> LifecycleScripts<'a> {
   ) {
     if has_lifecycle_scripts(package, &package_path) {
       if self.can_run_scripts(&package.id.nv) {
-        if !self.strategy.has_run(package) {
+        if !self.has_run_scripts(package) {
           self
             .packages_with_scripts
             .push((package, package_path.into_owned()));
         }
-      } else if !self.strategy.has_run(package)
+      } else if !self.has_run_scripts(package)
         && (self.config.explicit_install || !self.strategy.has_warned(package))
       {
         // Skip adding `esbuild` as it is known that it can work properly without lifecycle script
@@ -149,22 +153,32 @@ impl<'a> LifecycleScripts<'a> {
     self,
     snapshot: &NpmResolutionSnapshot,
     packages: &[NpmResolutionPackage],
-    root_node_modules_dir_path: Option<&Path>,
+    root_node_modules_dir_path: &Path,
     progress_bar: &ProgressBar,
   ) -> Result<(), AnyError> {
     self.warn_not_run_scripts()?;
     let get_package_path =
       |p: &NpmResolutionPackage| self.strategy.package_path(p);
     let mut failed_packages = Vec::new();
+    let mut bin_entries = BinEntries::new();
     if !self.packages_with_scripts.is_empty() {
+      let package_ids = self
+        .packages_with_scripts
+        .iter()
+        .map(|(p, _)| &p.id)
+        .collect::<HashSet<_>>();
       // get custom commands for each bin available in the node_modules dir (essentially
       // the scripts that are in `node_modules/.bin`)
-      let base =
+      let base = resolve_baseline_custom_commands(
-        resolve_baseline_custom_commands(snapshot, packages, get_package_path)?;
+        &mut bin_entries,
+        snapshot,
+        packages,
+        get_package_path,
+      )?;
       let init_cwd = &self.config.initial_cwd;
       let process_state = crate::npm::managed::npm_process_state(
         snapshot.as_valid_serialized(),
-        root_node_modules_dir_path,
+        Some(root_node_modules_dir_path),
       );
 
       let mut env_vars = crate::task_runner::real_env_vars();
@@ -221,7 +235,7 @@ impl<'a> LifecycleScripts<'a> {
                 custom_commands: custom_commands.clone(),
                 init_cwd,
                 argv: &[],
-                root_node_modules_dir: root_node_modules_dir_path,
+                root_node_modules_dir: Some(root_node_modules_dir_path),
                 stdio: Some(crate::task_runner::TaskIo {
                   stderr: TaskStdio::piped(),
                   stdout: TaskStdio::piped(),
@@ -262,6 +276,17 @@ impl<'a> LifecycleScripts<'a> {
         }
         self.strategy.did_run_scripts(package)?;
       }
+
+      // re-set up bin entries for the packages which we've run scripts for.
+      // lifecycle scripts can create files that are linked to by bin entries,
+      // and the only reliable way to handle this is to re-link bin entries
+      // (this is what PNPM does as well)
+      bin_entries.finish_only(
+        snapshot,
+        &root_node_modules_dir_path.join(".bin"),
+        |outcome| outcome.warn_if_failed(),
+        &package_ids,
+      )?;
     }
     if failed_packages.is_empty() {
       Ok(())
@@ -281,9 +306,10 @@ impl<'a> LifecycleScripts<'a> {
 // take in all (non copy) packages from snapshot,
 // and resolve the set of available binaries to create
 // custom commands available to the task runner
-fn resolve_baseline_custom_commands(
+fn resolve_baseline_custom_commands<'a>(
-  snapshot: &NpmResolutionSnapshot,
+  bin_entries: &mut BinEntries<'a>,
-  packages: &[NpmResolutionPackage],
+  snapshot: &'a NpmResolutionSnapshot,
+  packages: &'a [NpmResolutionPackage],
   get_package_path: impl Fn(&NpmResolutionPackage) -> PathBuf,
 ) -> Result<crate::task_runner::TaskCustomCommands, AnyError> {
   let mut custom_commands = crate::task_runner::TaskCustomCommands::new();
@@ -306,6 +332,7 @@ fn resolve_baseline_custom_commands(
   // doing it for packages that are set up already.
   // realistically, scripts won't be run very often so it probably isn't too big of an issue.
   resolve_custom_commands_from_packages(
+    bin_entries,
     custom_commands,
     snapshot,
     packages,
@@ -320,12 +347,12 @@ fn resolve_custom_commands_from_packages<
   'a,
   P: IntoIterator<Item = &'a NpmResolutionPackage>,
 >(
+  bin_entries: &mut BinEntries<'a>,
   mut commands: crate::task_runner::TaskCustomCommands,
   snapshot: &'a NpmResolutionSnapshot,
   packages: P,
   get_package_path: impl Fn(&'a NpmResolutionPackage) -> PathBuf,
 ) -> Result<crate::task_runner::TaskCustomCommands, AnyError> {
-  let mut bin_entries = BinEntries::new();
   for package in packages {
     let package_path = get_package_path(package);
 
@@ -333,7 +360,7 @@ fn resolve_custom_commands_from_packages<
       bin_entries.add(package, package_path);
     }
   }
-  let bins = bin_entries.into_bin_files(snapshot);
+  let bins: Vec<(String, PathBuf)> = bin_entries.collect_bin_files(snapshot);
   for (bin_name, script_path) in bins {
     commands.insert(
       bin_name.clone(),
@@ -356,7 +383,9 @@ fn resolve_custom_commands_from_deps(
   snapshot: &NpmResolutionSnapshot,
   get_package_path: impl Fn(&NpmResolutionPackage) -> PathBuf,
 ) -> Result<crate::task_runner::TaskCustomCommands, AnyError> {
+  let mut bin_entries = BinEntries::new();
   resolve_custom_commands_from_packages(
+    &mut bin_entries,
     baseline,
     snapshot,
     package

cli/npm/managed/resolvers/local.rs

@@ -55,6 +55,7 @@ use crate::util::progress_bar::ProgressMessagePrompt;
 use super::super::cache::NpmCache;
 use super::super::cache::TarballCache;
 use super::super::resolution::NpmResolution;
+use super::common::bin_entries;
 use super::common::NpmPackageFsResolver;
 use super::common::RegistryReadPermissionChecker;
 
@@ -329,8 +330,7 @@ async fn sync_resolution_with_fs(
   let mut cache_futures = FuturesUnordered::new();
   let mut newest_packages_by_name: HashMap<&String, &NpmResolutionPackage> =
     HashMap::with_capacity(package_partitions.packages.len());
-  let bin_entries =
+  let bin_entries = Rc::new(RefCell::new(bin_entries::BinEntries::new()));
-    Rc::new(RefCell::new(super::common::bin_entries::BinEntries::new()));
   let mut lifecycle_scripts =
     super::common::lifecycle_scripts::LifecycleScripts::new(
       lifecycle_scripts,
@@ -658,7 +658,28 @@ async fn sync_resolution_with_fs(
   // 7. Set up `node_modules/.bin` entries for packages that need it.
   {
     let bin_entries = std::mem::take(&mut *bin_entries.borrow_mut());
-    bin_entries.finish(snapshot, &bin_node_modules_dir_path)?;
+    bin_entries.finish(
+      snapshot,
+      &bin_node_modules_dir_path,
+      |setup_outcome| {
+        match setup_outcome {
+          bin_entries::EntrySetupOutcome::MissingEntrypoint {
+            package,
+            package_path,
+            ..
+          } if super::common::lifecycle_scripts::has_lifecycle_scripts(
+            package,
+            package_path,
+          ) && lifecycle_scripts.can_run_scripts(&package.id.nv)
+            && !lifecycle_scripts.has_run_scripts(package) =>
+          {
+            // ignore, it might get fixed when the lifecycle scripts run.
+            // if not, we'll warn then
+          }
+          outcome => outcome.warn_if_failed(),
+        }
+      },
+    )?;
   }
 
   // 8. Create symlinks for the workspace packages
@@ -708,7 +729,7 @@ async fn sync_resolution_with_fs(
     .finish(
       snapshot,
       &package_partitions.packages,
-      Some(root_node_modules_dir_path),
+      root_node_modules_dir_path,
       progress_bar,
     )
     .await?;

tests/registry/npm/@denotest/bin-created-by-lifecycle/1.0.0/install.mjs

@@ -0,0 +1,3 @@
+import * as fs from "node:fs";
+
+fs.writeFileSync("./testbin.js", "#!/usr/bin/env node\nconsole.log('run testbin');");

tests/registry/npm/@denotest/bin-created-by-lifecycle/1.0.0/package.json

@@ -0,0 +1,10 @@
+{
+  "name": "@denotest/bin-created-by-lifecycle",
+  "version": "1.0.0",
+  "scripts": {
+    "install": "node install.mjs"
+  },
+  "bin": {
+    "testbin": "testbin.js"
+  }
+}

tests/specs/npm/bin_entry_created_by_lifecycle/__test__.jsonc

@@ -0,0 +1,29 @@
+{
+  "tempDir": true,
+  "tests": {
+    "all_at_once": {
+      "steps": [
+        {
+          "args": "install --allow-scripts",
+          "output": "all_at_once_install.out"
+        },
+        { "args": "task run-testbin", "output": "run_testbin.out" }
+      ]
+    },
+    "separate_steps": {
+      "steps": [
+        { "if": "unix", "args": "install", "output": "install_warn.out" },
+        {
+          "if": "windows",
+          "args": "install",
+          "output": "install_warn_windows.out"
+        },
+        {
+          "args": "install --allow-scripts",
+          "output": "Initialize @denotest/bin-created-by-lifecycle@1.0.0: running 'install' script\n"
+        },
+        { "args": "task run-testbin", "output": "run_testbin.out" }
+      ]
+    }
+  }
+}

tests/specs/npm/bin_entry_created_by_lifecycle/all_at_once_install.out

@@ -0,0 +1,4 @@
+Download http://localhost:4260/@denotest%2fbin-created-by-lifecycle
+Download http://localhost:4260/@denotest/bin-created-by-lifecycle/1.0.0.tgz
+Initialize @denotest/bin-created-by-lifecycle@1.0.0
+Initialize @denotest/bin-created-by-lifecycle@1.0.0: running 'install' script

tests/specs/npm/bin_entry_created_by_lifecycle/install_warn.out

@@ -0,0 +1,10 @@
+Download http://localhost:4260/@denotest%2fbin-created-by-lifecycle
+Download http://localhost:4260/@denotest/bin-created-by-lifecycle/1.0.0.tgz
+Initialize @denotest/bin-created-by-lifecycle@1.0.0
+Warning Trying to set up 'testbin' bin for "[WILDCARD]bin-created-by-lifecycle", but the entry point "[WILDCARD]testbin.js" doesn't exist.
+Warning The following packages contained npm lifecycle scripts (preinstall/install/postinstall) that were not executed:
+┠─ npm:@denotest/bin-created-by-lifecycle@1.0.0
+┃
+┠─ This may cause the packages to not work correctly.
+┖─ To run lifecycle scripts, use the `--allow-scripts` flag with `deno install`:
+   deno install --allow-scripts=npm:@denotest/bin-created-by-lifecycle@1.0.0

tests/specs/npm/bin_entry_created_by_lifecycle/install_warn_windows.out

@@ -0,0 +1,9 @@
+Download http://localhost:4260/@denotest%2fbin-created-by-lifecycle
+Download http://localhost:4260/@denotest/bin-created-by-lifecycle/1.0.0.tgz
+Initialize @denotest/bin-created-by-lifecycle@1.0.0
+Warning The following packages contained npm lifecycle scripts (preinstall/install/postinstall) that were not executed:
+┠─ npm:@denotest/bin-created-by-lifecycle@1.0.0
+┃
+┠─ This may cause the packages to not work correctly.
+┖─ To run lifecycle scripts, use the `--allow-scripts` flag with `deno install`:
+   deno install --allow-scripts=npm:@denotest/bin-created-by-lifecycle@1.0.0

tests/specs/npm/bin_entry_created_by_lifecycle/package.json

@@ -0,0 +1,8 @@
+{
+  "dependencies": {
+    "@denotest/bin-created-by-lifecycle": "1.0.0"
+  },
+  "scripts": {
+    "run-testbin": "testbin"
+  }
+}

tests/specs/npm/bin_entry_created_by_lifecycle/run_testbin.out

@@ -0,0 +1,2 @@
+Task run-testbin testbin
+run testbin