FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: fix NODE_OPTIONS parsing bug by addaleax · Pull Request #22529 · nodejs/node · GitHub

/ node Public

src: fix NODE_OPTIONS parsing bug - #22529

Closed
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:fix-node-options
ConversationCommits (1)ChecksFiles changed
Closed

src: fix NODE_OPTIONS parsing bug#22529
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:fix-node-options

Conversation

addaleax commented Aug 26, 2018

Copy link
Copy Markdown
Member

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

src: fix NODE_OPTIONS parsing bug
c533499 
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

nodejs-github-bot commented Aug 26, 2018

Copy link
Copy Markdown
Collaborator

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/675/pipeline

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 26, 2018
addaleax mentioned this pull request Aug 26, 2018
Closed
addaleax added cli Issues and PRs related to the Node.js command line interface. fast-track PRs that do not need to wait for 48 hours to land. labels Aug 26, 2018

addaleax commented Aug 26, 2018

Copy link
Copy Markdown
Member Author

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.

BridgeAR approved these changes Aug 26, 2018
View reviewed changes

BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. Thanks for the quick fix.

mscdex reviewed Aug 26, 2018
View reviewed changes
Comment thread test/parallel/test-cli-node-options.js
// 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 ' +

mscdex Aug 26, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

addaleax Aug 26, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Aug 26, 2018

Copy link
Copy Markdown
Contributor

Simple fixes for regressions, should be fast tracked.

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

Trott commented Aug 26, 2018

Copy link
Copy Markdown
Member

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

targos commented Aug 26, 2018

Copy link
Copy Markdown
Member

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

addaleax commented Aug 26, 2018

Copy link
Copy Markdown
Member Author

Landed in 1c05b16

addaleax closed this Aug 26, 2018
addaleax deleted the fix-node-options branch August 26, 2018 12:01
addaleax added a commit that referenced this pull request Aug 26, 2018
src: fix NODE_OPTIONS parsing bug
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>
addaleax added a commit that referenced this pull request Aug 28, 2018
src: fix NODE_OPTIONS parsing bug
7e3ceb1 
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>
targos pushed a commit that referenced this pull request Sep 3, 2018
src: fix NODE_OPTIONS parsing bug
2c5dfef 
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>
targos mentioned this pull request Sep 5, 2018
Merged
targos pushed a commit that referenced this pull request Sep 6, 2018
src: fix NODE_OPTIONS parsing bug
7282768 
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>
This was referenced Sep 6, 2018
Closed
Closed
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

BridgeAR BridgeAR approved these changes
trivikr trivikr approved these changes
+3 more reviewers
mscdex mscdex left review comments
refack refack approved these changes
devsnek devsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. cli Issues and PRs related to the Node.js command line interface. fast-track PRs that do not need to wait for 48 hours to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

NODE_OPTIONS do not work properly anymore

9 participants

Back | FazBrowse Home | New Git URL