From 0c5fa57bc7ef6bdbe867c3a3dd53adefa155795f Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Wed, 18 Sep 2024 08:18:40 -0400 Subject: [PATCH] cli: ensure --run has proper pwd MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/54949 Refs: https://github.com/nodejs/node/pull/53600 Reviewed-By: Matthew Aitken Reviewed-By: Vinícius Lourenço Claro Cardoso --- doc/api/cli.md | 2 + src/node_task_runner.cc | 82 +++++++++----- src/node_task_runner.h | 3 +- .../run-script/invalid-json/package.json | 3 + .../run-script/invalid-schema/package.json | 9 ++ .../run-script/missing-scripts/package.json | 1 + test/fixtures/run-script/package.json | 4 +- test/message/node_run_non_existent.out | 4 +- test/parallel/test-node-run.js | 106 +++++++++++++++++- 9 files changed, 179 insertions(+), 35 deletions(-) create mode 100644 test/fixtures/run-script/invalid-json/package.json create mode 100644 test/fixtures/run-script/invalid-schema/package.json create mode 100644 test/fixtures/run-script/missing-scripts/package.json diff --git a/doc/api/cli.md b/doc/api/cli.md index c33977a0b2b..8938845c273 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -2110,6 +2110,8 @@ 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. +`--run` executes the command in the directory containing the related `package.json`. + For example, the following command will run the `test` script of the `package.json` in the current folder: diff --git a/src/node_task_runner.cc b/src/node_task_runner.cc index 08138e32669..32ef0aa2ad1 100644 --- a/src/node_task_runner.cc +++ b/src/node_task_runner.cc @@ -149,7 +149,7 @@ std::string EscapeShell(const std::string_view input) { #endif } - static const std::string_view forbidden_characters = + static constexpr std::string_view forbidden_characters = "[\t\n\r \"#$&'()*;<>?\\\\`|~]"; // Check if input contains any forbidden characters @@ -191,7 +191,7 @@ std::string EscapeShell(const std::string_view input) { void ProcessRunner::ExitCallback(uv_process_t* handle, int64_t exit_status, int term_signal) { - auto self = reinterpret_cast(handle->data); + const auto self = static_cast(handle->data); uv_close(reinterpret_cast(handle), nullptr); self->OnExit(exit_status, term_signal); } @@ -205,6 +205,9 @@ void ProcessRunner::OnExit(int64_t exit_status, int term_signal) { } void ProcessRunner::Run() { + // keeps the string alive until destructor + cwd = package_json_path_.parent_path().string(); + options_.cwd = cwd.c_str(); if (int r = uv_spawn(loop_, &process_, &options_)) { fprintf(stderr, "Error: %s\n", uv_strerror(r)); } @@ -246,14 +249,16 @@ FindPackageJson(const std::filesystem::path& cwd) { return {{package_json_path, raw_content, path_env_var}}; } -void RunTask(std::shared_ptr result, +void RunTask(const std::shared_ptr& result, std::string_view command_id, const std::vector& positional_args) { auto cwd = std::filesystem::current_path(); auto package_json = FindPackageJson(cwd); if (!package_json.has_value()) { - fprintf(stderr, "Can't read package.json\n"); + fprintf(stderr, + "Can't find package.json for directory %s\n", + cwd.string().c_str()); result->exit_code_ = ExitCode::kGenericUserError; return; } @@ -267,11 +272,21 @@ void RunTask(std::shared_ptr result, simdjson::ondemand::parser json_parser; simdjson::ondemand::document document; simdjson::ondemand::object main_object; - simdjson::error_code error = json_parser.iterate(raw_json).get(document); + if (json_parser.iterate(raw_json).get(document)) { + fprintf(stderr, "Can't parse %s\n", path.string().c_str()); + result->exit_code_ = ExitCode::kGenericUserError; + return; + } // If document is not an object, throw an error. - if (error || document.get_object().get(main_object)) { - fprintf(stderr, "Can't parse package.json\n"); + if (auto root_error = document.get_object().get(main_object)) { + if (root_error == simdjson::error_code::INCORRECT_TYPE) { + fprintf(stderr, + "Root value unexpected not an object for %s\n\n", + path.string().c_str()); + } else { + fprintf(stderr, "Can't parse %s\n", path.string().c_str()); + } result->exit_code_ = ExitCode::kGenericUserError; return; } @@ -279,34 +294,45 @@ void RunTask(std::shared_ptr result, // If package_json object doesn't have "scripts" field, throw an error. simdjson::ondemand::object scripts_object; if (main_object["scripts"].get_object().get(scripts_object)) { - fprintf(stderr, "Can't find \"scripts\" field in package.json\n"); + fprintf( + stderr, "Can't find \"scripts\" field in %s\n", path.string().c_str()); result->exit_code_ = ExitCode::kGenericUserError; return; } // If the command_id is not found in the scripts object, throw an error. std::string_view command; - if (scripts_object[command_id].get_string().get(command)) { - fprintf(stderr, - "Missing script: \"%.*s\"\n\n", - static_cast(command_id.size()), - command_id.data()); - fprintf(stderr, "Available scripts are:\n"); + if (auto command_error = + scripts_object[command_id].get_string().get(command)) { + if (command_error == simdjson::error_code::INCORRECT_TYPE) { + fprintf(stderr, + "Script \"%.*s\" is unexpectedly not a string for %s\n\n", + static_cast(command_id.size()), + command_id.data(), + path.string().c_str()); + } else { + fprintf(stderr, + "Missing script: \"%.*s\" for %s\n\n", + static_cast(command_id.size()), + command_id.data(), + path.string().c_str()); + fprintf(stderr, "Available scripts are:\n"); - // Reset the object to iterate over it again - scripts_object.reset(); - simdjson::ondemand::value value; - for (auto field : scripts_object) { - std::string_view key_str; - std::string_view value_str; - if (!field.unescaped_key().get(key_str) && !field.value().get(value) && - !value.get_string().get(value_str)) { - fprintf(stderr, - " %.*s: %.*s\n", - static_cast(key_str.size()), - key_str.data(), - static_cast(value_str.size()), - value_str.data()); + // Reset the object to iterate over it again + scripts_object.reset(); + simdjson::ondemand::value value; + for (auto field : scripts_object) { + std::string_view key_str; + std::string_view value_str; + if (!field.unescaped_key().get(key_str) && !field.value().get(value) && + !value.get_string().get(value_str)) { + fprintf(stderr, + " %.*s: %.*s\n", + static_cast(key_str.size()), + key_str.data(), + static_cast(value_str.size()), + value_str.data()); + } } } result->exit_code_ = ExitCode::kGenericUserError; diff --git a/src/node_task_runner.h b/src/node_task_runner.h index a498637f017..ec8f2adf45a 100644 --- a/src/node_task_runner.h +++ b/src/node_task_runner.h @@ -44,6 +44,7 @@ class ProcessRunner { std::vector env_vars_{}; std::unique_ptr env {}; // memory for options_.env std::unique_ptr arg {}; // memory for options_.args + std::string cwd; // OnExit is the callback function that is called when the process exits. void OnExit(int64_t exit_status, int term_signal); @@ -78,7 +79,7 @@ class ProcessRunner { std::optional> FindPackageJson(const std::filesystem::path& cwd); -void RunTask(std::shared_ptr result, +void RunTask(const std::shared_ptr& result, std::string_view command_id, const PositionalArgs& positional_args); PositionalArgs GetPositionalArgs(const std::vector& args); diff --git a/test/fixtures/run-script/invalid-json/package.json b/test/fixtures/run-script/invalid-json/package.json new file mode 100644 index 00000000000..22ad2cc50fa --- /dev/null +++ b/test/fixtures/run-script/invalid-json/package.json @@ -0,0 +1,3 @@ +{ + "scripts": {}, +} diff --git a/test/fixtures/run-script/invalid-schema/package.json b/test/fixtures/run-script/invalid-schema/package.json new file mode 100644 index 00000000000..59dfa8a201b --- /dev/null +++ b/test/fixtures/run-script/invalid-schema/package.json @@ -0,0 +1,9 @@ +{ + "scripts": { + "array": [], + "boolean": true, + "null": null, + "number": 1.0, + "object": {} + } +} diff --git a/test/fixtures/run-script/missing-scripts/package.json b/test/fixtures/run-script/missing-scripts/package.json new file mode 100644 index 00000000000..0967ef424bc --- /dev/null +++ b/test/fixtures/run-script/missing-scripts/package.json @@ -0,0 +1 @@ +{} diff --git a/test/fixtures/run-script/package.json b/test/fixtures/run-script/package.json index c103b48840f..138f47f2f97 100644 --- a/test/fixtures/run-script/package.json +++ b/test/fixtures/run-script/package.json @@ -10,6 +10,8 @@ "path-env": "path-env", "path-env-windows": "path-env.bat", "special-env-variables": "special-env-variables", - "special-env-variables-windows": "special-env-variables.bat" + "special-env-variables-windows": "special-env-variables.bat", + "pwd": "pwd", + "pwd-windows": "cd" } } diff --git a/test/message/node_run_non_existent.out b/test/message/node_run_non_existent.out index 3397d837b04..06b2ab0bb16 100644 --- a/test/message/node_run_non_existent.out +++ b/test/message/node_run_non_existent.out @@ -1,4 +1,4 @@ -Missing script: "non-existent-command" +Missing script: "non-existent-command" for * Available scripts are: test: echo "Error: no test specified" && exit 1 @@ -12,3 +12,5 @@ Available scripts are: path-env-windows: path-env.bat special-env-variables: special-env-variables special-env-variables-windows: special-env-variables.bat + pwd: pwd + pwd-windows: cd diff --git a/test/parallel/test-node-run.js b/test/parallel/test-node-run.js index 5f9b95d2ef9..14881f42900 100644 --- a/test/parallel/test-node-run.js +++ b/test/parallel/test-node-run.js @@ -1,6 +1,8 @@ 'use strict'; const common = require('../common'); +common.requireNoPackageJSONAbove(); + const { it, describe } = require('node:test'); const assert = require('node:assert'); @@ -15,7 +17,6 @@ describe('node --run [command]', () => { { cwd: __dirname }, ); assert.match(child.stderr, /ExperimentalWarning: Task runner is an experimental feature and might change at any time/); - assert.match(child.stderr, /Can't read package\.json/); assert.strictEqual(child.stdout, ''); assert.strictEqual(child.code, 1); }); @@ -26,7 +27,9 @@ describe('node --run [command]', () => { [ '--no-warnings', '--run', 'test'], { cwd: __dirname }, ); - assert.match(child.stderr, /Can't read package\.json/); + assert.match(child.stderr, /Can't find package\.json[\s\S]*/); + // Ensure we show the path that starting path for the search + assert(child.stderr.includes(__dirname)); assert.strictEqual(child.stdout, ''); assert.strictEqual(child.code, 1); }); @@ -53,6 +56,101 @@ describe('node --run [command]', () => { assert.strictEqual(child.code, 0); }); + it('chdirs into package directory', async () => { + const child = await common.spawnPromisified( + process.execPath, + [ '--no-warnings', '--run', `pwd${envSuffix}`], + { cwd: fixtures.path('run-script/sub-directory') }, + ); + assert.strictEqual(child.stdout.trim(), fixtures.path('run-script')); + assert.strictEqual(child.stderr, ''); + assert.strictEqual(child.code, 0); + }); + + it('includes actionable info when possible', async () => { + { + const child = await common.spawnPromisified( + process.execPath, + [ '--no-warnings', '--run', 'missing'], + { cwd: fixtures.path('run-script') }, + ); + assert.strictEqual(child.stdout, ''); + assert(child.stderr.includes(fixtures.path('run-script/package.json'))); + assert(child.stderr.includes('no test specified')); + assert.strictEqual(child.code, 1); + } + { + const child = await common.spawnPromisified( + process.execPath, + [ '--no-warnings', '--run', 'test'], + { cwd: fixtures.path('run-script/missing-scripts') }, + ); + assert.strictEqual(child.stdout, ''); + assert(child.stderr.includes(fixtures.path('run-script/missing-scripts/package.json'))); + assert.strictEqual(child.code, 1); + } + { + const child = await common.spawnPromisified( + process.execPath, + [ '--no-warnings', '--run', 'test'], + { cwd: fixtures.path('run-script/invalid-json') }, + ); + assert.strictEqual(child.stdout, ''); + assert(child.stderr.includes(fixtures.path('run-script/invalid-json/package.json'))); + assert.strictEqual(child.code, 1); + } + { + const child = await common.spawnPromisified( + process.execPath, + [ '--no-warnings', '--run', 'array'], + { cwd: fixtures.path('run-script/invalid-schema') }, + ); + assert.strictEqual(child.stdout, ''); + assert(child.stderr.includes(fixtures.path('run-script/invalid-schema/package.json'))); + assert.strictEqual(child.code, 1); + } + { + const child = await common.spawnPromisified( + process.execPath, + [ '--no-warnings', '--run', 'boolean'], + { cwd: fixtures.path('run-script/invalid-schema') }, + ); + assert.strictEqual(child.stdout, ''); + assert(child.stderr.includes(fixtures.path('run-script/invalid-schema/package.json'))); + assert.strictEqual(child.code, 1); + } + { + const child = await common.spawnPromisified( + process.execPath, + [ '--no-warnings', '--run', 'null'], + { cwd: fixtures.path('run-script/invalid-schema') }, + ); + assert.strictEqual(child.stdout, ''); + assert(child.stderr.includes(fixtures.path('run-script/invalid-schema/package.json'))); + assert.strictEqual(child.code, 1); + } + { + const child = await common.spawnPromisified( + process.execPath, + [ '--no-warnings', '--run', 'number'], + { cwd: fixtures.path('run-script/invalid-schema') }, + ); + assert.strictEqual(child.stdout, ''); + assert(child.stderr.includes(fixtures.path('run-script/invalid-schema/package.json'))); + assert.strictEqual(child.code, 1); + } + { + const child = await common.spawnPromisified( + process.execPath, + [ '--no-warnings', '--run', 'object'], + { cwd: fixtures.path('run-script/invalid-schema') }, + ); + assert.strictEqual(child.stdout, ''); + assert(child.stderr.includes(fixtures.path('run-script/invalid-schema/package.json'))); + assert.strictEqual(child.code, 1); + } + }); + it('appends positional arguments', async () => { const child = await common.spawnPromisified( process.execPath, @@ -120,7 +218,7 @@ describe('node --run [command]', () => { [ '--no-warnings', '--run', 'test'], { cwd: fixtures.path('run-script/cannot-parse') }, ); - assert.match(child.stderr, /Can't parse package\.json/); + assert.match(child.stderr, /Can't parse/); assert.strictEqual(child.stdout, ''); assert.strictEqual(child.code, 1); }); @@ -131,7 +229,7 @@ describe('node --run [command]', () => { [ '--no-warnings', '--run', 'test'], { cwd: fixtures.path('run-script/cannot-find-script') }, ); - assert.match(child.stderr, /Can't find "scripts" field in package\.json/); + assert.match(child.stderr, /Can't find "scripts" field in/); assert.strictEqual(child.stdout, ''); assert.strictEqual(child.code, 1); });