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

src: fix `--prof-process` CLI argument handling by addaleax · Pull Request #22790 · nodejs/node · GitHub

/ node Public

src: fix --prof-process CLI argument handling - #22790

Closed
addaleax wants to merge 4 commits into
nodejs:masterfrom
addaleax:fix-prof-process
ConversationCommits (4)ChecksFiles changed
Closed

src: fix --prof-process CLI argument handling#22790
addaleax wants to merge 4 commits into
nodejs:masterfrom
addaleax:fix-prof-process

Conversation

addaleax commented Sep 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Make sure that options after --prof-process are not treated
as Node.js options.

Fixes: #22786

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

src: fix --prof-process CLI argument handling
d86e38e 
Make sure that options after `--prof-process` are not treated
as Node.js options.

Fixes: nodejs#22786
addaleax requested a review from bengl September 10, 2018 10:09
nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 10, 2018
addaleax mentioned this pull request Sep 10, 2018
Closed
addaleax added the cli Issues and PRs related to the Node.js command line interface. label Sep 10, 2018

nodejs-github-bot commented Sep 10, 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/850/pipeline

addaleax commented Sep 10, 2018

Copy link
Copy Markdown
Member Author

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

joyeecheung approved these changes Sep 10, 2018
View reviewed changes
Comment thread test/parallel/test-tick-processor-arguments.js
assert(logfile);

// Make sure that the --preprocess argument is passed through correctly.
const { stdout } = spawnSync(

joyeecheung Sep 10, 2018

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

Maybe a note here about any arguments accepted by deps/v8/tools/tickprocessor.js would do in case V8 removes that in the future?

addaleax Sep 10, 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

Yup, done (and also fixed CI a bit) :)

mcollina commented Sep 10, 2018

Copy link
Copy Markdown
Member

I can confirm this fixes the problem for 0x.

mcollina approved these changes Sep 10, 2018
View reviewed changes

mcollina 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

addaleax commented Sep 10, 2018

Copy link
Copy Markdown
Member Author

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

richardlau commented Sep 10, 2018

Copy link
Copy Markdown
Member

I think we should note in https://github.com/nodejs/node/blob/master/doc/api/cli.md#--prof-process and https://github.com/nodejs/node/blob/master/doc/node.1 about the processing of subsequent options.

addaleax commented Sep 10, 2018

Copy link
Copy Markdown
Member Author

@richardlau Would a follow-up PR be okay? I’d also like to make --prof-process --help work, and it would be good to be able to refer to that (I don’t think it has ever worked in Node.js)

richardlau commented Sep 10, 2018

Copy link
Copy Markdown
Member

Would a follow-up PR be okay? I’d also like to make --prof-process --help work, and it would be good to be able to refer to that (I don’t think it has ever worked in Node.js)

👍 Fine by me.

mcollina commented Sep 10, 2018

Copy link
Copy Markdown
Member

Can we fast-track this and getting it into a release asap?

addaleax added the fast-track PRs that do not need to wait for 48 hours to land. label Sep 10, 2018

targos commented Sep 10, 2018

Copy link
Copy Markdown
Member

@mcollina Are you asking for a release sooner than usual (i.e. this week instead of the next one)?

mcollina commented Sep 10, 2018

Copy link
Copy Markdown
Member

If possible, yes. If no one is available to do it, it's not a big deal (next week is also ok). A significant number of flamegraph tools (not just clinic) are broken because of this.

addaleax mentioned this pull request Sep 11, 2018
Closed
2 tasks
addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Sep 11, 2018

addaleax commented Sep 11, 2018

Copy link
Copy Markdown
Member Author

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

addaleax commented Sep 12, 2018

Copy link
Copy Markdown
Member Author

I pushed a commit to skip the test on AIX completely; not sure why it’s failing there. (/cc @nodejs/platform-aix)

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

richardlau commented Sep 13, 2018

Copy link
Copy Markdown
Member

I pushed a commit to skip the test on AIX completely; not sure why it’s failing there. (/cc @nodejs/platform-aix)

There's this comment in the tick-processor tests:

node/test/tick-processor/test-tick-processor-unknown.js

Lines 4 to 10 in 19c0620

// TODO(mhdawson) Currently the test-tick-processor functionality in V8
// depends on addresses being smaller than a full 64 bits. AIX supports
// the full 64 bits and the result is that it does not process the
// addresses correctly and runs out of memory
// Disabling until we get a fix upstreamed into V8
if (common.isAIX)
common.skip('AIX address range too big for scripts.');

addaleax commented Sep 13, 2018

Copy link
Copy Markdown
Member Author

@richardlau Okay, thanks – makes sense!

targos commented Sep 14, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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

Rebuild Windows: https://ci.nodejs.org/job/node-test-commit-windows-fanned/20732/
Rebuild Linux: https://ci.nodejs.org/job/node-test-commit-linux/21563/

targos commented Sep 16, 2018

Copy link
Copy Markdown
Member

Landed in 23f8b02

targos closed this Sep 16, 2018
targos pushed a commit that referenced this pull request Sep 16, 2018
src: fix --prof-process CLI argument handling
f2ae0cb 
Make sure that options after `--prof-process` are not treated
as Node.js options.

Fixes: #22786

PR-URL: #22790
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Sep 16, 2018
src: fix --prof-process CLI argument handling
23f8b02 
Make sure that options after `--prof-process` are not treated
as Node.js options.

Fixes: #22786

PR-URL: #22790
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
addaleax deleted the fix-prof-process branch September 16, 2018 07:46
targos mentioned this pull request Sep 18, 2018
Merged
targos pushed a commit that referenced this pull request Sep 19, 2018
src: fix --prof-process CLI argument handling
466ed2e 
Make sure that options after `--prof-process` are not treated
as Node.js options.

Fixes: #22786

PR-URL: #22790
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Sep 20, 2018
src: fix --prof-process CLI argument handling
bc07612 
Make sure that options after `--prof-process` are not treated
as Node.js options.

Fixes: #22786

PR-URL: #22790
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
mcollina mentioned this pull request Sep 20, 2018
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

mcollina mcollina approved these changes
bengl bengl approved these changes
jasnell jasnell approved these changes
cjihrig cjihrig approved these changes
joyeecheung joyeecheung approved these changes
BridgeAR BridgeAR approved these changes
trivikr trivikr approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. 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.

--prof-process CLI args no longer work in 10.10.0
Back | FazBrowse Home | New Git URL