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: make `debug` an alias for `inspect` #11441

Merged
merged 2 commits into from Apr 12, 2017

Conversation

@ofrobots
Contributor

ofrobots commented Feb 17, 2017

node debug is now an alias of node inspect. This is intended to be a minimal change – it does
not get rid of the old debugger code. That can be done in a follow-on.

  • This commit has a circular dependency on nodejs/node-inspect#26. I propose that we land here, and then later land upstream.
  • test-debugger-pid.js disabled because it depends on ability to debug a
    pid in node-inspect. Once upstream adds this capability, we can enable
    the test back again.

/cc @nodejs/diagnostics @eugeneo @jkrems

Show outdated Hide outdated test/parallel/test-debug-brk-no-arg.js
@@ -1,13 +0,0 @@
'use strict';

This comment has been minimized.

@ofrobots

ofrobots Feb 17, 2017

Contributor

Removed because inspect behaviour is different from debug. The former stops before reaching the repl. This test doesn't make sense anymore.

@ofrobots

ofrobots Feb 17, 2017

Contributor

Removed because inspect behaviour is different from debug. The former stops before reaching the repl. This test doesn't make sense anymore.

@jasnell jasnell added the in progress label Feb 17, 2017

@joshgav joshgav changed the title from inspector: make `debug` and alias for `inspect` to inspector: make `debug` an alias for `inspect` Feb 22, 2017

@joshgav joshgav added the diag-agenda label Feb 23, 2017

@joshgav joshgav referenced this pull request Feb 23, 2017

Closed

Diag WG Meeting - 2017-02 #85

Show outdated Hide outdated deps/node-inspect/lib/_inspect.js
@@ -39,7 +39,7 @@ const [ InspectClient, createRepl ] =
const debuglog = util.debuglog('inspect');
exports.port = 9229;

This comment has been minimized.

@jkrems

jkrems Feb 23, 2017

Contributor

I assume this is to support node --{inspect,debug}-port=1234 {inspect,debug} my-script.js etc.? If so, I'd rather make that explicit.

@jkrems

jkrems Feb 23, 2017

Contributor

I assume this is to support node --{inspect,debug}-port=1234 {inspect,debug} my-script.js etc.? If so, I'd rather make that explicit.

This comment has been minimized.

@jkrems

jkrems Feb 23, 2017

Contributor

Maybe something like this? nodejs/node-inspect#26

@jkrems

jkrems Feb 23, 2017

Contributor

Maybe something like this? nodejs/node-inspect#26

This comment has been minimized.

@ofrobots

ofrobots Mar 14, 2017

Contributor

Done.

@ofrobots

ofrobots Mar 14, 2017

Contributor

Done.

@joshgav

Summary: I think we should wait to alias debug till Node integrates V8 5.8+, and not at the start of Node 8, assuming Node 8 uses V8 5.7. This provides more notification time for those depending on the legacy interface.

Also, there's a lot of code here unrelated to aliasing debug. Could that be separated into another PR? It would make review easier. Thanks!

