Skip to content
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

Merged
merged 1 commit into from
Jul 25, 2019

Conversation

perspectivus1
Copy link
Contributor

Same as #5492, but forking instead of spawning.

Note that in this implementation, I get this error when trying to debug user code:

Starting inspector on 127.0.0.1:24166 failed: address already in use

@paul-marechal
Copy link
Member

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.

@perspectivus1
Copy link
Contributor Author

The issue was indeed resolved. I checked it on Electron on Windows. I also checked browser build on Ubuntu backend.

@akosyakov
Copy link
Member

@akosyakov akosyakov added the debug issues that related to debug functionality label Jul 19, 2019
Signed-off-by: Eyal Barlev <eyal.barlev@sap.com>
@perspectivus1
Copy link
Contributor Author

@akosyakov, are we good to go? Is more cleanup needed?

@akosyakov
Copy link
Member

@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.

@akosyakov
Copy link
Member

@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?

@paul-marechal
Copy link
Member

paul-marechal commented Jul 24, 2019

@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;
Copy link
Member

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

Copy link
Member

@akosyakov akosyakov left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debug issues that related to debug functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants