deps: Add node-inspect

[jkrems]

Checklist

• make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

deps

Description of change

This adds a reimplementation of the old CLI debugger (node debug)
against the new debugger protocol (node --inspect). This is necessary
because the old protocol won't be supported in future versions of V8.

This just adds an additional make target (make test-node-inspect) but
will not include the new debugger in releases.

Fixes #7266

[jkrems]

This PR is supposed to start a conversation around both the code itself and how the debugger should be exposed.

Notes and references

• Tracking issue in nodejs/diagnostics
• The changeset could be reduced if we don't want to run the node-inspect test suite in node by restricting it to what would be published to npm.

The deps/node-inspect files were generated via:

rm -rf deps/node-inspect node-inspect-* && curl -sSL "https://github.com/buggerjs/node-inspect/archive/v1.10.4.tar.gz" | tar -xzvf - && mv node-inspect-* deps/node-inspect

[Trott]

Ideally, there's be a similar job for Windows in vcbuild.bat.

[jkrems]

Yep, didn't have a Windows machine set up to look into vcbuild.bat, that's why I only copied tools/test-npm.sh in the first push. Should be able to work on the Windows part next week.

[jkrems]

Windows part is done.

[bnoordhuis]

Didn't look too closely yet but this only checks in the files without further integration? That's not very useful by itself so what (and when) is the next step?

I agree with Rich that the tests should run on Windows so you should probably turn the shell script into a node script. Ideally, it gets integrated with tools/test.py somehow but I don't quite know how yet.

The fact that the test runner copies around files and runs npm install is less than pleasing. Better integration with the rest of the tree would let you drop the dependency on the tap and eslint packages and I don't think nlm is necessary in-tree.

[addaleax]

Btw, it might be quite helpful for reviewing to put the additions to deps/ into a separate commit?

[jkrems]

@addaleax Good point. Split the change into two commits. One for the bulk deps/ change, one for adding the make target and copying tools/test-npm.sh.

[jkrems]

That's not very useful by itself so what (and when) is the next step?

I agree that just enabling the tests is of limited use. It gets us "master doesn't break the CLI debugger" but not "bundle the CLI debugger". The latter would be the next step - and I would say it comes as soon as we agree that the CLI debugger is ready to ship with node. I'm hoping that this PR will surface any blockers for that 2nd step.

The fact that the test runner copies around files and runs npm install is less than pleasing.

Agreed. Honest answer: I just blindly copied whatever was done for the npm integration. I'm not super familiar with node's build setup (and where it's heading), so any pointers would be appreciated if this is not the right direction.

Better integration with the rest of the tree would let you drop the dependency on the tap and eslint packages and I don't think nlm is necessary in-tree.

I believe the idea is to keep node-inspect as a standalone project that can be developed out-of-tree. I had a version that was pulling in even more parts of node's build setup. I honestly regret even keeping the custom eslint rules etc.. It adds a lot of churn trying to keep the files in sync when working on the debugger project itself. I'd rather keep node-inspect self-contained and reduce the amount of assumptions / degree of integration into node core's setup.

P.S.: The fact that it currently runs tap explicitly is hopefully temporary while I work through some build issues. The goal should be that it's just npm install && npm test from node's perspective.

[bnoordhuis]

Can tap (the module) produce .tap files? Node's test runner prints tap output to both stdio and a file. The file is then picked up and parsed by the CI.

I suppose you could npm test | tee test-node-inspect.tap in the shell script but that won't work on Windows.

I just blindly copied whatever was done for the npm integration.

I don't think make test-npm runs as part of the regular CI. Perhaps only at release time? Maybe @Fishrock123 knows.

[MylesBorins]

the tap module can definitely output raw tap... just have to select it as a reporter. It is worth noting we are moving towards junit XML for CI... but we will be able to convert the tap stream to xml in real time with @jbergstroem's tap2xml utility

make test-npm is not run in CI.

[bnoordhuis]

But can it run in a tee-like mode? It needs to print to stdio for humans and to a file for machines.

[jkrems]

But can it run in a tee-like mode?

I looked trough the tap docs and it didn't look like tee-support was built-in. But if we continue with the make/vcbuild (or node script) wrapping, we could just do it there.

[gibfahn]

@bnoordhuis test-npm fix for CI is #7867. The script uses tee in bash and Tee-Object in Powershell.

[richardlau]

It probably wouldn't be too hard to write a cross platform tee in Node.js.

[jkrems]

@gibfahn Thanks for that link! I'll see tomorrow morning if I can steal that code for node-inspect purposes.

[gibfahn]

@jkrems Actually if you look at Ben's comment here, he'd rather have a general node script that can test any of our dependencies (that use npm test). So if I rewrite that PR to work for deps/npm, it should be straightforward to get it to work for the inspector as well.

[jkrems]

Got it, should've subscribed to that other PR. It looked stalled so I wasn't sure if we can count on it landing soon.

I assume that node script would basically take a source directory and then run npm install/npm run test-node in a properly sandboxed directory? If so, I could already update node-inspect to expose a test-node script (potentially just an alias for the test script).

[addaleax]

I’ve gone ahead and landed this in b1fc774...fd18b65. Thanks for the PR, @jkrems! 🎉

[joshgav]

Thank you @addaleax @jkrems and everyone who's worked on bringing this in! Great to see it landed and primed to land in 7.6.0 (#11185 (comment)).

I think we now need to decide if/how/when to deprecate or change behavior of node debug myscript.js. A proposal might be:

1. Add a warning message to node debug that it will become an alias to node inspect soon and recommending use of node inspect.
2. Actually make node debug an alias to node inspect, perhaps in the same release which first includes V8 >= 5.8.

Also to be sure we're on the same page, I think we expect updates to "upstream" nodejs/node-inspect to be manually integrated into nodejs/node occasionally via a PR, as we do with npm currently (e.g. #11020).

[jkrems]

I think we expect updates to "upstream" nodejs/node-inspect to be manually integrated into nodejs/node occasionally via a PR, as we do with npm currently

👍

perhaps in the same release which first includes V8 >= 5.8.

Was there a decision on this? IIRC 5.8 was the release that would potentially break the older debug protocol and there was uncertainty about pulling it into node 8.x or not. Anyhow - I would say the alias should, if at all possible, happen before we're forced to drop the old debugger. That way we have some cycles to make additional changes (like the -p <pid> things).

[MylesBorins]

we've opted to not land this on v6.x due to the the inspect being provided being an older version. It could prove difficult to keep this up to date.

Please let me know if we should reconsider

[jkrems]

👍 People on 6.x already have a working bundled CLI debugger & if they really want node-inspect, they can install it from npm.