WIP: Add support for side-by-side PS Core preview on Linux/macOS #1366

rkeithhill · 2018-06-14T02:42:41Z

PR Summary

This PR supports listing the symlinks for pwsh & pwsh-preview.

The preferred default version is pwsh if both are present however, the
default will be pwsh-preview if it's installed and pwsh isn't.

So this only support side-by-side on Linux/macOS up to two versions:
stable and preview.

In the future, we may want to do what we do on Windows and search thru
the install dir for pwsh.
The would be /opt/microsoft/poweshell on LInux.
Not sure what it is on macOS.

Or maybe we should just do that now. Thoughts?

I marked this as WIP because I need someone to test this on Linux and macOS. :-)

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets.
Please mark anything not applicable to this PR NA.

PR has a meaningful title
Summarized changes
This PR is ready to merge and is not work in progress
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

This PR supports listing the symlinks for pwsh & pwsh-preview. The preferred default version is pwsh if both are present however, the default will be pwsh-preview if it's installed and pwsh isn't. So this only support side-by-side on Linux/macOS up to two versions: stable and preview.

TylerLeonhardt

LGTM, Keith 😄 just a small comment.

TylerLeonhardt · 2018-06-14T04:35:45Z

src/platform.ts

+                    versionName: "PowerShell Core Preview",
+                    exePath: linuxPreviewExePath,
+                });
+            }


I refactored this a bit to define the paths conditionally and then do the logic. I think it's a little DRY-er. Totally, not a blocker but what do you think?

let osExePath, osPreviewExePath; if (platformDetails.operatingSystem === OperatingSystem.MacOS) { osExePath = macOSExePath; osPreviewExePath = macOSPreviewExePath; } else if (platformDetails.operatingSystem === OperatingSystem.Linux) { osExePath = linuxExePath; osPreviewExePath = linuxPreviewExePath; } if ((defaultExePath === osExePath) && fs.existsSync(osPreviewExePath)) { paths.push({ versionName: "PowerShell Core Preview", exePath: linuxPreviewExePath, }); }

LGTM. Can you make this change on the PR branch and merge? I think I've allowed it to be edited. If not, I should be able to get to this tonight.

Updated. Please test on macOS and Linux.

TylerLeonhardt · 2018-06-18T22:53:39Z

src/platform.ts

+            osPreviewExePath = linuxPreviewExePath;
+        }
+
+        if ((osExePath === defaultExePath) && fs.existsSync(osPreviewExePath)) {


It's super rare, but isn't it possible for them to have legacy (powershell) and preview (pwsh-preview)?

If so, this if check might have to be:

defaultExePath !== osPreviewExePath && ...

I can fix this by testing for the legacy path here or maybe we should just remove support for the legacy "powershell"on Linux and macOS. This was abandoned somewhere around the transition from 6.0.0 alpha to beta. At this point, I'm not sure why anyone would still be running an alpha of 6.0.0 on Linux/macOS.

totally on board. We should only support 6.0.0 release and onward.

TylerLeonhardt · 2018-06-18T23:36:14Z

Is there any work that needs to be done here:
https://github.com/PowerShell/vscode-powershell/blob/master/test/platform.test.ts

I just saw that it had to something with the exe path.

rkeithhill · 2018-06-19T02:29:29Z

Those tests still pass for me on Windows. Looks like the tests mainly check to see that we get back the correct 32/64-bit version of Windows PowerShell on Windows.

TylerLeonhardt · 2018-06-19T15:47:52Z

Sounds good :)

rjmholt

This is awesome! Thanks @rkeithhill!

rjmholt

I have yet to test this btw. Will do this week.

TylerLeonhardt · 2018-06-27T00:22:08Z

So I was messing with the VSCode & PowerShell snaps... Snap is essentially an attempt at a "universal *nix package manager... more info here. Snap is a Canonical thing

Anyway... on all of their supported OS's you should be able to run:

sudo snap install --classic vscode
sudo snap install --classic powershell
code --install-extension="ms-vscode.powershell"

and get VSCode, PowerShell and the extension installed...

However the extension fails because the "powershell exe" it is trying to use is /usr/bin/powershell but that doesn't exist... Snap does something different:

PS /home/parallels/Desktop> which powershell                                    
/snap/bin/powershell
PS /home/parallels/Desktop> which pwsh                                          
/snap/powershell/2/opt/powershell/pwsh

NOTE: the 2 in the which pwsh changes every version... apparently we can replace 2 with current and it resolves.

we should probably handle this in the linux case so we can support the Snap build of PowerShell out of the box.

@rkeithhill would you be ok adding a check for this in this PR?

rkeithhill · 2018-06-27T00:44:02Z

Are those paths on Linux or macOS? Also, is the binary called powershell?? Or is that just a dir name? I thought the binary would be either pwsh or pwsh-preview. And speaking of PowerShell preview, do we need to handle that case with snap?

TylerLeonhardt · 2018-06-27T00:48:53Z

