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

src: add --run-from runtime flag #57523

Open
wants to merge 10 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
20 changes: 20 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -2294,6 +2294,26 @@ The following environment variables are set when running a script with `--run`:
* `NODE_RUN_PACKAGE_JSON_PATH`: The path to the `package.json` that is being
processed.

### `--run-from=<path>`

<!-- YAML
added: REPLACEME
-->

> Stability: 1.0 - Early development

Run a `package.json` script from a specified path to a `package.json` file
or path to the containing folder of a `package.json` file.

The script is run from the current working directory, not from the
`package.json` containing folder.

```bash
node --run-from=/app/package.json --run test
# Or
node --run-from=/app/ --run test
```

### `--secure-heap-min=n`

<!-- YAML
Expand Down
3 changes: 3 additions & 0 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1216,6 +1216,9 @@ PerProcessOptionsParser::PerProcessOptionsParser(
AddOption("--run",
"Run a script specified in package.json",
&PerProcessOptions::run);
AddOption("--run-from",
"Run a package.json script in a specific working directory",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this change the working directory of the Node process, or is it only about where to find the package.json file?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sets the Node process's working directory

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm... the naming here was discussed in the linked issue. The name run-dir was suggested in the case that the option only set the cwd for the run command. If we're going to have this generally set the cwd for the process then it should definitely be named something else

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said, the documentation update would imply that it only works with the --run command.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it has a more general effect of setting the initial cwd for the process, and assuming we are ok with such a flag, I think it should be named --cwd

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the use case for setting the initial cwd for the process? Aside from telling Node where to find package.json, which could be done with a more isolated flag.

&PerProcessOptions::run_from);
AddOption(
"--disable-wasm-trap-handler",
"Disable trap-handler-based WebAssembly bound checks. V8 will insert "
Expand Down
1 change: 1 addition & 0 deletions src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,7 @@ class PerProcessOptions : public Options {
bool print_version = false;
std::string experimental_sea_config;
std::string run;
std::string run_from;

#ifdef NODE_HAVE_I18N_SUPPORT
std::string icu_data_dir;
Expand Down
25 changes: 24 additions & 1 deletion src/node_task_runner.cc
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,30 @@ 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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not work since this doesn't add the current node_modules to path environment variable.

if (!node::per_process::cli_options->run_from.empty()) {
cwd = node::per_process::cli_options->run_from;
if (!std::filesystem::is_directory(cwd)) {
// If the given path is a file and the file name is package.json, the
// parent directory is used
std::filesystem::path p(cwd);
if (std::filesystem::is_regular_file(p) &&
p.filename() == "package.json") {
cwd = p.parent_path().string();
} else {
fprintf(stderr, "Error: %s is not a directory\n", cwd.c_str());
init_result->exit_code_ = ExitCode::kGenericUserError;
return;
}
}
// Adding node_modules/.bin under cwd to PATH environment variable
std::filesystem::path nmBin =
std::filesystem::path(cwd) / "node_modules" / ".bin";
if (std::filesystem::is_directory(nmBin)) {
path_env_var_ = nmBin.string() + env_var_separator + path_env_var_;
}
Comment on lines +224 to +229
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anonrig I tried to process the path given with --run-from and add the node_modules/.bin directory under the cwd to the PATH Without changing the cwd of the parent process, only the cwd of the spawned process will be set, is this a good solution ?

} else {
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));
Expand Down
42 changes: 42 additions & 0 deletions test/parallel/test-node-run.js
Original file line number Diff line number Diff line change
Expand Up @@ -222,4 +222,46 @@ describe('node --run [command]', () => {
assert.strictEqual(child.stdout, '');
assert.strictEqual(child.code, 1);
});

it('runs script in a custom working directory using --run-from', async () => {
const customDir = fixtures.path('run-script');
const child = await common.spawnPromisified(
process.execPath,
[ '--run-from', customDir, '--run', `pwd${envSuffix}` ],

{ cwd: fixtures.path('run-script/sub-directory') }
);

assert.strictEqual(child.stdout.trim(), customDir);
assert.strictEqual(child.stderr, '');
assert.strictEqual(child.code, 0);
});

it('runs script in a custom working directory when --run-from is provided as a package.json file path', async () => {
const pkgFilePath = fixtures.path('run-script/package.json');
const expectedDir = fixtures.path('run-script');
const child = await common.spawnPromisified(
process.execPath,
[ '--run-from', pkgFilePath, '--run', 'pwd' ],
{ cwd: fixtures.path('run-script/sub-directory') }
);

assert.strictEqual(child.stdout.trim(), expectedDir);
assert.strictEqual(child.stderr, '');
assert.strictEqual(child.code, 0);
});

it('--run-from should be no-op when used without --run', async () => {
const dirPath = fixtures.path('run-script/package.json');

const child = await common.spawnPromisified(
process.execPath,
[ '--run-from', dirPath, '--print', 'process.cwd()' ],
{ cwd: process.cwd() }
);

assert.strictEqual(child.stderr, '');
assert.strictEqual(child.stdout, process.cwd() + '\n');
assert.strictEqual(child.code, 0);
});
});
Loading