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: enable --inspect-brk in v6 #12615

Closed
wants to merge 1 commit into
base: v6.x-staging
from

Conversation

Projects
None yet
9 participants
@refack
Member

refack commented Apr 24, 2017

--inspect-brk doesn't exist in v6, leaving no consistent
way to start node with inspector activated and breaking on first line.
Add --inspect-brk[=port] as an undocumented option to v6.x

Fixes: #12364

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)

inspector, debugger

@refack refack self-assigned this Apr 24, 2017

@refack refack changed the title from inspector: backport --inspect-brk to [wip] inspector: backport --inspect-brk Apr 24, 2017

@refack refack removed the in progress label Apr 24, 2017

@refack refack changed the title from [wip] inspector: backport --inspect-brk to inspector: backport --inspect-brk Apr 24, 2017

@refack refack changed the title from inspector: backport --inspect-brk to inspector: implement --inspect-brk Apr 24, 2017

@refack refack changed the title from inspector: implement --inspect-brk to inspector: enable --inspect-brk Apr 24, 2017

@refack

This comment has been minimized.

Show comment
Hide comment
Member

refack commented Apr 24, 2017

@refack refack changed the title from inspector: enable --inspect-brk to inspector: enable --inspect-brk in v6 Apr 24, 2017

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Apr 24, 2017

Member

@MylesBorins why semver-minor if it's undocumented?

Member

refack commented Apr 24, 2017

@MylesBorins why semver-minor if it's undocumented?

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Apr 24, 2017

Member

CI fails because of #12540

Member

refack commented Apr 24, 2017

CI fails because of #12540

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Apr 24, 2017

Member

why semver-minor if it's undocumented?

Why not? It's a new API. We haven't clearly defined whether our semver API is the public interface or the API docs. In any case, is there a reason not to err on the side of caution and make it a semver-minor?

Also why not document it?

Member

gibfahn commented Apr 24, 2017

why semver-minor if it's undocumented?

Why not? It's a new API. We haven't clearly defined whether our semver API is the public interface or the API docs. In any case, is there a reason not to err on the side of caution and make it a semver-minor?

Also why not document it?

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Apr 24, 2017

Member

Why not? It's a new API.

Also why not document it?

Either/or, right?
Document - semver-minor
Undocumented - semver-none

Also why not document it?

Since it's a workaround for proper version detection, and it'll bring up the great big experimental warning...
But I'm ± 0 on that.

Member

refack commented Apr 24, 2017

Why not? It's a new API.

Also why not document it?

Either/or, right?
Document - semver-minor
Undocumented - semver-none

Also why not document it?

Since it's a workaround for proper version detection, and it'll bring up the great big experimental warning...
But I'm ± 0 on that.

@jasnell jasnell added the lts-agenda label Apr 24, 2017

@jasnell

I'm -1 on this until a decision is made by both @nodejs/lts and @nodejs/ctc

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Jul 31, 2017

Member

@refack Is this ready to land? Or are you waiting for more eyes?

I want to backport a test or two, then land.

Member

refack commented Jul 31, 2017

@refack Is this ready to land? Or are you waiting for more eyes?

I want to backport a test or two, then land.

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Jul 31, 2017

Member

Fwiw this cannot land until the LTS group vote on it. It should also not land until we are ready to do a minor

Member

MylesBorins commented Jul 31, 2017

Fwiw this cannot land until the LTS group vote on it. It should also not land until we are ready to do a minor

@jasnell

jasnell approved these changes Aug 1, 2017

LGTM to land when we're ready for a lts minor.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 13, 2017

Member

Needs a rebase. Adding blocked based on #12615 (comment)

Member

Trott commented Aug 13, 2017

Needs a rebase. Adding blocked based on #12615 (comment)

@Trott Trott added the blocked label Aug 13, 2017

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Aug 13, 2017

Member

Not much point rebasing now, it'll be blocked until October when the next minor is planned. It's in nodejs/Release#228 so it'll get reviewed before then.

It's a pity we didn't land this sooner, I've shown about 6 people how to use the inspector, and not having --inspect-brk in 6.x adds needless complexity (they basically have to know about the legacy --debug-brk flag, and they have to always pass 2 flags in).

Member

gibfahn commented Aug 13, 2017

Not much point rebasing now, it'll be blocked until October when the next minor is planned. It's in nodejs/Release#228 so it'll get reviewed before then.

It's a pity we didn't land this sooner, I've shown about 6 people how to use the inspector, and not having --inspect-brk in 6.x adds needless complexity (they basically have to know about the legacy --debug-brk flag, and they have to always pass 2 flags in).

@mhdawson mhdawson referenced this pull request Aug 28, 2017

Closed

Meeting Aug 28th, 2017 #243

@refack refack removed their assignment Sep 8, 2017

@MylesBorins MylesBorins removed the lts-agenda label Sep 19, 2017

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 19, 2017

Member

Needs a rebase.

Committee agreed it can go in

Member

MylesBorins commented Sep 19, 2017

Needs a rebase.

Committee agreed it can go in

@Trott Trott removed the blocked label Sep 19, 2017

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Sep 20, 2017

Member

It rebased clean onto v6.x-staging.

Landed in d9d34be

@MylesBorins I assume we wanted it landed, tell me if not.

Member

sam-github commented Sep 20, 2017

It rebased clean onto v6.x-staging.

Landed in d9d34be

@MylesBorins I assume we wanted it landed, tell me if not.

@sam-github sam-github closed this Sep 20, 2017

@MylesBorins MylesBorins reopened this Sep 20, 2017

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 20, 2017

Member

backed this out. Not planning to land the minors until next month when we are prepping the minor release

Member

MylesBorins commented Sep 20, 2017

backed this out. Not planning to land the minors until next month when we are prepping the minor release

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Sep 20, 2017

Member

OK, though I fear it will just have to be rebased again. c'est la vie.

Member

sam-github commented Sep 20, 2017

OK, though I fear it will just have to be rebased again. c'est la vie.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack
Member

refack commented Sep 20, 2017

MylesBorins added a commit that referenced this pull request Oct 5, 2017

inspector: enable --inspect-brk
PR-URL: #12615
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Oct 5, 2017

Member

Landed in 5db4c6a

Member

MylesBorins commented Oct 5, 2017

Landed in 5db4c6a

@MylesBorins MylesBorins closed this Oct 5, 2017

@refack refack deleted the refack:v6-inspector-brk branch Oct 5, 2017

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Oct 5, 2017

Member

@MylesBorins is there an RC or nightly for the v6.x branch? I'd like to test this with some IDEs

Member

refack commented Oct 5, 2017

@MylesBorins is there an RC or nightly for the v6.x branch? I'd like to test this with some IDEs

@MylesBorins MylesBorins referenced this pull request Oct 17, 2017

Merged

v6.12.0 proposal #16263

MylesBorins added a commit that referenced this pull request Oct 25, 2017

inspector: enable --inspect-brk
PR-URL: #12615
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request Nov 3, 2017

Merged

v4.8.6 proposal #16500

MylesBorins added a commit that referenced this pull request Nov 6, 2017

2017-11-07, Version 6.12.0 'Boron' (LTS)
Notable Changes:

* assert:
  - assert.fail() can now take one or two arguments (Rich Trott)
    #12293
* crypto:
  - add sign/verify support for RSASSA-PSS (Tobias Nießen)
    #11705
* deps:
  - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu)
    #16691
  - upgrade libuv to 1.15.0 (cjihrig)
    #15745
  - upgrade libuv to 1.14.1 (cjihrig)
    #14866
  - upgrade libuv to 1.13.1 (cjihrig)
    #14117
  - upgrade libuv to 1.12.0 (cjihrig)
    #13306
* fs:
  - Add support for fs.write/fs.writeSync(fd, buffer, cb) and
    fs.write/fs.writeSync(fd, buffer, offset, cb) as documented
    (Andreas Lind) #7856
* inspector:
  - enable --inspect-brk (Refael Ackermann)
    #12615
* process:
  - add --redirect-warnings command line argument (James M Snell)
    #10116
* src:
  - allow CLI args in env with NODE_OPTIONS (Sam Roberts)
    #12028)
  - --abort-on-uncaught-exception in NODE_OPTIONS (Sam Roberts)
    #13932
  - allow --tls-cipher-list in NODE_OPTIONS (Sam Roberts)
    #13172
  - use SafeGetenv() for NODE_REDIRECT_WARNINGS (Sam Roberts)
    #12677
* test:
  - remove common.fail() (Rich Trott)
    #12293

PR-URL: #16263

MylesBorins added a commit that referenced this pull request Nov 7, 2017

2017-11-07, Version 6.12.0 'Boron' (LTS)
Notable Changes:

* assert:
  - assert.fail() can now take one or two arguments (Rich Trott)
    #12293
* crypto:
  - add sign/verify support for RSASSA-PSS (Tobias Nießen)
    #11705
* deps:
  - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu)
    #16691
  - upgrade libuv to 1.15.0 (cjihrig)
    #15745
  - upgrade libuv to 1.14.1 (cjihrig)
    #14866
  - upgrade libuv to 1.13.1 (cjihrig)
    #14117
  - upgrade libuv to 1.12.0 (cjihrig)
    #13306
* fs:
  - Add support for fs.write/fs.writeSync(fd, buffer, cb) and
    fs.write/fs.writeSync(fd, buffer, offset, cb) as documented
    (Andreas Lind) #7856
* inspector:
  - enable --inspect-brk (Refael Ackermann)
    #12615
* process:
  - add --redirect-warnings command line argument (James M Snell)
    #10116
* src:
  - allow CLI args in env with NODE_OPTIONS (Sam Roberts)
    #12028)
  - --abort-on-uncaught-exception in NODE_OPTIONS (Sam Roberts)
    #13932
  - allow --tls-cipher-list in NODE_OPTIONS (Sam Roberts)
    #13172
  - use SafeGetenv() for NODE_REDIRECT_WARNINGS (Sam Roberts)
    #12677
* test:
  - remove common.fail() (Rich Trott)
    #12293

PR-URL: #16263

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

2017-11-07, Version 6.12.0 'Boron' (LTS)
Notable Changes:

* assert:
  - assert.fail() can now take one or two arguments (Rich Trott)
    nodejs#12293
* crypto:
  - add sign/verify support for RSASSA-PSS (Tobias Nießen)
    nodejs#11705
* deps:
  - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu)
    nodejs#16691
  - upgrade libuv to 1.15.0 (cjihrig)
    nodejs#15745
  - upgrade libuv to 1.14.1 (cjihrig)
    nodejs#14866
  - upgrade libuv to 1.13.1 (cjihrig)
    nodejs#14117
  - upgrade libuv to 1.12.0 (cjihrig)
    nodejs#13306
* fs:
  - Add support for fs.write/fs.writeSync(fd, buffer, cb) and
    fs.write/fs.writeSync(fd, buffer, offset, cb) as documented
    (Andreas Lind) nodejs#7856
* inspector:
  - enable --inspect-brk (Refael Ackermann)
    nodejs#12615
* process:
  - add --redirect-warnings command line argument (James M Snell)
    nodejs#10116
* src:
  - allow CLI args in env with NODE_OPTIONS (Sam Roberts)
    nodejs#12028)
  - --abort-on-uncaught-exception in NODE_OPTIONS (Sam Roberts)
    nodejs#13932
  - allow --tls-cipher-list in NODE_OPTIONS (Sam Roberts)
    nodejs#13172
  - use SafeGetenv() for NODE_REDIRECT_WARNINGS (Sam Roberts)
    nodejs#12677
* test:
  - remove common.fail() (Rich Trott)
    nodejs#12293

PR-URL: nodejs#16263
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment