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

lib: deprecate node --debug at runtime #10970

Closed
wants to merge 1 commit into
base: master
from

Conversation

@joshgav
Member

joshgav commented Jan 23, 2017

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

lib, debug


This PR is a runtime deprecation for node --debug; it replaces #10276. A docs deprecation landed previously at #10320.

The @nodejs/diagnostics WG discussed in our most recent meeting whether to land a runtime deprecation in 7.x or 8.x and tentatively concluded that this depends on the version of V8 shipped with Node@8, see #9789 and #10117. That is, if Node@8 ships with V8 5.7 and thus the legacy Debugger remains available, we may be able to delay landing the runtime deprecation till 8.x as well.

But if Node@8 will ship with V8 5.8 and thus no legacy Debugger, we should include this deprecation in the next 7.x release. Even though we don't typically allow deprecations in the middle of a major, this would be an exception since it needs to be included before 8.x.

It was also raised that even if Node@8 will ship with V8 5.7 and the legacy Debugger, Node's policy is to deprecate for two major versions before removing a feature, so perhaps even in this case we should land the runtime deprecation already in a 7.x release.

@jasnell: Per #10116, I'm reserving DEP0062 for this deprecation, and will note in that thread too.

/cc @ofrobots

Show outdated Hide outdated test/parallel/test-debug-port-from-cmdline.js
'Starting debugger agent.',
'Debugger listening on 127.0.0.1:' + debugPort,
/Starting debugger agent./,
/\(node:\d+\) DeprecationWarning:.*/,

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 24, 2017

Member

Can you make the regex more specific? It catches all DeprecationWarnings now.

@bnoordhuis

bnoordhuis Jan 24, 2017

Member

Can you make the regex more specific? It catches all DeprecationWarnings now.

This comment has been minimized.

@jasnell

jasnell Jan 24, 2017

Member

