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-dir runtime flag #57523

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

mertcanaltin
Copy link
Member

@mertcanaltin mertcanaltin commented Mar 17, 2025

add --run-dir runtime flag

Fixes #57489

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/config

@nodejs-github-bot 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 mertcanaltin requested a review from jasnell March 17, 2025 19:50
@aduh95
Copy link
Contributor

aduh95 commented Mar 17, 2025

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
Copy link
Member

anonrig commented Mar 17, 2025

cc @flakey5

@anonrig
Copy link
Member

anonrig commented Mar 17, 2025

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

@anonrig 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
Copy link
Contributor

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.

@mertcanaltin mertcanaltin changed the title src: added node --run-in src: add --run-in runtime flag Mar 17, 2025
@mertcanaltin mertcanaltin changed the title src: add --run-in runtime flag src: add --run-directory runtime flag Mar 17, 2025
@mertcanaltin mertcanaltin changed the title src: add --run-directory runtime flag src: add --run-in-directory runtime flag Mar 17, 2025
mertcanaltin and others added 2 commits March 17, 2025 23:49
Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>
Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

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

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

codecov bot commented Mar 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.24%. Comparing base (38390e5) to head (1491a81).
Report is 16 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #57523      +/-   ##
==========================================
+ Coverage   90.22%   90.24%   +0.01%     
==========================================
  Files         629      629              
  Lines      184845   184955     +110     
  Branches    36205    36234      +29     
==========================================
+ Hits       166784   166915     +131     
+ Misses      11016    11014       -2     
+ Partials     7045     7026      -19     
Files with missing lines Coverage Δ
src/node_options.cc 85.36% <100.00%> (+0.14%) ⬆️
src/node_options.h 98.33% <ø> (ø)
src/node_task_runner.cc 89.78% <100.00%> (+0.39%) ⬆️

... and 47 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mertcanaltin and others added 2 commits March 18, 2025 11:41
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95
Copy link
Contributor

aduh95 commented Mar 18, 2025

Can you add a test that uses --run-in without --run to validate the behavior?

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@@ -1216,6 +1216,9 @@ PerProcessOptionsParser::PerProcessOptionsParser(
AddOption("--run",
"Run a script specified in package.json",
&PerProcessOptions::run);
AddOption("--run-dir",
Copy link
Member

Choose a reason for hiding this comment

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

I've seen that this has already been raised. While I don't have a better name in mind, I think the current name is misleading

Copy link
Member Author

@mertcanaltin mertcanaltin Mar 18, 2025

Choose a reason for hiding this comment

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

I agree with @GeoffreyBooth suggestion

--run-path +1

Copy link
Member Author

@mertcanaltin mertcanaltin Mar 18, 2025

Choose a reason for hiding this comment

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

I will wait for further advice for the collective decision; please feel free to give suggestion 🚀

Copy link
Member

Choose a reason for hiding this comment

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

@mertcanaltin , IMHO the most common term used in such context is working_directory or working_dir.
E.g. "cwd" stands for "current working directory", "pwd" - "print working directory", etc.

Another example is the WORKING_DIRECTORY attribute used by CMake.

The new code would read: cwd = node::per_process::cli_options->working_dir;
I believe it is more natural than cwd = node::per_process::cli_options->run_dir;

Copy link
Member

Choose a reason for hiding this comment

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

The --cwd is also a great name proposed above by @targos

Copy link
Member

Choose a reason for hiding this comment

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

Please disregard my previous posts. I read through the discussion in the linked issue and see why the "working directory" term is not used. I am actually puzzled why the new flag is needed at all. Users can always provide the full path for the --run parameter.

Copy link
Member Author

Choose a reason for hiding this comment

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

@vmoroz, thank you very much for your suggestions. I think adding this flag gives users an easy and clear way to specify the correct directory and ensures that commands work in the intended context. This extra flexibility can provide a good experience in scenarios where precise control over the working directory is needed, such as when running new API documentation tools.

@mertcanaltin mertcanaltin added the review wanted PRs that need reviews. label Mar 19, 2025
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@anonrig anonrig added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Mar 19, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 19, 2025
@nodejs-github-bot
Copy link
Collaborator

@GeoffreyBooth
Copy link
Member

I'm not opposed to this feature, but there are multiple requests for renaming, from different people: #57523 (comment), #57523 (comment), #57523 (comment), #57523 (comment). There also seems to be some confusion/lack of consensus about whether this should only affect --run or the entire process.

I'd like to see this land in some form, so I don't want to block this, but I think the above should be resolved before someone lands this.

@anonrig
Copy link
Member

anonrig commented Mar 20, 2025

I'd like to see this land in some form, so I don't want to block this, but I think the above should be resolved before someone lands this.

We can land this as experimental, and later iterate on this. New documentation infra changes require this to be implemented, so I recommend merging and iterating.

@GeoffreyBooth
Copy link
Member

I recommend merging and iterating.

This flag currently lacks experimental in the name. It will be disruptive to users to land one name and then rename it. It won't take long to reach a consensus here; I think it's worth taking the time to do so to avoid the user disruption.

@anonrig anonrig added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Mar 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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. notable-change PRs with changes that should be highlighted in changelogs. review wanted PRs that need reviews. semver-minor PRs that contain new features and should be released in the next minor version. tsc-agenda Issues and PRs to discuss during the meetings of the TSC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specify working directory for node --run