From 5a41bcf9ca4af8d8a5ae08192f4598e0ec34cfb5 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Sat, 8 Jun 2024 00:20:52 +0300 Subject: [PATCH] src: traverse parent folders while running `--run` PR-URL: https://github.com/nodejs/node/pull/53154 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Daniel Lemire Reviewed-By: James M Snell --- doc/api/cli.md | 18 ++- src/node_task_runner.cc | 116 ++++++++++-------- src/node_task_runner.h | 42 +++++-- .../run-script/sub-directory/.gitkeep | 0 .../sub-directory/node_modules/.bin/.gitkeep | 0 test/parallel/test-node-run.js | 27 +++- 6 files changed, 130 insertions(+), 73 deletions(-) create mode 100644 test/fixtures/run-script/sub-directory/.gitkeep create mode 100644 test/fixtures/run-script/sub-directory/node_modules/.bin/.gitkeep diff --git a/doc/api/cli.md b/doc/api/cli.md index c81917c4e19f57..b461b11cf4071c 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -1957,6 +1957,11 @@ 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 @@ -1964,9 +1969,13 @@ changes: 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. diff --git a/src/node_task_runner.cc b/src/node_task_runner.cc index 82dcfdc5c3cf70..08138e326691f1 100644 --- a/src/node_task_runner.cc +++ b/src/node_task_runner.cc @@ -1,36 +1,28 @@ #include "node_task_runner.h" #include "util.h" -#include #include // 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 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 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 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 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,8 +109,8 @@ 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); } @@ -139,19 +118,12 @@ void ProcessRunner::SetEnvironmentVariables(const std::string& current_bin_path, // 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(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> +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 result, std::string_view command_id, const std::vector& 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 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& 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; } diff --git a/src/node_task_runner.h b/src/node_task_runner.h index ea430a6e32c747..a498637f017dc9 100644 --- a/src/node_task_runner.h +++ b/src/node_task_runner.h @@ -8,11 +8,12 @@ #include "spawn_sync.h" #include "uv.h" +#include #include #include +#include -namespace node { -namespace task_runner { +namespace node::task_runner { using PositionalArgs = std::vector; @@ -23,9 +24,10 @@ using PositionalArgs = std::vector; class ProcessRunner { public: ProcessRunner(std::shared_ptr 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> +FindPackageJson(const std::filesystem::path& cwd); + void RunTask(std::shared_ptr result, std::string_view command_id, const PositionalArgs& positional_args); PositionalArgs GetPositionalArgs(const std::vector& 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 diff --git a/test/fixtures/run-script/sub-directory/.gitkeep b/test/fixtures/run-script/sub-directory/.gitkeep new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/test/fixtures/run-script/sub-directory/node_modules/.bin/.gitkeep b/test/fixtures/run-script/sub-directory/node_modules/.bin/.gitkeep new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/test/parallel/test-node-run.js b/test/parallel/test-node-run.js index 89c014bddd2f1b..75f9343c2aa716 100644 --- a/test/parallel/test-node-run.js +++ b/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); + }); });