Skip to content
This repository was archived by the owner on Feb 1, 2022. It is now read-only.

Prefer --inspect-brk over --debug-brk#43

Merged
hybrist merged 1 commit intonodejs:masterfrom
ofrobots:inspect-brk
Apr 6, 2017
Merged

Prefer --inspect-brk over --debug-brk#43
hybrist merged 1 commit intonodejs:masterfrom
ofrobots:inspect-brk

Conversation

@ofrobots
Copy link
Copy Markdown
Contributor

@ofrobots ofrobots commented Apr 6, 2017

Node 8.x no longer has --debug-brk and --inspect-brk is backwards
compatible with Node 7.

Ref: nodejs/node#12197.

I will be cherry-picking this on nodejs/node#11441.

hybrist
hybrist reviewed Apr 6, 2017
View reviewed changes
Comment thread lib/_inspect.js Outdated
@@ -98,7 +98,7 @@ function runScript(script, scriptArgs, inspectHost, inspectPort, childPrint) {
return new Promise((resolve) => {
const args = [
'--inspect',
Copy link
Copy Markdown
Collaborator

@hybrist hybrist Apr 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

If we make this change, should this just be --inspect-brk=${inspectPort}?

P.S.: What I mean is that we don't need to pass --inspect anymore.

Copy link
Copy Markdown
Contributor Author

@ofrobots ofrobots Apr 6, 2017

Choose a reason for hiding this comment

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

Ack.

@hybrist
Copy link
Copy Markdown
Collaborator

hybrist commented Apr 6, 2017 •
edited
Loading

Just looked it up: This will drop support for node <7.6.0. Will publish as major for the release to npm.

@ofrobots
Copy link
Copy Markdown
Contributor Author

ofrobots commented Apr 6, 2017

Is it possible to take a dependency on semver for this module? If not, I can use regexes to avoid the major due to <7.6.0.

@hybrist
Copy link
Copy Markdown
Collaborator

hybrist commented Apr 6, 2017

I would prefer delaying the pain of getting 3rd party modules working in our bundled build (where we get away with just dumping the files into node right now). So 👍 for a regex solution even though it's not "perfectly clean".

@ofrobots
fix: use --inspect-brk with Node 8+
2c1ed27 
Node 8.x no longer has --debug-brk.
@ofrobots
Copy link
Copy Markdown
Contributor Author

ofrobots commented Apr 6, 2017

Done. PTAL.

hybrist
hybrist approved these changes Apr 6, 2017
View reviewed changes
Copy link
Copy Markdown
Collaborator

@hybrist hybrist left a comment

Choose a reason for hiding this comment

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

🎉

@hybrist
Copy link
Copy Markdown
Collaborator

hybrist commented Apr 6, 2017

Passes locally, CI: https://ci.nodejs.org/job/node-inspect-continuous-integration/34/

@hybrist hybrist changed the title prefer --inspect-brk over --debug-brk Prefer --inspect-brk over --debug-brk Apr 6, 2017
@hybrist
Copy link
Copy Markdown
Collaborator

hybrist commented Apr 6, 2017

That OSX failure looks scary but I'd blame the latest nightly rather than this change:

node::FatalException(v8::Isolate*, v8::Local<v8::Value>, v8::Local<v8::Message>) [/Users/iojs/build/workspace/node-inspect-continuous-integration/MACHINE/osx1010/node-v8.0.0-nightly20170405491d59da84-darwin-x64/bin/node]
3:  < v8::V8::ToLocalEmpty() [/Users/iojs/build/workspace/node-inspect-continuous-integration/MACHINE/osx1010/node-v8.0.0-nightly20170405491d59da84-darwin-x64/bin/node]
4: node::inspector::AgentImpl::CallAndPauseOnStart(v8::FunctionCallbackInfo<v8::Value> const&) [/Users/iojs/build/workspace/node-inspect-continuous-integration/MACHINE/osx1010/node-v8.0.0-nightly20170405491d59da84-darwin-x64/bin/node]
5: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [/Users/iojs/build/workspace/node-inspect-continuous-integration/MACHINE/osx1010/node-v8.0.0-nightly20170405491d59da84-darwin-x64/bin/node]
6: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/Users/iojs/build/workspace/node-inspect-continuous-integration/MACHINE/osx1010/node-v8.0.0-nightly20170405491d59da84-darwin-x64/bin/node]
7:  < v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/Users/iojs/build/workspace/node-inspect-continuous-integration/MACHINE/osx1010/node-v8.0.0-nightly20170405491d59da84-darwin-x64/bin/node]

@hybrist hybrist merged commit 8ef696d into nodejs:master Apr 6, 2017
ofrobots added a commit to ofrobots/node that referenced this pull request Apr 12, 2017
@ofrobots
deps: cherry-pick node-inspect#43
b7608ac 
Node 8.x no longer has --debug-brk.

Ref: nodejs/node-inspect#43
PR-URL: nodejs#11441
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
Reviewed-By: joshgav - Josh Gavant <[email protected]>
Reviewed-By: cjihrig - Colin Ihrig <[email protected]>
Reviewed-By: targos - Michaël Zasso <[email protected]>
@hybrist hybrist mentioned this pull request Apr 12, 2017
Closed
4 tasks
This was referenced Apr 23, 2017
Closed
Closed
@hybrist hybrist mentioned this pull request Apr 26, 2017
Closed
@janlazo janlazo mentioned this pull request Dec 10, 2017
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@hybrist hybrist hybrist approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ofrobots @hybrist