Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 4 commits into from
Closed

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

wants to merge 4 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Sep 10, 2018 •
edited
Loading

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

@addaleax
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 addaleax requested a review from bengl September 10, 2018 10:09
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 10, 2018
@addaleax addaleax mentioned this pull request Sep 10, 2018
Closed
@addaleax addaleax added the cli Issues and PRs related to the Node.js command line interface. label Sep 10, 2018
@nodejs-github-bot
Copy link
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
Copy link
Member Author

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@mcollina
Copy link
Member

I can confirm this fixes the problem for 0x.

mcollina
mcollina approved these changes Sep 10, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@addaleax
Copy link
Member Author

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

@richardlau
Copy link
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
Copy link
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
Copy link
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
Copy link
Member

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

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

targos commented Sep 10, 2018

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

@mcollina
Copy link
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 addaleax mentioned this pull request Sep 11, 2018
Closed
2 tasks
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 11, 2018
@addaleax
Copy link
Member Author

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

@addaleax
Copy link
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
Copy link
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
Copy link
Member Author

@richardlau Okay, thanks – makes sense!

@targos
Copy link
Member

targos commented Sep 14, 2018 •
edited
Loading

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
Copy link
Member

targos commented Sep 16, 2018

Landed in 23f8b02

@targos targos closed this Sep 16, 2018
targos pushed a commit that referenced this pull request Sep 16, 2018
@addaleax @targos
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
@addaleax @targos
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 addaleax deleted the fix-prof-process branch September 16, 2018 07:46
@targos targos mentioned this pull request Sep 18, 2018
Merged
targos pushed a commit that referenced this pull request Sep 19, 2018
@addaleax @targos
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
@addaleax @targos
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 mcollina mentioned this pull request Sep 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@bengl bengl bengl approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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