-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Ensure node-based debug adapters spawn same node exec as Theia #5508
Conversation
packages/debug/src/node/vscode/vscode-debug-adapter-contribution.ts
Outdated
Show resolved
Hide resolved
Please sign the ECA, overall the change looks good :) Did you manage to reproduce the issue with debugging that you mentioned? I would assume it fixed. |
The issue was indeed resolved. I checked it on Electron on Windows. I also checked browser build on Ubuntu backend. |
@perspectivus1 please clean up the history from merge and fix up commits, see https://github.com/theia-ide/theia/blob/ak/pr_template/doc/pull-requests.md#checklist-commit-history |
Signed-off-by: Eyal Barlev <eyal.barlev@sap.com>
@akosyakov, are we good to go? Is more cleanup needed? |
@marechal-p did you check that this variant work both in electron and browser, if so please merge. Please provide a comment next time what your approve means. |
@kittaakos How do you check Electron? Is it enough to check from sources or it should be verified from bundled? Could you give it a try? |
@akosyakov I tested on a windows machine both the browser and electron version, I will let Akos try the bundled version because I just had too many problems trying to port the bundling scripts from theia-apps to the main repo. From my fiddling, I saw that the example packages shouldn't be part of the main workspace, but rather be independent packages so that we can apply the bundling more easily. This is out of scope for this PR, but this is my conclusion for now. |
|
||
if (isForkOptions(processOptions)) { | ||
options.stdio.push('ipc'); | ||
options.env = environment.electron.runAsNodeEnv(); | ||
options.execArgv = (executable as DebugAdapterForkExecutable).execArgv; |
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.
is not it redundant? executable
is already spread into processOptions
here: https://github.com/theia-ide/theia/pull/5508/files#diff-0edd7bfb7a3a6822ad4c2bf344b5f8f1R71
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.
It seems to work, let's merge and see whether someone will complain next days.
It is unfortunate that we don't have easy way to test a bundled application.
Same as #5492, but forking instead of spawning.
Note that in this implementation, I get this error when trying to debug user code: