diff --git a/crates/pm/src/service/install.rs b/crates/pm/src/service/install.rs index 86571c671d..8588940d9d 100644 --- a/crates/pm/src/service/install.rs +++ b/crates/pm/src/service/install.rs @@ -344,6 +344,10 @@ pub async fn install_packages( let should_resolve = !crate::fs::try_exists(&cache_flag_path).await?; let semaphore = Arc::clone(&semaphore); + // Check if this is an optional dependency + let is_optional = + package.optional == Some(true) || package.dev_optional == Some(true); + let task = tokio::spawn(async move { let _permit = semaphore .acquire() @@ -368,6 +372,13 @@ pub async fn install_packages( cache_path.display(), e ); + if is_optional { + tracing::warn!( + "Optional dependency {name} download failed (ignored): {e}" + ); + PROGRESS_BAR.inc(1); + return Ok(()); + } return Err(anyhow::anyhow!("{name} download failed: {e}")); } } @@ -384,12 +395,21 @@ pub async fn install_packages( update_package_binary(&cwd_clone.join(&path), &name).await?; Ok(()) } - Err(e) => Err(anyhow::anyhow!( - "Copy failed {} to {}: {}", - cache_path.display(), - cwd_clone.join(&path).display(), - e - )), + Err(e) => { + if is_optional { + tracing::warn!( + "Optional dependency {name} clone failed (ignored): {e}" + ); + PROGRESS_BAR.inc(1); + return Ok(()); + } + Err(anyhow::anyhow!( + "Copy failed {} to {}: {}", + cache_path.display(), + cwd_clone.join(&path).display(), + e + )) + } } }); tasks.push(task); @@ -686,4 +706,49 @@ mod tests { omit_dev_optional.insert(OmitType::Optional); assert!(should_omit_package(&dev_optional_pkg, &omit_dev_optional)); } + + #[test] + fn test_is_optional_dependency() { + // Test helper to verify is_optional detection logic + // This mirrors the logic used in install_packages + + // Regular package - not optional + let regular_pkg = Package::default(); + let is_optional = + regular_pkg.optional == Some(true) || regular_pkg.dev_optional == Some(true); + assert!(!is_optional, "Regular package should not be optional"); + + // Optional package + let optional_pkg = Package { + optional: Some(true), + ..Package::default() + }; + let is_optional = + optional_pkg.optional == Some(true) || optional_pkg.dev_optional == Some(true); + assert!(is_optional, "Package with optional=true should be optional"); + + // Dev optional package + let dev_optional_pkg = Package { + dev_optional: Some(true), + ..Package::default() + }; + let is_optional = + dev_optional_pkg.optional == Some(true) || dev_optional_pkg.dev_optional == Some(true); + assert!( + is_optional, + "Package with dev_optional=true should be optional" + ); + + // Package with optional=false explicitly + let not_optional_pkg = Package { + optional: Some(false), + ..Package::default() + }; + let is_optional = + not_optional_pkg.optional == Some(true) || not_optional_pkg.dev_optional == Some(true); + assert!( + !is_optional, + "Package with optional=false should not be optional" + ); + } } diff --git a/crates/pm/src/service/package.rs b/crates/pm/src/service/package.rs index b7f579176b..d4e74b8ef4 100644 --- a/crates/pm/src/service/package.rs +++ b/crates/pm/src/service/package.rs @@ -13,12 +13,14 @@ use utoo_ruborist::model::package_json::parse_bin_field; use super::script::ScriptService; /// Execution queues for package scripts and binary linking +/// Each entry is (PackageInfo, is_optional) where is_optional indicates if the package +/// is an optional dependency (based on edge type in dependency graph) #[derive(Default)] pub struct ExecutionQueues { - pub preinstall: Vec>, - pub bin_linking: Vec>, - pub install: Vec>, - pub postinstall: Vec>, + pub preinstall: Vec<(Rc, bool)>, + pub bin_linking: Vec<(Rc, bool)>, + pub install: Vec<(Rc, bool)>, + pub postinstall: Vec<(Rc, bool)>, } pub struct PackageService; @@ -141,11 +143,12 @@ impl PackageService { } /// Collect packages from memory PackageLock object with early filtering + /// Returns Vec<(PackageInfo, is_optional)> where is_optional is determined by the edge type pub async fn collect_packages_from_lock( package_lock: &PackageLock, root_path: &Path, ignore_scripts: bool, - ) -> Result> { + ) -> Result> { tracing::debug!("Collecting packages from memory lock..."); let mut packages = Vec::new(); @@ -216,6 +219,10 @@ impl PackageService { } }; + // Check if this package is an optional dependency (based on edge type) + let is_optional = + lock_package.optional == Some(true) || lock_package.dev_optional == Some(true); + let package_info = PackageInfo { path: package_path, bin_files, @@ -224,42 +231,43 @@ impl PackageService { fullname, }; - packages.push(package_info); + packages.push((package_info, is_optional)); } Ok(packages) } /// Create execution queues with bins_only parameter support + /// Takes Vec<(PackageInfo, is_optional)> where is_optional indicates edge type pub fn create_execution_queues_with_options( - packages: Vec, + packages: Vec<(PackageInfo, bool)>, ignore_scripts: bool, ) -> Result { tracing::debug!("Creating execution queues with options..."); let mut queues = ExecutionQueues::default(); - for package in packages { + for (package, is_optional) in packages { let package = Rc::new(package); // Script queues - skip in bins_only mode if !ignore_scripts { if package.scripts.preinstall.is_some() { tracing::debug!("Adding {} to preinstall queue", package.path.display()); - queues.preinstall.push(Rc::clone(&package)); + queues.preinstall.push((Rc::clone(&package), is_optional)); } if package.scripts.install.is_some() { tracing::debug!("Adding {} to install queue", package.path.display()); - queues.install.push(Rc::clone(&package)); + queues.install.push((Rc::clone(&package), is_optional)); } if package.scripts.postinstall.is_some() { tracing::debug!("Adding {} to postinstall queue", package.path.display()); - queues.postinstall.push(Rc::clone(&package)); + queues.postinstall.push((Rc::clone(&package), is_optional)); } } // Binary linking queue - always process if package has bin files if !package.bin_files.is_empty() { tracing::debug!("Adding {} to bin linking queue", package.path.display()); - queues.bin_linking.push(Rc::clone(&package)); + queues.bin_linking.push((Rc::clone(&package), is_optional)); } } @@ -311,7 +319,11 @@ impl PackageService { } /// Execute script queue for a specific script type - async fn execute_script_queue(queue: &[Rc], script_name: &str) -> Result<()> { + /// Queue contains (PackageInfo, is_optional) tuples where is_optional indicates edge type + async fn execute_script_queue( + queue: &[(Rc, bool)], + script_name: &str, + ) -> Result<()> { use futures; let queue_start = std::time::Instant::now(); @@ -323,7 +335,7 @@ impl PackageService { let script_tasks: Vec<_> = queue .iter() - .filter_map(|package| { + .filter_map(|(package, is_optional)| { let script_option = match script_name { "preinstall" => &package.scripts.preinstall, "install" => &package.scripts.install, @@ -334,6 +346,7 @@ impl PackageService { script_option.as_ref().map(|script| { let package = Rc::clone(package); let script = script.clone(); + let is_optional = *is_optional; async move { log_progress(&format!("{} {}", package.fullname, script_name)); let start = std::time::Instant::now(); @@ -355,16 +368,22 @@ impl PackageService { script ); PROGRESS_BAR.inc(1); - result + (is_optional, result) } }) }) .collect(); // Wait for all script tasks to complete - let script_results: Vec> = futures::future::join_all(script_tasks).await; - for result in script_results { - result?; + let script_results: Vec<(bool, Result<()>)> = futures::future::join_all(script_tasks).await; + for (is_optional, result) in script_results { + if let Err(e) = result { + if is_optional { + tracing::warn!("Optional dependency script failed (ignored): {e}"); + } else { + return Err(e); + } + } } let queue_elapsed = queue_start.elapsed(); @@ -378,8 +397,10 @@ impl PackageService { } /// Execute binary file linking for packages - async fn execute_binary_linking(queue: &[Rc]) -> Result<()> { - for package in queue { + /// Queue contains (PackageInfo, is_optional) tuples - is_optional is not used here + /// as binary linking happens only for successfully installed packages + async fn execute_binary_linking(queue: &[(Rc, bool)]) -> Result<()> { + for (package, _is_optional) in queue { if !package.bin_files.is_empty() { tracing::debug!("Linking binary files for {}", package.fullname); for (bin_name, relative_path) in &package.bin_files { @@ -792,8 +813,9 @@ mod tests { }; // Prepare queues: only bin linking queue has this package + // The bool indicates is_optional (false = not optional) let queues = ExecutionQueues { - bin_linking: vec![Rc::new(package_info)], + bin_linking: vec![(Rc::new(package_info), false)], ..Default::default() }; @@ -904,7 +926,7 @@ mod tests { assert_eq!(packages_bins_only.len(), 2); // full-package, bin-only (script-only and no-hooks excluded) // Verify the collected packages have correct bin_files - for package_info in &packages_bins_only { + for (package_info, _is_optional) in &packages_bins_only { assert!( !package_info.bin_files.is_empty(), "Package {} should have bin_files in ignore_scripts mode", @@ -983,6 +1005,162 @@ mod tests { // Should only collect the cross-platform package (win-only filtered out by platform check) assert_eq!(packages_collected.len(), 1); - assert_eq!(packages_collected[0].fullname, "cross-platform"); + assert_eq!(packages_collected[0].0.fullname, "cross-platform"); + } + + #[tokio::test] + async fn test_collect_packages_from_lock_optional_flag() { + use serde_json::json; + use std::collections::HashMap; + use tempfile::TempDir; + use utoo_ruborist::lock::{LockPackage, PackageLock}; + + let temp_dir = TempDir::new().unwrap(); + + let mut packages = HashMap::new(); + + // Regular (non-optional) package + packages.insert( + "node_modules/regular-pkg".to_string(), + LockPackage { + name: Some("regular-pkg".to_string()), + version: Some("1.0.0".to_string()), + resolved: Some("registry-url".to_string()), + bin: Some(json!({"tool": "index.js"})), + has_install_script: Some(false), + optional: None, + ..LockPackage::default() + }, + ); + + // Optional package + packages.insert( + "node_modules/optional-pkg".to_string(), + LockPackage { + name: Some("optional-pkg".to_string()), + version: Some("1.0.0".to_string()), + resolved: Some("registry-url".to_string()), + bin: Some(json!({"tool": "index.js"})), + has_install_script: Some(false), + optional: Some(true), + ..LockPackage::default() + }, + ); + + // Dev optional package + packages.insert( + "node_modules/dev-optional-pkg".to_string(), + LockPackage { + name: Some("dev-optional-pkg".to_string()), + version: Some("1.0.0".to_string()), + resolved: Some("registry-url".to_string()), + bin: Some(json!({"tool": "index.js"})), + has_install_script: Some(false), + dev_optional: Some(true), + ..LockPackage::default() + }, + ); + + let package_lock = + PackageLock::new("test-project".to_string(), "1.0.0".to_string(), packages); + + // Create package directories + let node_modules = temp_dir.path().join("node_modules"); + std::fs::create_dir_all(&node_modules).unwrap(); + + for pkg_name in &["regular-pkg", "optional-pkg", "dev-optional-pkg"] { + let package_dir = node_modules.join(pkg_name); + std::fs::create_dir_all(&package_dir).unwrap(); + let package_json = json!({ + "name": pkg_name, + "version": "1.0.0" + }); + std::fs::write( + package_dir.join("package.json"), + serde_json::to_string_pretty(&package_json).unwrap(), + ) + .unwrap(); + } + + // Collect packages + let result = + PackageService::collect_packages_from_lock(&package_lock, temp_dir.path(), true).await; + assert!(result.is_ok()); + let packages_collected = result.unwrap(); + assert_eq!(packages_collected.len(), 3); + + // Verify is_optional flags are correctly set + for (pkg_info, is_optional) in &packages_collected { + match pkg_info.fullname.as_str() { + "regular-pkg" => { + assert!(!is_optional, "regular-pkg should not be optional"); + } + "optional-pkg" => { + assert!(is_optional, "optional-pkg should be optional"); + } + "dev-optional-pkg" => { + assert!(is_optional, "dev-optional-pkg should be optional"); + } + _ => panic!("Unexpected package: {}", pkg_info.fullname), + } + } + } + + #[tokio::test] + async fn test_execute_script_queue_optional_failure_ignored() { + use std::fs; + use tempfile::TempDir; + + // Create a temporary directory for the test package + let temp_dir = TempDir::new().unwrap(); + let package_path = temp_dir.path(); + + // Create a package.json with a failing script + let package_json = serde_json::json!({ + "name": "test-optional-fail", + "version": "1.0.0", + "scripts": { + "postinstall": "exit 1" + } + }); + fs::write( + package_path.join("package.json"), + serde_json::to_string_pretty(&package_json).unwrap(), + ) + .unwrap(); + + // Create PackageInfo with failing script + let package_info = PackageInfo { + path: package_path.to_path_buf(), + bin_files: vec![], + scripts: Scripts { + preinstall: None, + install: None, + postinstall: Some("exit 1".to_string()), + prepare: None, + preprepare: None, + postprepare: None, + prepublish: None, + }, + name: "test-optional-fail".to_string(), + fullname: "test-optional-fail".to_string(), + }; + + // Test with is_optional = true: should NOT return error + let queue_optional: Vec<(Rc, bool)> = + vec![(Rc::new(package_info.clone()), true)]; + let result = PackageService::execute_script_queue(&queue_optional, "postinstall").await; + assert!( + result.is_ok(), + "Optional dependency script failure should be ignored" + ); + + // Test with is_optional = false: should return error + let queue_required: Vec<(Rc, bool)> = vec![(Rc::new(package_info), false)]; + let result = PackageService::execute_script_queue(&queue_required, "postinstall").await; + assert!( + result.is_err(), + "Required dependency script failure should return error" + ); } } diff --git a/crates/ruborist/src/resolver/edges.rs b/crates/ruborist/src/resolver/edges.rs index 2d4f161f54..ee3ac204bc 100644 --- a/crates/ruborist/src/resolver/edges.rs +++ b/crates/ruborist/src/resolver/edges.rs @@ -80,18 +80,21 @@ impl DependencySource for VersionManifest { where F: FnMut(EdgeType, &str, &str), { - iter_deps(self.dependencies.as_ref(), EdgeType::Prod, &mut f); + // npm registry may copy optionalDependencies into dependencies (legacy bug) + // Skip deps that also appear in optionalDependencies to avoid duplicate edges + let optional_deps = self.optional_dependencies.as_ref(); + for (name, spec) in self.dependencies.as_ref().into_iter().flatten() { + if !optional_deps.is_some_and(|opt| opt.contains_key(name)) { + f(EdgeType::Prod, name, spec); + } + } if include_dev { iter_deps(self.dev_dependencies.as_ref(), EdgeType::Dev, &mut f); } if !legacy_peer_deps { iter_deps(self.peer_dependencies.as_ref(), EdgeType::Peer, &mut f); } - iter_deps( - self.optional_dependencies.as_ref(), - EdgeType::Optional, - &mut f, - ); + iter_deps(optional_deps, EdgeType::Optional, &mut f); } }