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.

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

inspector: document bad usage for --inspect-port #12581

Closed

Conversation

sam-github
Copy link
Contributor

Document --inspect-port, and fix the reporting for when it is misused.

The option requires an argument, but when the argument was omitted, the
error message incorrectly reported --inspect-port as being bad, as if
was not supported at all:

% node --inspect-port
node: bad option: --inspect-port
% node --none-such
node: bad option: --none-such

It is now correctly reported as requiring an argument:

% ./node --inspect-port
./node: --inspect-port requires an argument
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

inspector

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 22, 2017
doc/api/cli.md Outdated
-->

Set the host:port to be used when the inspector is activated.
Useful when the activating the inspector by sending the `SIGUSR1` signal.
Copy link
Member

Choose a reason for hiding this comment

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

Extra the.

@mscdex mscdex added the inspector Issues and PRs related to the V8 inspector protocol label Apr 22, 2017
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Needs tests. Also, I would handle it in node_debug_options.cc, not node.cc.

@sam-github
Copy link
Contributor Author

I would handle it in node_debug_options.cc, not node.cc.

The only way to do that is to exit(9) from node_debug_options.cc, are you OK with that?

The structure of the code suggests to me that is not intended, but I don't understand why the debug options parsing was broken out into of the main option parsing loop in the first place. Because it was broken out into a function that returns boolean it can only indicate consumed/not consumed, and has no way of returning an error state. I could change the return to an int: 0 = consumed, 1 = not consumed, -1 means error -- but then how to return the specific error string? Add a return string arg, too? Seems a mess.

@bnoordhuis
Copy link
Member

The only way to do that is to exit(9) from node_debug_options.cc, are you OK with that?

Yes. There is at least one other exit statement in that file.

@joshgav
Copy link
Contributor

joshgav commented Apr 22, 2017

IIRC I didn't document --inspect-port while working on this cause I thought it could use some review before becoming public. Also, we may want to clarify its use cases and whether they can be met with the --inspect=9229 syntax (again IIRC 😉 ).

@sam-github
Copy link
Contributor Author

I think inspect-port is for use with SIGUSR1, ultimately, whereas --inspect=9999 will actually activate the inspector.

@sam-github
Copy link
Contributor Author

@bnoordhuis shouldn't that be exit(9)? What's with the 12? This and node_revert.cc seem to be the only uses of 12 in node, the other CLI arg errors I've found are 9.

@sam-github
Copy link
Contributor Author

@richardlau
Copy link
Member

Exit code 12 is specific to the --debug, --inspect and/or --debug-brk options (https://nodejs.org/api/process.html#process_exit_codes)

@bnoordhuis
Copy link
Member

@sam-github Rebase needed.

@sam-github sam-github force-pushed the document-inspect-port-usage branch 2 times, most recently from e779313 to 0a9a015 Compare May 17, 2017 17:00
@sam-github
Copy link
Contributor Author

@sam-github
Copy link
Contributor Author

@bnoordhuis PTAL

@sam-github sam-github force-pushed the document-inspect-port-usage branch from 0a9a015 to 8cf9130 Compare May 17, 2017 17:48
@sam-github
Copy link
Contributor Author

argh, lint error if you don't require common, lint error if you require it and don't use it.

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


// Tests that node exits consistently on bad option syntax.

common.noop(); // Avoid lint errors if common is not required, or not used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing this, you can just write require('../common'); on the line 2 and don't assign the result of require anywhere.

@sam-github
Copy link
Contributor Author

Document --inspect-port, and fix the reporting for when it is misused.

The option requires an argument, but when the argument was omitted, the
error message incorrectly reported --inspect-port as being bad, as if
was not supported at all:

    % node --inspect-port
    node: bad option: --inspect-port
    % node --none-such
    node: bad option: --none-such

It is now correctly reported as requiring an argument:

    % ./node --inspect-port
    ./node: --inspect-port requires an argument
@sam-github sam-github force-pushed the document-inspect-port-usage branch from e45f7e5 to 4a773f5 Compare May 18, 2017 16:01
@sam-github
Copy link
Contributor Author

@sam-github
Copy link
Contributor Author

OK, nice and green, @bnoordhuis

@gibfahn gibfahn dismissed bnoordhuis’s stale review May 23, 2017 20:05

Requested changes have been made.

@sam-github
Copy link
Contributor Author

sam-github commented May 24, 2017

Landed in 6c45b26

edit by @addaleax: force-pushed, now landed in 3954ea9

@sam-github sam-github closed this May 24, 2017
@sam-github sam-github deleted the document-inspect-port-usage branch May 24, 2017 18:26
addaleax pushed a commit that referenced this pull request May 24, 2017
Document --inspect-port, and fix the reporting for when it is misused.

The option requires an argument, but when the argument was omitted, the
error message incorrectly reported --inspect-port as being bad, as if
was not supported at all:

    % node --inspect-port
    node: bad option: --inspect-port
    % node --none-such
    node: bad option: --none-such

It is now correctly reported as requiring an argument:

    % ./node --inspect-port
    ./node: --inspect-port requires an argument

PR-URL: #12581
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request May 25, 2017
Document --inspect-port, and fix the reporting for when it is misused.

The option requires an argument, but when the argument was omitted, the
error message incorrectly reported --inspect-port as being bad, as if
was not supported at all:

    % node --inspect-port
    node: bad option: --inspect-port
    % node --none-such
    node: bad option: --none-such

It is now correctly reported as requiring an argument:

    % ./node --inspect-port
    ./node: --inspect-port requires an argument

PR-URL: #12581
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request May 28, 2017
Document --inspect-port, and fix the reporting for when it is misused.

The option requires an argument, but when the argument was omitted, the
error message incorrectly reported --inspect-port as being bad, as if
was not supported at all:

    % node --inspect-port
    node: bad option: --inspect-port
    % node --none-such
    node: bad option: --none-such

It is now correctly reported as requiring an argument:

    % ./node --inspect-port
    ./node: --inspect-port requires an argument

PR-URL: #12581
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell mentioned this pull request May 28, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@MylesBorins
Copy link
Contributor

I don't believe this is applicable to v6.x

Please feel free to change label if I am mistaken

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants