Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: ensure --run has proper pwd #53600

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -2022,6 +2022,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:

Expand Down
75 changes: 50 additions & 25 deletions src/node_task_runner.cc
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,9 @@ void ProcessRunner::OnExit(int64_t exit_status, int term_signal) {
}

void ProcessRunner::Run() {
// keeps the string alive until destructor
auto cwd = package_json_path_.parent_path();
options_.cwd = cwd.c_str();
if (int r = uv_spawn(loop_, &process_, &options_)) {
fprintf(stderr, "Error: %s\n", uv_strerror(r));
}
Expand Down Expand Up @@ -253,7 +256,7 @@ void RunTask(std::shared_ptr<InitializationResultImpl> result,
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.c_str());
result->exit_code_ = ExitCode::kGenericUserError;
return;
}
Expand All @@ -269,44 +272,66 @@ void RunTask(std::shared_ptr<InitializationResultImpl> result,
simdjson::ondemand::object main_object;
simdjson::error_code error = json_parser.iterate(raw_json).get(document);

if (error) {
fprintf(stderr, "Can't parse %s\n", path.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");
auto root_error = document.get_object().get(main_object);
if (root_error) {
if (root_error == simdjson::error_code::INCORRECT_TYPE) {
fprintf(stderr,
"Root value unexpected not an object for %s\n\n",
path.c_str());
} else {
fprintf(stderr, "Can't parse %s\n", path.c_str());
}
result->exit_code_ = ExitCode::kGenericUserError;
return;
}

// 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.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<int>(command_id.size()),
command_id.data());
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<int>(key_str.size()),
key_str.data(),
static_cast<int>(value_str.size()),
value_str.data());
auto command_error = scripts_object[command_id].get_string().get(command);
if (command_error) {
if (command_error == simdjson::error_code::INCORRECT_TYPE) {
fprintf(stderr,
"Script \"%.*s\" is unexpectedly not a string for %s\n\n",
static_cast<int>(command_id.size()),
command_id.data(),
path.c_str());
} else {
fprintf(stderr,
"Missing script: \"%.*s\" for %s\n\n",
static_cast<int>(command_id.size()),
command_id.data(),
path.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<int>(key_str.size()),
key_str.data(),
static_cast<int>(value_str.size()),
value_str.data());
}
}
}
result->exit_code_ = ExitCode::kGenericUserError;
Expand Down
3 changes: 3 additions & 0 deletions test/fixtures/run-script/invalid-json/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"scripts": {},
}
9 changes: 9 additions & 0 deletions test/fixtures/run-script/invalid-schema/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"scripts": {
"array": [],
"boolean": true,
"null": null,
"number": 1.0,
"object": {}
}
}
1 change: 1 addition & 0 deletions test/fixtures/run-script/missing-scripts/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
4 changes: 3 additions & 1 deletion test/fixtures/run-script/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
}
4 changes: 3 additions & 1 deletion test/message/node_run_non_existent.out
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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
102 changes: 100 additions & 2 deletions test/parallel/test-node-run.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
'use strict';

const common = require('../common');
common.requireNoPackageJSONAbove();

const { it, describe } = require('node:test');
const assert = require('node:assert');

Expand All @@ -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);
});
Expand All @@ -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));
bmeck marked this conversation as resolved.
Show resolved Hide resolved
assert.strictEqual(child.stdout, '');
assert.strictEqual(child.code, 1);
});
Expand All @@ -53,6 +56,101 @@ describe('node --run [command]', () => {
assert.strictEqual(child.code, 0);
});

it.only('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'],
bmeck marked this conversation as resolved.
Show resolved Hide resolved
{ 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,
Expand Down
Loading