From 3cf0879779b3c20fd31abb1193989d1799c3e0b7 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Fri, 6 Sep 2024 10:22:16 -0700 Subject: [PATCH 1/3] Create parent if not exists --- cli/npm/managed/resolvers/local.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/npm/managed/resolvers/local.rs b/cli/npm/managed/resolvers/local.rs index eb6f9de9b5a9e4..2aab1407186b97 100644 --- a/cli/npm/managed/resolvers/local.rs +++ b/cli/npm/managed/resolvers/local.rs @@ -1043,7 +1043,7 @@ fn symlink_package_dir( new_path: &Path, ) -> Result<(), AnyError> { let new_parent = new_path.parent().unwrap(); - if new_parent.file_name().unwrap() != "node_modules" { + if new_parent.file_name().unwrap() != "node_modules" || !new_parent.exists() { // create the parent folder that will contain the symlink fs::create_dir_all(new_parent) .with_context(|| format!("Creating '{}'", new_parent.display()))?; From bb729a3eb8efaee54af531aaa2a331e645a73fe3 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Fri, 6 Sep 2024 10:46:20 -0700 Subject: [PATCH 2/3] Test --- .../__test__.jsonc | 15 +++++++++++++ .../deno.json | 7 ++++++ .../install.out | 22 +++++++++++++++++++ .../package1/package.json | 5 +++++ .../package2/package.json | 5 +++++ 5 files changed, 54 insertions(+) create mode 100644 tests/specs/install/workspace_node_modules_not_exists/__test__.jsonc create mode 100644 tests/specs/install/workspace_node_modules_not_exists/deno.json create mode 100644 tests/specs/install/workspace_node_modules_not_exists/install.out create mode 100644 tests/specs/install/workspace_node_modules_not_exists/package1/package.json create mode 100644 tests/specs/install/workspace_node_modules_not_exists/package2/package.json diff --git a/tests/specs/install/workspace_node_modules_not_exists/__test__.jsonc b/tests/specs/install/workspace_node_modules_not_exists/__test__.jsonc new file mode 100644 index 00000000000000..a39eb5e1f7052a --- /dev/null +++ b/tests/specs/install/workspace_node_modules_not_exists/__test__.jsonc @@ -0,0 +1,15 @@ +// Regression test for https://github.com/denoland/deno/issues/25493 +// +// `deno install` where we need to create a `node_modules` for a workspace member +// due to conflicting package versions, but when we go to set it up, the +// symlink target path (`package2/node_modules/chalk`) has `node_modules` as its parent, +// and it doesn't exist yet +{ + "tempDir": true, + "steps": [ + { + "args": "install", + "output": "install.out" + } + ] +} diff --git a/tests/specs/install/workspace_node_modules_not_exists/deno.json b/tests/specs/install/workspace_node_modules_not_exists/deno.json new file mode 100644 index 00000000000000..b3cca048fd6fb5 --- /dev/null +++ b/tests/specs/install/workspace_node_modules_not_exists/deno.json @@ -0,0 +1,7 @@ +{ + "nodeModulesDir": "auto", + "workspace": [ + "./package1", + "./package2" + ] +} diff --git a/tests/specs/install/workspace_node_modules_not_exists/install.out b/tests/specs/install/workspace_node_modules_not_exists/install.out new file mode 100644 index 00000000000000..22f7e430723786 --- /dev/null +++ b/tests/specs/install/workspace_node_modules_not_exists/install.out @@ -0,0 +1,22 @@ +[UNORDERED_START] +Download http://localhost:4260/chalk +Download http://localhost:4260/ansi-styles +Download http://localhost:4260/supports-color +Download http://localhost:4260/color-convert +Download http://localhost:4260/has-flag +Download http://localhost:4260/color-name +Download http://localhost:4260/chalk/chalk-4.1.2.tgz +Download http://localhost:4260/chalk/chalk-5.0.1.tgz +Download http://localhost:4260/supports-color/supports-color-7.2.0.tgz +Download http://localhost:4260/ansi-styles/ansi-styles-4.3.0.tgz +Download http://localhost:4260/has-flag/has-flag-4.0.0.tgz +Download http://localhost:4260/color-convert/color-convert-2.0.1.tgz +Download http://localhost:4260/color-name/color-name-1.1.4.tgz +Initialize supports-color@7.2.0 +Initialize chalk@4.1.2 +Initialize ansi-styles@4.3.0 +Initialize color-name@1.1.4 +Initialize color-convert@2.0.1 +Initialize has-flag@4.0.0 +Initialize chalk@5.0.1 +[UNORDERED_END] diff --git a/tests/specs/install/workspace_node_modules_not_exists/package1/package.json b/tests/specs/install/workspace_node_modules_not_exists/package1/package.json new file mode 100644 index 00000000000000..0c5e77484bd5c8 --- /dev/null +++ b/tests/specs/install/workspace_node_modules_not_exists/package1/package.json @@ -0,0 +1,5 @@ +{ + "dependencies": { + "chalk": "4.1.2" + } +} diff --git a/tests/specs/install/workspace_node_modules_not_exists/package2/package.json b/tests/specs/install/workspace_node_modules_not_exists/package2/package.json new file mode 100644 index 00000000000000..48d3d69170a59f --- /dev/null +++ b/tests/specs/install/workspace_node_modules_not_exists/package2/package.json @@ -0,0 +1,5 @@ +{ + "dependencies": { + "chalk": "5.0.1" + } +} From 3d813913920ec1aa7fb4759612ed3aba3c440eab Mon Sep 17 00:00:00 2001 From: Nathan Whitaker Date: Fri, 6 Sep 2024 11:03:02 -0700 Subject: [PATCH 3/3] Cache whether created yet --- cli/npm/managed/resolvers/local.rs | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/cli/npm/managed/resolvers/local.rs b/cli/npm/managed/resolvers/local.rs index 2aab1407186b97..d0fd523e22597a 100644 --- a/cli/npm/managed/resolvers/local.rs +++ b/cli/npm/managed/resolvers/local.rs @@ -10,6 +10,7 @@ use std::cmp::Ordering; use std::collections::hash_map::Entry; use std::collections::BTreeMap; use std::collections::HashMap; +use std::collections::HashSet; use std::fs; use std::path::Path; use std::path::PathBuf; @@ -620,6 +621,9 @@ async fn sync_resolution_with_fs( let mut found_names: HashMap<&String, &PackageNv> = HashMap::new(); + // set of node_modules in workspace packages that we've already ensured exist + let mut existing_child_node_modules_dirs: HashSet = HashSet::new(); + // 4. Create symlinks for package json dependencies { for remote in npm_install_deps_provider.remote_pkgs() { @@ -667,8 +671,15 @@ async fn sync_resolution_with_fs( ); if install_in_child { // symlink the dep into the package's child node_modules folder - let dest_path = - remote.base_dir.join("node_modules").join(&remote.alias); + let dest_node_modules = remote.base_dir.join("node_modules"); + if !existing_child_node_modules_dirs.contains(&dest_node_modules) { + fs::create_dir_all(&dest_node_modules).with_context(|| { + format!("Creating '{}'", dest_node_modules.display()) + })?; + existing_child_node_modules_dirs.insert(dest_node_modules.clone()); + } + let mut dest_path = dest_node_modules; + dest_path.push(&remote.alias); symlink_package_dir(&local_registry_package_path, &dest_path)?; } else { @@ -1043,7 +1054,7 @@ fn symlink_package_dir( new_path: &Path, ) -> Result<(), AnyError> { let new_parent = new_path.parent().unwrap(); - if new_parent.file_name().unwrap() != "node_modules" || !new_parent.exists() { + if new_parent.file_name().unwrap() != "node_modules" { // create the parent folder that will contain the symlink fs::create_dir_all(new_parent) .with_context(|| format!("Creating '{}'", new_parent.display()))?;