★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/22529Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

Closed
wants to merge 1 commit into from

Conversation

Reviewers

@mscdex mscdex

@refack refack

@devsnek devsnek

@BridgeAR BridgeAR

@trivikr trivikr

Assignees
No one assigned
Labels
C++ cli fast-track
Projects
None yet
Milestone
No milestone
9 participants
@addaleax @nodejs-github-bot @refack @Trott @targos @mscdex @devsnek @BridgeAR @trivikr
@addaleax
Copy link
Member

addaleax commented Aug 26, 2018

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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
@addaleax
src: fix NODE_OPTIONS parsing bug 
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
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
Loading status checks…
c533499
@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added the C++ label Aug 26, 2018

@addaleax addaleax referenced this pull request Aug 26, 2018

Closed

NODE_OPTIONS do not work properly anymore #22526

@addaleax addaleax added cli fast-track labels Aug 26, 2018

@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

addaleax commented Aug 26, 2018

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

I have labeled this fast-track, mostly because this isn’t a complex change by itself and it would be important that this goes into the same release as #22392 (and I think we may want to do a v10.x this week). Feel free to 👍 or 👎 this comment depending on your opinion on that.
@devsnek

devsnek approved these changes Aug 26, 2018
View changes

@BridgeAR

BridgeAR approved these changes Aug 26, 2018
View changes

Copy link
Member

BridgeAR left a comment

LGTM. Thanks for the quick fix.
@mscdex

mscdex reviewed Aug 26, 2018
View changes

test/parallel/test-cli-node-options.js
@@ -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 ' +

This comment has been minimized.

Sign in to view
Copy link
@mscdex

mscdex Aug 26, 2018

Contributor

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?

This comment has been minimized.

Sign in to view
Copy link
@addaleax

addaleax Aug 26, 2018

Author Member

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…
@refack

refack approved these changes Aug 26, 2018
View changes

@refack

This comment has been minimized.

Sign in to view
Copy link
Member

refack commented Aug 26, 2018

Simple fixes for regressions, should be fast tracked.

Resume CI:https://ci.nodejs.org/job/node-test-commit/20911/
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Aug 26, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/16755/
@targos

This comment has been minimized.

Sign in to view
Copy link
Member

targos commented Aug 26, 2018

Resumed: https://ci.nodejs.org/job/node-test-pull-request/16757/
@trivikr

trivikr approved these changes Aug 26, 2018
View changes

@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

addaleax commented Aug 26, 2018

Landed in 1c05b16

@addaleax addaleax closed this Aug 26, 2018

@addaleax addaleax deleted the addaleax:fix-node-options branch Aug 26, 2018

addaleax added a commit that referenced this pull request Aug 26, 2018

@addaleax
src: fix NODE_OPTIONS parsing bug 
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>
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
Loading status checks…
1c05b16

addaleax added a commit that referenced this pull request Aug 28, 2018

@addaleax
src: fix NODE_OPTIONS parsing bug 
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>
Verified
This commit was signed with a verified signature.
@addaleax addaleax Anna Henningsen
GPG key ID: 9C63F3A6CD2AD8F9 Learn about signing commits
7e3ceb1

targos added a commit that referenced this pull request Sep 3, 2018

@addaleax @targos
src: fix NODE_OPTIONS parsing bug 
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
2c5dfef

@targos targos referenced this pull request Sep 5, 2018

Merged

Release proposal: v10.10.0 #22716

targos added a commit that referenced this pull request Sep 6, 2018

@addaleax @targos
src: fix NODE_OPTIONS parsing bug 
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>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
7282768

This was referenced Sep 6, 2018

Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.