src: add --max-semi-space-size to the options allowed in NODE_OPTIONS #44436
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nodejs-github-bot
added
c++
|
Can you add a test to test/parallel/test-cli-node-options.js? Look for |
|
Test added. I guess this test doesn't actually test whether passing |
|
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
approved these changes
Aug 31, 2022
github-actions
bot
removed
the
request-ci
ehoogeveen-medweb
force-pushed
the
allow-v8-option-in-node-options
branch
from
4a87691
to
b546a13
Compare
bnoordhuis
approved these changes
Sep 1, 2022
github-actions
bot
removed
the
request-ci
This was referenced Sep 2, 2022
evanlucas
approved these changes
Sep 15, 2022
github-actions
bot
removed
the
request-ci
18 tasks
jasnell
approved these changes
Oct 2, 2022
This was referenced Oct 4, 2022
|
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. |
|
I rebased for the change from |
github-actions
bot
removed
the
request-ci
theanarkh
added
the
commit-queue
nodejs-github-bot
added
commit-queue-failed
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/.ncuhttps://github.com/nodejs/node/actions/runs/3370181684 |
theanarkh
added
the
author ready
15 tasks
|
Hmm, I didn't realize that my email address was set to private. I've made it public. |
|
@lpinca Hi, can you help with this PR ? I am not sure which label we should add. |
aduh95
approved these changes
Nov 15, 2022
aduh95
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 0d8a782 |
|
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
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>
Merged
danielleadams
pushed a commit
that referenced
this pull request
Dec 30, 2022
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
Dec 30, 2022
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
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
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
--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_OPTIONSappears 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 byNODE_OPTIONS.It appears that this issue was not contemplated in the original request, although there is a comment asking for it: #42511 (comment)