@rkeithhill AFAIK, snap is only linux:

TylerLeonhardt · 2018-06-27T00:53:17Z

the binary is called pwsh found here:
/snap/powershell/2/opt/powershell/pwsh but we can rely on:
/snap/bin/powershell or /snap/bin/pwsh (I prefer pwsh)
which are symlinks

and we should also handle the pwsh-preview scenario for snap as well:
/snap/bin/pwsh-preview

TylerLeonhardt · 2018-06-27T00:53:40Z

I've been talking to @TravisEz13 about this who has been driving the Snap work

TylerLeonhardt · 2018-06-27T00:54:20Z

I can't wait to install VSCode & PowerShell on my OpenWRT based Router lol

rkeithhill · 2018-06-28T02:33:16Z

@tylerl0706 Can you test this in a Snap environment?

TylerLeonhardt · 2018-06-28T16:55:31Z

gladly :) I'll test now.

TylerLeonhardt · 2018-06-28T17:02:43Z

@rkeithhill it works!

TylerLeonhardt · 2018-06-28T17:06:06Z

src/platform.ts

+            osExePath = macOSExePath;
+            osPreviewExePath = macOSPreviewExePath;
+        } else if (platformDetails.operatingSystem === OperatingSystem.Linux) {
+            osExePath = linuxExePath;


do we not need to do the same ternary op here to check for snap?

Not needed here because getDefaultPowerShellPath already returns just one of: linuxExePath, linuxPreviewExePath, snapExePath, snapPreviewExePath. It searches in that order.

BTW I get the impression (perhaps incorrect impression) that on a Snap system, PowerShell will only be at /snap/bin/pwsh* and that you wouldn't also have PowerShell at /usr/bin/pwsh*. And on a straight (non-Snap) Linux system, there would be no /snap/bin/pwsh*. That is, these two sets are mutually exclusive. Is that correct?

So snap is really just a package manager that you first install with apt. So you could have installed pwsh by Snap and Apt on the same system.

Not quite sure why one would do that... but it's physically possible. I guess I could see something along the lines of installing pwsh with apt and pwsh-preview with snap maybe...

So it's physically possible to have a /snap/bin/pwsh and a /usr/bin/pwsh.

However! I just learned that /snap/bin/powershell gets created but /usr/bin/powershell does not. So if you installed PowerShell with Snap and Apt, typing pwsh at the terminal will get you the apt version... and typing powershell at the prompt will get you the Snap version.

This might be TMI but I figured I would share.

TylerLeonhardt · 2018-06-28T17:22:31Z

You might hate me for saying this... but I've thought about this part of the code for a bit and wonder if there's some opportunity to clean up the code a bit...

maybe something like this:


function getAvailablePowerShellExes() {
    let exes = [];
    
    if(Windows) {
        ... similar as below
    }
    if(macOS) {
        ... similar to below
    }
    if(Linux) {
        linuxExes = ["/usr/bin/pwsh", "/snap/bin/pwsh", "/usr/bin/pwsh-preview", "/snap/bin/pwsh-preview"]
        linuxExes.forEach((exe) => {
            if(exe exists) {
                exes.push(exe);
            }
        });
    }
    return exes;
}

function getDefualtPowerShellExe() {
    return getAvailablePowerShellExes()[0];
}

Note that the array is in the order of what we consider the default.
Also that this is psuedocode :)

if we're super worried about perf (in this case, I don't think we are) we could optionally give getAvailablePowerShellExes a parameter bool called stopAtFirstExe that will break out of the for loop when it finds a valid exe.

I wont block on this, but I feel like it might be worth it...

rkeithhill · 2018-06-29T02:50:29Z

Not at all. I have no particularly affinity for this code. :-) We could make that change but we can't get rid of the test against the default PowerShell exe simply because we don't want the same entry to show up twice. Also, I don't think perf is that much of a concern here and we do want to list all the available PowerShell instances that can be used.

TylerLeonhardt · 2018-06-29T20:41:27Z

Thanks for putting up with my comments 😄

We could make that change but we can't get rid of the test against the default PowerShell exe simply because we don't want the same entry to show up twice

I don't think I quite follow how we would end up with something showing up twice? Think you could elaborate?

rkeithhill · 2018-07-01T23:32:30Z

The issue is that for some reason (I don't remember) in the current impl of getAvailablePowerShellExes we use getDefaultPowerShellPath to get at least the one path we know will be on the system. But we could probably remove that and simplify the impl. That said, I don't think we can use the first result of getAvailable for getDefault because getDefault takes an extra parameter - use32 - which on Windows 64-bit, requests the 32-bit version of PowerShell.

rkeithhill · 2018-07-01T23:44:51Z

Updated to simplify per your suggestion. Can you test again on Linux/macOS?

TylerLeonhardt · 2018-07-02T23:14:49Z

@rkeithhill no surprise your code works beautifully :)

* Add support for side-by-side PS Core preview on Linux/macOS This PR supports listing the symlinks for pwsh & pwsh-preview. The preferred default version is pwsh if both are present however, the default will be pwsh-preview if it's installed and pwsh isn't. So this only support side-by-side on Linux/macOS up to two versions: stable and preview. * Adjust name of default PS Core on Linux/macOS * Make a bit DRY-er. Thx @tylerl0706 * Remove support for pre-ship PS Core Linux/macOS binary name - powershell * Add support for PowerShell Core installed in a Snap * Simplify impl of getting available PS on Linux/macOS

rkeithhill requested review from TylerLeonhardt and rjmholt June 14, 2018 02:42

Adjust name of default PS Core on Linux/macOS

65d5a56

TylerLeonhardt approved these changes Jun 14, 2018

View reviewed changes

Make a bit DRY-er. Thx @tylerl0706

2ef97e4

TylerLeonhardt reviewed Jun 18, 2018

View reviewed changes

Remove support for pre-ship PS Core Linux/macOS binary name - powershell

fc22305

rjmholt approved these changes Jun 19, 2018

View reviewed changes

rjmholt reviewed Jun 19, 2018

View reviewed changes

Add support for PowerShell Core installed in a Snap

d85f943

TylerLeonhardt reviewed Jun 28, 2018

View reviewed changes

Simplify impl of getting available PS on Linux/macOS

7545dfd

rkeithhill merged commit 0c7c1bd into master Jul 2, 2018

rkeithhill deleted the rkeithhill/add-pwsh-preview-support branch July 2, 2018 23:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add support for side-by-side PS Core preview on Linux/macOS #1366

WIP: Add support for side-by-side PS Core preview on Linux/macOS #1366

rkeithhill commented Jun 14, 2018

TylerLeonhardt left a comment

TylerLeonhardt Jun 14, 2018

rkeithhill Jun 14, 2018

rkeithhill Jun 14, 2018

TylerLeonhardt Jun 18, 2018

rkeithhill Jun 19, 2018

TylerLeonhardt Jun 19, 2018

TylerLeonhardt commented Jun 18, 2018

rkeithhill commented Jun 19, 2018

TylerLeonhardt commented Jun 19, 2018

rjmholt left a comment

rjmholt left a comment

TylerLeonhardt commented Jun 27, 2018 •

edited

Loading

rkeithhill commented Jun 27, 2018

TylerLeonhardt commented Jun 27, 2018

TylerLeonhardt commented Jun 27, 2018 •

edited

Loading

TylerLeonhardt commented Jun 27, 2018

TylerLeonhardt commented Jun 27, 2018 •

edited

Loading

rkeithhill commented Jun 28, 2018

TylerLeonhardt commented Jun 28, 2018

TylerLeonhardt commented Jun 28, 2018

TylerLeonhardt Jun 28, 2018

rkeithhill Jun 29, 2018

rkeithhill Jun 29, 2018

TylerLeonhardt Jun 29, 2018

TylerLeonhardt Jun 29, 2018

TylerLeonhardt commented Jun 28, 2018 •

edited

Loading

rkeithhill commented Jun 29, 2018

TylerLeonhardt commented Jun 29, 2018

rkeithhill commented Jul 1, 2018

rkeithhill commented Jul 1, 2018

TylerLeonhardt commented Jul 2, 2018

WIP: Add support for side-by-side PS Core preview on Linux/macOS #1366

WIP: Add support for side-by-side PS Core preview on Linux/macOS #1366

Conversation

rkeithhill commented Jun 14, 2018

PR Summary

PR Checklist

TylerLeonhardt left a 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

Choose a reason for hiding this comment

TylerLeonhardt commented Jun 18, 2018

rkeithhill commented Jun 19, 2018

TylerLeonhardt commented Jun 19, 2018

rjmholt left a comment

Choose a reason for hiding this comment

rjmholt left a comment

Choose a reason for hiding this comment

TylerLeonhardt commented Jun 27, 2018 • edited Loading

rkeithhill commented Jun 27, 2018

TylerLeonhardt commented Jun 27, 2018

TylerLeonhardt commented Jun 27, 2018 • edited Loading

TylerLeonhardt commented Jun 27, 2018

TylerLeonhardt commented Jun 27, 2018 • edited Loading

rkeithhill commented Jun 28, 2018

TylerLeonhardt commented Jun 28, 2018

TylerLeonhardt commented Jun 28, 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

TylerLeonhardt commented Jun 28, 2018 • edited Loading

rkeithhill commented Jun 29, 2018

TylerLeonhardt commented Jun 29, 2018

rkeithhill commented Jul 1, 2018

rkeithhill commented Jul 1, 2018

TylerLeonhardt commented Jul 2, 2018

TylerLeonhardt commented Jun 27, 2018 •

edited

Loading

TylerLeonhardt commented Jun 27, 2018 •

edited

Loading

TylerLeonhardt commented Jun 27, 2018 •

edited

Loading

TylerLeonhardt commented Jun 28, 2018 •

edited

Loading