diff --git a/cli/npm/managed/resolvers/common/bin_entries.rs b/cli/npm/managed/resolvers/common/bin_entries.rs index 4524ce8326..e4a1845689 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) } diff --git a/cli/npm/managed/resolvers/common/lifecycle_scripts.rs b/cli/npm/managed/resolvers/common/lifecycle_scripts.rs index 5735f52482..5c5755c819 100644 --- a/cli/npm/managed/resolvers/common/lifecycle_scripts.rs +++ b/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::>(); // get custom commands for each bin available in the node_modules dir (essentially // the scripts that are in `node_modules/.bin`) - let base = - resolve_baseline_custom_commands(snapshot, packages, get_package_path)?; + let base = resolve_baseline_custom_commands( + &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( - snapshot: &NpmResolutionSnapshot, - packages: &[NpmResolutionPackage], +fn resolve_baseline_custom_commands<'a>( + bin_entries: &mut BinEntries<'a>, + snapshot: &'a NpmResolutionSnapshot, + packages: &'a [NpmResolutionPackage], get_package_path: impl Fn(&NpmResolutionPackage) -> PathBuf, ) -> Result { 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, >( + 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 { - 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 { + let mut bin_entries = BinEntries::new(); resolve_custom_commands_from_packages( + &mut bin_entries, baseline, snapshot, package diff --git a/cli/npm/managed/resolvers/local.rs b/cli/npm/managed/resolvers/local.rs index eddb0dc9b6..50c5bd2689 100644 --- a/cli/npm/managed/resolvers/local.rs +++ b/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 = - Rc::new(RefCell::new(super::common::bin_entries::BinEntries::new())); + let bin_entries = Rc::new(RefCell::new(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?; diff --git a/tests/registry/npm/@denotest/bin-created-by-lifecycle/1.0.0/install.mjs b/tests/registry/npm/@denotest/bin-created-by-lifecycle/1.0.0/install.mjs new file mode 100644 index 0000000000..31020fcdf9 --- /dev/null +++ b/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');"); \ No newline at end of file diff --git a/tests/registry/npm/@denotest/bin-created-by-lifecycle/1.0.0/package.json b/tests/registry/npm/@denotest/bin-created-by-lifecycle/1.0.0/package.json new file mode 100644 index 0000000000..ad8dea002e --- /dev/null +++ b/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" + } +} \ No newline at end of file diff --git a/tests/specs/npm/bin_entry_created_by_lifecycle/__test__.jsonc b/tests/specs/npm/bin_entry_created_by_lifecycle/__test__.jsonc new file mode 100644 index 0000000000..665aec823d --- /dev/null +++ b/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" } + ] + } + } +} diff --git a/tests/specs/npm/bin_entry_created_by_lifecycle/all_at_once_install.out b/tests/specs/npm/bin_entry_created_by_lifecycle/all_at_once_install.out new file mode 100644 index 0000000000..bfaba3caf2 --- /dev/null +++ b/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 diff --git a/tests/specs/npm/bin_entry_created_by_lifecycle/install_warn.out b/tests/specs/npm/bin_entry_created_by_lifecycle/install_warn.out new file mode 100644 index 0000000000..864a3f6f51 --- /dev/null +++ b/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 diff --git a/tests/specs/npm/bin_entry_created_by_lifecycle/install_warn_windows.out b/tests/specs/npm/bin_entry_created_by_lifecycle/install_warn_windows.out new file mode 100644 index 0000000000..6838088735 --- /dev/null +++ b/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 diff --git a/tests/specs/npm/bin_entry_created_by_lifecycle/package.json b/tests/specs/npm/bin_entry_created_by_lifecycle/package.json new file mode 100644 index 0000000000..9a8941ed9c --- /dev/null +++ b/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" + } +} diff --git a/tests/specs/npm/bin_entry_created_by_lifecycle/run_testbin.out b/tests/specs/npm/bin_entry_created_by_lifecycle/run_testbin.out new file mode 100644 index 0000000000..a03f8bc58e --- /dev/null +++ b/tests/specs/npm/bin_entry_created_by_lifecycle/run_testbin.out @@ -0,0 +1,2 @@ +Task run-testbin testbin +run testbin