Show outdated Hide outdated src/node_debug_options.cc
@@ -93,15 +93,15 @@ bool DebugOptions::ParseOption(const std::string& option) {
}
// --debug and --inspect are mutually exclusive
if (option_name == "--debug") {
/*if (option_name == "--debug") {

This comment has been minimized.

@joshgav

joshgav Mar 7, 2017

Member

--debug is the primary way to activate the legacy debugger interface. Aliasing it to --inspect instead would make it impossible to start that interface. That would break tools which still depend on it.

I think we should keep --debug associated with the legacy debugger as long as possible, i.e. till we update to V8 5.8+.

@joshgav

joshgav Mar 7, 2017

Member

--debug is the primary way to activate the legacy debugger interface. Aliasing it to --inspect instead would make it impossible to start that interface. That would break tools which still depend on it.

I think we should keep --debug associated with the legacy debugger as long as possible, i.e. till we update to V8 5.8+.

Show outdated Hide outdated lib/internal/bootstrap_node.js
NativeModule.require('_debugger').start();
} else if (process.argv[1] === 'inspect') {
} else if (process.argv[1] === 'inspect' || process.argv[1] === 'debug') {

This comment has been minimized.

@joshgav

joshgav Mar 7, 2017

Member

Per my previous comment, I think we should not yet alias --debug to --inspect. The reason for not yet aliasing --debug doesn't apply to node debug, but for consistency's sake, we should probably not yet alias node debug to node inspect either.

@joshgav

joshgav Mar 7, 2017

Member

Per my previous comment, I think we should not yet alias --debug to --inspect. The reason for not yet aliasing --debug doesn't apply to node debug, but for consistency's sake, we should probably not yet alias node debug to node inspect either.

@joshgav joshgav referenced this pull request Mar 13, 2017

Closed

Diag WG Meeting - 2017-03 #89

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Mar 14, 2017

Contributor
  • @joshgav I have updated the PR to only make debug and alias of inspect. --debug will continue to start the old debug protocol. PTAL.
  • @jkrems This includes nodejs/node-inspect#26. This is a circular dependency, but I propose we land it here first (assuming tests pass upstream). Also, once node-inspect has the ability to debug PIDs, we can re-enable test-debugger-pid.js.
Contributor

ofrobots commented Mar 14, 2017

  • @joshgav I have updated the PR to only make debug and alias of inspect. --debug will continue to start the old debug protocol. PTAL.
  • @jkrems This includes nodejs/node-inspect#26. This is a circular dependency, but I propose we land it here first (assuming tests pass upstream). Also, once node-inspect has the ability to debug PIDs, we can re-enable test-debugger-pid.js.

This PR no longer affects --debug

Show outdated Hide outdated test/parallel/test-debug-usage.js
const expectedUsageMessage = `Usage: node debug script.js
node debug <host>:<port>
node debug -p <pid>
const expectedUsageMessage = `Usage: node-inspect script.js

This comment has been minimized.

@jkrems

jkrems Mar 14, 2017

Contributor

Note to future self: This test will need to be updated once we pull in @addaleax's usage text fix. https://github.com/nodejs/node-inspect/pull/20/files

@jkrems

jkrems Mar 14, 2017

Contributor

Note to future self: This test will need to be updated once we pull in @addaleax's usage text fix. https://github.com/nodejs/node-inspect/pull/20/files

@jkrems

This comment has been minimized.

Show comment
Hide comment
@jkrems

jkrems Mar 14, 2017

Contributor

👍 to "cherry-picking" that change (I'll try to do another dependency pull soon so the two code bases don't drift apart too much).

Contributor

jkrems commented Mar 14, 2017

👍 to "cherry-picking" that change (I'll try to do another dependency pull soon so the two code bases don't drift apart too much).

@ofrobots ofrobots added this to the 8.0.0 milestone Mar 15, 2017

@bnoordhuis

LGTM modulo comment.

Show outdated Hide outdated test/parallel/test-debug-brk.js
@@ -65,6 +65,6 @@ function test(extraArgs, stdoutPattern) {
test(['-e', '0']);
test(['-e', '0', 'foo']);
test(['-p', 'process.argv[1]', 'foo'], /^\s*foo\s*$/);
test(['-p', 'process.argv[1]', 'foo']);

This comment has been minimized.

@bnoordhuis

bnoordhuis Apr 4, 2017

Member

You're leaving behind dead code with this change. Remove stdoutPattern and outputMatched from test(). There may be more.

@bnoordhuis

bnoordhuis Apr 4, 2017

Member

You're leaving behind dead code with this change. Remove stdoutPattern and outputMatched from test(). There may be more.

This comment has been minimized.

@ofrobots

ofrobots Apr 4, 2017

Contributor

Done.

@ofrobots

ofrobots Apr 4, 2017

Contributor

Done.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Apr 4, 2017

Member

I'm afraid this needs a rebase. Seems some conflicting changes were landed today.

Member

bnoordhuis commented Apr 4, 2017

I'm afraid this needs a rebase. Seems some conflicting changes were landed today.

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Apr 4, 2017

Contributor

Yep, working on that now. In any case, I think it might make sense to land after #11431 and #12197 have landed to minimize conflicts.

Contributor

ofrobots commented Apr 4, 2017

Yep, working on that now. In any case, I think it might make sense to land after #11431 and #12197 have landed to minimize conflicts.

Show outdated Hide outdated lib/internal/bootstrap_node.js
@@ -76,6 +71,12 @@
});
} else if (process.argv[1] === 'inspect' || process.argv[1] === 'debug') {
if (process.argv[1] === 'debug') {
process.emitWarning(
'`node debug` is deprecated. Please use `node inspect` instead.',

This comment has been minimized.

@ofrobots

ofrobots Apr 4, 2017

Contributor

This assumes that this PR will land after #12197. @jasnell I am changing the message for DEP0062, is that acceptable, or should this be a different deprecation ID? Note that the behaviour is unchanged node debug used to print DEP0062 and it will continue to do so, but with a better message.

@ofrobots

ofrobots Apr 4, 2017

Contributor

This assumes that this PR will land after #12197. @jasnell I am changing the message for DEP0062, is that acceptable, or should this be a different deprecation ID? Note that the behaviour is unchanged node debug used to print DEP0062 and it will continue to do so, but with a better message.

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Apr 4, 2017

Contributor

Rebased. However, this won't pass the CI just yet. #12197 needs to land first before I can start looking at tests.

Contributor

ofrobots commented Apr 4, 2017

Rebased. However, this won't pass the CI just yet. #12197 needs to land first before I can start looking at tests.

@jkrems jkrems referenced this pull request Apr 4, 2017

Closed

src: Remove support for --debug #12197

4 of 4 tasks complete
@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Apr 6, 2017

Member

@ofrobots #12197 just landed

Member

targos commented Apr 6, 2017

@ofrobots #12197 just landed

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Apr 6, 2017

Contributor

Rebased. I needed to cherry-pick a fix from node-inspect. This is ready for review again.

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

Contributor

ofrobots commented Apr 6, 2017

Rebased. I needed to cherry-pick a fix from node-inspect. This is ready for review again.

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

@jkrems jkrems referenced this pull request Apr 6, 2017

Closed

[WIP] deps: Update node-inspect to v1.11.0 #12214

3 of 3 tasks complete
@joshgav

joshgav approved these changes Apr 6, 2017

code LGTM, a couple considerations in comments though

Show outdated Hide outdated lib/internal/bootstrap_node.js
if (process.argv[1] === 'debug') {
process.emitWarning(
'`node debug` is deprecated. Please use `node inspect` instead.',
'DeprecationWarning', 'DEP0062');

This comment has been minimized.

@joshgav

joshgav Apr 6, 2017

Member

Technically DEP0062 is for node --inspect (activate the websocket server) and node debug (use the builtin CLI debugger) should get another one.

@joshgav

joshgav Apr 6, 2017

Member

Technically DEP0062 is for node --inspect (activate the websocket server) and node debug (use the builtin CLI debugger) should get another one.

This comment has been minimized.

@ofrobots

ofrobots Apr 6, 2017

Contributor

It is a bit of a technicality butnode debug used to produce DEP0062 before this change. There is no other place in the code that can produce DEP0062 anymore as --debug is no longer an option. I think it is simpler to continue to use DEP0062 with an updated message. @jasnell thoughts? How do I reserve a new deprecation message, if there is a preference for that?

@ofrobots

ofrobots Apr 6, 2017

Contributor

It is a bit of a technicality butnode debug used to produce DEP0062 before this change. There is no other place in the code that can produce DEP0062 anymore as --debug is no longer an option. I think it is simpler to continue to use DEP0062 with an updated message. @jasnell thoughts? How do I reserve a new deprecation message, if there is a preference for that?

This comment has been minimized.

@targos

targos Apr 7, 2017

Member

I think we should assign a new code and transition DEP0062 to End-of-Life.
Otherwise the code would have a different meaning in 8.x and 7.x.

@targos

targos Apr 7, 2017

Member

I think we should assign a new code and transition DEP0062 to End-of-Life.
Otherwise the code would have a different meaning in 8.x and 7.x.

This comment has been minimized.

@ofrobots

ofrobots Apr 7, 2017

Contributor

Done.

@ofrobots

ofrobots Apr 7, 2017

Contributor

Done.

@@ -82,10 +82,10 @@ void PrintDebuggerReadyMessage(const std::string& host,
return;
}
fprintf(out,
"Debugger listening on port %d.\n"
"Debugger listening on %s:%d.\n"

This comment has been minimized.

@joshgav

joshgav Apr 6, 2017

Member

might be best to defer to #11207 for changes to the hint text, although I agree we should include the host.

@joshgav

joshgav Apr 6, 2017

Member

might be best to defer to #11207 for changes to the hint text, although I agree we should include the host.

This comment has been minimized.

@ofrobots

ofrobots Apr 7, 2017

Contributor

I do need the change here for the tests to work correctly. I suspect the last one to land will need to resolve some merge conflicts.

@ofrobots

ofrobots Apr 7, 2017

Contributor

I do need the change here for the tests to work correctly. I suspect the last one to land will need to resolve some merge conflicts.

@targos

This comment has been minimized.

Show comment
Hide comment
Show outdated Hide outdated test/parallel/test-debug-usage.js
node debug -p <pid>
`;
const expectedLines = [
/^\(node:\d+\) \[DEP0062\] DeprecationWarning:/,

This comment has been minimized.

@targos

targos Apr 8, 2017

Member

DEP0068

@targos

targos Apr 8, 2017

Member

DEP0068

This comment has been minimized.

@jasnell

jasnell Apr 8, 2017

Member

DEP00XX, the actual number should be assigned when the pr lands

@jasnell

jasnell Apr 8, 2017

Member

DEP00XX, the actual number should be assigned when the pr lands

This comment has been minimized.

@ofrobots

ofrobots Apr 10, 2017

Contributor

Done.

@ofrobots

ofrobots Apr 10, 2017

Contributor

Done.

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Apr 10, 2017

Contributor

Looks like there are related CI failures on Windows.

Contributor

cjihrig commented Apr 10, 2017

Looks like there are related CI failures on Windows.

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Apr 11, 2017

Contributor

CI is looking good (mac failure is unrelated, aix seems to have passed despite the shown status). Will land shortly.

Contributor

ofrobots commented Apr 11, 2017

CI is looking good (mac failure is unrelated, aix seems to have passed despite the shown status). Will land shortly.

@targos

targos approved these changes Apr 11, 2017

ofrobots added some commits Mar 14, 2017

src,test: debug is now an alias for inspect
`node debug` is now an alias of `node inspect`. This is intended to be
a minimal change – it does not get rid of the the debugger code. That
can be done in a follow-on.

PR-URL: #11441
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: joshgav - Josh Gavant <josh.gavant@outlook.com>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
deps: cherry-pick node-inspect#43
Node 8.x no longer has --debug-brk.

Ref: nodejs/node-inspect#43
PR-URL: #11441
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: joshgav - Josh Gavant <josh.gavant@outlook.com>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>

@ofrobots ofrobots merged commit a235ccd into nodejs:master Apr 12, 2017

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Apr 12, 2017

Contributor

Landed as a912425...a235ccd.

Contributor

ofrobots commented Apr 12, 2017

Landed as a912425...a235ccd.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 10, 2017

Member

Assuming the diag-agenda label can be removed now that this is merged.

Member

addaleax commented May 10, 2017

Assuming the diag-agenda label can be removed now that this is merged.

@jasnell jasnell referenced this pull request May 11, 2017

Closed

8.0.0 Release Proposal #12220

@js-choi js-choi referenced this pull request Aug 13, 2017

Closed

Support --inspect #363

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment