From 36ebc03f177cc7db5deb93f4d403cafbed756eb5 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Tue, 24 Sep 2024 12:23:57 -0700 Subject: [PATCH] fix(cli): Warn on not-run lifecycle scripts with global cache (#25786) Refactors the lifecycle scripts code to extract out the common functionality and then uses that to provide a warning in the global resolver. While ideally we would still support them with the global cache, for now a warning is at least better than the status quo (where people are unaware why their packages aren't working). --- cli/args/flags.rs | 3 +- cli/args/mod.rs | 10 +- cli/npm/managed/resolvers/common.rs | 5 +- .../{local => common}/bin_entries.rs | 41 +- .../resolvers/common/lifecycle_scripts.rs | 306 +++++++++++++++ cli/npm/managed/resolvers/global.rs | 100 ++++- cli/npm/managed/resolvers/local.rs | 362 ++++++------------ cli/npm/managed/resolvers/mod.rs | 1 + .../install_deprecated_package/install.out | 4 +- .../npm/lifecycle_scripts/__test__.jsonc | 29 +- .../all_lifecycles_not_run.out | 4 +- .../all_lifecycles_not_run_global.out | 13 + .../future_install_all_lifecycles_not_run.out | 4 +- .../lifecycle_scripts/node_gyp_not_found.out | 1 + .../lifecycle_scripts/node_gyp_not_run.out | 4 +- .../lifecycle_scripts/only_warns_first1.out | 4 +- 16 files changed, 590 insertions(+), 301 deletions(-) rename cli/npm/managed/resolvers/{local => common}/bin_entries.rs (90%) create mode 100644 cli/npm/managed/resolvers/common/lifecycle_scripts.rs create mode 100644 tests/specs/npm/lifecycle_scripts/all_lifecycles_not_run_global.out diff --git a/cli/args/flags.rs b/cli/args/flags.rs index d325ce7bcb..10fa07bed2 100644 --- a/cli/args/flags.rs +++ b/cli/args/flags.rs @@ -544,7 +544,8 @@ pub enum CaData { #[derive(Clone, Debug, Eq, PartialEq, Default)] pub struct LifecycleScriptsConfig { pub allowed: PackagesAllowedScripts, - pub initial_cwd: Option, + pub initial_cwd: PathBuf, + pub root_dir: PathBuf, } #[derive(Debug, Clone, Eq, PartialEq, Default)] diff --git a/cli/args/mod.rs b/cli/args/mod.rs index b8a05f325c..1c92777ae3 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -1652,14 +1652,8 @@ impl CliOptions { pub fn lifecycle_scripts_config(&self) -> LifecycleScriptsConfig { LifecycleScriptsConfig { allowed: self.flags.allow_scripts.clone(), - initial_cwd: if matches!( - self.flags.allow_scripts, - PackagesAllowedScripts::None - ) { - None - } else { - Some(self.initial_cwd.clone()) - }, + initial_cwd: self.initial_cwd.clone(), + root_dir: self.workspace().root_dir_path(), } } } diff --git a/cli/npm/managed/resolvers/common.rs b/cli/npm/managed/resolvers/common.rs index 1893aa56ae..620daf4b30 100644 --- a/cli/npm/managed/resolvers/common.rs +++ b/cli/npm/managed/resolvers/common.rs @@ -1,5 +1,8 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. +pub mod bin_entries; +pub mod lifecycle_scripts; + use std::collections::HashMap; use std::io::ErrorKind; use std::path::Path; @@ -134,7 +137,7 @@ impl RegistryReadPermissionChecker { /// Caches all the packages in parallel. pub async fn cache_packages( - packages: Vec, + packages: &[NpmResolutionPackage], tarball_cache: &Arc, ) -> Result<(), AnyError> { let mut futures_unordered = futures::stream::FuturesUnordered::new(); diff --git a/cli/npm/managed/resolvers/local/bin_entries.rs b/cli/npm/managed/resolvers/common/bin_entries.rs similarity index 90% rename from cli/npm/managed/resolvers/local/bin_entries.rs rename to cli/npm/managed/resolvers/common/bin_entries.rs index 980a2653b7..25a020c2bc 100644 --- a/cli/npm/managed/resolvers/local/bin_entries.rs +++ b/cli/npm/managed/resolvers/common/bin_entries.rs @@ -12,12 +12,12 @@ use std::path::Path; use std::path::PathBuf; #[derive(Default)] -pub(super) struct BinEntries { +pub struct BinEntries<'a> { /// Packages that have colliding bin names - collisions: HashSet, - seen_names: HashMap, + collisions: HashSet<&'a NpmPackageId>, + seen_names: HashMap<&'a str, &'a NpmPackageId>, /// The bin entries - entries: Vec<(NpmResolutionPackage, PathBuf)>, + entries: Vec<(&'a NpmResolutionPackage, PathBuf)>, } /// Returns the name of the default binary for the given package. @@ -31,37 +31,32 @@ fn default_bin_name(package: &NpmResolutionPackage) -> &str { .map_or(package.id.nv.name.as_str(), |(_, name)| name) } -impl BinEntries { - pub(super) fn new() -> Self { +impl<'a> BinEntries<'a> { + pub fn new() -> Self { Self::default() } /// Add a new bin entry (package with a bin field) - pub(super) fn add( + pub fn add( &mut self, - package: NpmResolutionPackage, + package: &'a NpmResolutionPackage, package_path: PathBuf, ) { // check for a new collision, if we haven't already // found one match package.bin.as_ref().unwrap() { deno_npm::registry::NpmPackageVersionBinEntry::String(_) => { - let bin_name = default_bin_name(&package); + let bin_name = default_bin_name(package); - if let Some(other) = self - .seen_names - .insert(bin_name.to_string(), package.id.clone()) - { - self.collisions.insert(package.id.clone()); + if let Some(other) = self.seen_names.insert(bin_name, &package.id) { + self.collisions.insert(&package.id); self.collisions.insert(other); } } deno_npm::registry::NpmPackageVersionBinEntry::Map(entries) => { for name in entries.keys() { - if let Some(other) = - self.seen_names.insert(name.to_string(), package.id.clone()) - { - self.collisions.insert(package.id.clone()); + if let Some(other) = self.seen_names.insert(name, &package.id) { + self.collisions.insert(&package.id); self.collisions.insert(other); } } @@ -117,7 +112,7 @@ impl BinEntries { } /// Collect the bin entries into a vec of (name, script path) - pub(super) fn into_bin_files( + pub fn into_bin_files( mut self, snapshot: &NpmResolutionSnapshot, ) -> Vec<(String, PathBuf)> { @@ -133,7 +128,7 @@ impl BinEntries { /// Finish setting up the bin entries, writing the necessary files /// to disk. - pub(super) fn finish( + pub fn finish( mut self, snapshot: &NpmResolutionSnapshot, bin_node_modules_dir_path: &Path, @@ -162,8 +157,8 @@ impl BinEntries { // that has a bin entry, then sort them by depth fn sort_by_depth( snapshot: &NpmResolutionSnapshot, - bin_entries: &mut [(NpmResolutionPackage, PathBuf)], - collisions: &mut HashSet, + bin_entries: &mut [(&NpmResolutionPackage, PathBuf)], + collisions: &mut HashSet<&NpmPackageId>, ) { enum Entry<'a> { Pkg(&'a NpmPackageId), @@ -217,7 +212,7 @@ fn sort_by_depth( }); } -pub(super) fn set_up_bin_entry( +pub fn set_up_bin_entry( package: &NpmResolutionPackage, bin_name: &str, #[allow(unused_variables)] bin_script: &str, diff --git a/cli/npm/managed/resolvers/common/lifecycle_scripts.rs b/cli/npm/managed/resolvers/common/lifecycle_scripts.rs new file mode 100644 index 0000000000..a3c72634b3 --- /dev/null +++ b/cli/npm/managed/resolvers/common/lifecycle_scripts.rs @@ -0,0 +1,306 @@ +// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. + +use super::bin_entries::BinEntries; +use crate::args::LifecycleScriptsConfig; +use deno_npm::resolution::NpmResolutionSnapshot; +use deno_semver::package::PackageNv; +use std::borrow::Cow; +use std::rc::Rc; + +use std::path::Path; +use std::path::PathBuf; + +use deno_core::error::AnyError; +use deno_npm::NpmResolutionPackage; + +pub trait LifecycleScriptsStrategy { + fn can_run_scripts(&self) -> bool { + true + } + fn package_path(&self, package: &NpmResolutionPackage) -> PathBuf; + + fn warn_on_scripts_not_run( + &self, + packages: &[(&NpmResolutionPackage, PathBuf)], + ) -> Result<(), AnyError>; + + fn has_warned(&self, package: &NpmResolutionPackage) -> bool; + + fn has_run(&self, package: &NpmResolutionPackage) -> bool; + + fn did_run_scripts( + &self, + package: &NpmResolutionPackage, + ) -> Result<(), AnyError>; +} + +pub struct LifecycleScripts<'a> { + packages_with_scripts: Vec<(&'a NpmResolutionPackage, PathBuf)>, + packages_with_scripts_not_run: Vec<(&'a NpmResolutionPackage, PathBuf)>, + + config: &'a LifecycleScriptsConfig, + strategy: Box, +} + +impl<'a> LifecycleScripts<'a> { + pub fn new( + config: &'a LifecycleScriptsConfig, + strategy: T, + ) -> Self { + Self { + config, + packages_with_scripts: Vec::new(), + packages_with_scripts_not_run: Vec::new(), + strategy: Box::new(strategy), + } + } +} + +fn has_lifecycle_scripts( + package: &NpmResolutionPackage, + package_path: &Path, +) -> bool { + if let Some(install) = package.scripts.get("install") { + // default script + if !is_broken_default_install_script(install, package_path) { + return true; + } + } + package.scripts.contains_key("preinstall") + || package.scripts.contains_key("postinstall") +} + +// npm defaults to running `node-gyp rebuild` if there is a `binding.gyp` file +// but it always fails if the package excludes the `binding.gyp` file when they publish. +// (for example, `fsevents` hits this) +fn is_broken_default_install_script(script: &str, package_path: &Path) -> bool { + script == "node-gyp rebuild" && !package_path.join("binding.gyp").exists() +} + +impl<'a> LifecycleScripts<'a> { + fn can_run_scripts(&self, package_nv: &PackageNv) -> bool { + if !self.strategy.can_run_scripts() { + return false; + } + use crate::args::PackagesAllowedScripts; + match &self.config.allowed { + PackagesAllowedScripts::All => true, + // TODO: make this more correct + PackagesAllowedScripts::Some(allow_list) => allow_list.iter().any(|s| { + let s = s.strip_prefix("npm:").unwrap_or(s); + s == package_nv.name || s == package_nv.to_string() + }), + PackagesAllowedScripts::None => false, + } + } + /// Register a package for running lifecycle scripts, if applicable. + /// + /// `package_path` is the path containing the package's code (its root dir). + /// `package_meta_path` is the path to serve as the base directory for lifecycle + /// script-related metadata (e.g. to store whether the scripts have been run already) + pub fn add( + &mut self, + package: &'a NpmResolutionPackage, + package_path: Cow, + ) { + if has_lifecycle_scripts(package, &package_path) { + if self.can_run_scripts(&package.id.nv) { + if !self.strategy.has_run(package) { + self + .packages_with_scripts + .push((package, package_path.into_owned())); + } + } else if !self.strategy.has_run(package) + && !self.strategy.has_warned(package) + { + self + .packages_with_scripts_not_run + .push((package, package_path.into_owned())); + } + } + } + + pub fn warn_not_run_scripts(&self) -> Result<(), AnyError> { + if !self.packages_with_scripts_not_run.is_empty() { + self + .strategy + .warn_on_scripts_not_run(&self.packages_with_scripts_not_run)?; + } + Ok(()) + } + + pub async fn finish( + self, + snapshot: &NpmResolutionSnapshot, + packages: &[NpmResolutionPackage], + root_node_modules_dir_path: Option<&Path>, + ) -> Result<(), AnyError> { + self.warn_not_run_scripts()?; + let get_package_path = + |p: &NpmResolutionPackage| self.strategy.package_path(p); + let mut failed_packages = Vec::new(); + if !self.packages_with_scripts.is_empty() { + // 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 init_cwd = &self.config.initial_cwd; + let process_state = crate::npm::managed::npm_process_state( + snapshot.as_valid_serialized(), + root_node_modules_dir_path, + ); + + let mut env_vars = crate::task_runner::real_env_vars(); + env_vars.insert( + crate::args::NPM_RESOLUTION_STATE_ENV_VAR_NAME.to_string(), + process_state, + ); + for (package, package_path) in self.packages_with_scripts { + // add custom commands for binaries from the package's dependencies. this will take precedence over the + // baseline commands, so if the package relies on a bin that conflicts with one higher in the dependency tree, the + // correct bin will be used. + let custom_commands = resolve_custom_commands_from_deps( + base.clone(), + package, + snapshot, + get_package_path, + )?; + for script_name in ["preinstall", "install", "postinstall"] { + if let Some(script) = package.scripts.get(script_name) { + if script_name == "install" + && is_broken_default_install_script(script, &package_path) + { + continue; + } + let exit_code = crate::task_runner::run_task( + crate::task_runner::RunTaskOptions { + task_name: script_name, + script, + cwd: &package_path, + env_vars: env_vars.clone(), + custom_commands: custom_commands.clone(), + init_cwd, + argv: &[], + root_node_modules_dir: root_node_modules_dir_path, + }, + ) + .await?; + if exit_code != 0 { + log::warn!( + "error: script '{}' in '{}' failed with exit code {}", + script_name, + package.id.nv, + exit_code, + ); + failed_packages.push(&package.id.nv); + // assume if earlier script fails, later ones will fail too + break; + } + } + } + self.strategy.did_run_scripts(package)?; + } + } + if failed_packages.is_empty() { + Ok(()) + } else { + Err(AnyError::msg(format!( + "failed to run scripts for packages: {}", + failed_packages + .iter() + .map(|p| p.to_string()) + .collect::>() + .join(", ") + ))) + } + } +} + +// 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], + get_package_path: impl Fn(&NpmResolutionPackage) -> PathBuf, +) -> Result { + let mut custom_commands = crate::task_runner::TaskCustomCommands::new(); + custom_commands + .insert("npx".to_string(), Rc::new(crate::task_runner::NpxCommand)); + + custom_commands + .insert("npm".to_string(), Rc::new(crate::task_runner::NpmCommand)); + + custom_commands + .insert("node".to_string(), Rc::new(crate::task_runner::NodeCommand)); + + custom_commands.insert( + "node-gyp".to_string(), + Rc::new(crate::task_runner::NodeGypCommand), + ); + + // TODO: this recreates the bin entries which could be redoing some work, but the ones + // we compute earlier in `sync_resolution_with_fs` may not be exhaustive (because we skip + // 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( + custom_commands, + snapshot, + packages, + get_package_path, + ) +} + +// resolves the custom commands from an iterator of packages +// and adds them to the existing custom commands. +// note that this will overwrite any existing custom commands +fn resolve_custom_commands_from_packages< + 'a, + P: IntoIterator, +>( + 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); + + if package.bin.is_some() { + bin_entries.add(package, package_path); + } + } + let bins = bin_entries.into_bin_files(snapshot); + for (bin_name, script_path) in bins { + commands.insert( + bin_name.clone(), + Rc::new(crate::task_runner::NodeModulesFileRunCommand { + command_name: bin_name, + path: script_path, + }), + ); + } + + Ok(commands) +} + +// resolves the custom commands from the dependencies of a package +// and adds them to the existing custom commands. +// note that this will overwrite any existing custom commands. +fn resolve_custom_commands_from_deps( + baseline: crate::task_runner::TaskCustomCommands, + package: &NpmResolutionPackage, + snapshot: &NpmResolutionSnapshot, + get_package_path: impl Fn(&NpmResolutionPackage) -> PathBuf, +) -> Result { + resolve_custom_commands_from_packages( + baseline, + snapshot, + package + .dependencies + .values() + .map(|id| snapshot.package_from_id(id).unwrap()), + get_package_path, + ) +} diff --git a/cli/npm/managed/resolvers/global.rs b/cli/npm/managed/resolvers/global.rs index 7f8f285f3e..187e6b2774 100644 --- a/cli/npm/managed/resolvers/global.rs +++ b/cli/npm/managed/resolvers/global.rs @@ -2,16 +2,19 @@ //! Code for global npm cache resolution. +use std::borrow::Cow; use std::path::Path; use std::path::PathBuf; use std::sync::Arc; +use crate::colors; use async_trait::async_trait; use deno_ast::ModuleSpecifier; use deno_core::error::AnyError; use deno_core::url::Url; use deno_npm::NpmPackageCacheFolderId; use deno_npm::NpmPackageId; +use deno_npm::NpmResolutionPackage; use deno_npm::NpmSystemInfo; use deno_runtime::deno_fs::FileSystem; use deno_runtime::deno_node::NodePermissions; @@ -19,10 +22,14 @@ use node_resolver::errors::PackageFolderResolveError; use node_resolver::errors::PackageNotFoundError; use node_resolver::errors::ReferrerNotFoundError; +use crate::args::LifecycleScriptsConfig; +use crate::cache::FastInsecureHasher; + use super::super::cache::NpmCache; use super::super::cache::TarballCache; use super::super::resolution::NpmResolution; use super::common::cache_packages; +use super::common::lifecycle_scripts::LifecycleScriptsStrategy; use super::common::NpmPackageFsResolver; use super::common::RegistryReadPermissionChecker; @@ -34,6 +41,7 @@ pub struct GlobalNpmPackageResolver { resolution: Arc, system_info: NpmSystemInfo, registry_read_permission_checker: RegistryReadPermissionChecker, + lifecycle_scripts: LifecycleScriptsConfig, } impl GlobalNpmPackageResolver { @@ -43,6 +51,7 @@ impl GlobalNpmPackageResolver { tarball_cache: Arc, resolution: Arc, system_info: NpmSystemInfo, + lifecycle_scripts: LifecycleScriptsConfig, ) -> Self { Self { registry_read_permission_checker: RegistryReadPermissionChecker::new( @@ -53,6 +62,7 @@ impl GlobalNpmPackageResolver { tarball_cache, resolution, system_info, + lifecycle_scripts, } } } @@ -149,8 +159,7 @@ impl NpmPackageFsResolver for GlobalNpmPackageResolver { let package_partitions = self .resolution .all_system_packages_partitioned(&self.system_info); - - cache_packages(package_partitions.packages, &self.tarball_cache).await?; + cache_packages(&package_partitions.packages, &self.tarball_cache).await?; // create the copy package folders for copy in package_partitions.copy_packages { @@ -159,6 +168,18 @@ impl NpmPackageFsResolver for GlobalNpmPackageResolver { .ensure_copy_package(©.get_package_cache_folder_id())?; } + let mut lifecycle_scripts = + super::common::lifecycle_scripts::LifecycleScripts::new( + &self.lifecycle_scripts, + GlobalLifecycleScripts::new(self, &self.lifecycle_scripts.root_dir), + ); + for package in &package_partitions.packages { + let package_folder = self.cache.package_folder_for_nv(&package.id.nv); + lifecycle_scripts.add(package, Cow::Borrowed(&package_folder)); + } + + lifecycle_scripts.warn_not_run_scripts()?; + Ok(()) } @@ -172,3 +193,78 @@ impl NpmPackageFsResolver for GlobalNpmPackageResolver { .ensure_registry_read_permission(permissions, path) } } + +struct GlobalLifecycleScripts<'a> { + resolver: &'a GlobalNpmPackageResolver, + path_hash: u64, +} + +impl<'a> GlobalLifecycleScripts<'a> { + fn new(resolver: &'a GlobalNpmPackageResolver, root_dir: &Path) -> Self { + let mut hasher = FastInsecureHasher::new_without_deno_version(); + hasher.write(root_dir.to_string_lossy().as_bytes()); + let path_hash = hasher.finish(); + Self { + resolver, + path_hash, + } + } + + fn warned_scripts_file(&self, package: &NpmResolutionPackage) -> PathBuf { + self + .package_path(package) + .join(format!(".scripts-warned-{}", self.path_hash)) + } +} + +impl<'a> super::common::lifecycle_scripts::LifecycleScriptsStrategy + for GlobalLifecycleScripts<'a> +{ + fn can_run_scripts(&self) -> bool { + false + } + fn package_path(&self, package: &NpmResolutionPackage) -> PathBuf { + self.resolver.cache.package_folder_for_nv(&package.id.nv) + } + + fn warn_on_scripts_not_run( + &self, + packages: &[(&NpmResolutionPackage, PathBuf)], + ) -> std::result::Result<(), deno_core::anyhow::Error> { + log::warn!("{} The following packages contained npm lifecycle scripts ({}) that were not executed:", colors::yellow("Warning"), colors::gray("preinstall/install/postinstall")); + for (package, _) in packages { + log::warn!("┠─ {}", colors::gray(format!("npm:{}", package.id.nv))); + } + log::warn!("┃"); + log::warn!( + "┠─ {}", + colors::italic("This may cause the packages to not work correctly.") + ); + log::warn!("┠─ {}", colors::italic("Lifecycle scripts are only supported when using a `node_modules` directory.")); + log::warn!( + "┠─ {}", + colors::italic("Enable it in your deno config file:") + ); + log::warn!("┖─ {}", colors::bold("\"nodeModulesDir\": \"auto\"")); + + for (package, _) in packages { + std::fs::write(self.warned_scripts_file(package), "")?; + } + Ok(()) + } + + fn did_run_scripts( + &self, + _package: &NpmResolutionPackage, + ) -> std::result::Result<(), deno_core::anyhow::Error> { + Ok(()) + } + + fn has_warned(&self, package: &NpmResolutionPackage) -> bool { + self.warned_scripts_file(package).exists() + } + + fn has_run(&self, _package: &NpmResolutionPackage) -> bool { + false + } +} diff --git a/cli/npm/managed/resolvers/local.rs b/cli/npm/managed/resolvers/local.rs index c582c369e2..5a90f252de 100644 --- a/cli/npm/managed/resolvers/local.rs +++ b/cli/npm/managed/resolvers/local.rs @@ -2,8 +2,6 @@ //! Code for local node_modules resolution. -mod bin_entries; - use std::borrow::Cow; use std::cell::RefCell; use std::cmp::Ordering; @@ -18,11 +16,9 @@ use std::rc::Rc; use std::sync::Arc; use crate::args::LifecycleScriptsConfig; -use crate::args::PackagesAllowedScripts; use crate::colors; use async_trait::async_trait; use deno_ast::ModuleSpecifier; -use deno_core::anyhow; use deno_core::anyhow::Context; use deno_core::error::AnyError; use deno_core::futures::stream::FuturesUnordered; @@ -272,77 +268,10 @@ impl NpmPackageFsResolver for LocalNpmPackageResolver { } } -// 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], - local_registry_dir: &Path, -) -> Result { - let mut custom_commands = crate::task_runner::TaskCustomCommands::new(); - custom_commands - .insert("npx".to_string(), Rc::new(crate::task_runner::NpxCommand)); - - custom_commands - .insert("npm".to_string(), Rc::new(crate::task_runner::NpmCommand)); - - custom_commands - .insert("node".to_string(), Rc::new(crate::task_runner::NodeCommand)); - - custom_commands.insert( - "node-gyp".to_string(), - Rc::new(crate::task_runner::NodeGypCommand), - ); - - // TODO: this recreates the bin entries which could be redoing some work, but the ones - // we compute earlier in `sync_resolution_with_fs` may not be exhaustive (because we skip - // 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( - custom_commands, - snapshot, - packages, - local_registry_dir, - ) -} - -// resolves the custom commands from an iterator of packages -// and adds them to the existing custom commands. -// note that this will overwrite any existing custom commands -fn resolve_custom_commands_from_packages< - 'a, - P: IntoIterator, ->( - mut commands: crate::task_runner::TaskCustomCommands, - snapshot: &'a NpmResolutionSnapshot, - packages: P, - local_registry_dir: &Path, -) -> Result { - let mut bin_entries = bin_entries::BinEntries::new(); - for package in packages { - let package_path = - local_node_modules_package_path(local_registry_dir, package); - - if package.bin.is_some() { - bin_entries.add(package.clone(), package_path); - } - } - let bins = bin_entries.into_bin_files(snapshot); - for (bin_name, script_path) in bins { - commands.insert( - bin_name.clone(), - Rc::new(crate::task_runner::NodeModulesFileRunCommand { - command_name: bin_name, - path: script_path, - }), - ); - } - - Ok(commands) -} - -fn local_node_modules_package_path( +/// `node_modules/.deno//node_modules/` +/// +/// Where the actual package is stored. +fn local_node_modules_package_contents_path( local_registry_dir: &Path, package: &NpmResolutionPackage, ) -> PathBuf { @@ -354,62 +283,6 @@ fn local_node_modules_package_path( .join(&package.id.nv.name) } -// resolves the custom commands from the dependencies of a package -// and adds them to the existing custom commands. -// note that this will overwrite any existing custom commands. -fn resolve_custom_commands_from_deps( - baseline: crate::task_runner::TaskCustomCommands, - package: &NpmResolutionPackage, - snapshot: &NpmResolutionSnapshot, - local_registry_dir: &Path, -) -> Result { - resolve_custom_commands_from_packages( - baseline, - snapshot, - package - .dependencies - .values() - .map(|id| snapshot.package_from_id(id).unwrap()), - local_registry_dir, - ) -} - -fn can_run_scripts( - allow_scripts: &PackagesAllowedScripts, - package_nv: &PackageNv, -) -> bool { - match allow_scripts { - PackagesAllowedScripts::All => true, - // TODO: make this more correct - PackagesAllowedScripts::Some(allow_list) => allow_list.iter().any(|s| { - let s = s.strip_prefix("npm:").unwrap_or(s); - s == package_nv.name || s == package_nv.to_string() - }), - PackagesAllowedScripts::None => false, - } -} - -// npm defaults to running `node-gyp rebuild` if there is a `binding.gyp` file -// but it always fails if the package excludes the `binding.gyp` file when they publish. -// (for example, `fsevents` hits this) -fn is_broken_default_install_script(script: &str, package_path: &Path) -> bool { - script == "node-gyp rebuild" && !package_path.join("binding.gyp").exists() -} - -fn has_lifecycle_scripts( - package: &NpmResolutionPackage, - package_path: &Path, -) -> bool { - if let Some(install) = package.scripts.get("install") { - // default script - if !is_broken_default_install_script(install, package_path) { - return true; - } - } - package.scripts.contains_key("preinstall") - || package.scripts.contains_key("postinstall") -} - /// Creates a pnpm style folder structure. #[allow(clippy::too_many_arguments)] async fn sync_resolution_with_fs( @@ -460,9 +333,15 @@ 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(bin_entries::BinEntries::new())); - let mut packages_with_scripts = Vec::with_capacity(2); - let mut packages_with_scripts_not_run = Vec::new(); + let bin_entries = + Rc::new(RefCell::new(super::common::bin_entries::BinEntries::new())); + let mut lifecycle_scripts = + super::common::lifecycle_scripts::LifecycleScripts::new( + lifecycle_scripts, + LocalLifecycleScripts { + deno_local_registry_dir: &deno_local_registry_dir, + }, + ); let packages_with_deprecation_warnings = Arc::new(Mutex::new(Vec::new())); for package in &package_partitions.packages { if let Some(current_pkg) = @@ -518,9 +397,7 @@ async fn sync_resolution_with_fs( .await??; if package.bin.is_some() { - bin_entries_to_setup - .borrow_mut() - .add(package.clone(), package_path); + bin_entries_to_setup.borrow_mut().add(package, package_path); } if let Some(deprecated) = &package.deprecated { @@ -538,21 +415,7 @@ async fn sync_resolution_with_fs( let sub_node_modules = folder_path.join("node_modules"); let package_path = join_package_name(&sub_node_modules, &package.id.nv.name); - if has_lifecycle_scripts(package, &package_path) { - let scripts_run = folder_path.join(".scripts-run"); - let has_warned = folder_path.join(".scripts-warned"); - if can_run_scripts(&lifecycle_scripts.allowed, &package.id.nv) { - if !scripts_run.exists() { - packages_with_scripts.push(( - package.clone(), - package_path, - scripts_run, - )); - } - } else if !scripts_run.exists() && !has_warned.exists() { - packages_with_scripts_not_run.push((has_warned, package.id.nv.clone())); - } - } + lifecycle_scripts.add(package, package_path.into()); } while let Some(result) = cache_futures.next().await { @@ -789,74 +652,12 @@ async fn sync_resolution_with_fs( } } - if !packages_with_scripts.is_empty() { - // 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, - &package_partitions.packages, - &deno_local_registry_dir, - )?; - let init_cwd = lifecycle_scripts.initial_cwd.as_deref().unwrap(); - let process_state = crate::npm::managed::npm_process_state( - snapshot.as_valid_serialized(), - Some(root_node_modules_dir_path), - ); - - let mut env_vars = crate::task_runner::real_env_vars(); - env_vars.insert( - crate::args::NPM_RESOLUTION_STATE_ENV_VAR_NAME.to_string(), - process_state, - ); - for (package, package_path, scripts_run_path) in packages_with_scripts { - // add custom commands for binaries from the package's dependencies. this will take precedence over the - // baseline commands, so if the package relies on a bin that conflicts with one higher in the dependency tree, the - // correct bin will be used. - let custom_commands = resolve_custom_commands_from_deps( - base.clone(), - &package, - snapshot, - &deno_local_registry_dir, - )?; - for script_name in ["preinstall", "install", "postinstall"] { - if let Some(script) = package.scripts.get(script_name) { - if script_name == "install" - && is_broken_default_install_script(script, &package_path) - { - continue; - } - let exit_code = - crate::task_runner::run_task(crate::task_runner::RunTaskOptions { - task_name: script_name, - script, - cwd: &package_path, - env_vars: env_vars.clone(), - custom_commands: custom_commands.clone(), - init_cwd, - argv: &[], - root_node_modules_dir: Some(root_node_modules_dir_path), - }) - .await?; - if exit_code != 0 { - anyhow::bail!( - "script '{}' in '{}' failed with exit code {}", - script_name, - package.id.nv, - exit_code, - ); - } - } - } - fs::write(scripts_run_path, "")?; - } - } - { let packages_with_deprecation_warnings = packages_with_deprecation_warnings.lock(); if !packages_with_deprecation_warnings.is_empty() { log::warn!( - "{} Following packages are deprecated:", + "{} The following packages are deprecated:", colors::yellow("Warning") ); let len = packages_with_deprecation_warnings.len(); @@ -870,7 +671,7 @@ async fn sync_resolution_with_fs( ); } else { log::warn!( - "┗─ {}", + "┖─ {}", colors::gray(format!("npm:{:?} ({})", package_id, msg)) ); } @@ -878,36 +679,13 @@ async fn sync_resolution_with_fs( } } - if !packages_with_scripts_not_run.is_empty() { - log::warn!("{} Following packages contained npm lifecycle scripts ({}) that were not executed:", colors::yellow("Warning"), colors::gray("preinstall/install/postinstall")); - - for (_, package_nv) in packages_with_scripts_not_run.iter() { - log::warn!("┠─ {}", colors::gray(format!("npm:{package_nv}"))); - } - - log::warn!("┃"); - log::warn!( - "┠─ {}", - colors::italic("This may cause the packages to not work correctly.") - ); - log::warn!("┗─ {}", colors::italic("To run lifecycle scripts, use the `--allow-scripts` flag with `deno install`:")); - let packages_comma_separated = packages_with_scripts_not_run - .iter() - .map(|(_, p)| format!("npm:{p}")) - .collect::>() - .join(","); - log::warn!( - " {}", - colors::bold(format!( - "deno install --allow-scripts={}", - packages_comma_separated - )) - ); - - for (scripts_warned_path, _) in packages_with_scripts_not_run { - let _ignore_err = fs::write(scripts_warned_path, ""); - } - } + lifecycle_scripts + .finish( + snapshot, + &package_partitions.packages, + Some(root_node_modules_dir_path), + ) + .await?; setup_cache.save(); drop(single_process_lock); @@ -916,6 +694,98 @@ async fn sync_resolution_with_fs( Ok(()) } +/// `node_modules/.deno//` +fn local_node_modules_package_folder( + local_registry_dir: &Path, + package: &NpmResolutionPackage, +) -> PathBuf { + local_registry_dir.join(get_package_folder_id_folder_name( + &package.get_package_cache_folder_id(), + )) +} + +struct LocalLifecycleScripts<'a> { + deno_local_registry_dir: &'a Path, +} + +impl<'a> LocalLifecycleScripts<'a> { + /// `node_modules/.deno//.scripts-run` + fn ran_scripts_file(&self, package: &NpmResolutionPackage) -> PathBuf { + local_node_modules_package_folder(self.deno_local_registry_dir, package) + .join(".scripts-run") + } + + /// `node_modules/.deno//.scripts-warned` + fn warned_scripts_file(&self, package: &NpmResolutionPackage) -> PathBuf { + local_node_modules_package_folder(self.deno_local_registry_dir, package) + .join(".scripts-warned") + } +} + +impl<'a> super::common::lifecycle_scripts::LifecycleScriptsStrategy + for LocalLifecycleScripts<'a> +{ + fn package_path(&self, package: &NpmResolutionPackage) -> PathBuf { + local_node_modules_package_contents_path( + self.deno_local_registry_dir, + package, + ) + } + + fn did_run_scripts( + &self, + package: &NpmResolutionPackage, + ) -> std::result::Result<(), deno_core::anyhow::Error> { + std::fs::write(self.ran_scripts_file(package), "")?; + Ok(()) + } + + fn warn_on_scripts_not_run( + &self, + packages: &[(&NpmResolutionPackage, std::path::PathBuf)], + ) -> Result<(), AnyError> { + if !packages.is_empty() { + log::warn!("{} The following packages contained npm lifecycle scripts ({}) that were not executed:", colors::yellow("Warning"), colors::gray("preinstall/install/postinstall")); + + for (package, _) in packages { + log::warn!("┠─ {}", colors::gray(format!("npm:{}", package.id.nv))); + } + + log::warn!("┃"); + log::warn!( + "┠─ {}", + colors::italic("This may cause the packages to not work correctly.") + ); + log::warn!("┖─ {}", colors::italic("To run lifecycle scripts, use the `--allow-scripts` flag with `deno install`:")); + let packages_comma_separated = packages + .iter() + .map(|(p, _)| format!("npm:{}", p.id.nv)) + .collect::>() + .join(","); + log::warn!( + " {}", + colors::bold(format!( + "deno install --allow-scripts={}", + packages_comma_separated + )) + ); + + for (package, _) in packages { + let _ignore_err = fs::write(self.warned_scripts_file(package), ""); + } + } + Ok(()) + } + + fn has_warned(&self, package: &NpmResolutionPackage) -> bool { + self.warned_scripts_file(package).exists() + } + + fn has_run(&self, package: &NpmResolutionPackage) -> bool { + self.ran_scripts_file(package).exists() + } +} + // Uses BTreeMap to preserve the ordering of the elements in memory, to ensure // the file generated from this datastructure is deterministic. // See: https://github.com/denoland/deno/issues/24479 diff --git a/cli/npm/managed/resolvers/mod.rs b/cli/npm/managed/resolvers/mod.rs index f5d9e4b057..234a6e4dba 100644 --- a/cli/npm/managed/resolvers/mod.rs +++ b/cli/npm/managed/resolvers/mod.rs @@ -54,6 +54,7 @@ pub fn create_npm_fs_resolver( tarball_cache, resolution, system_info, + lifecycle_scripts, )), } } diff --git a/tests/specs/install/install_deprecated_package/install.out b/tests/specs/install/install_deprecated_package/install.out index 34e9010a37..95c0759092 100644 --- a/tests/specs/install/install_deprecated_package/install.out +++ b/tests/specs/install/install_deprecated_package/install.out @@ -2,5 +2,5 @@ Add npm:@denotest/deprecated-package@1.0.0 Download http://localhost:4260/@denotest/deprecated-package Download http://localhost:4260/@denotest/deprecated-package/1.0.0.tgz Initialize @denotest/deprecated-package@1.0.0 -Warning Following packages are deprecated: -┗─ npm:@denotest/deprecated-package@1.0.0 (Deprecated version) +Warning The following packages are deprecated: +┖─ npm:@denotest/deprecated-package@1.0.0 (Deprecated version) diff --git a/tests/specs/npm/lifecycle_scripts/__test__.jsonc b/tests/specs/npm/lifecycle_scripts/__test__.jsonc index f7a722a8b2..201e424974 100644 --- a/tests/specs/npm/lifecycle_scripts/__test__.jsonc +++ b/tests/specs/npm/lifecycle_scripts/__test__.jsonc @@ -1,7 +1,7 @@ { + "tempDir": true, "tests": { "node_gyp": { - "tempDir": true, "steps": [ { "args": "cache --allow-scripts=npm:@denotest/node-addon main.js", @@ -14,7 +14,6 @@ ] }, "run_without_scripts": { - "tempDir": true, "steps": [ { "args": "run -A main.js", @@ -24,11 +23,9 @@ ] }, "implicit_node_gyp": { - "tempDir": true, "steps": [ { "envs": { - // I don't think this will work on windows "PATH": "" }, "args": "cache --allow-scripts implicit_node_gyp.js", @@ -38,7 +35,6 @@ ] }, "lifecycle_scripts": { - "tempDir": true, "steps": [ { // without running scripts (should warn) @@ -55,8 +51,25 @@ } ] }, + "global_lifecycle_scripts": { + "steps": [ + { + "args": ["eval", "Deno.removeSync('deno.json')"], + "output": "" + }, + { + // without running scripts (should warn) + "args": "install -e all_lifecycles.js", + "output": "all_lifecycles_not_run_global.out" + }, + { + // should not warn + "args": "install -e all_lifecycles.js", + "output": "" + } + ] + }, "only_warns_first": { - "tempDir": true, "steps": [ { // without running scripts (should warn) @@ -96,7 +109,6 @@ ] }, "lifecycle_scripts_conflicting_bin": { - "tempDir": true, "steps": [ { // we import @denotest/says-hello-on-install, which executes `say-hello` from `@denotest/say-hello` in its @@ -110,7 +122,6 @@ ] }, "fsevents_default_install_script": { - "tempDir": true, "steps": [ { "if": "mac", @@ -125,7 +136,6 @@ ] }, "lifecycle_scripts_no_deno_json": { - "tempDir": true, "steps": [ { "args": ["eval", "Deno.removeSync('deno.json')"], @@ -138,7 +148,6 @@ ] }, "lifecycle_scripts_no_deno_json_conflicting_bin": { - "tempDir": true, "steps": [ { "args": ["eval", "Deno.removeSync('deno.json')"], diff --git a/tests/specs/npm/lifecycle_scripts/all_lifecycles_not_run.out b/tests/specs/npm/lifecycle_scripts/all_lifecycles_not_run.out index e63e2d22b3..09324c8452 100644 --- a/tests/specs/npm/lifecycle_scripts/all_lifecycles_not_run.out +++ b/tests/specs/npm/lifecycle_scripts/all_lifecycles_not_run.out @@ -6,9 +6,9 @@ Download http://localhost:4260/@denotest/bin/1.0.0.tgz Initialize @denotest/node-lifecycle-scripts@1.0.0 Initialize @denotest/bin@1.0.0 [UNORDERED_END] -Warning Following packages contained npm lifecycle scripts (preinstall/install/postinstall) that were not executed: +Warning The following packages contained npm lifecycle scripts (preinstall/install/postinstall) that were not executed: ┠─ npm:@denotest/node-lifecycle-scripts@1.0.0 ┃ ┠─ This may cause the packages to not work correctly. -┗─ To run lifecycle scripts, use the `--allow-scripts` flag with `deno install`: +┖─ To run lifecycle scripts, use the `--allow-scripts` flag with `deno install`: deno install --allow-scripts=npm:@denotest/node-lifecycle-scripts@1.0.0 diff --git a/tests/specs/npm/lifecycle_scripts/all_lifecycles_not_run_global.out b/tests/specs/npm/lifecycle_scripts/all_lifecycles_not_run_global.out new file mode 100644 index 0000000000..93b5a14cc0 --- /dev/null +++ b/tests/specs/npm/lifecycle_scripts/all_lifecycles_not_run_global.out @@ -0,0 +1,13 @@ +[UNORDERED_START] +Download http://localhost:4260/@denotest/node-lifecycle-scripts +Download http://localhost:4260/@denotest/bin +Download http://localhost:4260/@denotest/node-lifecycle-scripts/1.0.0.tgz +Download http://localhost:4260/@denotest/bin/1.0.0.tgz +[UNORDERED_END] +Warning The following packages contained npm lifecycle scripts (preinstall/install/postinstall) that were not executed: +┠─ npm:@denotest/node-lifecycle-scripts@1.0.0 +┃ +┠─ This may cause the packages to not work correctly. +┠─ Lifecycle scripts are only supported when using a `node_modules` directory. +┠─ Enable it in your deno config file: +┖─ "nodeModulesDir": "auto" diff --git a/tests/specs/npm/lifecycle_scripts/future_install_all_lifecycles_not_run.out b/tests/specs/npm/lifecycle_scripts/future_install_all_lifecycles_not_run.out index e63e2d22b3..09324c8452 100644 --- a/tests/specs/npm/lifecycle_scripts/future_install_all_lifecycles_not_run.out +++ b/tests/specs/npm/lifecycle_scripts/future_install_all_lifecycles_not_run.out @@ -6,9 +6,9 @@ Download http://localhost:4260/@denotest/bin/1.0.0.tgz Initialize @denotest/node-lifecycle-scripts@1.0.0 Initialize @denotest/bin@1.0.0 [UNORDERED_END] -Warning Following packages contained npm lifecycle scripts (preinstall/install/postinstall) that were not executed: +Warning The following packages contained npm lifecycle scripts (preinstall/install/postinstall) that were not executed: ┠─ npm:@denotest/node-lifecycle-scripts@1.0.0 ┃ ┠─ This may cause the packages to not work correctly. -┗─ To run lifecycle scripts, use the `--allow-scripts` flag with `deno install`: +┖─ To run lifecycle scripts, use the `--allow-scripts` flag with `deno install`: deno install --allow-scripts=npm:@denotest/node-lifecycle-scripts@1.0.0 diff --git a/tests/specs/npm/lifecycle_scripts/node_gyp_not_found.out b/tests/specs/npm/lifecycle_scripts/node_gyp_not_found.out index 2f0ff11e28..06c856cd94 100644 --- a/tests/specs/npm/lifecycle_scripts/node_gyp_not_found.out +++ b/tests/specs/npm/lifecycle_scripts/node_gyp_not_found.out @@ -6,3 +6,4 @@ Initialize @denotest/node-addon-implicit-node-gyp@1.0.0 Warning node-gyp was used in a script, but was not listed as a dependency. Either add it as a dependency or install it globally (e.g. `npm install -g node-gyp`) [WILDCARD] error: script 'install' in '@denotest/node-addon-implicit-node-gyp@1.0.0' failed with exit code 1 +error: failed to run scripts for packages: @denotest/node-addon-implicit-node-gyp@1.0.0 diff --git a/tests/specs/npm/lifecycle_scripts/node_gyp_not_run.out b/tests/specs/npm/lifecycle_scripts/node_gyp_not_run.out index 2803bcb4d2..e0e51b26c0 100644 --- a/tests/specs/npm/lifecycle_scripts/node_gyp_not_run.out +++ b/tests/specs/npm/lifecycle_scripts/node_gyp_not_run.out @@ -1,11 +1,11 @@ Download http://localhost:4260/@denotest/node-addon Download http://localhost:4260/node-gyp [WILDCARD] -Warning Following packages contained npm lifecycle scripts (preinstall/install/postinstall) that were not executed: +Warning The following packages contained npm lifecycle scripts (preinstall/install/postinstall) that were not executed: ┠─ npm:@denotest/node-addon@1.0.0 ┃ ┠─ This may cause the packages to not work correctly. -┗─ To run lifecycle scripts, use the `--allow-scripts` flag with `deno install`: +┖─ To run lifecycle scripts, use the `--allow-scripts` flag with `deno install`: deno install --allow-scripts=npm:@denotest/node-addon@1.0.0 error: Uncaught (in promise) Error: Cannot find module './build/Release/node_addon' [WILDCARD] diff --git a/tests/specs/npm/lifecycle_scripts/only_warns_first1.out b/tests/specs/npm/lifecycle_scripts/only_warns_first1.out index e1985d08cb..947777b5ba 100644 --- a/tests/specs/npm/lifecycle_scripts/only_warns_first1.out +++ b/tests/specs/npm/lifecycle_scripts/only_warns_first1.out @@ -6,11 +6,11 @@ Download http://localhost:4260/@denotest/bin/1.0.0.tgz Initialize @denotest/node-lifecycle-scripts@1.0.0 Initialize @denotest/bin@1.0.0 [UNORDERED_END] -Warning Following packages contained npm lifecycle scripts (preinstall/install/postinstall) that were not executed: +Warning The following packages contained npm lifecycle scripts (preinstall/install/postinstall) that were not executed: ┠─ npm:@denotest/node-lifecycle-scripts@1.0.0 ┃ ┠─ This may cause the packages to not work correctly. -┗─ To run lifecycle scripts, use the `--allow-scripts` flag with `deno install`: +┖─ To run lifecycle scripts, use the `--allow-scripts` flag with `deno install`: deno install --allow-scripts=npm:@denotest/node-lifecycle-scripts@1.0.0 error: Uncaught SyntaxError: The requested module 'npm:@denotest/node-lifecycle-scripts' does not provide an export named 'value' [WILDCARD]