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

src: Remove support for --debug #12197

Closed
wants to merge 2 commits into
base: master
from

Conversation

@jkrems
Contributor

jkrems commented Apr 3, 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)

doc, src, test

I wasn't 100% sure how to deal with the tests. For now I just deleted them but I would open a ticket to investigate which of them should be ported to comparable --inspect tests.

@jkrems

This comment has been minimized.

Show comment
Hide comment
@jkrems

jkrems Apr 3, 2017

Contributor

There's a lot of code in src/node.cc that is effectively dead now. But it would be a lot less disruptive (~ prone to conflicts) to remove it after #11431 lands, so I'd like to keep it around for now.

Contributor

jkrems commented Apr 3, 2017

There's a lot of code in src/node.cc that is effectively dead now. But it would be a lot less disruptive (~ prone to conflicts) to remove it after #11431 lands, so I'd like to keep it around for now.

@jkrems jkrems referenced this pull request Apr 3, 2017

Closed

Switch the CLI debugger to V8 inspector #11421

14 of 14 tasks complete
@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Apr 3, 2017

Member

I wonder if test-cluster-disconnect-handles.js should be rewritten. (Not necessarily in this PR.) Thoughts?

The other tests that are removed seem like they can go.

Maybe the test-cluster-disconnect-handles.js test can be moved to known_issues until it is rewritten to work without the debugger protocol.

Member

Trott commented Apr 3, 2017

I wonder if test-cluster-disconnect-handles.js should be rewritten. (Not necessarily in this PR.) Thoughts?

The other tests that are removed seem like they can go.

Maybe the test-cluster-disconnect-handles.js test can be moved to known_issues until it is rewritten to work without the debugger protocol.

@jkrems

This comment has been minimized.

Show comment
Hide comment
@jkrems

jkrems Apr 3, 2017

Contributor

