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

inspector: add --inspect-brk flag #8979

Merged
merged 1 commit into from
Jan 3, 2017
Merged

Conversation

joshgav
Copy link
Contributor

@joshgav joshgav commented Oct 7, 2016

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

inspector

Description of change

add --inspect-brk flag to provide same behavior as --debug-brk flag but for inspector. This allows complete separation of inspector and debugger (old debugger) flags.

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 7, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Oct 7, 2016

If this gets merged, it should have tests.

@jasnell jasnell added the wip Issues and PRs that are still a work in progress. label Oct 7, 2016
@eugeneo
Copy link
Contributor

eugeneo commented Oct 7, 2016

@mscdex mscdex added the inspector Issues and PRs related to the V8 inspector protocol label Oct 7, 2016
@joshgav joshgav force-pushed the inspect-brk branch 2 times, most recently from 5f41374 to 24d5e39 Compare October 11, 2016 20:41
@joshgav joshgav removed the wip Issues and PRs that are still a work in progress. label Oct 11, 2016
@@ -13,7 +13,7 @@ if (cluster.isMaster) {

function fork(offset, execArgv) {
if (execArgv)
cluster.setupMaster({execArgv});
cluster.setupMaster({args: execArgv});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strangely it didn't work for me without it. Let me check again though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay figured out this one for the moment and fixed in latest commit. Now I need to figure out how to run code inside the worker process - which I'm not sure how to do since it breaks immediately with --inspect-brk, orphaning the child. Any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshgav I think you'd have to connect to the debugger to move past the break. There are other tests that exercise --debug-brk, but I don't think any that do it with the cluster module. Maybe this would be better to move to a separate test.

@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
@nojvek
Copy link

nojvek commented Oct 20, 2016

Should this be visible in in the help text of node -h. Currently inspect is insider knowledge and not discoverable. Inspector has been merged in node for a while, is its status still experimental?

@joshgav joshgav closed this Dec 15, 2016
@joshgav joshgav reopened this Dec 15, 2016
joshgav added a commit to joshgav/node that referenced this pull request Dec 15, 2016
add an --inspect-brk option which breaks on
first line of user script. same behavior as old
--debug-brk flag.

PR-URL: nodejs#8979
Reviewed-By: <tbd>
Reviewed-By: <tbd>
@joshgav
Copy link
Contributor Author

joshgav commented Dec 15, 2016

Rebased, moved test to a better place, and added docs to command-line help.

@cjihrig @ofrobots @eugeneo LGTY? Thanks!

@joshgav
Copy link
Contributor Author

joshgav commented Jan 3, 2017

CI passed, going to land: https://ci.nodejs.org/job/node-test-pull-request/5690/

add an --inspect-brk option which breaks on
first line of user script. same behavior as old
--debug-brk flag.

PR-URL: nodejs#8979
Reviewed-By: Eugene Ostroukhov <eostroukhov@chromium.org>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@keybase.io>
@joshgav
Copy link
Contributor Author

joshgav commented Jan 3, 2017

landed in 6ff3b03

@targos
Copy link
Member

targos commented Jan 28, 2017

This is semver-minor, right? Adding the label.

@targos targos added semver-minor PRs that contain new features and should be released in the next minor version. dont-land-on-v4.x labels Jan 28, 2017
@italoacasas
Copy link
Contributor

This is not landing clearly in v7, there is plan to backport this ?

@joshgav
Copy link
Contributor Author

joshgav commented Feb 3, 2017

@italoacasas backported in #11149, which should be followed with #11114 to fix the misaligned help text. Would you do the landing on the 7.x branch, or would I do that? Thanks!

@italoacasas
Copy link
Contributor

italoacasas commented Feb 4, 2017

@joshgav for the v7.x-staging branch I think everyone can land the backport PR after the proper review. At this point is like landing something in master.

@jasnell jasnell mentioned this pull request Apr 4, 2017
" --inspect[=host:port] activate inspector on host:port\n"
" (default: 127.0.0.1:9229)\n"
" --inspect-brk[=host:port] activate inspector on host:port\n"
" and break at start of user script\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--inspect-port was omitted from the docs, see #12581

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol 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.