-
Notifications
You must be signed in to change notification settings - Fork 29k
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
debug: try to wait for a debugger before running extensions #108141
Conversation
CI failure is an existing issue in master |
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 could not really figure out the purpose of the new arguments. One thing to keep in mind is that we should check if the properties need to be unset or changed when the window reloads, similar to this code:
vscode/src/vs/code/electron-main/window.ts
Lines 751 to 759 in a7caa97
// Some configuration things get inherited if the window is being reloaded and we are | |
// in extension development mode. These options are all development related. | |
if (this.isExtensionDevelopmentHost && cli) { | |
configuration.verbose = cli.verbose; | |
configuration['inspect-extensions'] = cli['inspect-extensions']; | |
configuration['inspect-brk-extensions'] = cli['inspect-brk-extensions']; | |
configuration.debugId = cli.debugId; | |
configuration['extensions-dir'] = cli['extensions-dir']; | |
} |
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 have reviewed the changes in the extension host area.
I would make extHost.protocol.ts
-> IEnvironment.debugExpected
an optional argument and only set it for the web worker extension host. I believe that is the one that needs this special treatment, since the other extension hosts are nodejs based and can be launched with --inspect-brk
and do not need this mechanism.
src/vs/workbench/services/extensions/common/remoteExtensionHost.ts
Outdated
Show resolved
Hide resolved
src/vs/workbench/services/extensions/electron-browser/localProcessExtensionHost.ts
Outdated
Show resolved
Hide resolved
Thanks for the reviews! It looks like in node-debug2 uses --inspect-brk-extensions, but in js-debug at one point (probably while initially getting extension host dev working) that was changed for |
5e09c47
to
827241e
Compare
thanks for the callout -- it looks like |
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.
👍 The changes in extHostExtensionService.ts
and webWorkerExtensionHost.ts
look good to me.
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 thing to consider is to rename debugRenderer
to extensionDebugRenderer
if this is only for extension debugging. That way we keep the properties grouped together that belong to extension debug.
It's also used for debugging webviews. But, VS Code does not know whether the debugger will attach to webviews, extensions or both, it's only asked to expose the renderer. |
@connor4312 I see. I feel |
It's specific to electron renderers. In web we can never debug the 'renderer', as a tab cannot start another tab in debug mode. In theory if running serverful web locally we could actually launch a debugged browser instance on the desktop, but this is probably not worth doing... |
…-ext-host-debugger
Fixes #106698
Fixes #99222
See discussion here: #106698 (comment). This is the same problem we were facing on the web previously, so I implemented it in a way which will solve both.
Most of the changes here are piping data around and adding it to environments. I added a separate
debugExpected
from just theisExtensionDevelopmentDebug
, sinceisExtensionDevelopmentDebug
is true whenever a debugging is enabled at all, which includes running./scripts/code-cli
-- using that as the "wait" signal would causecode-cli
to take an extra 5 seconds to run extensions.Tagging Joh for review seems like you've been touching much of this code most recently according to gitlens.