src: traverse parent folders while running `--run` #53154

anonrig · 2024-05-26T00:20:07Z

This adds the most requested 2 features to node --run:

Traversing parent folders until package.json file is found.
Prepending node_modules/.bin for each subdirectory of the current folder to PATH variable (specifically for monorepos)

I've also updated the documentation. I think we are in good hands, and getting ready to make the runner stable.

With the current changes and the overhead of checking if it's a directory for each node_modules/.bin inside each ancestor folder, here's the updated benchmark results:

❯ hyperfine '../node/out/Release/node --run test' 'npm run test' -i -w 10
Benchmark 1: ../node/out/Release/node --run test
  Time (mean ± σ):      22.7 ms ±   1.2 ms    [User: 17.0 ms, System: 2.7 ms]
  Range (min … max):    20.9 ms …  25.8 ms    118 runs

Benchmark 2: npm run test
  Time (mean ± σ):     106.0 ms ±   4.0 ms    [User: 82.5 ms, System: 14.1 ms]
  Range (min … max):   101.3 ms … 116.1 ms    28 runs

Summary
  ../node/out/Release/node --run test ran
    4.67 ± 0.31 times faster than npm run test

cc @nodejs/cpp-reviewers

GeoffreyBooth · 2024-05-26T02:15:36Z

Appending node_modules/.bin for each subdirectory of the current folder to PATH variable (specifically for monorepos)

I thought this one was hard/next to impossible to do safely for Windows because of the short limit of the path variable there? There was a comment to this effect on an earlier thread.

anonrig · 2024-05-26T04:09:49Z

Appending node_modules/.bin for each subdirectory of the current folder to PATH variable (specifically for monorepos)

I thought this one was hard/next to impossible to do safely for Windows because of the short limit of the path variable there? There was a comment to this effect on an earlier thread.

I found the following solution online, might fix it:

Create a new system environment variable, say 'NEWPATH'
Assign the bin directory location to 'NEWPATH'
Now append '; %NEWPATH%' to the PATH environment variable

GeoffreyBooth · 2024-05-26T05:14:18Z

Since the path part is potentially problematic, should it be a separate PR? The traversing up the tree part is straightforward and could just land I'd think.

doc/api/cli.md

ryanccn · 2024-05-26T06:14:46Z

Also, checking if the node_modules/.bin directory for each ancestor actually exists before adding it to PATH could possibly alleviate the issue.

src/node_task_runner.cc

anonrig · 2024-05-26T11:39:17Z

Also, checking if the node_modules/.bin directory for each ancestor actually exists before adding it to PATH could possibly alleviate the issue.

This would reduce the performance by a significant margin, and isn't in par with npm (but that's not a supportive argument just a fact)

ryanccn · 2024-05-26T12:25:58Z

This would reduce the performance by a significant margin

It's a very minor cost compared to the other work that's being done, and skipping these checks would really just offload the work to the subprocesses being run (such as the shell), which would have to look through all of these nonexistent paths for binaries to execute, so it doesn't prevent the performance cost.

src/node_task_runner.cc

isaacs · 2024-05-27T00:43:53Z

Appending node_modules/.bin for each subdirectory of the current folder to PATH variable (specifically for monorepos)

I think you mean each parent directory, not each sub directory?

I haven't checked the implementation, but you can probably get away with only adding node_modules/.bin to the path for every parent directory you find containing a package.json file, and can also further check if {dir}/node_modules/.bin is a directory. It adds some stats, but it'll help keep the PATH env within limits, and if you don't do those stats, then the OS will end up doing them anyway when it goes to look up executables. (This is close to what npm does.)

Also, the node_modules/.bin should be pre-pended to the PATH env, not appended. Eg, if I have eslint installed globally, and a different version in the current project, I want the one in the closest containing project that has it, not the one at /usr/local/bin/eslint.

anonrig · 2024-05-27T15:43:47Z

@isaacs Thanks for the comment!

I think you mean each parent directory, not each sub directory?

Yes, I've updated the CLI to use ancestor keyword.

I haven't checked the implementation, but you can probably get away with only adding node_modules/.bin to the path for every parent directory you find containing a package.json file, and can also further check if {dir}/node_modules/.bin is a directory. It adds some stats, but it'll help keep the PATH env within limits, and if you don't do those stats, then the OS will end up doing them anyway when it goes to look up executables. (This is close to what npm does.)

Why do you think checking package_json is good? I'm currently checking if node_modules/.bin is a directory.

Also, the node_modules/.bin should be pre-pended to the PATH env, not appended. Eg, if I have eslint installed globally, and a different version in the current project, I want the one in the closest containing project that has it, not the one at /usr/local/bin/eslint.

Yes, I'm pre-pending although I'm not sure how did you come to this conclusion? If it's my choice of words in the docs, can you point me to the correct place to fix it?

nodejs-github-bot · 2024-05-30T22:47:04Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59555/

nodejs-github-bot · 2024-05-31T01:34:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59556/

nodejs-github-bot · 2024-05-31T02:25:40Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59557/

nodejs-github-bot · 2024-05-31T11:06:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59561/

nodejs-github-bot · 2024-05-31T12:29:38Z

Commit Queue failed

- Loading data for nodejs/node/pull/53154
✔  Done loading data for nodejs/node/pull/53154
----------------------------------- PR info ------------------------------------
Title      src: traverse parent folders while running `--run` (#53154)
Author     Yagiz Nizipli  (@anonrig)
Branch     anonrig:traverse-until-root -> nodejs:main
Labels     c++, semver-minor, needs-ci
Commits    1
 - src: traverse parent folders while running `--run`
Committers 1
 - Yagiz Nizipli 
PR-URL: https://github.com/nodejs/node/pull/53154
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Daniel Lemire 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/53154
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Daniel Lemire 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 26 May 2024 00:20:07 GMT
   ✔  Approvals: 2
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/53154#pullrequestreview-2086340450
   ✔  - Daniel Lemire (@lemire): https://github.com/nodejs/node/pull/53154#pullrequestreview-2088970099
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2024-05-31T11:06:06Z: https://ci.nodejs.org/job/node-test-pull-request/59561/
- Querying data for job/node-test-pull-request/59561/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/9318146805

nodejs-github-bot · 2024-05-31T15:55:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59571/

nodejs-github-bot · 2024-06-01T00:39:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59589/

nodejs-github-bot · 2024-06-07T18:16:20Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59686/

nodejs-github-bot · 2024-06-07T19:42:22Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59688/

nodejs-github-bot · 2024-06-07T21:20:54Z

Landed in 5731e2b

PR-URL: #53154 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) #53221 cli: * (SEMVER-MINOR) add `NODE_RUN_PACKAGE_JSON_PATH` env (Yagiz Nizipli) #53058 * (SEMVER-MINOR) add `NODE_RUN_SCRIPT_NAME` env to `node --run` (Yagiz Nizipli) #53032 doc: * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) #53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) #52762 * add StefanStojanovic to collaborators (StefanStojanovic) #53118 * add Marco Ippolito to TSC (Rafael Gonzaga) #53008 fs: * mark recursive cp methods as stable (Théo LUDWIG) #53127 lib: * (SEMVER-MINOR) add EventSource Client (Aras Abbasi) #51575 * (SEMVER-MINOR) replace MessageEvent with undici's (Matthew Aitken) #52370 module: * (SEMVER-MINOR) print amount of load time of a cjs module (Vinicius Lourenço) #52213 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) #53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) #52762 src: * (SEMVER-MINOR) traverse parent folders while running `--run` (Yagiz Nizipli) #53154 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) #53124 test_runner: * (SEMVER-MINOR) add snapshot testing (Colin Ihrig) #53169 * (SEMVER-MINOR) add context.fullName (Colin Ihrig) #53169 * (SEMVER-MINOR) support module mocking (Colin Ihrig) #52848 PR-URL: #53379

PR-URL: nodejs#53154 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) nodejs#53221 cli: * (SEMVER-MINOR) add `NODE_RUN_PACKAGE_JSON_PATH` env (Yagiz Nizipli) nodejs#53058 * (SEMVER-MINOR) add `NODE_RUN_SCRIPT_NAME` env to `node --run` (Yagiz Nizipli) nodejs#53032 doc: * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) nodejs#53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) nodejs#52762 * add StefanStojanovic to collaborators (StefanStojanovic) nodejs#53118 * add Marco Ippolito to TSC (Rafael Gonzaga) nodejs#53008 fs: * mark recursive cp methods as stable (Théo LUDWIG) nodejs#53127 lib: * (SEMVER-MINOR) add EventSource Client (Aras Abbasi) nodejs#51575 * (SEMVER-MINOR) replace MessageEvent with undici's (Matthew Aitken) nodejs#52370 module: * (SEMVER-MINOR) print amount of load time of a cjs module (Vinicius Lourenço) nodejs#52213 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) nodejs#53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) nodejs#52762 src: * (SEMVER-MINOR) traverse parent folders while running `--run` (Yagiz Nizipli) nodejs#53154 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) nodejs#53124 test_runner: * (SEMVER-MINOR) add snapshot testing (Colin Ihrig) nodejs#53169 * (SEMVER-MINOR) add context.fullName (Colin Ihrig) nodejs#53169 * (SEMVER-MINOR) support module mocking (Colin Ihrig) nodejs#52848 PR-URL: nodejs#53379

PR-URL: nodejs#53154 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: buffer: * (SEMVER-MINOR) add .bytes() method to Blob (Matthew Aitken) nodejs#53221 cli: * (SEMVER-MINOR) add `NODE_RUN_PACKAGE_JSON_PATH` env (Yagiz Nizipli) nodejs#53058 * (SEMVER-MINOR) add `NODE_RUN_SCRIPT_NAME` env to `node --run` (Yagiz Nizipli) nodejs#53032 doc: * (SEMVER-MINOR) add context.assert docs (Colin Ihrig) nodejs#53169 * (SEMVER-MINOR) improve explanation about built-in modules (Joyee Cheung) nodejs#52762 * add StefanStojanovic to collaborators (StefanStojanovic) nodejs#53118 * add Marco Ippolito to TSC (Rafael Gonzaga) nodejs#53008 fs: * mark recursive cp methods as stable (Théo LUDWIG) nodejs#53127 lib: * (SEMVER-MINOR) add EventSource Client (Aras Abbasi) nodejs#51575 * (SEMVER-MINOR) replace MessageEvent with undici's (Matthew Aitken) nodejs#52370 module: * (SEMVER-MINOR) print amount of load time of a cjs module (Vinicius Lourenço) nodejs#52213 net: * (SEMVER-MINOR) add new net.server.listen tracing channel (Paolo Insogna) nodejs#53136 process: * (SEMVER-MINOR) add process.getBuiltinModule(id) (Joyee Cheung) nodejs#52762 src: * (SEMVER-MINOR) traverse parent folders while running `--run` (Yagiz Nizipli) nodejs#53154 src,permission: * (SEMVER-MINOR) --allow-wasi & prevent WASI exec (Rafael Gonzaga) nodejs#53124 test_runner: * (SEMVER-MINOR) add snapshot testing (Colin Ihrig) nodejs#53169 * (SEMVER-MINOR) add context.fullName (Colin Ihrig) nodejs#53169 * (SEMVER-MINOR) support module mocking (Colin Ihrig) nodejs#52848 PR-URL: nodejs#53379

anonrig requested review from mcollina, lemire, jasnell, GeoffreyBooth and benjamingr May 26, 2024 00:20

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels May 26, 2024

anonrig force-pushed the traverse-until-root branch 2 times, most recently from 6a4d9b0 to 595b3f5 Compare May 26, 2024 00:38

anonrig requested review from tniessen and joyeecheung May 26, 2024 00:40

anonrig added the semver-minor PRs that contain new features and should be released in the next minor version. label May 26, 2024

anonrig force-pushed the traverse-until-root branch from 595b3f5 to 94a5694 Compare May 26, 2024 00:43

ryanccn reviewed May 26, 2024

View reviewed changes

doc/api/cli.md Outdated Show resolved Hide resolved

mcollina reviewed May 26, 2024

View reviewed changes

src/node_task_runner.cc Outdated Show resolved Hide resolved

RafaelGSS reviewed May 26, 2024

View reviewed changes

src/node_task_runner.cc Show resolved Hide resolved

anonrig force-pushed the traverse-until-root branch from 94a5694 to 09844aa Compare May 27, 2024 15:40

anonrig force-pushed the traverse-until-root branch 3 times, most recently from 0a32d40 to 8213c4f Compare May 27, 2024 19:22

anonrig requested review from RafaelGSS and mcollina May 27, 2024 19:29

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label May 31, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 31, 2024

jasnell approved these changes Jun 1, 2024

View reviewed changes

anonrig added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jun 7, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 7, 2024

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 7, 2024

anonrig mentioned this pull request Jun 7, 2024

v22.3.0 proposal #53379

Merged

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 7, 2024

nodejs-github-bot merged commit 5731e2b into nodejs:main Jun 7, 2024
65 checks passed

RafaelGSS pushed a commit that referenced this pull request Jun 10, 2024

src: traverse parent folders while running --run

5a41bcf

PR-URL: #53154 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: James M Snell <jasnell@gmail.com>

marco-ippolito added the backport-blocked-v20.x PRs that should land on the v20.x-staging branch but are blocked by another PR's pending backport. label Jul 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: traverse parent folders while running `--run` #53154

src: traverse parent folders while running `--run` #53154

anonrig commented May 26, 2024 •

edited

Loading

GeoffreyBooth commented May 26, 2024

anonrig commented May 26, 2024

GeoffreyBooth commented May 26, 2024

ryanccn commented May 26, 2024

anonrig commented May 26, 2024

ryanccn commented May 26, 2024

isaacs commented May 27, 2024 •

edited

Loading

anonrig commented May 27, 2024

nodejs-github-bot commented May 30, 2024

nodejs-github-bot commented May 31, 2024

nodejs-github-bot commented May 31, 2024

nodejs-github-bot commented May 31, 2024

nodejs-github-bot commented May 31, 2024

nodejs-github-bot commented May 31, 2024

nodejs-github-bot commented Jun 1, 2024

nodejs-github-bot commented Jun 7, 2024

nodejs-github-bot commented Jun 7, 2024

nodejs-github-bot commented Jun 7, 2024

src: traverse parent folders while running --run #53154

src: traverse parent folders while running --run #53154

Conversation

anonrig commented May 26, 2024 • edited Loading

GeoffreyBooth commented May 26, 2024

anonrig commented May 26, 2024

GeoffreyBooth commented May 26, 2024

ryanccn commented May 26, 2024

anonrig commented May 26, 2024

ryanccn commented May 26, 2024

isaacs commented May 27, 2024 • edited Loading

anonrig commented May 27, 2024

nodejs-github-bot commented May 30, 2024

nodejs-github-bot commented May 31, 2024

nodejs-github-bot commented May 31, 2024

nodejs-github-bot commented May 31, 2024

nodejs-github-bot commented May 31, 2024

nodejs-github-bot commented May 31, 2024

nodejs-github-bot commented Jun 1, 2024

nodejs-github-bot commented Jun 7, 2024

nodejs-github-bot commented Jun 7, 2024

nodejs-github-bot commented Jun 7, 2024

src: traverse parent folders while running `--run` #53154

src: traverse parent folders while running `--run` #53154

anonrig commented May 26, 2024 •

edited

Loading

isaacs commented May 27, 2024 •

edited

Loading