src: add `--run-from` runtime flag by mertcanaltin · Pull Request #57523 · nodejs/node

mertcanaltin · 2025-03-17T19:50:01Z

add --run-from runtime flag

nodejs-github-bot · 2025-03-17T19:50:06Z

Review requested:

@nodejs/config

aduh95 · 2025-03-17T20:25:24Z

The commit message doesn't adhere to our guidelines, it should start with an imperative verb so e.g. src: add `--run-in` runtime flag

anonrig · 2025-03-17T20:30:41Z

cc @flakey5

anonrig · 2025-03-17T20:31:51Z

We need to also change the pull-request title and description to reflect the changes.

github-actions · 2025-03-17T20:32:10Z

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @anonrig.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

anonrig

LGTM if tests pass (and the new requested test is added)

GeoffreyBooth · 2025-03-17T21:47:10Z

so e.g. src: add `--run-in` runtime flag

I find the name --run-dir confusing; you're not running a directory, you're specifying the folder to look in to find the package.json file. Why not just ask for the path to the package.json file directly? Or allow either a path to the file or the path to the containing folder. Something like:

node --run-path=./app/package.json --run test:backend
# Or
node --run-path=./app --run test:backend

mertcanaltin · 2025-03-17T22:01:11Z

so e.g. src: add `--run-in` runtime flag

I find the name --run-dir confusing; you're not running a directory, you're specifying the folder to look in to find the package.json file. Why not just ask for the path to the package.json file directly? Or allow either a path to the file or the path to the containing folder. Something like:
node --run-path=./app/package.json --run test:backend
# Or
node --run-path=./app --run test:backend

I understand your concern, I will wait for a few more ideas to make a collective decision, thank you very much for your suggestion 🙏

GeoffreyBooth · 2025-03-21T20:07:18Z

the ask is for a way of setting the working directory of the --run command, not the process as a whole.

What does it mean to apply only to --run? Does that just tell it where to find package.json or are there other effects?

mertcanaltin · 2025-03-21T20:20:32Z

the ask is for a way of setting the working directory of the --run command, not the process as a whole.

What does it mean to apply only to --run? Does that just tell it where to find package.json or are there other effects?

only determines the directory where the --run command finds the package.json file; it does not change the process-wide working directory.

GeoffreyBooth · 2025-03-21T21:16:00Z

only determines the directory where the --run command finds the package.json file; it does not change the process-wide working directory.

Okay, can it accept as input either the path to the containing folder or the path directly to the package.json file?

(As in, if this isn't the current behavior is it possible to update the PR so that this is possible.)

targos · 2025-03-21T21:20:15Z

I'm not sure this feature will work as expected if it doesn't set the process's cwd. Most scripts I write wouldn't work FWIW

mertcanaltin · 2025-03-21T21:27:25Z

only determines the directory where the --run command finds the package.json file; it does not change the process-wide working directory.

Okay, can it accept as input either the path to the containing folder or the path directly to the package.json file?

(As in, if this isn't the current behavior is it possible to update the PR so that this is possible.)

yes, I'm trying now

mertcanaltin · 2025-03-21T21:46:27Z

hello i sent an improvement @GeoffreyBooth @ovflowd @targos

I tried to implement that if given a package.json directly, this directory is now selected as the parent directory

Co-authored-by: Geoffrey Booth <webadmin@geoffreybooth.com>

GeoffreyBooth · 2025-03-21T23:07:52Z

I'm not sure this feature will work as expected if it doesn't set the process's cwd. Most scripts I write wouldn't work FWIW

I'm wondering how this can work in practice too. Does it solve #57489? Are the scripts in that package.json runnable from a different CWD?

anonrig · 2025-03-21T23:43:44Z


 void ProcessRunner::Run() {
  // keeps the string alive until destructor
-  cwd = package_json_path_.parent_path().string();


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

anonrig · 2025-03-21T23:49:54Z

only determines the directory where the --run command finds the package.json file; it does not change the process-wide working directory.

@mertcanaltin this is wrong. you're also changing the cwd of the process that we are spawning.

To answer other reviewers: I think this solution has lots of flaws. It doesn't properly propagate path variable (according to the given --run-from), and makes lots of unnecessary system calls.

mertcanaltin · 2025-03-22T00:36:56Z

only determines the directory where the --run command finds the package.json file; it does not change the process-wide working directory.

@mertcanaltin this is wrong. you're also changing the cwd of the process that we are spawning.

To answer other reviewers: I think this solution has lots of flaws. It doesn't properly propagate path variable (according to the given --run-from), and makes lots of unnecessary system calls.

now only the cwd of the spawned process (options_.cwd) is set, without changing the process-wide cwd. Also, if the file path is given for --run-from, the parent directory is used. I hope this addresses your concerns.

mertcanaltin · 2025-03-22T00:49:21Z

+    // 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_;
+    }


@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 ?

mertcanaltin · 2025-03-23T17:34:50Z

Thank you very much for your valuable contributions and feedback on this PR. I just found out that @flakey5 is already working on it, so I was unknowingly working on it. Therefore, I will close this PR 🚀

mertcanaltin requested review from anonrig and richardlau March 17, 2025 19:50

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 17, 2025

mertcanaltin requested a review from jasnell March 17, 2025 19:50

anonrig reviewed Mar 17, 2025

View reviewed changes

Comment thread src/node_task_runner.cc Outdated

Comment thread src/node_task_runner.cc Outdated

Comment thread doc/api/cli.md Outdated

Comment thread doc/api/cli.md Outdated

anonrig added semver-minor PRs that contain new features and should be released in the next minor version. notable-change PRs with changes that should be highlighted in changelogs. labels Mar 17, 2025

mertcanaltin force-pushed the mert/added-run-in branch from 2b7efd1 to 75b88f0 Compare March 17, 2025 20:35

mertcanaltin changed the title ~~src: added node --run-in~~ src: add --run-in runtime flag Mar 17, 2025

mertcanaltin changed the title ~~src: add --run-in runtime flag~~ src: add --run-directory runtime flag Mar 17, 2025

mertcanaltin changed the title ~~src: add --run-directory runtime flag~~ src: add --run-in-directory runtime flag Mar 17, 2025

anonrig reviewed Mar 17, 2025

View reviewed changes

Comment thread test/parallel/test-node-run.js

anonrig reviewed Mar 17, 2025

View reviewed changes

Comment thread src/node_task_runner.cc Outdated

anonrig reviewed Mar 17, 2025

View reviewed changes

Comment thread src/node_options.cc Outdated

anonrig reviewed Mar 17, 2025

View reviewed changes

Comment thread src/node_options.cc Outdated

mertcanaltin force-pushed the mert/added-run-in branch from ddc39f0 to 8d8a3e5 Compare March 17, 2025 20:48

anonrig reviewed Mar 17, 2025

View reviewed changes

Comment thread test/parallel/test-node-run.js Outdated

anonrig approved these changes Mar 17, 2025

View reviewed changes

mertcanaltin changed the title ~~src: add --run-in-directory runtime flag~~ src: add --run-dir runtime flag Mar 17, 2025

anonrig reviewed Mar 17, 2025

View reviewed changes

Comment thread test/parallel/test-node-run.js Outdated

anonrig reviewed Mar 17, 2025

View reviewed changes

Comment thread test/parallel/test-node-run.js Outdated

mertcanaltin force-pushed the mert/added-run-in branch from f5f5cc7 to bb2eaad Compare March 21, 2025 20:11

src: add --run-from runtime flag

4416cc0

mertcanaltin force-pushed the mert/added-run-in branch from affcb49 to 4416cc0 Compare March 21, 2025 20:13

anonrig approved these changes Mar 21, 2025

View reviewed changes

update --run-from tests for package.json file path handling

b93de16

mertcanaltin added 2 commits March 22, 2025 00:48

lint

d08981b

optimized if condition

f374130

GeoffreyBooth reviewed Mar 21, 2025

View reviewed changes

Comment thread doc/api/cli.md Outdated

Update cli.md

94717dd

Co-authored-by: Geoffrey Booth <webadmin@geoffreybooth.com>

GeoffreyBooth reviewed Mar 21, 2025

View reviewed changes

Comment thread doc/api/cli.md

GeoffreyBooth reviewed Mar 21, 2025

View reviewed changes

Comment thread doc/api/cli.md

Update cli.md

6e5d7d5

Co-authored-by: Geoffrey Booth <webadmin@geoffreybooth.com>

added backtick

7b23a7d

anonrig requested changes Mar 21, 2025

View reviewed changes

refactored

6194d3d

mertcanaltin added 2 commits March 22, 2025 03:40

lint md

fc6a438

refactored

d4aa938

mertcanaltin commented Mar 22, 2025

View reviewed changes

flakey5 mentioned this pull request Mar 22, 2025

src: add --run-from flag #57592

Open

mertcanaltin closed this Mar 23, 2025

Uh oh!

Conversation

mertcanaltin commented Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Mar 17, 2025

Uh oh!

aduh95 commented Mar 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anonrig commented Mar 17, 2025

Uh oh!

anonrig commented Mar 17, 2025

Uh oh!

github-actions Bot commented Mar 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anonrig left a comment

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth commented Mar 17, 2025

Uh oh!

Uh oh!

Uh oh!

mertcanaltin commented Mar 17, 2025

Uh oh!

GeoffreyBooth commented Mar 21, 2025

Uh oh!

mertcanaltin commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GeoffreyBooth commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

targos commented Mar 21, 2025

Uh oh!

mertcanaltin commented Mar 21, 2025

Uh oh!

mertcanaltin commented Mar 21, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

GeoffreyBooth commented Mar 21, 2025

Uh oh!

anonrig Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

anonrig commented Mar 21, 2025

Uh oh!

mertcanaltin commented Mar 22, 2025

Uh oh!

mertcanaltin Mar 22, 2025

Choose a reason for hiding this comment

Uh oh!

mertcanaltin commented Mar 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

mertcanaltin commented Mar 17, 2025 •

edited

Loading

mertcanaltin commented Mar 21, 2025 •

edited

Loading

GeoffreyBooth commented Mar 21, 2025 •

edited

Loading

mertcanaltin commented Mar 23, 2025 •

edited

Loading