Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Add support for Windows Subsystem Linux #156

Closed
wants to merge 1 commit into from

Conversation

bzoz
Copy link
Contributor

@bzoz bzoz commented Jul 25, 2017

Add a new useWSL boolean attribute to launch arguments. If set to true, debugger will launch Node.js (or given runtimeExecutable) using Windows Subsystem Linux.

When the debugger is instructed to use Linux subsystem it will first test if it is installed. This is done by checking if %WINDIR%\System32\bash.exe (for 64-bit VS Code) or %WINDIR%\SYSNATIVE\bash.exe (for 32-bit VS Code) is present. If found, it is then used as runtimeExecutable, with user-specified runtimeExecutable (with all appropriate parameters) passed as the command to execute (using bash -c parameter). Mapping of the source filenames (the localRoot and remoteRoot attributes) is also automatically adjusted.

For best results, this should be used together with useWSL implementation for the inspector protocol (microsoft/vscode-node-debug2#126)

/cc @yodurr

Add new "useWSL" launch attribute. When used, Bash on Windows will be
used to launch runtimeExecutable. Source files directory mapping will
also be automatically added.
@weinand weinand self-assigned this Jul 28, 2017
@weinand weinand added this to the August 2017 milestone Aug 9, 2017
@weinand
Copy link
Contributor

weinand commented Aug 16, 2017

@bzoz thanks a lot for the PR.

I made a first review pass over the PR and here are some (high-level) comments:

  1. If the WSL logic lives in the debug adapter (DA), it cannot be shared between different adapters. So "node-debug2" or "python-debug" (and any other DA) need their own copy of the implementation. This is not ideal.

    What do think about a solution were the WSL logic mostly lives in VS Code itself and all DAs could profit from it easily? One approach for this is to use the "runInTerminalRequest" of the Debug Adapter Protocol (DAP). Debug adapters use this request to launch the debuggee in either the "integratedTerminal" or the "externalTerminal". Only the third terminal option "internalConsole" is currently implemented in the DA itself (but we probably could move this to VS Code as well).
    The code lives here: https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/debug/electron-browser/terminalSupport.ts.

    We could introduce an additional argument "useWSL" for the "runInTerminalRequest" to transport that information from the DA back to VS Code.
    In addition we could return information about the remote/local path mapping from the "runInTerminalRequest" and the DA could use this to map the paths.

    What do you think about that approach?

  2. Extension host debugging: I don't think that we have to support WSL for EH because we do not really start another VS Code here, but instead we connect to the already running VS Code and let it create another window.

    So when VS Code runs on the Windows side, then the extension to be debugged will run in that same VS Code instance as well. It is not possible to run the extension on the linux side.

    Did you really verify that Extension Host debugging through WSL works (and in fact makes a difference)?

  3. Please try to keep PRs as minimal as possible. Please avoid renames because they makes the review more difficult (without changing the semantics).

Now I will try to make the PR run in the latest version of VS Code...

/cc @roblourens

@bzoz
Copy link
Contributor Author

bzoz commented Aug 17, 2017

  1. Moving the WSL support to the VS Code would be the best solution. The internalConsole should also be moved, I'll implement that.

  2. I did not test this with extension host, I've added the code as mirror of other implementations. You are right, there is no sense to keep it.

I've rebased this PR on master: https://github.com/janeasystems/vscode-node-debug/tree/bartek-wsl-rb

@weinand
Copy link
Contributor

weinand commented Aug 17, 2017

@bzoz please don't jump too quickly on my suggestions/questions.
Let's discuss first! ;-)

Today I played with "useWSL" and it worked great so far.

Here is another finding/idea:

I noticed that our version/protocol probing code in the extension.ts is still running but results in bogus results: it tries to find the node version on Windows and picks either node-debug or node-debug2 based on the version. This must be adapted to WSL as well.

But when thinking about this change in extension.ts, the following idea came to my mind:
Before moving the WSL code to VS Code we could first try to move the WSL code from the adapter into the extension.ts. This would at least factor out the WSL code from node-debug and node-debug2 into a single location.

At the moment the extension.ts implements a command startSession that is used to "massage" a launch config before it is passed to the DA. startSession fills in missing attributes and determines the debug type based on the version probing or the protocol attribute.
See https://github.com/Microsoft/vscode-node-debug/blob/master/src/node/extension/extension.ts#L194

Here we could add the WSL logic: if useWSL is detected, we could modify the runtimeExecutable and runtimeArgs attributes to inject the "bash.exe" and we could modify the localRoot and remoteRoot attributes for path mapping.

BTW, in the August release we will deprecate the startSession command and introduce "real API": a DebugConfigurationProvider with a resolveDebugConfiguration method: https://github.com/Microsoft/vscode/blob/master/src/vs/vscode.proposed.d.ts#L274

So, eventually the WSL logic would live in a DebugConfigurationProvider. resolveDebugConfiguration.

node-debug has a DebugConfigurationProvider branch with a preview of resolveDebugConfiguration:
https://github.com/Microsoft/vscode-node-debug/blob/aweinand/DebugConfigurationProvider/src/node/extension/configurationProvider.ts#L68

"One more thing": more than one "DebugConfigurationProvider" can be registered for a debug type. So in theory it would be possible to have a "WSL" extension that adds the WSL logic to the node-debug extension... but this is probably way too ambitious ;-)

@bzoz
Copy link
Contributor Author

bzoz commented Aug 22, 2017

I think that moving this to extension.ts or to resolveDebugConfiguration would be confusing. The automatic "WLS-ization" of calls (for other debug adapters) would probably end up not working more often than not.

I think that adding something like "subsystemLinuxConsole" to the runInTerminalRequest would be most straightforward and easiest for other debug adapters author to implement.

We could also separate the WSL detection and parameter mangling to a separate package and publish that. It would be easy for everyone to implement WSL support by themselves. VSCode could use this package too, in "subsystemLinuxConsole" implementation.

@weinand
Copy link
Contributor

weinand commented Aug 22, 2017

Just to be clear: extension.ts or resolveDebugConfiguration are private to the Node debugger extension (every debugger extension would have their own). So I'm not saying that the WSL logic is automatically available for all debug extensions.

The only benefit is that the WSL code would be shared between the node-debug and node-debug2 adapters (because they are called from the extension.ts depending on the node version being used).

But actually I tried to implement a transformation of a given launch config into a WSL launch config (without touching the DA) and this was harder than I thought (and I gave up for now).

Then I looked into moving the "internalConsole" code into VS Code so that all three terminal types are handled on the VS Code side. But this turned out to be impossible because this code would have to move to VS Code as well (which would require the introduction of quite a few
events that would have to be passed from VS Code to the DA).

So moving the "internalConsole" to VS Code seems impractical (which is probably the reason why I didn't do this in the first place). This makes adding a "subsystemLinuxConsole" to the "runInTerminalRequest" less interesting because it does not cover all cases.

With this I'm back at square one ;-)

I like your suggestion to separate the WSL detection and parameter mangling.
So I think we should go in that direction first.

Initially it would be fine to only separate that logic into a separate file.
If other debugger extension authors are interested we can create an npm module later.

Could you try to simplify/refactor your PR a bit:

  • remove the WSL logic for EH
  • Why do you need wslInternalBashPath and wslExternalBashPath in parallel? You already know the single console that is being used and you could only determine the path that you need.
  • In addition I do not really understand the 32/64 bit logic to determine those paths. Factoring this inlined code into functions would probably help.
  • Does your code already supports the new 64 bit version of VS Code for Windows?
  • the localRoot on ILaunchArgs surprised me because I don't think that this can be affected by "useWSL" at all. It is either specified by the user or it could default to the ${workspaceRoot}.

@nelak
Copy link

nelak commented Aug 23, 2017

Adding my 2cents to this, you'll probably also want to be able to load the environment variables configured in .bashrc when launching wsl e.g.: nvm support but depending on your .bashrc additonal arguments may be required. For the default ubuntu WSL install you should use the -ci flags instead or the environment won't load.

# ~/.bashrc: executed by bash(1) for non-login shells.
# see /usr/share/doc/bash/examples/startup-files (in the package bash-doc)
# for examples

# If not running interactively, don't do anything
case $- in
    *i*) ;;
      *) return;;
esac```

@bzoz
Copy link
Contributor Author

bzoz commented Aug 29, 2017

Refactored the code. Since the changes are significant, I've opened another PR: #158

@bzoz
Copy link
Contributor Author

bzoz commented Aug 31, 2017

Closing this in favor of #158.
@weinand, PTAL at the new PR.

@bzoz bzoz closed this Aug 31, 2017
@weinand weinand mentioned this pull request Sep 25, 2017
1 task
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants