-
Notifications
You must be signed in to change notification settings - Fork 41
VS Code debugger #248
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
VS Code debugger #248
Conversation
This is fantastic! Thank you @tschneidereit and @itowlson for all your hard work on this feature. |
@andreiltd thank you for your kind words! Since Ivan and I both did meaningful parts of the work here, it'd be fantastic if you could do a proper review of the whole PR. I'll then do the work to address whatever feedback you'll have, and we can get this landed. |
debugger/vscode-dap-extension/src/activateStarlingMonkeyDebugger.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments. IIUC, the debugging server in its current form does not support multiple sessions. Is that correct? If so, we should probably explicitly raise an error when attempting to create parallel sessions?
Yes, that's a limitation of the current implementation (I think @tschneidereit had plans for multi-component debugging, which we know we'll need, but wanted to get something out there in the meantime). I'll look at detecting and erroring. |
This commit introduces a substantial new feature: debugging support via the [Debug Adapter Protocol](https://microsoft.github.io/debug-adapter-protocol/). While for now the debugger is experimental and not quite ready for production use, it already covers a decent amount of functionality: it supports breakpoints, single-stepping into and over function calls, stack and (local and global) scope inspection, and modifying locals and object properties. Building on the platform support added in bytecodealliance#218, the extension has two key components: 1. a DAP implementation as a Node.js script that starts StarlingMonkey, starts a server for the debug session, and translates between the messages StarlingMonkey sends/receives and the DAP 2. a VS Code extension providing launch configurations and settings for the debugger, and initiates the debug session
Signed-off-by: Till Schneidereit <[email protected]>
Signed-off-by: itowlson <[email protected]>
Signed-off-by: itowlson <[email protected]>
Signed-off-by: itowlson <[email protected]>
Signed-off-by: itowlson <[email protected]>
Signed-off-by: itowlson <[email protected]>
5347a94
to
4da734a
Compare
Signed-off-by: itowlson <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀
…included Signed-off-by: itowlson <[email protected]>
We had an issue where the packaged extension was failing to activate because the |
|
||
**`ts` workspace:** `npm run compile` | ||
|
||
**`spin-ts` workspace:** `spin build` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This (and the folder it refers to) would be good to remove before landing. I think that really boils down to removing this line and the folder, but there might be other things I'm missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tschneidereit Where would you suggest we keep tests for non-Wasmtime hosts? I think there's value in such tests existing. For myself, I'd find it useful for them to be present in this repo, even if not everybody is able to run all of them, but happy to move them off to the other hosts' host orgs if preferred. (Although if we did that it would still be useful to link them from here.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such tests should indeed exist, yes! Given that we'll want to do integration with the Spin SDK more generally, I think the tests should logically live there, though. Additionally, hosting downstream embedding-specific tests here would open a can of worms, where it's not clear which ones we'd accept here and which we wouldn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small tweak remaining, then we should go ahead and land this! 🥳
Signed-off-by: itowlson <[email protected]>
Supersedes #219, on which it is based with a few tweaks / features / bug fixes (with @tschneidereit's involvement and consent).