src: traverse parent folders while running --run

PR-URL: #53154
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: James M Snell <jasnell@gmail.com>

doc/api/cli.md

@@ -1957,16 +1957,25 @@ changes:
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/53058
     description: NODE_RUN_PACKAGE_JSON_PATH environment variable is added.
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/53154
+    description: Traverses up to the root directory and finds
+                 a `package.json` file to run the command from, and updates
+                 `PATH` environment variable accordingly.
 -->
 
 > Stability: 1.1 - Active development
 
 This runs a specified command from a package.json's `"scripts"` object.
 If no `"command"` is provided, it will list the available scripts.
 
-`--run` prepends `./node_modules/.bin`, relative to the current
-working directory, to the `PATH` in order to execute the binaries from
-dependencies.
+`--run` will traverse up to the root directory and finds a `package.json`
+file to run the command from.
+
+`--run` prepends `./node_modules/.bin` for each ancestor of
+the current directory, to the `PATH` in order to execute the binaries from
+different folders where multiple `node_modules` directories are present, if
+`ancestor-folder/node_modules/.bin` is a directory.
 
 For example, the following command will run the `test` script of
 the `package.json` in the current folder:
@@ -1991,9 +2000,6 @@ cases.
 Some features of other `run` implementations that are intentionally excluded
 are:
 
-* Searching for `package.json` files outside the current folder.
-* Prepending the `.bin` or `node_modules/.bin` paths of folders outside the
-  current folder.
 * Running `pre` or `post` scripts in addition to the specified script.
 * Defining package manager-specific environment variables.
 

src/node_task_runner.cc

@@ -1,36 +1,28 @@
 #include "node_task_runner.h"
 #include "util.h"
 
-#include <filesystem>
 #include <regex>  // NOLINT(build/c++11)
 
 namespace node::task_runner {
 
 #ifdef _WIN32
-static constexpr const char* bin_path = "\\node_modules\\.bin";
+static constexpr const char* env_var_separator = ";";
 #else
-static constexpr const char* bin_path = "/node_modules/.bin";
+static constexpr const char* env_var_separator = ":";
 #endif  // _WIN32
 
 ProcessRunner::ProcessRunner(std::shared_ptr<InitializationResultImpl> result,
-                             std::string_view package_json_path,
+                             const std::filesystem::path& package_json_path,
                              std::string_view script_name,
                              std::string_view command,
-                             const PositionalArgs& positional_args) {
+                             std::string_view path_env_var,
+                             const PositionalArgs& positional_args)
+    : init_result(std::move(result)),
+      package_json_path_(package_json_path),
+      script_name_(script_name),
+      path_env_var_(path_env_var) {
   memset(&options_, 0, sizeof(uv_process_options_t));
 
-  // Get the current working directory.
-  char cwd[PATH_MAX_BYTES];
-  size_t cwd_size = PATH_MAX_BYTES;
-  CHECK_EQ(uv_cwd(cwd, &cwd_size), 0);
-  CHECK_GT(cwd_size, 0);
-
-#ifdef _WIN32
-  std::string current_bin_path = cwd + std::string(bin_path) + ";";
-#else
-  std::string current_bin_path = cwd + std::string(bin_path) + ":";
-#endif  // _WIN32
-
   // Inherit stdin, stdout, and stderr from the parent process.
   options_.stdio_count = 3;
   child_stdio[0].flags = UV_INHERIT_FD;
@@ -46,18 +38,13 @@ ProcessRunner::ProcessRunner(std::shared_ptr<InitializationResultImpl> result,
   options_.flags |= UV_PROCESS_WINDOWS_VERBATIM_ARGUMENTS;
 #endif
 
-  init_result = std::move(result);
-
   // Set the process handle data to this class instance.
   // This is used to access the class instance from the OnExit callback.
   // It is required because libuv doesn't allow passing lambda functions as a
   // callback.
   process_.data = this;
 
-  SetEnvironmentVariables(current_bin_path,
-                          std::string_view(cwd, cwd_size),
-                          package_json_path,
-                          script_name);
+  SetEnvironmentVariables();
 
   std::string command_str(command);
 
@@ -73,12 +60,7 @@ ProcessRunner::ProcessRunner(std::shared_ptr<InitializationResultImpl> result,
   }
 
 #ifdef _WIN32
-  // We check whether file_ ends with cmd.exe in a case-insensitive manner.
-  // C++20 provides ends_with, but we roll our own for compatibility.
-  const char* cmdexe = "cmd.exe";
-  if (file_.size() >= strlen(cmdexe) &&
-      StringEqualNoCase(cmdexe,
-                        file_.c_str() + file_.size() - strlen(cmdexe))) {
+  if (file_.ends_with("cmd.exe")) {
     // If the file is cmd.exe, use the following command line arguments:
     // "/c" Carries out the command and exit.
     // "/d" Disables execution of AutoRun commands.
@@ -106,10 +88,7 @@ ProcessRunner::ProcessRunner(std::shared_ptr<InitializationResultImpl> result,
   options_.args[argc] = nullptr;
 }
 
-void ProcessRunner::SetEnvironmentVariables(const std::string& current_bin_path,
-                                            std::string_view cwd,
-                                            std::string_view package_json_path,
-                                            std::string_view script_name) {
+void ProcessRunner::SetEnvironmentVariables() {
   uv_env_item_t* env_items;
   int env_count;
   CHECK_EQ(0, uv_os_environ(&env_items, &env_count));
@@ -130,28 +109,21 @@ void ProcessRunner::SetEnvironmentVariables(const std::string& current_bin_path,
 #endif  // _WIN32
 
     if (StringEqualNoCase(name.c_str(), "path")) {
-      // Add bin_path to the beginning of the PATH
-      value = current_bin_path + value;
+      // Add path env variable to the beginning of the PATH
+      value = path_env_var_ + value;
     }
     env_vars_.push_back(name + "=" + value);
   }
   uv_os_free_environ(env_items, env_count);
 
   // Add NODE_RUN_SCRIPT_NAME environment variable to the environment
   // to indicate which script is being run.
-  env_vars_.push_back("NODE_RUN_SCRIPT_NAME=" + std::string(script_name));
+  env_vars_.push_back("NODE_RUN_SCRIPT_NAME=" + script_name_);
 
   // Add NODE_RUN_PACKAGE_JSON_PATH environment variable to the environment to
   // indicate which package.json is being processed.
-  if (std::filesystem::path(package_json_path).is_absolute()) {
-    // TODO(anonrig): Traverse up the directory tree until we find a
-    // package.json
-    env_vars_.push_back("NODE_RUN_PACKAGE_JSON_PATH=" +
-                        std::string(package_json_path));
-  } else {
-    auto path = std::filesystem::path(cwd) / std::string(package_json_path);
-    env_vars_.push_back("NODE_RUN_PACKAGE_JSON_PATH=" + path.string());
-  }
+  env_vars_.push_back("NODE_RUN_PACKAGE_JSON_PATH=" +
+                      package_json_path_.string());
 
   env = std::unique_ptr<char*[]>(new char*[env_vars_.size() + 1]);
   options_.env = env.get();
@@ -240,19 +212,58 @@ void ProcessRunner::Run() {
   uv_run(loop_, UV_RUN_DEFAULT);
 }
 
+std::optional<std::tuple<std::filesystem::path, std::string, std::string>>
+FindPackageJson(const std::filesystem::path& cwd) {
+  auto package_json_path = cwd / "package.json";
+  std::string raw_content;
+  std::string path_env_var;
+  auto root_path = cwd.root_path();
+
+  for (auto directory_path = cwd;
+       !std::filesystem::equivalent(root_path, directory_path);
+       directory_path = directory_path.parent_path()) {
+    // Append "path/node_modules/.bin" to the env var, if it is a directory.
+    auto node_modules_bin = directory_path / "node_modules" / ".bin";
+    if (std::filesystem::is_directory(node_modules_bin)) {
+      path_env_var += node_modules_bin.string() + env_var_separator;
+    }
+
+    if (raw_content.empty()) {
+      package_json_path = directory_path / "package.json";
+      // This is required for Windows because std::filesystem::path::c_str()
+      // returns wchar_t* on Windows, and char* on other platforms.
+      std::string contents = package_json_path.string();
+      USE(ReadFileSync(&raw_content, contents.c_str()) > 0);
+    }
+  }
+
+  // This means that there is no package.json until the root directory.
+  // In this case, we just return nullopt, which will terminate the process..
+  if (raw_content.empty()) {
+    return std::nullopt;
+  }
+
+  return {{package_json_path, raw_content, path_env_var}};
+}
+
 void RunTask(std::shared_ptr<InitializationResultImpl> result,
              std::string_view command_id,
              const std::vector<std::string_view>& positional_args) {
-  std::string_view path = "package.json";
-  std::string raw_json;
+  auto cwd = std::filesystem::current_path();
+  auto package_json = FindPackageJson(cwd);
 
-  // No need to exclude BOM since simdjson will skip it.
-  if (ReadFileSync(&raw_json, path.data()) < 0) {
+  if (!package_json.has_value()) {
     fprintf(stderr, "Can't read package.json\n");
     result->exit_code_ = ExitCode::kGenericUserError;
     return;
   }
 
+  // - path: Path to the package.json file.
+  // - raw_json: Raw content of the package.json file.
+  // - path_env_var: This represents the `PATH` environment variable.
+  //   It always ends with ";" or ":" depending on the platform.
+  auto [path, raw_json, path_env_var] = *package_json;
+
   simdjson::ondemand::parser json_parser;
   simdjson::ondemand::document document;
   simdjson::ondemand::object main_object;
@@ -302,8 +313,8 @@ void RunTask(std::shared_ptr<InitializationResultImpl> result,
     return;
   }
 
-  auto runner =
-      ProcessRunner(result, path, command_id, command, positional_args);
+  auto runner = ProcessRunner(
+      result, path, command_id, command, path_env_var, positional_args);
   runner.Run();
 }
 
@@ -317,10 +328,11 @@ PositionalArgs GetPositionalArgs(const std::vector<std::string>& args) {
   if (auto dash_dash = std::find(args.begin(), args.end(), "--");
       dash_dash != args.end()) {
     PositionalArgs positional_args{};
+    positional_args.reserve(args.size() - (dash_dash - args.begin()));
     for (auto it = dash_dash + 1; it != args.end(); ++it) {
       // SAFETY: The following code is safe because the lifetime of the
       // arguments is guaranteed to be valid until the end of the task runner.
-      positional_args.push_back(std::string_view(it->c_str(), it->size()));
+      positional_args.emplace_back(it->c_str(), it->size());
     }
     return positional_args;
   }

src/node_task_runner.h

@@ -8,11 +8,12 @@
 #include "spawn_sync.h"
 #include "uv.h"
 
+#include <filesystem>
 #include <optional>
 #include <string_view>
+#include <tuple>
 
-namespace node {
-namespace task_runner {
+namespace node::task_runner {
 
 using PositionalArgs = std::vector<std::string_view>;
 
@@ -23,9 +24,10 @@ using PositionalArgs = std::vector<std::string_view>;
 class ProcessRunner {
  public:
   ProcessRunner(std::shared_ptr<InitializationResultImpl> result,
-                std::string_view package_json_path,
+                const std::filesystem::path& package_json_path,
                 std::string_view script_name,
-                std::string_view command_id,
+                std::string_view command,
+                std::string_view path_env_var,
                 const PositionalArgs& positional_args);
   void Run();
   static void ExitCallback(uv_process_t* req,
@@ -45,26 +47,44 @@ class ProcessRunner {
 
   // OnExit is the callback function that is called when the process exits.
   void OnExit(int64_t exit_status, int term_signal);
-  void SetEnvironmentVariables(const std::string& bin_path,
-                               std::string_view cwd,
-                               std::string_view package_json_path,
-                               std::string_view script_name);
+  void SetEnvironmentVariables();
 
 #ifdef _WIN32
   std::string file_ = "cmd.exe";
 #else
   std::string file_ = "/bin/sh";
 #endif  // _WIN32
+
+  // Represents the absolute path to the package.json file.
+  std::filesystem::path package_json_path_;
+  // Represents the name of the script that is being run.
+  std::string script_name_;
+  // Represents PATH environment variable that contains
+  // all subdirectory paths appended with node_modules/.bin suffix.
+  std::string path_env_var_;
 };
 
+// This function traverses up to the root directory.
+// While traversing up, if it finds a package.json file, it reads its content.
+// If it cannot find a package.json file, it returns std::nullopt.
+// Otherwise, it returns a tuple of:
+// - the path to the package.json file
+// - package.json file content
+// - `path_env_var` variable
+//
+// For example, on POSIX, it returns the following for `path_env_var`,
+// if the current directory is `/anonrig`:
+// `/anonrig/node_modules/.bin:/node_modules/.bin`
+std::optional<std::tuple<std::filesystem::path, std::string, std::string>>
+FindPackageJson(const std::filesystem::path& cwd);
+
 void RunTask(std::shared_ptr<InitializationResultImpl> result,
              std::string_view command_id,
              const PositionalArgs& positional_args);
 PositionalArgs GetPositionalArgs(const std::vector<std::string>& args);
-std::string EscapeShell(const std::string_view command);
+std::string EscapeShell(std::string_view command);
 
-}  // namespace task_runner
-}  // namespace node
+}  // namespace node::task_runner
 
 #endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 

test/parallel/test-node-run.js

@@ -1,4 +1,3 @@
-// Flags: --expose-internals
 'use strict';
 
 const common = require('../common');
@@ -8,7 +7,7 @@ const assert = require('node:assert');
 const fixtures = require('../common/fixtures');
 const envSuffix = common.isWindows ? '-windows' : '';
 
-describe('node run [command]', () => {
+describe('node --run [command]', () => {
   it('should emit experimental warning', async () => {
     const child = await common.spawnPromisified(
       process.execPath,
@@ -70,13 +69,21 @@ describe('node run [command]', () => {
     assert.strictEqual(child.code, 0);
   });
 
-  it('should set PATH environment variable to node_modules/.bin', async () => {
+  it('should set PATH environment variable with paths appended with node_modules/.bin', async () => {
     const child = await common.spawnPromisified(
       process.execPath,
       [ '--no-warnings', '--run', `path-env${envSuffix}`],
-      { cwd: fixtures.path('run-script') },
+      { cwd: fixtures.path('run-script/sub-directory') },
     );
     assert.ok(child.stdout.includes(fixtures.path('run-script/node_modules/.bin')));
+
+    // The following test ensures that we do not add paths that does not contain
+    // "node_modules/.bin"
+    assert.ok(!child.stdout.includes(fixtures.path('node_modules/.bin')));
+
+    // The following test ensures that we add paths that contains "node_modules/.bin"
+    assert.ok(child.stdout.includes(fixtures.path('run-script/sub-directory/node_modules/.bin')));
+
     assert.strictEqual(child.stderr, '');
     assert.strictEqual(child.code, 0);
   });
@@ -94,4 +101,16 @@ describe('node run [command]', () => {
     assert.strictEqual(child.stderr, '');
     assert.strictEqual(child.code, 0);
   });
+
+  it('will search parent directories for a package.json file', async () => {
+    const packageJsonPath = fixtures.path('run-script/package.json');
+    const child = await common.spawnPromisified(
+      process.execPath,
+      [ '--no-warnings', '--run', `special-env-variables${envSuffix}`],
+      { cwd: fixtures.path('run-script/sub-directory') },
+    );
+    assert.ok(child.stdout.includes(packageJsonPath));
+    assert.strictEqual(child.stderr, '');
+    assert.strictEqual(child.code, 0);
+  });
 });