Add `isWsl` flag to session/action objects #3046

rfgamaral · 2018-05-22T19:39:04Z

The isWsl flag denotes if the running shell is a Windows Subsystem for Linux (WSL) shell by pattern matching the user configurable shell property.

This flag has support for invoking WSL with multiple executables:

bash.exe
debian.exe
kali.exe
opensuse.exe
ubuntu.exe
wsl.exe

I'm not sure if this is something you guys want to add to Hyper core but it's just a small flag to help plugin developers easily know if the running shell is WSL or not, just like they currently need to know if the host OS is Windows and Linux (provided by process.platform) to properly do their plugin magic. This will come in handy to better support WSL within plugins.

Let me know if you have any questions or a better idea to implement this.

The `isWsl` flag denotes if the running shell is a Windows Subsystem for Linux (WSL) shell by pattern matching the user configurable `shell` property. This flag has support for invoking WSL with multiple executables: - bash.exe - debian.exe - kali.exe - opensuse.exe - ubuntu.exe - wsl.exe

Tyriar · 2018-05-22T20:20:21Z

app/ui/window.js

@@ -93,6 +93,7 @@ module.exports = class Window {
          cwd: process.argv[1] && isAbsolute(process.argv[1]) ? process.argv[1] : cfgDir,
          splitDirection: undefined,
          shell: cfg.shell,
+          isWsl: /(?:bash|debian|kali|opensuse|ubuntu|wsl)\.exe$/.test(cfg.shell),


I believe there is also ubuntu1804.exe

I must admit I added these by intuition because I couldn't find a document with all the distros exectuables. I only use Debian myself. I'll add that version to the list soon.

However, as the number of distros may grow and some of them may have multiple executables (case in point), a regex is probably not the best approach. But I honestly can't think of another way to detect if Hyper is running a WSL shell.

Any other ideas?

Yes actually 😄

I only know about that exe because I recently did something very similar in vscode. Check out the list I came up with https://github.com/Microsoft/vscode/blob/86adb527f12c12c1c04806cfa0cd435f98475a6f/src/vs/workbench/parts/terminal/node/windowsShellHelper.ts#L13 (which also includes powershell and cmd, everything else is wsl).

Based on your list, I guess I have a couple of entries to fix 😄

I might follow your approach instead of regex, it will be easier to maintain in the long run.

@Tyriar Changed the implementation, could you please re-review? I've also took the liberty of using path.sep instead of the hard-coded / in one other instance.

I don't know the code base too well so can't give a full review, the list of exes looks good though 👍

Me neither, I'm just digging through code and making small changes here and there 🤣

chabou

I would invert the way WSL is detected to be easier to maintain

chabou · 2018-05-23T21:44:18Z

app/utils/wsl.js

+  'debian.exe',
+  'opensuse-42.exe',
+  'sles-12.exe'
+];


Can you transform this into something like:

const WIN_SHELLL_EXECUTABLES = [ 'cmd.exe', 'powershell.exe' ] function isWslShell(shell) { return process.platform === 'win32' && !WIN_SHELL_EXECUTABLES.indexOf(shell.split(path.sep).pop()) !== -1; }

Be careful with empty/missing shell config.

It would be easier to maintain like @Tyriar suggested.

I disagree, there are many other non-WSL shells: git bash, python, gchi, etc.

Yeah, either we have a full list for valid WSL shells, or non-WSL shells. Since the function is called isWslShell and the property is isWsl, I think it makes more sense to have a list of valid WSL shells. Let me know how you want me to proceed @chabou.

I'll handle the empty/missing shell configuration though :)

Oh yes, you're right.
But, in this case, it will take some effort to keep this list up to date.

It might, it all depends on how frequent new Linux distros are going to be supported by WSL and if any of them will include different exes than the default ones. But I honestly can't think of any other way to detect if the active/running shell is WSL or not. If you have a suggestion, I'm all hears :)

chabou · 2018-05-23T21:48:24Z

Can you give some examples of plugins that need to know if current shell is running WSL or not?

rfgamaral · 2018-05-23T23:59:33Z

