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.

Sign up for GitHub

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

Jump to bottom

Ensure node-based debug adapters spawn same node exec as Theia #5492

Closed
wants to merge 5 commits into from
Closed

Ensure node-based debug adapters spawn same node exec as Theia #5492

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5c7fbc8
Ensure node-based debug adapter spawn same node as Theia
perspectivus1 Jun 17, 2019
ff1f50f
Merge https://github.com/theia-ide/theia
perspectivus1 Jul 17, 2019
3d01560
Fixed spawning debug processes in Electron
perspectivus1 Jul 17, 2019
f8d930a
Spawn Node.js-based debug adapter using Electron's embedded Node.js
perspectivus1 Jul 17, 2019
452ec24
removing use of runAsNodeEnv(), as we always spawn debug adapters (an…
perspectivus1 Jul 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions packages/debug/src/node/debug-adapter-factory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,11 @@ import {
CommunicationProvider,
DebugAdapterSession,
DebugAdapterSessionFactory,
DebugAdapterFactory
DebugAdapterFactory,
DebugAdapterSpawnExecutable
} from '../common/debug-model';
import { DebugAdapterSessionImpl } from './debug-adapter-session';
import { environment } from '@theia/application-package';

/**
* [DebugAdapterFactory](#DebugAdapterFactory) implementation based on
Expand Down Expand Up @@ -66,8 +68,18 @@ export class LaunchBasedDebugAdapterFactory implements DebugAdapterFactory {
const isForkOptions = (forkOptions: RawForkOptions | any): forkOptions is RawForkOptions =>
!!forkOptions && !!forkOptions.modulePath;

const options: { stdio: (string|number)[], env?: object } = { stdio: ['pipe', 'pipe', 2] };
if (!isForkOptions(executable)) {
if ((executable as DebugAdapterSpawnExecutable).command === 'node') {
if (environment.electron.is()) {
options.env = environment.electron.runAsNodeEnv();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kittaakos, is this what you meant?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks much better now, logically it is correct.

Can we move this Node.js-specific customization to the process-factory?
What's your take on that, @akosyakov?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why execPath is used only in else branch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this Node.js-specific customization to the process-factory?
What's your take on that, @akosyakov?

Do you mean if command === process.execPath? for node i don't think so, it is not the same

I don't think this code should use process factory at all, but it is another issue. User processes (running in user dev env like terminals and task) should be created by a factory, all other should be created vis node js APIs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this Node.js-specific customization to the process-factory?

So, should I move the implementation elsewhere?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

execPath contains the full path of the Electron executable; whereas we want to spawn Electron's embedded node executable.

I am not sure if it is true. But please correct me if I am wrong.
If I have Node.js v10.15.3 available from the PATH and I start a Theia-based electron application which comes with electron 3.1.7 (as of today), then I will run with a different Node.js version than I am expecting.

Akoss-MacBook-Pro:theia akos.kitta$ npx electron -i
> console.log(process.versions.node)
10.2.0
undefined
> .exit

Does this make sense?

Copy link
Contributor

@kittaakos kittaakos Jul 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we want to spawn Electron's embedded node executable.

This part is not true, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When Theia runs in node, we can control the node executable that is used to spawn the debug adapter, because we can infer it from process.execPath.

When Theia runs in Electron, process.execPath contains a full path to the electron executable. Therefore, we have 2 choices:

  1. Spawn a node process using the first node executable found in the PATH environment variable.
  2. Fork an electron process using the ELECTRON_RUN_AS_NODE environment variable, which will use Electron's embedded node.

However, the current implementation of AbstractVSCodeDebugAdapterContribution.provideDebugAdapterExecutable() creates a DebugAdapterExecutable that causes Theia to spawn, rather than fork debug adapters.

In other words, in the case of a node-based debug adapter, the code above always spawn a node process, regardless of whether Theia runs in Electron or in node and regardless of the value of the ELECTRON_RUN_AS_NODE environment variable.

If this is the desired behavior, then this PR fulfills the requirement.

If we wish to use Electron's embedded node, I will have to modify PR #5508.

Please advise

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forking is a proper solution. It is the same way as VS Code does it. I would be in favor of closing this PR and modifying #5508.

} else {
(executable as DebugAdapterSpawnExecutable).command = process.execPath;
}
}
}

const processOptions: RawProcessOptions | RawForkOptions = { ...executable };
const options = { stdio: ['pipe', 'pipe', 2] };

if (isForkOptions(processOptions)) {
options.stdio.push('ipc');
Expand Down