-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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
Introduce PowerShell installer stub #18639
base: dev/cazamor/sui/ext-page/beautify
Are you sure you want to change the base?
Introduce PowerShell installer stub #18639
Conversation
@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 think there was some discussion before about disabling this for enterprises. I know the files for that are:
Is there a way to add |
7a426b4
to
9842bdc
Compare
void PowershellInstallationProfileGenerator::GenerateProfiles(std::vector<winrt::com_ptr<implementation::Profile>>& profiles) | ||
{ | ||
auto profile{ CreateDynamicProfile(RS_(L"PowerShellInstallationProfileName")) }; | ||
profile->Commandline(winrt::hstring{ fmt::format(FMT_COMPILE(L"cmd /k winget install --interactive --id Microsoft.PowerShell & echo. & echo {} & exit"), RS_(L"PowerShellInstallationInstallerGuidance")) }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
profile->Commandline(winrt::hstring{ fmt::format(FMT_COMPILE(L"cmd /k winget install --interactive --id Microsoft.PowerShell & echo. & echo {} & exit"), RS_(L"PowerShellInstallationInstallerGuidance")) }); | |
profile->Commandline(winrt::hstring{ fmt::format(FMT_COMPILE(L"cmd /k winget install --interactive --id Microsoft.PowerShell --source winget & echo. & echo {} & exit"), RS_(L"PowerShellInstallationInstallerGuidance")) }); |
nit: If this is done on a machine where WinGet hasn't been invoked before, the user will be prompted to accept msstore
agreements first which don't seem relevant for this use case. Suggest to add explicit source argument so that user isn't prompted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Terminal shouldn't pass explicit agreements for the msstore source. That's something an IT administrator can do for their enterprise users, or an individual can do on their own, but applications shouldn't pass that on behalf of an end user. Just an FYI :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's why I didn't suggest the --accept-source-agreements
since we require agreements to be interactive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When doing any programmatic winget operations, the source should always be provided, as well as explicitly specifying the Identifier, not just the identifier as a query string.
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. |
Lol, and now the page refreshes and I see @mdanish-kh suggesting the "winget" source as well as interactive... :) |
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 |
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). |
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 FWIW, the WSL team once told us they didn't have an API to list Linux distributions and they recommended shelling out to 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. |
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.
I'm not sure what you are referring to for this comment. |
Overall the experience looks great to me! |
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. |
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. |
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. |
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