At first I had moved all tests to test/disabled/* (before I turned it into deletions). There's definitely value in skimming through those tests to make sure we're not losing coverage. E.g. I'm not sure we have the same coverage now for --inspect that we used to have for --debug. @ofrobots did some of that work already.

My current proposal would be to go through these tests in a follow-up and port the ones that would still make sense. Happy to already move one or more to known_issues/ if we identify ones that definitely should be preserved in some form.

Contributor

jkrems commented Apr 3, 2017

At first I had moved all tests to test/disabled/* (before I turned it into deletions). There's definitely value in skimming through those tests to make sure we're not losing coverage. E.g. I'm not sure we have the same coverage now for --inspect that we used to have for --debug. @ofrobots did some of that work already.

My current proposal would be to go through these tests in a follow-up and port the ones that would still make sense. Happy to already move one or more to known_issues/ if we identify ones that definitely should be preserved in some form.

@ofrobots

LGTM. Thanks for putting together this PR! The test changes would overlap/conflict with #11441, so it would make sense to land this first and rework the tests in either #11441 or a follow on.

@addaleax addaleax added this to the 8.0.0 milestone Apr 4, 2017

@Trott

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

LGTM
Enjoy seeing all those removed lines!!!

@jkrems

This comment has been minimized.

Show comment
Hide comment
@jkrems

jkrems Apr 4, 2017

Contributor

Scanning some of the other recent PR builds, the windows failure doesn't seem to be related to my change.

not ok 32 parallel/test-cli-syntax
  ---
  duration_ms: 3.250
  severity: fail
  stack: |-
    
    assert.js:82
      throw new assert.AssertionError({
      ^
    AssertionError: 'c:\\workspace\\node-test-binary-windows\\RUN_SUBSET\\3\\VS_VERSION\\vcbt2015\\label\\win10\\Release\\node.exe: either --check o === 'c:\\workspace\\node-test-binary-windows\\RUN_SUBSET\\3\\VS_VERSION\\vcbt2015\\label\\win10\\Release\\node.exe: either --check o
        at c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vcbt2015\label\win10\test\parallel\test-cli-syntax.js:127:12
        at Array.forEach (native)
        at c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vcbt2015\label\win10\test\parallel\test-cli-syntax.js:123:20
        at Array.forEach (native)
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vcbt2015\label\win10\test\parallel\test-cli-syntax.js:122:19)
        at Module._compile (module.js:607:30)
        at Object.Module._extensions..js (module.js:618:10)
        at Module.load (module.js:516:32)
        at tryModuleLoad (module.js:466:12)
        at Function.Module._load (module.js:458:3
Contributor

jkrems commented Apr 4, 2017

Scanning some of the other recent PR builds, the windows failure doesn't seem to be related to my change.

not ok 32 parallel/test-cli-syntax
  ---
  duration_ms: 3.250
  severity: fail
  stack: |-
    
    assert.js:82
      throw new assert.AssertionError({
      ^
    AssertionError: 'c:\\workspace\\node-test-binary-windows\\RUN_SUBSET\\3\\VS_VERSION\\vcbt2015\\label\\win10\\Release\\node.exe: either --check o === 'c:\\workspace\\node-test-binary-windows\\RUN_SUBSET\\3\\VS_VERSION\\vcbt2015\\label\\win10\\Release\\node.exe: either --check o
        at c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vcbt2015\label\win10\test\parallel\test-cli-syntax.js:127:12
        at Array.forEach (native)
        at c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vcbt2015\label\win10\test\parallel\test-cli-syntax.js:123:20
        at Array.forEach (native)
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vcbt2015\label\win10\test\parallel\test-cli-syntax.js:122:19)
        at Module._compile (module.js:607:30)
        at Object.Module._extensions..js (module.js:618:10)
        at Module.load (module.js:516:32)
        at tryModuleLoad (module.js:466:12)
        at Function.Module._load (module.js:458:3
@jasnell

jasnell approved these changes Apr 4, 2017

@cjihrig

cjihrig approved these changes Apr 4, 2017

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Apr 4, 2017

Member

Scanning some of the other recent PR builds, the windows failure doesn't seem to be related to my change.

Should be fixed in #12212

Member

gibfahn commented Apr 4, 2017

Scanning some of the other recent PR builds, the windows failure doesn't seem to be related to my change.

Should be fixed in #12212

@joshgav

This comment has been minimized.

Show comment
Hide comment
@joshgav

joshgav Apr 4, 2017

Member

Would it perhaps be better to port the tests to --inspect rather than deleting and adding them back?

Member

joshgav commented Apr 4, 2017

Would it perhaps be better to port the tests to --inspect rather than deleting and adding them back?

@jkrems

This comment has been minimized.

Show comment
Hide comment
@jkrems

jkrems Apr 4, 2017

Contributor

@joshgav The idea is to have a way to land the removal now so we can include it comfortably in 8.x. Some of those tests are already duplicated by existing tests, some will be replaced by new --inspect tests, and pulling that apart correctly is something I'd rather not do under time pressure (as would be the case with this PR).

P.S.: Also there's overlap with test changes as part of #11441, making things even more complicated.

Contributor

jkrems commented Apr 4, 2017

@joshgav The idea is to have a way to land the removal now so we can include it comfortably in 8.x. Some of those tests are already duplicated by existing tests, some will be replaced by new --inspect tests, and pulling that apart correctly is something I'd rather not do under time pressure (as would be the case with this PR).

P.S.: Also there's overlap with test changes as part of #11441, making things even more complicated.

@targos

targos approved these changes Apr 6, 2017

@targos targos self-assigned this Apr 6, 2017

targos added a commit to targos/node that referenced this pull request Apr 6, 2017

src: remove support for --debug
In the 2017-04-05 meeting, the CTC agreed to remove support for the
legacy debugger in 8.0.0. This is the first step in this direction.

Refs: nodejs/CTC#94
PR-URL: nodejs#12197
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Apr 6, 2017

Member

Landed in 47f8f74

Member

targos commented Apr 6, 2017

Landed in 47f8f74

@targos targos closed this Apr 6, 2017

@jkrems jkrems deleted the jkrems:jk-rm-debug-flag-minimal branch Apr 6, 2017

MylesBorins added a commit that referenced this pull request Apr 18, 2017

test: skip irrelevant test on Windows
Skip test-cluster-disconnect-handles on Windows.

PR-URL: #12261
Ref: #12197 (comment)
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins added a commit that referenced this pull request Apr 19, 2017

test: skip irrelevant test on Windows
Skip test-cluster-disconnect-handles on Windows.

PR-URL: #12261
Ref: #12197 (comment)
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

@sam-github sam-github referenced this pull request Apr 22, 2017

Closed

inspector: restore --debug-brk alias #12580

0 of 4 tasks complete

sam-github added a commit to sam-github/node that referenced this pull request Apr 22, 2017

inspector: restore --debug-brk alias
--inspect-brk didn't exist prior to 7.6.0, and --debug-brk doesn't exist
after nodejs#12197, leaving no consistent
way to start node with inspector activated and breaking on first line.
Add --debug-brk back in as an undocumented option until 7.x is no longer
supported.

Fixes: nodejs#12364

jkrems added a commit to jkrems/node that referenced this pull request May 4, 2017

inspector: restore --debug-brk alias
--inspect-brk didn't exist prior to 7.6.0, and --debug-brk doesn't exist
after nodejs#12197, leaving no consistent
way to start node with inspector activated and breaking on first line.
Add --debug-brk back in as an undocumented option until 7.x is no longer
supported.

Fixes: nodejs#12364

@jasnell jasnell referenced this pull request May 11, 2017

Closed

8.0.0 Release Proposal #12220

andrew749 added a commit to michielbaird/node that referenced this pull request Jul 19, 2017

test: skip irrelevant test on Windows
Skip test-cluster-disconnect-handles on Windows.

PR-URL: nodejs/node#12261
Ref: nodejs/node#12197 (comment)
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment