-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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: fix NODE_OPTIONS parsing bug #22529
Conversation
I, uhm, might have messed up by using a `substr(start, end)` signature when `std::string` actually uses `substr(start, len)`. Fix that. Fixes: nodejs#22526 Refs: nodejs#22392
CI: https://ci.nodejs.org/job/node-test-pull-request/16754/ I have labeled this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the quick fix.
@@ -29,6 +31,9 @@ expect('--v8-pool-size=10', 'B\n'); | |||
expect('--trace-event-categories node', 'B\n'); | |||
// eslint-disable-next-line no-template-curly-in-string | |||
expect('--trace-event-file-pattern {pid}-${rotation}.trace_events', 'B\n'); | |||
// eslint-disable-next-line no-template-curly-in-string | |||
expect('--trace-event-file-pattern {pid}-${rotation}.trace_events ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible we could avoid hard-coding all of these options somehow with 22490 or would that require too much node options metadata to know what (if any) arguments to pass for each?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would work for a number of options (mostly the simple boolean ones), but there are quite a few that change output in some way…
Simple fixes for regressions, should be fast tracked. |
Landed in 1c05b16 |
I, uhm, might have messed up by using a `substr(start, end)` signature when `std::string` actually uses `substr(start, len)`. Fix that. Fixes: #22526 Refs: #22392 PR-URL: #22529 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
I, uhm, might have messed up by using a `substr(start, end)` signature when `std::string` actually uses `substr(start, len)`. Fix that. Fixes: #22526 Refs: #22392 PR-URL: #22529 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
I, uhm, might have messed up by using a `substr(start, end)` signature when `std::string` actually uses `substr(start, len)`. Fix that. Fixes: #22526 Refs: #22392 PR-URL: #22529 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
I, uhm, might have messed up by using a `substr(start, end)` signature when `std::string` actually uses `substr(start, len)`. Fix that. Fixes: #22526 Refs: #22392 PR-URL: #22529 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
I, uhm, might have messed up by using a
substr(start, end)
signature whenstd::string
actually usessubstr(start, len)
. Fix that.Fixes: #22526
Refs: #22392
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes