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

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

Closed

Conversation

joshgav
Copy link
Contributor

@joshgav joshgav commented Feb 9, 2017

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

lib


Backport of #10970. Same except removed the new deprecation code.

@joshgav
Copy link
Contributor Author

joshgav commented Feb 9, 2017

@sam-github
Copy link
Contributor

I thought runtime deprecations were semver-major, can we put this on 7.x?

@joshgav joshgav added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 10, 2017
@jasnell
Copy link
Member

jasnell commented Feb 10, 2017

They are. This is a special case since the old debugger is disappearing in 8.0.0

@joshgav
Copy link
Contributor Author

joshgav commented Feb 10, 2017

@jasnell

the old debugger is disappearing in 8.0.0

To clarify, that depends on whether 8 uses V8 5.7 or 5.8, see #10970 (comment).

Mentioning that again here, cause if we are going 5.7 for Node@8 we might not want to land this backport.

@jasnell
Copy link
Member

jasnell commented Feb 10, 2017

True... should have said 8-ish ;-)

@sam-github
Copy link
Contributor

sam-github commented Feb 10, 2017

Maybe we should not add this message in 7.x, since it can break things badly for code that is spawning node with --debug and looking at the stdout from node, and instead we can land in 8.x (because the --debug will go away sometime), and at the time we actually remove --debug, we can replace it with a message saying "use --inspect instead" (keeping that code just to the end of the major). Generally you want to give warnings that a break will occur in the future, but retroactively applying a breaking major change to 7.x, just to warn about how we will be applying a breaking major in 8.x without giving proper warning doesn't sound like its getting us much.

@joshgav
Copy link
Contributor Author

joshgav commented Feb 11, 2017

@sam-github

looking at the stdout from node

FWIW the warning goes to stderr, but your point still stands. Also users can specify --no-deprecation to avoid the warning.

@sam-github
Copy link
Contributor

FWIW, I don't have really strong feelings about this, I just wanted to make the suggestion.

I also think that merging a semver-major into 7.x deserves a blog post, this situation is one we forced into, its easy to justify, but it does violate the official stability policy.

@italoacasas
Copy link
Contributor

Well, for sure I'm not going to be releasing this on v7.6.0 (Feb 21), but would be nice if you guys made a decision before Feb 28 and we can do an RC with the deprecation. Btw I think made a lot of sense to land this on v7.x.

(Maybe someone can write a blog about the intent of doing this to see the community response?)

@joshgav
Copy link
Contributor Author

joshgav commented Feb 16, 2017

The original commit (#10970) has been in master for a week now so we need to decide whether to include this backport in the next 7.x release (would be 7.7.0). Need CTC approval because it's an exception to semver policy, so tagging ctc-agenda. Perhaps we can vote via approvals/LGTMs of this PR?

@italoacasas

Maybe someone can write a blog about the intent of doing this to see the community response?

Good suggestion, I'll try to write up something next week, or perhaps @ofrobots you'd like to?

@joshgav
Copy link
Contributor Author

joshgav commented Feb 22, 2017

CTC approved landing in next 7.x release.

@italoacasas
Copy link
Contributor

italoacasas commented Feb 23, 2017

Ok, I will include this in the next proposal and should be part of 7.7.0 (Feb 28).

Are we doing the blog post? I really think we should inform this to the community before releasing.

@joshgav
Copy link
Contributor Author

joshgav commented Feb 23, 2017

@italoacasas

Are we doing the blog post?

PTAL: nodejs/nodejs.org#1156

@joshgav joshgav force-pushed the deprecate-dash-dash-debug-7.x branch from 46439e7 to d1b83b9 Compare February 23, 2017 19:22
@italoacasas
Copy link
Contributor

italoacasas commented Feb 25, 2017

@joshgav can we land this yet?

@italoacasas italoacasas added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Feb 25, 2017
@italoacasas
Copy link
Contributor

Landed in v7.x-staging

italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 27, 2017
PR-URL: nodejs#11275
Backport-of: nodejs#10970
Reviewed-By: James M Snell <jasnell@gmail.com>
@italoacasas italoacasas mentioned this pull request Feb 27, 2017
italoacasas added a commit to italoacasas/node that referenced this pull request Feb 28, 2017
Notables changes:
* child_process: spawnSync() exit code now is null when the child is killed via signal (cjihrig)
[nodejs#11288](nodejs#11288)
* http: new functions to access the headers for an outgoing HTTP message (Brian White)
[nodejs#11562](nodejs#11562)
* lib: deprecate node --debug at runtime (Josh Gavant)
[nodejs#11275](nodejs#11275)
* tls: new tls.TLSSocket() supports sec ctx options (Sam Roberts)
[nodejs#11005](nodejs#11005)
* url: adding URL.prototype.toJSON support (Michaël Zasso)
[nodejs#11236](nodejs#11236)
italoacasas added a commit to italoacasas/node that referenced this pull request Feb 28, 2017
Notables changes:

* child_process: spawnSync() exit code now is null when the child is killed via signal (cjihrig)
[nodejs#11288](nodejs#11288)
* http: new functions to access the headers for an outgoing HTTP message (Brian White)
[nodejs#11562](nodejs#11562)
* lib: deprecate node --debug at runtime (Josh Gavant)
[nodejs#11275](nodejs#11275)
* tls: new tls.TLSSocket() supports sec ctx options (Sam Roberts)
[nodejs#11005](nodejs#11005)
* url: adding URL.prototype.toJSON support (Michaël Zasso)
[nodejs#11236](nodejs#11236)
* doc: items in the API documentation may now have changelogs (Anna Henningsen)
[nodejs#11489](nodejs#11489)

PR-URL: nodejs#11553
italoacasas added a commit to italoacasas/node that referenced this pull request Mar 1, 2017
Notables changes:

* child_process: spawnSync() exit code now is null when the child is killed via signal (cjihrig)
[nodejs#11288](nodejs#11288)
* http: new functions to access the headers for an outgoing HTTP message (Brian White)
[nodejs#11562](nodejs#11562)
* lib: deprecate node --debug at runtime (Josh Gavant)
[nodejs#11275](nodejs#11275)
* tls: new tls.TLSSocket() supports sec ctx options (Sam Roberts)
[nodejs#11005](nodejs#11005)
* url: adding URL.prototype.toJSON support (Michaël Zasso)
[nodejs#11236](nodejs#11236)
* doc: items in the API documentation may now have changelogs (Anna Henningsen)
[nodejs#11489](nodejs#11489)
* crypto: adding support for OPENSSL_CONF again (Sam Roberts)
[nodejs#11006](nodejs#11006)
* src: adding support for trace-event tracing (misterpoe)
[nodejs#11106](nodejs#11106)

PR-URL: nodejs#11553
italoacasas added a commit to italoacasas/node that referenced this pull request Mar 1, 2017
Notables changes:

* child_process: spawnSync() exit code now is null when the child is killed via signal (cjihrig)
[nodejs#11288](nodejs#11288)
* http: new functions to access the headers for an outgoing HTTP message (Brian White)
[nodejs#11562](nodejs#11562)
* lib: deprecate node --debug at runtime (Josh Gavant)
[nodejs#11275](nodejs#11275)
* tls: new tls.TLSSocket() supports sec ctx options (Sam Roberts)
[nodejs#11005](nodejs#11005)
* url: adding URL.prototype.toJSON support (Michaël Zasso)
[nodejs#11236](nodejs#11236)
* doc: items in the API documentation may now have changelogs (Anna Henningsen)
[nodejs#11489](nodejs#11489)
* crypto: adding support for OPENSSL_CONF again (Sam Roberts)
[nodejs#11006](nodejs#11006)
* src: adding support for trace-event tracing (misterpoe)
[nodejs#11106](nodejs#11106)

PR-URL: nodejs#11553
italoacasas added a commit to italoacasas/node that referenced this pull request Mar 1, 2017
Notables changes:

* child_process: spawnSync() exit code now is null when the child is killed via signal (cjihrig)
[nodejs#11288](nodejs#11288)
* http: new functions to access the headers for an outgoing HTTP message (Brian White)
[nodejs#11562](nodejs#11562)
* lib: deprecate node --debug at runtime (Josh Gavant)
[nodejs#11275](nodejs#11275)
* tls: new tls.TLSSocket() supports sec ctx options (Sam Roberts)
[nodejs#11005](nodejs#11005)
* url: adding URL.prototype.toJSON support (Michaël Zasso)
[nodejs#11236](nodejs#11236)
* doc: items in the API documentation may now have changelogs (Anna Henningsen)
[nodejs#11489](nodejs#11489)
* crypto: adding support for OPENSSL_CONF again (Sam Roberts)
[nodejs#11006](nodejs#11006)
* src: adding support for trace-event tracing (misterpoe)
[nodejs#11106](nodejs#11106)

PR-URL: nodejs#11553
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
    Notables changes:

    * child_process: spawnSync() exit code now is null when the child is killed via signal (cjihrig)
    [#11288](nodejs/node#11288)
    * http: new functions to access the headers for an outgoing HTTP message (Brian White)
    [#11562](nodejs/node#11562)
    * lib: deprecate node --debug at runtime (Josh Gavant)
    [#11275](nodejs/node#11275)
    * tls: new tls.TLSSocket() supports sec ctx options (Sam Roberts)
    [#11005](nodejs/node#11005)
    * url: adding URL.prototype.toJSON support (Michaël Zasso)
    [#11236](nodejs/node#11236)
    * doc: items in the API documentation may now have changelogs (Anna Henningsen)
    [#11489](nodejs/node#11489)
    * crypto: adding support for OPENSSL_CONF again (Sam Roberts)
    [#11006](nodejs/node#11006)
    * src: adding support for trace-event tracing (misterpoe)
    [#11106](nodejs/node#11106)

    PR-URL: nodejs/node#11553

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants