Skip to content

inspector: enable --inspect-brk in v6#12615

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.

Sign up for GitHub

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

Jump to bottom
Closed
refack wants to merge 1 commit into from
Closed

inspector: enable --inspect-brk in v6 #12615

refack wants to merge 1 commit into from

Conversation

@refack
Copy link
Contributor

@refackrefack commented Apr 24, 2017•
edited
Loading

--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

@nodejs-github-botnodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. debugger inspector Issues and PRs related to the V8 inspector protocol labels Apr 24, 2017
@refackrefack added wip Issues and PRs that are still a work in progress. backported-to-v6.x and removed build Issues and PRs related to build files or the CI. v6.x labels Apr 24, 2017
@refackrefack self-assigned this Apr 24, 2017
@refackrefack changed the title inspector: backport --inspect-brk[wip] inspector: backport --inspect-brkApr 24, 2017
@refackrefack mentioned this pull request Apr 24, 2017
Closed
@refackrefackforce-pushed the v6-inspector-brk branch 2 times, most recently from 979d02a to 4e8ae63CompareApril 24, 2017 03:55
@refackrefack removed the wip Issues and PRs that are still a work in progress. label Apr 24, 2017
@refackrefack changed the title [wip] inspector: backport --inspect-brkinspector: backport --inspect-brkApr 24, 2017
@refackrefack changed the title inspector: backport --inspect-brkinspector: implement --inspect-brkApr 24, 2017
@refackrefack changed the title inspector: implement --inspect-brkinspector: enable --inspect-brkApr 24, 2017
@mscdexmscdex added wip Issues and PRs that are still a work in progress. v6.x and removed wip Issues and PRs that are still a work in progress. backported-to-v6.x labels Apr 24, 2017
@MylesBorinsMylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 24, 2017
@refack
Copy link
ContributorAuthor

refack commented Apr 24, 2017

CI: https://ci.nodejs.org/job/node-test-commit/9373/

@refackrefack changed the title inspector: enable --inspect-brkinspector: enable --inspect-brk in v6Apr 24, 2017
@refack
Copy link
ContributorAuthor

refack commented Apr 24, 2017

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

@refack
Copy link
ContributorAuthor

refack commented Apr 24, 2017

CI fails because of #12540

@gibfahn
Copy link
Member

gibfahn commented Apr 24, 2017•
edited
Loading

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
Copy link
ContributorAuthor

refack commented Apr 24, 2017•
edited
Loading

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.

@mhdawsonmhdawson mentioned this pull request Aug 28, 2017
Closed
@MylesBorinsMylesBorinsforce-pushed the v6.x-staging branch 2 times, most recently from aaf4e13 to 31f572cCompareSeptember 5, 2017 16:50
@refackrefack removed their assignment Sep 8, 2017
@gibfahngibfahn mentioned this pull request Sep 16, 2017
Closed
@MylesBorins
Copy link
Contributor

MylesBorins commented Sep 19, 2017

Needs a rebase.

Committee agreed it can go in

@TrottTrott removed the blocked PRs that are blocked by other issues or PRs. label Sep 19, 2017
@sam-github
Copy link
Contributor

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.

@MylesBorins
Copy link
Contributor

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
Copy link
Contributor

sam-github commented Sep 20, 2017

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

@refack
Copy link
ContributorAuthor

refack commented Sep 20, 2017

Rebases cleanly indeed.
https://ci.nodejs.org/job/node-test-pull-request/10168/

MylesBorins pushed a commit that referenced this pull request Oct 5, 2017
@refack@MylesBorins
inspector: enable --inspect-brk
5db4c6a
PR-URL: #12615 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

MylesBorins commented Oct 5, 2017

Landed in 5db4c6a

@refackrefack deleted the v6-inspector-brk branch October 5, 2017 02:58
@refack
Copy link
ContributorAuthor

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

@MylesBorinsMylesBorins mentioned this pull request Oct 17, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
@refack@MylesBorins
inspector: enable --inspect-brk
e31ab7c
PR-URL: #12615 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Nov 3, 2017
Merged
MylesBorins added a commit that referenced this pull request Nov 6, 2017
@MylesBorins
2017-11-07, Version 6.12.0 'Boron' (LTS)
c6a2ec5
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
@MylesBorins
2017-11-07, Version 6.12.0 'Boron' (LTS)
70dc6d5
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
@snyk-botsnyk-bot mentioned this pull request Mar 18, 2020
Open
@snyk-botsnyk-bot mentioned this pull request Apr 1, 2020
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes

@hybristhybristhybrist approved these changes

+2 more reviewers

@sam-githubsam-githubsam-github approved these changes

@gibfahngibfahngibfahn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.inspectorIssues and PRs related to the V8 inspector protocolsemver-minorPRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@refack@gibfahn@sam-github@hybrist@MylesBorins@Trott@jasnell@mscdex@nodejs-github-bot