From c371b2a492c60f47ce6b96b4df129c5d01706e1b Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Tue, 12 Nov 2024 09:23:39 -0800 Subject: 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 | 139 +++++++++++++++++++----- 1 file changed, 110 insertions(+), 29 deletions(-) (limited to 'cli/npm/managed/resolvers/common/bin_entries.rs') diff --git a/cli/npm/managed/resolvers/common/bin_entries.rs b/cli/npm/managed/resolvers/common/bin_entries.rs index 4524ce832..e4a184568 100644 --- a/cli/npm/managed/resolvers/common/bin_entries.rs +++ b/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( - mut self, + pub fn collect_bin_files( + &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 - /// to disk. - pub fn finish( + fn set_up_entries_filtered( 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( - package: &NpmResolutionPackage, - bin_name: &str, +pub fn set_up_bin_entry<'a>( + package: &'a NpmResolutionPackage, + 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, 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 { 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( - _package: &NpmResolutionPackage, - bin_name: &str, +fn symlink_bin_entry<'a>( + package: &'a NpmResolutionPackage, + bin_name: &'a str, bin_script: &str, - package_path: &Path, + package_path: &'a Path, bin_node_modules_dir_path: &Path, -) -> Result<(), AnyError> { +) -> Result, 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!( - "{} Trying to set up '{}' bin for \"{}\", but the entry point \"{}\" doesn't exist.", - deno_terminal::colors::yellow("Warning"), + return Ok(EntrySetupOutcome::MissingEntrypoint { bin_name, - package_path.display(), - original.display() - ); - return Ok(()); + package_path, + entrypoint: original, + package, + }); } 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) } -- cgit v1.2.3