Might be better to use common.expectWarning() (I may have the name wrong... there's a function for this tho :-) ..)

@jasnell

jasnell Jan 24, 2017

Member

Might be better to use common.expectWarning() (I may have the name wrong... there's a function for this tho :-) ..)

This comment has been minimized.

@joshgav

joshgav Jan 24, 2017

Member

I looked into common.expectWarning() but it won't be possible to use here since we're checking stderr from a child process, so left logic as is.

Added more of the deprecation warning to the regex.

@joshgav

joshgav Jan 24, 2017

Member

I looked into common.expectWarning() but it won't be possible to use here since we're checking stderr from a child process, so left logic as is.

Added more of the deprecation warning to the regex.

Show outdated Hide outdated test/parallel/test-debug-port-from-cmdline.js
];
assert.strictEqual(outputLines.length, expectedLines.length);
for (let i = 0; i < expectedLines.length; i++)
assert(expectedLines[i].includes(outputLines[i]));
assert(outputLines[i].match(expectedLines[i]));

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 24, 2017

Member

assert(expectedLines[i].test(outputLines[i]));?

@bnoordhuis

bnoordhuis Jan 24, 2017

Member

assert(expectedLines[i].test(outputLines[i]));?

Show outdated Hide outdated test/parallel/test-debug-signal-cluster.js
@@ -29,6 +29,10 @@ let pids;
child.stderr.on('data', function(data) {
const childStderrOutputString = data.toString();
if (childStderrOutputString
.match(/DeprecationWarning: node --debug is deprecated\./))

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 24, 2017

Member

.test(...) if you don't use the match result.

@bnoordhuis

bnoordhuis Jan 24, 2017

Member

.test(...) if you don't use the match result.

@ofrobots ofrobots referenced this pull request Jan 24, 2017

Closed

deps: update V8 to 5.5 #9618

2 of 2 tasks complete
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jan 24, 2017

Member

This depends somewhat on landing the deprecation codes PR

Member

jasnell commented Jan 24, 2017

This depends somewhat on landing the deprecation codes PR

Show outdated Hide outdated test/parallel/test-debug-signal-cluster.js
@@ -29,6 +29,10 @@ let pids;
child.stderr.on('data', function(data) {
const childStderrOutputString = data.toString();
if (/DeprecationWarning: node --debug is deprecated\./
.test(childStderrOutputString))

This comment has been minimized.

@Trott

Trott Jan 31, 2017

Member

This seems to rely on only one line coming in per data event whereas subsequent lines suggest that it is possible that more than one line will come in at a time.

Rather than this check, it might be better to add the deprecation text three times to the expectedContent array defined earlier in the test.

@Trott

Trott Jan 31, 2017

Member

This seems to rely on only one line coming in per data event whereas subsequent lines suggest that it is possible that more than one line will come in at a time.

Rather than this check, it might be better to add the deprecation text three times to the expectedContent array defined earlier in the test.

This comment has been minimized.

@joshgav

joshgav Feb 1, 2017

Member

subsequent lines suggest that it is possible that more than one line will come in at a time

yes, I see that now. will work on it and your other comments.

@joshgav

joshgav Feb 1, 2017

Member

subsequent lines suggest that it is possible that more than one line will come in at a time

yes, I see that now. will work on it and your other comments.

This comment has been minimized.

@joshgav

joshgav Feb 1, 2017

Member

@Trott as I reviewed I think it might be best to keep the deprecation warning isolated from the existing part of the test so it can be better understood and maintained/removed in the future. So instead I updated to address multi-line incoming data, PTAL and let me know what you think. Thanks!

@joshgav

joshgav Feb 1, 2017

Member

@Trott as I reviewed I think it might be best to keep the deprecation warning isolated from the existing part of the test so it can be better understood and maintained/removed in the future. So instead I updated to address multi-line incoming data, PTAL and let me know what you think. Thanks!

Show outdated Hide outdated test/parallel/test-debug-port-from-cmdline.js
@@ -38,11 +38,12 @@ function processStderrLine(line) {
function assertOutputLines() {
const expectedLines = [
'Starting debugger agent.',
'Debugger listening on 127.0.0.1:' + debugPort,
/Starting debugger agent./,

This comment has been minimized.

@Trott

Trott Jan 31, 2017

Member

Probably a good idea to start with ^ and end with $ for this and the other regular expressions.

@Trott

Trott Jan 31, 2017

Member

Probably a good idea to start with ^ and end with $ for this and the other regular expressions.

Show outdated Hide outdated test/parallel/test-debug-port-from-cmdline.js
'Debugger listening on 127.0.0.1:' + debugPort,
/Starting debugger agent./,
/\(node:\d+\) DeprecationWarning: node --debug is deprecated./,
/Debugger listening on 127.0.0.1:\d+/

This comment has been minimized.

@Trott

Trott Jan 31, 2017

Member

I'd prefer we match on the actual debugPort:

new RegExp(`^Debugger listening on 127.0.0.1:${debugPort}$`);
@Trott

Trott Jan 31, 2017

Member

I'd prefer we match on the actual debugPort:

new RegExp(`^Debugger listening on 127.0.0.1:${debugPort}$`);
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Jan 31, 2017

Member

@joshgav Is there a specific aspect of this that requires ctc-review? Or is that just a general "CTC ought to know about and look at this?"

Member

Trott commented Jan 31, 2017

@joshgav Is there a specific aspect of this that requires ctc-review? Or is that just a general "CTC ought to know about and look at this?"

@evanlucas

LGTM with @Trott's nits addressed. I'm also +1 for landing this in v7.x. I think we should give as much of a heads up as possible and April is coming fast.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jan 31, 2017

Member

just a heads up. The static deprecation codes PR landed in master. It is a semver-major. If this PR gets backported to v7, the static id code on the emitWarning should be removed.

Member

jasnell commented Jan 31, 2017

just a heads up. The static deprecation codes PR landed in master. It is a semver-major. If this PR gets backported to v7, the static id code on the emitWarning should be removed.

@mhdawson

LGTM subject to addressing Rich's comments on the tests.

@ofrobots

LGTM subject to existing nits.

@joshgav

This comment has been minimized.

Show comment
Hide comment
@joshgav

joshgav Feb 1, 2017

Member

@Trott

Is there a specific aspect of this that requires ctc-review?

Whether to land in 7.x or wait till 8.x, which in part depends on whether 8.x uses V8 5.7 or 5.8.

Updating to address comments now.

Member

joshgav commented Feb 1, 2017

@Trott

Is there a specific aspect of this that requires ctc-review?

Whether to land in 7.x or wait till 8.x, which in part depends on whether 8.x uses V8 5.7 or 5.8.

Updating to address comments now.

@jasnell

Almost there

Show outdated Hide outdated test/parallel/test-debug-port-from-cmdline.js
@@ -8,6 +8,12 @@ const args = ['--interactive', '--debug-port=' + debugPort];
const childOptions = { stdio: ['pipe', 'pipe', 'pipe', 'ipc'] };
const child = spawn(process.execPath, args, childOptions);
const reDeprecationWarning = new RegExp(
/^\(node:\d+\) DeprecationWarning: /.source +

This comment has been minimized.

@jasnell

jasnell Feb 1, 2017

Member

This check will need to look for the static deprecation identifier also.

@jasnell

jasnell Feb 1, 2017

Member

This check will need to look for the static deprecation identifier also.

Show outdated Hide outdated test/parallel/test-debug-signal-cluster.js
/node --debug is deprecated. /.source +
/Please use node --inspect instead./.source
);

This comment has been minimized.

@jasnell

jasnell Feb 1, 2017

Member

here also... the warning will include the static identifier in the output

@jasnell

jasnell Feb 1, 2017

Member

here also... the warning will include the static identifier in the output

@joshgav

This comment has been minimized.

Show comment
Hide comment
@joshgav

joshgav Feb 1, 2017

Member

@jasnell that's what I get for not rebasing first :) updated tests and added doc too.

Member

joshgav commented Feb 1, 2017

@jasnell that's what I get for not rebasing first :) updated tests and added doc too.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell approved these changes Feb 1, 2017

Show outdated Hide outdated doc/api/deprecations.md
Type: Runtime
`--debug` activates the legacy V8 debugger which is to be removed in favor of
Inspector.

This comment has been minimized.

@jasnell

jasnell Feb 1, 2017

Member

This can be expanded a bit to point out the use of the --inspect alternative and the reasons why --debug is being removed.

@jasnell

jasnell Feb 1, 2017

Member

This can be expanded a bit to point out the use of the --inspect alternative and the reasons why --debug is being removed.

@joshgav

This comment has been minimized.

Show comment
Hide comment
@joshgav

joshgav Feb 1, 2017

Member

new CI to be sure failures aren't due to interim updates: https://ci.nodejs.org/job/node-test-pull-request/6153/

Member

joshgav commented Feb 1, 2017

new CI to be sure failures aren't due to interim updates: https://ci.nodejs.org/job/node-test-pull-request/6153/

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Feb 1, 2017

Member

Test changes LGTM if CI is green.

Member

Trott commented Feb 1, 2017

Test changes LGTM if CI is green.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 2, 2017

Member

Looks like CI crashed and burned. New run: https://ci.nodejs.org/job/node-test-pull-request/6164/

Member

jasnell commented Feb 2, 2017

Looks like CI crashed and burned. New run: https://ci.nodejs.org/job/node-test-pull-request/6164/

@joshgav

This comment has been minimized.

Show comment
Hide comment
@joshgav

joshgav Feb 2, 2017

Member

Unfortunately looks like the CI failure is a real problem with one of the tests, I have an update to one of the regex's to try.

Member

joshgav commented Feb 2, 2017

Unfortunately looks like the CI failure is a real problem with one of the tests, I have an update to one of the regex's to try.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@joshgav

This comment has been minimized.

Show comment
Hide comment
@joshgav

joshgav Feb 3, 2017

Member

sigh... one more time with another fix: https://ci.nodejs.org/job/node-test-pull-request/6198/

Member

joshgav commented Feb 3, 2017

sigh... one more time with another fix: https://ci.nodejs.org/job/node-test-pull-request/6198/

@joshgav

This comment has been minimized.

Show comment
Hide comment
@joshgav

joshgav Feb 3, 2017

Member

armv7-wheezy failure is related, aix might be related, others are on test-fs-error-messages.js and don't seem to be related.

I thought about following @Trott's suggestion earlier to use a string instead of a regex, but that won't work with the test as is because the deprecation warning includes a nondeterministic PID.

@cjihrig since you've been working on cluster, could you take a look at the diff for test-debug-signal-cluster.js and see if you might have suggestions? Thanks!

Member

joshgav commented Feb 3, 2017

armv7-wheezy failure is related, aix might be related, others are on test-fs-error-messages.js and don't seem to be related.

I thought about following @Trott's suggestion earlier to use a string instead of a regex, but that won't work with the test as is because the deprecation warning includes a nondeterministic PID.

@cjihrig since you've been working on cluster, could you take a look at the diff for test-debug-signal-cluster.js and see if you might have suggestions? Thanks!

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 4, 2017

Member

Failing in fs-error-message.js was due to a bad land on my part. The commit has been reverted in master.

Member

jasnell commented Feb 4, 2017

Failing in fs-error-message.js was due to a bad land on my part. The commit has been reverted in master.

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Feb 8, 2017

Member

+1 to landing on v7.x

Member

rvagg commented Feb 8, 2017

+1 to landing on v7.x

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Feb 8, 2017

Member

+1 to landing on v7.x as long as we get the word out early to give people a chance to let us know if it is going to break stuff. We should be prepared for a quick revert if the extra warning causes anything to stop working in the ecosystem

Member

MylesBorins commented Feb 8, 2017

+1 to landing on v7.x as long as we get the word out early to give people a chance to let us know if it is going to break stuff. We should be prepared for a quick revert if the extra warning causes anything to stop working in the ecosystem

@joshgav

This comment has been minimized.

Show comment
Hide comment
@joshgav

joshgav Feb 8, 2017

Member

The failures on ARM, FreeBSD and Windows are related to the updated test 😢 . I hope to test locally on Windows later today (have to get my Windows box updated!). My best guess is that it has to do with capturing the last newline in the regex, but I don't know why that would effect FreeBSD...

Ideas from others would be great so I can get this landed!

Member

joshgav commented Feb 8, 2017

The failures on ARM, FreeBSD and Windows are related to the updated test 😢 . I hope to test locally on Windows later today (have to get my Windows box updated!). My best guess is that it has to do with capturing the last newline in the regex, but I don't know why that would effect FreeBSD...

Ideas from others would be great so I can get this landed!

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Feb 8, 2017

Member

Are you sure the FreeBSD issue isn't being caused by a stalled job from a previous test run rather than a problem with this change?

Member

Trott commented Feb 8, 2017

Are you sure the FreeBSD issue isn't being caused by a stalled job from a previous test run rather than a problem with this change?

@joshgav

This comment has been minimized.

Show comment
Hide comment
@joshgav

joshgav Feb 9, 2017

Member

All tests pass now (ARM is a false negative), but had to refactor the tests a bit and add --no-deprecation to bypass the cluster signal one.

@Trott @jasnell @evanlucas could you review again when you have a moment? Thanks.

Member

joshgav commented Feb 9, 2017

All tests pass now (ARM is a false negative), but had to refactor the tests a bit and add --no-deprecation to bypass the cluster signal one.

@Trott @jasnell @evanlucas could you review again when you have a moment? Thanks.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Feb 9, 2017

Member

@Trott @jasnell @evanlucas could you review again when you have a moment?

Seems A-OK to me.

Member

Trott commented Feb 9, 2017

@Trott @jasnell @evanlucas could you review again when you have a moment?

Seems A-OK to me.

@joshgav joshgav referenced this pull request Feb 9, 2017

Closed

(backport 7.x) lib: deprecate node --debug at runtime #11275

4 of 4 tasks complete
Show outdated Hide outdated doc/api/deprecations.md
Type: Runtime
`--debug` activates the legacy V8 debugger, which has been removed as of V8 5.8.

This comment has been minimized.

@Trott

Trott Feb 9, 2017

Member

Is it worth being specific about V8 removing the JSON API and not the debugger per se? Or would that just be more confusing? (And am I even correct about that? :-P )

@Trott

Trott Feb 9, 2017

Member

Is it worth being specific about V8 removing the JSON API and not the debugger per se? Or would that just be more confusing? (And am I even correct about that? :-P )

This comment has been minimized.

@joshgav

joshgav Feb 9, 2017

Member

How about "V8 debugger API"?

@joshgav

joshgav Feb 9, 2017

Member

How about "V8 debugger API"?

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 10, 2017

Member

Still LGTM

Member

jasnell commented Feb 10, 2017

Still LGTM

lib: deprecate node --debug at runtime
Fixes: #9789
PR-URL: #10970
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@joshgav

This comment has been minimized.

Show comment
Hide comment
@joshgav

joshgav Feb 10, 2017

Member

Landed in dfdd911. Thanks!

Member

joshgav commented Feb 10, 2017

Landed in dfdd911. Thanks!

@joshgav joshgav closed this Feb 10, 2017

joshgav added a commit that referenced this pull request Feb 10, 2017

lib: deprecate node --debug at runtime
Fixes: #9789
PR-URL: #10970
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@joshgav

This comment has been minimized.

Show comment
Hide comment
@joshgav

joshgav Feb 10, 2017

Member

Backport in progress at #11275

Member

joshgav commented Feb 10, 2017

Backport in progress at #11275

@Trott Trott removed the ctc-review label Feb 13, 2017

@jkrems jkrems referenced this pull request Feb 16, 2017

Closed

Switch the CLI debugger to V8 inspector #11421

14 of 14 tasks complete

KryDos added a commit to KryDos/node that referenced this pull request Feb 25, 2017

lib: deprecate node --debug at runtime
Fixes: nodejs#9789
PR-URL: nodejs#10970
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 27, 2017

lib: deprecate node --debug at runtime
PR-URL: nodejs#11275
Backport-of: nodejs#10970
Reviewed-By: James M Snell <jasnell@gmail.com>

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

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