@chabou Plugins like hyper-tab-icons, hypercwd and hyper-statusline will need special handling to work within WSL (they currently don't; the last 2 have open issues regarding that). There could be others out there but these are the ones I'm personally interested in using and thus fixing their WSL issues.

To give a more in-depth context, lets use hypercwd as an example.

This plugin opens new tabs with the same directory as the current tab in Hyper. It does this by reading the current tab directory with lsof on Linux and relies on the current tab directory that is printed on the Command Prompt / PowerShell on Windows (which is always the absolute path).

The Windows technique does not work for WSL because it's a Linux prompt with color codes in the mix and one could always be using a prompt configuration to show a relative path, for instance. You can't also rely on lsof because although WSL is Linux, Hyper is still running on Windows and lsof doesn't exist on Windows.

In other words, WSL needs special handling and hypercwd needs to know which method to apply to detect the absolute path for the current directory.

rfgamaral · 2018-05-25T13:33:04Z

@chabou What do you think of the current implementation? Could we get this merged? It would help immensely to have this implemented so I could start working on fixing some plugins WSL issues.

chabou · 2018-05-29T11:23:00Z

lib/reducers/sessions.js

@@ -41,7 +43,8 @@ const reducer = (state = initialState, action) => {
          cols: action.cols,
          rows: action.rows,
          uid: action.uid,
-          shell: action.shell.split('/').pop(),
+          shell: action.shell.split(path.sep).pop(),
+          isWsl: action.isWsl,


Can you determine if this is a WSL shell here?

This is better to let plugins modify the action.shell value (in a middleware for example). In this case, this is better to call isWslShell later in the reducer than in the action creator.

To be sure, are you suggesting I change this to isWsl: isWslShell(action.shell)?

yes.
This is a derivated data and derivation should occur in the reducer.

@chabou There's a little problem with that I'm not sure how to handle given that I don't know the code base so well. The isWsl function was created as an app util function and this reducer is inside the lib folder. I'm not sure I can/should import stuff from app into lib (and vice-versa for that matter). How should I proceed?

I would suggest to move app/utils/wsl.js to lib/utils/wsl.js or move your function directly into lib/reducers/sessions.js because your function will not be used anywhere else.

innerlee · 2018-06-22T10:04:54Z

according to microsoft/WSL#2715 (comment), /bin/wslpath and /init are the only binaries wsl was injected into. So you can check the existence of these two files?

machinshin · 2019-05-24T00:29:05Z

@rfgamaral : any updates on this thread/PR?

Stanzilla · 2019-06-13T23:27:17Z

Instead of hardcoding exe names, could we do something like this instead? microsoft/terminal#1050

Tyriar reviewed May 22, 2018

View reviewed changes

rfgamaral added 5 commits May 22, 2018 21:40

Add support for additional ubuntu1804.exe

58c203c

Remove non-capturing groups to simplify regex

db62515

Create util fn based on array instead of regex

1479099

Use proper platform-specific file separator

9e4dfe7

Fix linting errors

e613f3e

chabou suggested changes May 23, 2018

View reviewed changes

Handle null/undefined shell configuration

d998b5b

rfgamaral mentioned this pull request May 28, 2018

Windows 10 Blank Status Line henrikruscon/hyper-statusline#75

Open

chabou suggested changes May 29, 2018

View reviewed changes

rauchg added the WIP label Dec 4, 2018

Stanzilla closed this Sep 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `isWsl` flag to session/action objects #3046

Add `isWsl` flag to session/action objects #3046

rfgamaral commented May 22, 2018 •

edited

Loading

Tyriar May 22, 2018

rfgamaral May 22, 2018

Tyriar May 22, 2018

rfgamaral May 23, 2018

rfgamaral May 23, 2018

Tyriar May 23, 2018

rfgamaral May 23, 2018

chabou left a comment

chabou May 23, 2018

Tyriar May 23, 2018

rfgamaral May 24, 2018 •

edited

Loading

chabou May 24, 2018

rfgamaral May 24, 2018

chabou commented May 23, 2018

rfgamaral commented May 23, 2018

rfgamaral commented May 25, 2018

chabou May 29, 2018

rfgamaral May 29, 2018

chabou May 29, 2018

rfgamaral May 29, 2018

chabou May 29, 2018

innerlee commented Jun 22, 2018

machinshin commented May 24, 2019

Stanzilla commented Jun 13, 2019

Add isWsl flag to session/action objects #3046

Add isWsl flag to session/action objects #3046

Conversation

rfgamaral commented May 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chabou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfgamaral May 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chabou commented May 23, 2018

rfgamaral commented May 23, 2018

rfgamaral commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

innerlee commented Jun 22, 2018

machinshin commented May 24, 2019

Stanzilla commented Jun 13, 2019

Add `isWsl` flag to session/action objects #3046

Add `isWsl` flag to session/action objects #3046

rfgamaral commented May 22, 2018 •

edited

Loading

rfgamaral May 24, 2018 •

edited

Loading