From dfa4c9a45ecdfc92e2fac2a9c49a829fbdfc64ef Mon Sep 17 00:00:00 2001 From: Miles Johnson Date: Sun, 1 Oct 2023 11:34:05 -0700 Subject: [PATCH 1/3] Add tests. --- crates/cli/tests/run_node_test.rs | 13 +++++++++++++ crates/node/platform/src/actions/run_target.rs | 5 ++++- crates/node/tool/src/node_tool.rs | 5 ++++- crates/node/tool/src/npm_tool.rs | 5 ++++- crates/node/tool/src/pnpm_tool.rs | 5 ++++- crates/node/tool/src/yarn_tool.rs | 5 ++++- crates/rust/tool/src/rust_tool.rs | 4 ++-- tests/fixtures/node/base/execBin.js | 3 +++ tests/fixtures/node/base/moon.yml | 3 +++ 9 files changed, 41 insertions(+), 7 deletions(-) create mode 100644 tests/fixtures/node/base/execBin.js diff --git a/crates/cli/tests/run_node_test.rs b/crates/cli/tests/run_node_test.rs index a44bb7f8248..82fca33bb07 100644 --- a/crates/cli/tests/run_node_test.rs +++ b/crates/cli/tests/run_node_test.rs @@ -366,6 +366,19 @@ fn avoids_postinstall_recursion() { assert.success(); } +#[test] +fn can_exec_global_bin_as_child_process() { + let sandbox = node_sandbox(); + + let assert = sandbox.run_moon(|cmd| { + cmd.arg("run").arg("node:execBinSelf"); + }); + + let output = assert.output(); + + assert!(predicate::str::contains("v18.0.0").eval(&output)); +} + mod install_deps { use super::*; diff --git a/crates/node/platform/src/actions/run_target.rs b/crates/node/platform/src/actions/run_target.rs index e7b640c1758..6a1444b5e13 100644 --- a/crates/node/platform/src/actions/run_target.rs +++ b/crates/node/platform/src/actions/run_target.rs @@ -189,7 +189,10 @@ pub fn create_target_command( } }; - command.env("PATH", get_path_env_var(&node.tool.get_tool_dir())); + command.env( + "PATH", + get_path_env_var(node.tool.get_bin_path()?.parent().unwrap()), + ); prepare_target_command(&mut command, context, task, &node.config)?; diff --git a/crates/node/tool/src/node_tool.rs b/crates/node/tool/src/node_tool.rs index cd90a6ae64b..46be9690735 100644 --- a/crates/node/tool/src/node_tool.rs +++ b/crates/node/tool/src/node_tool.rs @@ -78,7 +78,10 @@ impl NodeTool { let mut cmd = Command::new(self.get_npx_path()?); if !self.global { - cmd.env("PATH", get_path_env_var(&self.tool.get_tool_dir())); + cmd.env( + "PATH", + get_path_env_var(self.tool.get_bin_path()?.parent().unwrap()), + ); } cmd.args(exec_args) diff --git a/crates/node/tool/src/npm_tool.rs b/crates/node/tool/src/npm_tool.rs index b11ce68c737..52e9007c80b 100644 --- a/crates/node/tool/src/npm_tool.rs +++ b/crates/node/tool/src/npm_tool.rs @@ -117,7 +117,10 @@ impl DependencyManager for NpmTool { }; if !self.global { - cmd.env("PATH", get_path_env_var(&self.tool.get_tool_dir())); + cmd.env( + "PATH", + get_path_env_var(self.tool.get_bin_path()?.parent().unwrap()), + ); } cmd.env("PROTO_NODE_BIN", node.get_bin_path()?); diff --git a/crates/node/tool/src/pnpm_tool.rs b/crates/node/tool/src/pnpm_tool.rs index 742fe0a11a8..22ecea144ed 100644 --- a/crates/node/tool/src/pnpm_tool.rs +++ b/crates/node/tool/src/pnpm_tool.rs @@ -123,7 +123,10 @@ impl DependencyManager for PnpmTool { }; if !self.global { - cmd.env("PATH", get_path_env_var(&self.tool.get_tool_dir())); + cmd.env( + "PATH", + get_path_env_var(self.tool.get_bin_path()?.parent().unwrap()), + ); } cmd.env("PROTO_NODE_BIN", node.get_bin_path()?); diff --git a/crates/node/tool/src/yarn_tool.rs b/crates/node/tool/src/yarn_tool.rs index c0933f499cf..4e80ea90ffc 100644 --- a/crates/node/tool/src/yarn_tool.rs +++ b/crates/node/tool/src/yarn_tool.rs @@ -179,7 +179,10 @@ impl DependencyManager for YarnTool { }; if !self.global { - cmd.env("PATH", get_path_env_var(&self.tool.get_tool_dir())); + cmd.env( + "PATH", + get_path_env_var(self.tool.get_bin_path()?.parent().unwrap()), + ); } cmd.env("PROTO_NODE_BIN", node.get_bin_path()?); diff --git a/crates/rust/tool/src/rust_tool.rs b/crates/rust/tool/src/rust_tool.rs index 84a63a58423..1ec0781e26c 100644 --- a/crates/rust/tool/src/rust_tool.rs +++ b/crates/rust/tool/src/rust_tool.rs @@ -84,8 +84,8 @@ impl Tool for RustTool { // When offline and the tool doesn't exist, fallback to the global binary } else if proto_core::is_offline() { debug!( - "No internet connection and Rust has not been setup, falling back to global binary in PATH" - ); + "No internet connection and Rust has not been setup, falling back to global binary in PATH" + ); self.global = true; diff --git a/tests/fixtures/node/base/execBin.js b/tests/fixtures/node/base/execBin.js new file mode 100644 index 00000000000..7fb650504f1 --- /dev/null +++ b/tests/fixtures/node/base/execBin.js @@ -0,0 +1,3 @@ +const { spawn } = require('child_process'); + +spawn('node', ['--version'], { stdio: 'inherit' }); diff --git a/tests/fixtures/node/base/moon.yml b/tests/fixtures/node/base/moon.yml index 3beb048bc8a..14bbf97d853 100644 --- a/tests/fixtures/node/base/moon.yml +++ b/tests/fixtures/node/base/moon.yml @@ -12,6 +12,9 @@ tasks: mjs: command: node args: ./mjsFile.mjs + execBinSelf: + command: node + args: ./execBin.js # Runner cases exitCodeNonZero: From 2dbb82ecb37ee92e05500657f0f1393cfda05aef Mon Sep 17 00:00:00 2001 From: Miles Johnson Date: Sun, 1 Oct 2023 11:52:04 -0700 Subject: [PATCH 2/3] Add more tests. --- crates/cli/tests/run_node_test.rs | 11 +++++++++++ crates/core/test-utils/src/configs.rs | 1 + tests/fixtures/node/base/execBin.js | 4 ++-- tests/fixtures/node/postinstall/package.json | 7 +++++++ tests/fixtures/node/postinstall/postinstall.js | 3 +++ 5 files changed, 24 insertions(+), 2 deletions(-) create mode 100644 tests/fixtures/node/postinstall/package.json create mode 100644 tests/fixtures/node/postinstall/postinstall.js diff --git a/crates/cli/tests/run_node_test.rs b/crates/cli/tests/run_node_test.rs index 82fca33bb07..ed3138de1ab 100644 --- a/crates/cli/tests/run_node_test.rs +++ b/crates/cli/tests/run_node_test.rs @@ -379,6 +379,17 @@ fn can_exec_global_bin_as_child_process() { assert!(predicate::str::contains("v18.0.0").eval(&output)); } +#[test] +fn can_exec_global_bin_as_child_process_from_postinstall() { + let sandbox = node_sandbox(); + + let assert = sandbox.run_moon(|cmd| { + cmd.arg("run").arg("postinstall:noop"); + }); + + assert.success(); +} + mod install_deps { use super::*; diff --git a/crates/core/test-utils/src/configs.rs b/crates/core/test-utils/src/configs.rs index 7d79e2c5b65..035a1a8c78d 100644 --- a/crates/core/test-utils/src/configs.rs +++ b/crates/core/test-utils/src/configs.rs @@ -284,6 +284,7 @@ pub fn get_node_fixture_configs() -> ( projects: Some(PartialWorkspaceProjects::Sources(FxHashMap::from_iter([ ("node".into(), "base".to_owned()), ("lifecycles".into(), "lifecycles".to_owned()), + ("postinstall".into(), "postinstall".to_owned()), ( "postinstallRecursion".into(), "postinstall-recursion".to_owned(), diff --git a/tests/fixtures/node/base/execBin.js b/tests/fixtures/node/base/execBin.js index 7fb650504f1..3c1b7c265ec 100644 --- a/tests/fixtures/node/base/execBin.js +++ b/tests/fixtures/node/base/execBin.js @@ -1,3 +1,3 @@ -const { spawn } = require('child_process'); +const { spawnSync } = require('child_process'); -spawn('node', ['--version'], { stdio: 'inherit' }); +spawnSync('node', ['--version'], { stdio: 'inherit' }); diff --git a/tests/fixtures/node/postinstall/package.json b/tests/fixtures/node/postinstall/package.json new file mode 100644 index 00000000000..c12aaec245f --- /dev/null +++ b/tests/fixtures/node/postinstall/package.json @@ -0,0 +1,7 @@ +{ + "name": "test-node-postinstall", + "version": "1.0.0", + "scripts": { + "postinstall": "node ./postinstall.js" + } +} diff --git a/tests/fixtures/node/postinstall/postinstall.js b/tests/fixtures/node/postinstall/postinstall.js new file mode 100644 index 00000000000..3c1b7c265ec --- /dev/null +++ b/tests/fixtures/node/postinstall/postinstall.js @@ -0,0 +1,3 @@ +const { spawnSync } = require('child_process'); + +spawnSync('node', ['--version'], { stdio: 'inherit' }); From 56c4cbbabc1121bbf419bc9e142d8f497d17cfe3 Mon Sep 17 00:00:00 2001 From: Miles Johnson Date: Sun, 1 Oct 2023 12:17:12 -0700 Subject: [PATCH 3/3] Polish. --- .yarn/versions/fdfb2523.yml | 9 +++++++++ crates/core/tool/src/lib.rs | 12 ++++++++++-- crates/node/platform/src/actions/run_target.rs | 16 +++++++++++----- crates/node/tool/src/node_tool.rs | 4 ++-- crates/node/tool/src/npm_tool.rs | 7 +++++-- crates/node/tool/src/pnpm_tool.rs | 7 +++++-- crates/node/tool/src/yarn_tool.rs | 7 +++++-- packages/cli/CHANGELOG.md | 6 ++++++ 8 files changed, 53 insertions(+), 15 deletions(-) create mode 100644 .yarn/versions/fdfb2523.yml diff --git a/.yarn/versions/fdfb2523.yml b/.yarn/versions/fdfb2523.yml new file mode 100644 index 00000000000..1f2a5d9c201 --- /dev/null +++ b/.yarn/versions/fdfb2523.yml @@ -0,0 +1,9 @@ +releases: + "@moonrepo/cli": patch + "@moonrepo/core-linux-arm64-gnu": patch + "@moonrepo/core-linux-arm64-musl": patch + "@moonrepo/core-linux-x64-gnu": patch + "@moonrepo/core-linux-x64-musl": patch + "@moonrepo/core-macos-arm64": patch + "@moonrepo/core-macos-x64": patch + "@moonrepo/core-windows-x64-msvc": patch diff --git a/crates/core/tool/src/lib.rs b/crates/core/tool/src/lib.rs index 90b6414654b..00aa15c8e85 100644 --- a/crates/core/tool/src/lib.rs +++ b/crates/core/tool/src/lib.rs @@ -18,9 +18,17 @@ use std::path::Path; /// other binaries of the same name. Otherwise, tooling like nvm will /// intercept execution and break our processes. We can work around this /// by prepending the `PATH` environment variable. -pub fn get_path_env_var(bin_dir: &Path) -> std::ffi::OsString { +pub fn prepend_path_env_var(paths: I) -> std::ffi::OsString +where + I: IntoIterator, + V: AsRef, +{ let path = env::var("PATH").unwrap_or_default(); - let mut paths = vec![bin_dir.to_path_buf()]; + + let mut paths = paths + .into_iter() + .map(|p| p.as_ref().to_path_buf()) + .collect::>(); paths.extend(env::split_paths(&path).collect::>()); diff --git a/crates/node/platform/src/actions/run_target.rs b/crates/node/platform/src/actions/run_target.rs index 6a1444b5e13..8f34ed35770 100644 --- a/crates/node/platform/src/actions/run_target.rs +++ b/crates/node/platform/src/actions/run_target.rs @@ -10,7 +10,7 @@ use moon_node_tool::NodeTool; use moon_process::Command; use moon_project::Project; use moon_task::Task; -use moon_tool::{get_path_env_var, DependencyManager, Tool, ToolError}; +use moon_tool::{prepend_path_env_var, DependencyManager, Tool, ToolError}; use moon_utils::{get_cache_dir, path, string_vec}; use rustc_hash::FxHashMap; use starbase_styles::color; @@ -163,6 +163,7 @@ pub fn create_target_command( let node_bin = node.get_bin_path()?; let node_options = create_node_options(&node.config, context, task)?; let mut command = Command::new(node.get_shim_path().unwrap_or(node_bin)); + let mut is_package_manager = false; match task.command.as_str() { "node" | "nodejs" => { @@ -172,12 +173,15 @@ pub fn create_target_command( command = Command::new(node.get_npx_path()?); } "npm" => { + is_package_manager = true; command = node.get_npm()?.create_command(node)?; } "pnpm" => { + is_package_manager = true; command = node.get_pnpm()?.create_command(node)?; } "yarn" | "yarnpkg" => { + is_package_manager = true; command = node.get_yarn()?.create_command(node)?; } bin => { @@ -189,10 +193,12 @@ pub fn create_target_command( } }; - command.env( - "PATH", - get_path_env_var(node.tool.get_bin_path()?.parent().unwrap()), - ); + if !is_package_manager { + command.env( + "PATH", + prepend_path_env_var([node.tool.get_bin_path()?.parent().unwrap()]), + ); + } prepare_target_command(&mut command, context, task, &node.config)?; diff --git a/crates/node/tool/src/node_tool.rs b/crates/node/tool/src/node_tool.rs index 46be9690735..a379a3d6c63 100644 --- a/crates/node/tool/src/node_tool.rs +++ b/crates/node/tool/src/node_tool.rs @@ -8,7 +8,7 @@ use moon_platform_runtime::RuntimeReq; use moon_process::Command; use moon_terminal::{print_checkpoint, Checkpoint}; use moon_tool::{ - async_trait, get_path_env_var, load_tool_plugin, DependencyManager, Tool, ToolError, + async_trait, load_tool_plugin, prepend_path_env_var, DependencyManager, Tool, ToolError, }; use proto_core::{Id, ProtoEnvironment, Tool as ProtoTool}; use rustc_hash::FxHashMap; @@ -80,7 +80,7 @@ impl NodeTool { if !self.global { cmd.env( "PATH", - get_path_env_var(self.tool.get_bin_path()?.parent().unwrap()), + prepend_path_env_var([self.tool.get_bin_path()?.parent().unwrap()]), ); } diff --git a/crates/node/tool/src/npm_tool.rs b/crates/node/tool/src/npm_tool.rs index 52e9007c80b..050e0ad2546 100644 --- a/crates/node/tool/src/npm_tool.rs +++ b/crates/node/tool/src/npm_tool.rs @@ -4,7 +4,7 @@ use moon_logger::debug; use moon_node_lang::{npm, LockfileDependencyVersions, NPM}; use moon_process::Command; use moon_terminal::{print_checkpoint, Checkpoint}; -use moon_tool::{async_trait, get_path_env_var, load_tool_plugin, DependencyManager, Tool}; +use moon_tool::{async_trait, load_tool_plugin, prepend_path_env_var, DependencyManager, Tool}; use moon_utils::is_ci; use proto_core::{Id, ProtoEnvironment, Tool as ProtoTool, UnresolvedVersionSpec}; use rustc_hash::FxHashMap; @@ -119,7 +119,10 @@ impl DependencyManager for NpmTool { if !self.global { cmd.env( "PATH", - get_path_env_var(self.tool.get_bin_path()?.parent().unwrap()), + prepend_path_env_var([ + node.get_bin_path()?.parent().unwrap(), + self.tool.get_bin_path()?.parent().unwrap(), + ]), ); } diff --git a/crates/node/tool/src/pnpm_tool.rs b/crates/node/tool/src/pnpm_tool.rs index 22ecea144ed..570fc462b7a 100644 --- a/crates/node/tool/src/pnpm_tool.rs +++ b/crates/node/tool/src/pnpm_tool.rs @@ -4,7 +4,7 @@ use moon_logger::debug; use moon_node_lang::{pnpm, LockfileDependencyVersions, PNPM}; use moon_process::Command; use moon_terminal::{print_checkpoint, Checkpoint}; -use moon_tool::{async_trait, get_path_env_var, load_tool_plugin, DependencyManager, Tool}; +use moon_tool::{async_trait, load_tool_plugin, prepend_path_env_var, DependencyManager, Tool}; use moon_utils::is_ci; use proto_core::{Id, ProtoEnvironment, Tool as ProtoTool, UnresolvedVersionSpec, VersionReq}; use rustc_hash::FxHashMap; @@ -125,7 +125,10 @@ impl DependencyManager for PnpmTool { if !self.global { cmd.env( "PATH", - get_path_env_var(self.tool.get_bin_path()?.parent().unwrap()), + prepend_path_env_var([ + node.get_bin_path()?.parent().unwrap(), + self.tool.get_bin_path()?.parent().unwrap(), + ]), ); } diff --git a/crates/node/tool/src/yarn_tool.rs b/crates/node/tool/src/yarn_tool.rs index 4e80ea90ffc..f1f928dfdc9 100644 --- a/crates/node/tool/src/yarn_tool.rs +++ b/crates/node/tool/src/yarn_tool.rs @@ -5,7 +5,7 @@ use moon_node_lang::{yarn, LockfileDependencyVersions, YARN}; use moon_process::Command; use moon_terminal::{print_checkpoint, Checkpoint}; use moon_tool::{ - async_trait, get_path_env_var, load_tool_plugin, DependencyManager, Tool, ToolError, + async_trait, load_tool_plugin, prepend_path_env_var, DependencyManager, Tool, ToolError, }; use moon_utils::{get_workspace_root, is_ci}; use proto_core::{Id, ProtoEnvironment, Tool as ProtoTool, UnresolvedVersionSpec}; @@ -181,7 +181,10 @@ impl DependencyManager for YarnTool { if !self.global { cmd.env( "PATH", - get_path_env_var(self.tool.get_bin_path()?.parent().unwrap()), + prepend_path_env_var([ + node.get_bin_path()?.parent().unwrap(), + self.tool.get_bin_path()?.parent().unwrap(), + ]), ); } diff --git a/packages/cli/CHANGELOG.md b/packages/cli/CHANGELOG.md index a0f2309afd4..1a19693c6c7 100644 --- a/packages/cli/CHANGELOG.md +++ b/packages/cli/CHANGELOG.md @@ -10,6 +10,12 @@ - More accurately monitors signals (ctrl+c) and shutdowns. - Tasks can now be configured with a timeout. +## Unreleased + +#### 🐞 Fixes + +- Fixed some `PATH` inconsistencies when executing npm/pnpm/yarn binaries. + ## 1.14.3 #### 🚀 Updates