Skip to content

Introduce PowerShell installer stub #18639

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

Open
wants to merge 7 commits into
base: dev/cazamor/sui/extensions-page
Choose a base branch
from

Conversation

carlos-zamora
Copy link
Member

Summary of the Pull Request

Adds an "Install Latest PowerShell" stub to the new tab menu that installs the latest PowerShell on your machine when invoked. This is generated via a new PowershellInstallationProfileGenerator and leverages the new Extensions page to allow enabling/disabling the extension. The leftover stub is also automatically deleted to create a much cleaner experience.

References and Relevant Issues

Internal Spec: https://microsoft-my.sharepoint-df.com/:w:/p/chrnguyen/EchlDFoWsTBIudqQzVckHGABJ7zErJPK8aiLrS9nnFrlyw?e=QdmB1w
Targets #18633

@carlos-zamora
Copy link
Member Author

carlos-zamora commented Feb 28, 2025

Demo

Note 1: It's a really long gif, but it's worth it!
Note 2: This is running on a VM in Windows 10 because I didn't want to uninstall/reinstall PowerShell on my main machine over-and-over again.

powershell stub

@carlos-zamora
Copy link
Member Author

@nguyen-dows @StevenBucher98 @DHowett Take a look at the demo above. Let me know what you think and if anything's missing 😊

I worked hard to make that stub go away afterwards because I got annoyed by it, so I figured everybody else would too. Also added a comment into the JSON stubs. Not sure how many people are gonna read it, but it's something (and better than the profile disappearing from the settings UI, but the empty extension still being there).

Also made the decision to make the installer interactive because if you're installing it, I'm guessing you may want to configure it and be well aware that you're installing something.

One annoying thing is that the dynamic profile generators only trigger if you close and open terminal again. I tried to help make that more clear by adding a little localized phrase in the stub session. Not perfect, but it's something.

One concern I have is this: is it possible for somebody to modify the stub to install malicious.exe instead?
I'm pretty sure the answer to this is "no" because we're hashing the settings file, so we're already covered, but wanted to bring it to your attention.

@carlos-zamora
Copy link
Member Author

I think there was some discussion before about disabling this for enterprises. I know the files for that are:

Is there a way to add Windows.Terminal.InstallPowerShell by default? Is that something we want to do?

@carlos-zamora carlos-zamora force-pushed the dev/cazamor/sui/ext-page/powershell-stub branch from 7a426b4 to 9842bdc Compare February 28, 2025 02:34
@denelon
Copy link
Contributor

denelon commented Mar 1, 2025

I didn't look to see where the PowerShell installer was coming from. You could in theory use WinGet to install PowerShell interactively for this which would give you the hash of the PowerShell installer as a sanity check. I think the main concern is whether the default "winget" source is available to the user. In some enterprise environments, it might be removed or blocked. I'm not trying to throw wrenches in the works here, but just wanted to collaborate if it would improve the experience for the user.

@denelon
Copy link
Contributor

denelon commented Mar 1, 2025

Lol, and now the page refreshes and I see @mdanish-kh suggesting the "winget" source as well as interactive... :)

@denelon
Copy link
Contributor

denelon commented Mar 1, 2025

You might want to check if the winget source is configured before just trying to use it.

@mdanish-kh
Copy link
Contributor

mdanish-kh commented Mar 1, 2025

You might want to check if the winget source is configured before just trying to use it.

Would the simplest way be checking against the output of winget source export | ConvertFrom-Json? Because if the winget source has been blocked by the Group Policy Enable App Installer Default Source, it will not show up in the output of winget source export

@denelon
Copy link
Contributor

denelon commented Mar 3, 2025

That might be a good way to make the determination. GPO or a user who removed the source would both give an indication via source export (by the lack of having the "winget" source).

@DHowett
Copy link
Member

DHowett commented Mar 3, 2025

I am staunchly opposed to Terminal running a command to determine whether it should generate a profile. @denelon and @mdanish-kh, is there a way to figure this out without spawning and waiting for winget.exe?

FWIW, the WSL team once told us they didn't have an API to list Linux distributions and they recommended shelling out to wsl -l. Even in the best case, that added hundreds of ms to our startup time. In the worst, it added ten seconds because sometimes their COM server was down.

Winget is a packaged application. Given how unreliable packaged apps are, I am not introducing the launch of another packaged application to our startup process.

@denelon
Copy link
Contributor

denelon commented Mar 3, 2025

I don't know the internal mechanics of the profile generation in Terminal. I was just looking at the mechanics involved to have WinGet install PowerShell, and ensuring we weren't passing any implicit agreements for the user without their knowledge or consent, and making sure the package is available before attempting to install it.

Most of the WinGet core functionality is in COM, but there are certainly cold-start cases that can be slow, and shelling out to WinGet or the PowerShell cmdlets can take more time than one might want for a start-up scenario.

"is there a way to figure this out"

I'm not sure what you are referring to for this comment.

@StevenBucher98
Copy link

Overall the experience looks great to me!

@DHowett
Copy link
Member

DHowett commented Mar 3, 2025

"is there a way to figure this out"

I'm not sure what you are referring to for this comment.

Sorry, what I mean is... we don't want to display the PowerShell Installer if the user's not going to be able to use it either due to enterprise management (winget is disabled, source is disabled, our dynamic profile generators are disabled), user management (winget is disabled, source is uninstalled, our dynamic profile generators are disabled), or any other thing that may prevent the user from being successful here.

We need to be able to make that determination quickly with the information available in native data stores available in-proc, because there's a chance we will be doing it on every launch of Terminal.

@denelon
Copy link
Contributor

denelon commented Mar 3, 2025

OK, I understand the scenario a bit better now. I'm not sure if we have anything that would be "faster", but maybe @JohnMcPMS has some ideas.

@JohnMcPMS
Copy link
Member

I assume that the goal is to avoid showing this option in the face of guaranteed failure. I also assume there is at least some aversion to installing the MSIX version from the Store (I don't know its current support status).

If those are both true, then I think you must run/attempt to run a winget process to get the answer. Any attempt to short circuit that by reading GP registry would not be a supported path.

Comment on lines 185 to 208
WslDistroGenerator wslGenerator{};
_executeGenerator(wslGenerator);

AzureCloudShellGenerator acsGenerator{};
_executeGenerator(acsGenerator);

VisualStudioGenerator vsGenerator{};
_executeGenerator(vsGenerator);
Copy link
Member

Choose a reason for hiding this comment

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

Is it really necessary to extend the lifetime of these generators? They only get destroyed at the end of the scope now.

Copy link
Member Author

Choose a reason for hiding this comment

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

What motivated this change was that the PowershellCoreProfileGenerator already does all the work of finding the existing PowerShell instances; it uses that information to generate (and configure) the appropriate amount of PowerShell profiles.

We need an instance of the profile generator lying around so that we can reuse that information. That way, if we detect that there's an instance of PowerShell installed, we hide the installer stub.

Admittedly, it's not ideal, but its all in one function. Any ideas on how to get around it? Is it worth making the PowershellCoreProfileGenerator special? Or maybe I should pull out the GetPowerShellInstances() code into a static function and undo the work of keeping the generators around (I'll give that a try).


// We want the comment to be the first thing in the object,
// "closeOnExit" is the first property, so target that.
auto fragExtJson = _parseJSON(til::u16u8(fragExt->Json()));
Copy link
Member

Choose a reason for hiding this comment

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

To be honest, I kind of forgot how the extension code works, but this still seems kind of hacky to me.

The fragment in this case is based on an extension, right? Don't extensions generate profiles only? I'm missing the big picture how we go from profiles to JSON here, hence my confusion. Basically, since extensions generate profiles (including PowershellInstallationProfileGenerator), my assumption would be that this function doesn't need to deal with parsing JSON at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh this section is SUPER hacky! The sole purpose of this is to add the JSON comment so it appears like this:
image
image

So yeah, we have to parse the JSON, add the comment, then store the new JSON with the comment back in.
We have to do it twice:

  1. the settings.json blob representing the entire fragment extension
  2. the generated profile itself

Copy link
Member Author

Choose a reason for hiding this comment

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

Figured it'd be good to try and tell the user that this profile only appears if PowerShell is not installed. It's a weird little behavior that I figured at least a JSON comment would help point out, even if not many people read the JSON comment.

@DHowett DHowett marked this pull request as draft March 21, 2025 18:33
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/sui/ext-page/powershell-stub branch from 9842bdc to 71be4ee Compare April 17, 2025 00:52
@carlos-zamora
Copy link
Member Author

Note to self: tested the new version by manually changing isPowerShellInstalled to false. It should work as expected, but I want to verify it again with a fresh machine. Unfortunately, I need to set up a VM again to test that out, so that'll be something for me to do tomorrow. I'll report back.

Comment on lines 187 to 245
if (!isPowerShellInstalled)
{
// Only generate the installer stub profile if PowerShell isn't installed.
PowershellInstallationProfileGenerator pwshInstallationGenerator{};
_executeGenerator(pwshInstallationGenerator);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Using this as a thread to track some of the top-level discussion around preemptively disabling the installer generator. Specifically Dustin's concern here:

Sorry, what I mean is... we don't want to display the PowerShell Installer if the user's not going to be able to use it either due to enterprise management (winget is disabled, source is disabled, our dynamic profile generators are disabled), user management (winget is disabled, source is uninstalled, our dynamic profile generators are disabled), or any other thing that may prevent the user from being successful here.

We need to be able to make that determination quickly with the information available in native data stores available in-proc, because there's a chance we will be doing it on every launch of Terminal.

Motivation (link): (paraphrasing) out-of-proc query can REALLY impact our startup time

Solution discussion from John (link):

I assume that the goal is to avoid showing this option in the face of guaranteed failure. I also assume there is at least some aversion to installing the MSIX version from the Store (I don't know its current support status).

If those are both true, then I think you must run/attempt to run a winget process to get the answer. Any attempt to short circuit that by reading GP registry would not be a supported path.

Sounds like we may need to think of some other way to preemptively disable the installer generator. I'll noodle on this more tomorrow, but again, using this thread as a nicer way of tracking it.

Comment on lines 186 to 245
const auto isPowerShellInstalled = !powerShellGenerator.GetPowerShellInstances().empty();
if (!isPowerShellInstalled)
{
// Only generate the installer stub profile if PowerShell isn't installed.
PowershellInstallationProfileGenerator pwshInstallationGenerator{};
_executeGenerator(pwshInstallationGenerator);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

📝 Huh, a side effect of putting this in a scope and conditionally executing the installer's generator is that now the generator only appears in the Extensions page if PowerShell is not installed. This is because all the work to register the generator for the settings UI is in _executeGenerator().

I'm honestly kinda ok with this change. It means that for anybody that has PowerShell installed already, they won't even see the installer generator anywhere. But if the user doesn't have PowerShell 7 installed, they get the installer stub. From that point, the user can also manually disable the generator via the Extensions page.

@carlos-zamora carlos-zamora marked this pull request as ready for review April 21, 2025 17:58
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/sui/ext-page/beautify branch from f650f45 to b963d60 Compare April 21, 2025 22:02
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/sui/ext-page/powershell-stub branch from a00f333 to 4b35462 Compare April 21, 2025 22:36
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/sui/ext-page/beautify branch from b963d60 to f25e6fe Compare April 22, 2025 22:13
@carlos-zamora carlos-zamora force-pushed the dev/cazamor/sui/ext-page/powershell-stub branch from 4b35462 to 36d28e2 Compare April 22, 2025 23:59
Base automatically changed from dev/cazamor/sui/ext-page/beautify to dev/cazamor/sui/extensions-page April 24, 2025 22:24
Copy link

@Crypticslang11 Crypticslang11 left a comment

Choose a reason for hiding this comment

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

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 25, 2025
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 29, 2025
Copy link

github-actions bot commented May 6, 2025

@check-spelling-bot Report

🔴 Please review

See the 📂 files view, the 📜action log, or 📝 job summary for details.

Unrecognized words (1)

Schmes

These words are not needed and should be removed abcd ABCDEFGHIJ abcdefghijk ABCDEFGHIJKLMNOPQRS ABCDEFGHIJKLMNOPQRST ABCDEFGHIJKLMNOPQRSTUVWXY allocs appium Argb asan autocrlf backported bytebuffer cac CLE codenav codepath commandline COMMITID componentization constness dealloc deserializers DISPATCHNOTIFY DTest entrypoints EVENTID FINDUP fuzzer hlocal hstring IInput Interner keyscan Munged numlock offboarded pids positionals Productize pseudoterminal remoting renamer roadmap ruleset SELECTALL somefile Stringable tearoff TODOs touchpad TREX Unregistering USERDATA vectorize viewports wsl

To accept these unrecognized words as correct and remove the previously acknowledged and now absent words, you could run the following commands

... in a clone of the [email protected]:microsoft/terminal.git repository
on the dev/cazamor/sui/ext-page/powershell-stub branch (ℹ️ how do I use this?):

curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/v0.0.24/apply.pl' |
perl - 'https://github.com/microsoft/terminal/actions/runs/14849554154/attempts/1'
Errors (1)

See the 📂 files view, the 📜action log, or 📝 job summary for details.

❌ Errors Count
❌ ignored-expect-variant 2

See ❌ Event descriptions for more information.

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spelling/allow/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spelling/allow/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

If the flagged items are 🤯 false positives

If items relate to a ...

  • binary file (or some other file you wouldn't want to check at all).

    Please add a file path to the excludes.txt file matching the containing file.

    File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

    ^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

  • well-formed pattern.

    If you can write a pattern that would match it,
    try adding it to the patterns.txt file.

    Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

    Note that patterns can't match multiline strings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants