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.

Sign up for GitHub

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

Jump to bottom

src: add --max-semi-space-size to the options allowed in NODE_OPTIONS #44436

Merged
merged 2 commits into from
Nov 15, 2022
Merged

src: add --max-semi-space-size to the options allowed in NODE_OPTIONS #44436

merged 2 commits into from
Nov 15, 2022

Conversation

ehoogeveen-medweb
Copy link
Contributor

--max-semi-space-size was added to the Useful V8 options in #42575, but if you pass it in the NODE_OPTIONS environment variable, you still get node: --max-semi-space-size= is not allowed in NODE_OPTIONS.

Since the option is explicitly documented as being useful and since NODE_OPTIONS appears to be the supported way of passing parameters to the node process from npm, I think it should be included in the options passed through by NODE_OPTIONS.

It appears that this issue was not contemplated in the original request, although there is a comment asking for it: #42511 (comment)

@nodejs-github-bot 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 Aug 29, 2022
@bnoordhuis
Copy link
Member

Can you add a test to test/parallel/test-cli-node-options.js? Look for --max-old-space-size.

@ehoogeveen-medweb
Copy link
Contributor Author

Test added. I guess this test doesn't actually test whether passing --max-semi-space-size in NODE_OPTIONS works, but it should guard against node returning bad option if the flag is renamed or removed from V8 in the future.

@ehoogeveen-medweb
Copy link
Contributor Author

Hmm, not sure what to make of the ASan failure; I can't get much from the log. Is it a known intermittent failure?

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 1, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 1, 2022
This was referenced Sep 2, 2022
Open
Open
Open
Open
Open
Open
mhdawson
mhdawson approved these changes Sep 23, 2022
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Sep 26, 2022
Open
18 tasks
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Oct 4, 2022
Open
Open
@ehoogeveen-medweb
Copy link
Contributor Author

Is there anything I need to (or can) do to help this get merged? I don't know what the usual timeline is for changes to get merged.

@ehoogeveen-medweb
Copy link
Contributor Author

I rebased for the change from kAllowedInEnvironment to kAllowedInEnvvar on main.

@theanarkh theanarkh added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 1, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 1, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@theanarkh theanarkh added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 1, 2022
@nodejs-github-bot 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 Nov 1, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/44436
✔  Done loading data for nodejs/node/pull/44436
----------------------------------- PR info ------------------------------------
Title      src: add --max-semi-space-size to the options allowed in NODE_OPTIONS (#44436)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     ehoogeveen-medweb:allow-v8-option-in-node-options -> nodejs:main
Labels     c++, needs-ci
Commits    2
 - src: add --max-semi-space-size to the options allowed in NODE_OPTIONS
 - test: add a test for --max-semi-space-size
Committers 1
 - Emanuel Hoogeveen 
PR-URL: https://github.com/nodejs/node/pull/44436
Reviewed-By: Luigi Pinca 
Reviewed-By: Ben Noordhuis 
Reviewed-By: Evan Lucas 
Reviewed-By: Michael Dawson 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/44436
Reviewed-By: Luigi Pinca 
Reviewed-By: Ben Noordhuis 
Reviewed-By: Evan Lucas 
Reviewed-By: Michael Dawson 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - src: add --max-semi-space-size to the options allowed in NODE_OPTIONS
   ⚠  - test: add a test for --max-semi-space-size
   ℹ  This PR was created on Mon, 29 Aug 2022 13:54:41 GMT
   ✔  Approvals: 5
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/44436#pullrequestreview-1092400509
   ✔  - Ben Noordhuis (@bnoordhuis): https://github.com/nodejs/node/pull/44436#pullrequestreview-1092635321
   ✔  - Evan Lucas (@evanlucas): https://github.com/nodejs/node/pull/44436#pullrequestreview-1109389839
   ✔  - Michael Dawson (@mhdawson) (TSC): https://github.com/nodejs/node/pull/44436#pullrequestreview-1119041801
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/44436#pullrequestreview-1127616249
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-11-01T13:37:56Z: https://ci.nodejs.org/job/node-test-pull-request/47616/
- Querying data for job/node-test-pull-request/47616/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/3370181684

@theanarkh theanarkh added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 1, 2022
@github-actions github-actions bot mentioned this pull request Nov 2, 2022
Open
15 tasks
@ehoogeveen-medweb
Copy link
Contributor Author

Hmm, I didn't realize that my email address was set to private. I've made it public.

@theanarkh
Copy link
Contributor

@lpinca Hi, can you help with this PR ? I am not sure which label we should add.

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Nov 15, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 15, 2022
@nodejs-github-bot nodejs-github-bot merged commit 0d8a782 into nodejs:main Nov 15, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 0d8a782

@ehoogeveen-medweb ehoogeveen-medweb deleted the allow-v8-option-in-node-options branch November 15, 2022 13:56
@ehoogeveen-medweb
Copy link
Contributor Author

Thanks for the reviews and approvals, and the assistance with getting this merged! 🎉

ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
@ehoogeveen-medweb @ruyadorno
src: add --max-semi-space-size to the options allowed in NODE_OPTIONS
459d448 
PR-URL: #44436
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Nov 24, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@ehoogeveen-medweb @danielleadams
src: add --max-semi-space-size to the options allowed in NODE_OPTIONS
889e67f 
PR-URL: #44436
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@ehoogeveen-medweb @danielleadams
src: add --max-semi-space-size to the options allowed in NODE_OPTIONS
08b1dfa 
PR-URL: #44436
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@ehoogeveen-medweb @danielleadams
src: add --max-semi-space-size to the options allowed in NODE_OPTIONS
70ca26a 
PR-URL: #44436
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
@ehoogeveen-medweb @danielleadams
src: add --max-semi-space-size to the options allowed in NODE_OPTIONS
62e81b3 
PR-URL: #44436
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 approved these changes

@mhdawson mhdawson mhdawson approved these changes

@lpinca lpinca lpinca approved these changes

@evanlucas evanlucas evanlucas approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
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++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@ehoogeveen-medweb @bnoordhuis @nodejs-github-bot @theanarkh @jasnell @evanlucas @lpinca @mhdawson @aduh95