Remember installation options and used them to initialize options for the next installation

[reduckted]

Fixes #16886

PR Summary

When the Windows installer runs, it will save the selected options ("Add to Path Environment Variable", "Enable PowerShell remoting", etc.) to the registry. When the installer for the next version runs, the state of the options is loaded from the registry and used to initialize the installer's options.

PR Context

There has been a lot of issues logged about this:

• 'Run With PowerShell 7' Right-Click Context Menu Disappears On Windows 10 Updates? #14980
• Silent Update removes context menu entry if previously added #16886
• Windows MSI installer does not remember the previous installation settings #17222
• After automatic update, context menu entry "open Powershell Core 7 here" is gone #17445
• Powershell Core Windows Exporer integration disappears after automatic Windows Update #17967
• Missing Explorer Menus #18791

In summary, when the installer runs, it does not know about any of the options that were selected on the previous install. If you run the installer manually, you need to re-select the options that you selected during the previous install. If the installer is run automatically (such as via Windows Update), then the default options are used, potentially resulting in some of the options that you previously selected from being turned off (for example, context menus are removed).

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• Breaking changes
  • None
  • OR
  • Experimental feature(s) needed
    • Experimental feature name(s):
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed
    • Issue filed:
• Testing - New and feature
  • N/A or can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed
• Tooling
  • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
  • OR
  • I have considered the user experience from a tooling perspective and opened an issue in the relevant tool repository. This may include:
    • Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode
      (which runs in a different PS Host).
      • Issue filed:
    • Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
      • Issue filed:
    • Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
      • Issue filed:
    • Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).
      • Issue filed:

---

To implement this, I've used the "Remember Property" pattern with a slight twist. The pattern described in that blog post does not account for how a checkbox sets its initial state. If a property value is not blank, then the checkbox will be initially checked, regardless of what the CheckBoxValue is. So, for example, if ADD_PATH is set to 0, the checkbox will be checked. If ADD_PATH is set to 1, the checkbox will be checked. If ADD_PATH does not exist, then the checkbox will be unchecked. I won't go into the full details of how this is handled, because there's a detailed comment that explains it all.

The installer settings are saved to HKLM\Software\Microsoft\PowerShellCore\InstallerProperties. Preview versions are saved to a different key, but these settings will be shared between major versions. For example, PowerShell v8 will use the settings saved by PowerShell v7. I don't think that will be a problem, but just wanted to point it out. 😄 That registry key also remains behind after uninstalling PowerShell, so if you have to uninstall and reinstall for whatever reason, the settings that you originally used will be the defaults when you reinstall.

Technically the way that the property values are now handled could be considered a breaking change for some of those properties. Previously, some properties were compared to 1 and other properties were just checked if they had a value.

For example, previously if you ran the installer like this:

msiexec.exe /i PowerShell.msi ADD_EXPLORER_CONTEXT_MENU_OPENPOWERSHELL=YesAddTheContextMenu

Then the context menu would be created. Now if you do that, the installer will not create the context menu. The context menu is only created if the property value is 1.

Not all properties were treated this way. For example, ADD_PATH had to have a value of 1, as did a couple of the other properties.

It's worth noting that the documentation for running the installer from the command line uses an example where it sets all property values to 1. If you followed that example, then you won't see any change in behavior.

[reduckted]

Oops, I didn't realize there were automated tests for the installer. I'll get those fixed up as soon as I can.

[reduckted]

I'm pretty sure those "macos" build failures are unrelated to the changes from this PR.

[TravisEz13]

@heaths Can you help review this?

[TravisEz13]

I believe this is a bucket 3 breaking change that doesn't require committee approval. The behaviors that are broken were not intended behavior as you could see by passing no or false as variable value.

@reduckted Can we abort the install if one of the variable values is invalid with a specific exit code?

[heaths]

I don't advise permanent components. This means users will have a hard time changing in the future, and leaves cruft behind in the registration. Perhaps it's better to change the MajorUpgrade element to use afterInstallExecute. Not only is it faster to upgrade, it will prefer existing components until the product is removed (not via upgrade).

@TravisEz13 this would need to be well-tested, though. The current behavior removes the old product first then installs the new product. This is inefficient, but safe for refactorings that might otherwise change component GUIDs. Doubt that's happened in a very long time, though - not since I did a bunch of preview vs. stable refactorings a long time ago. May not be worth retaining that behavior anymore.

[heaths]

This seems unnecessarily complicated. If the property is not defined to any value, it's already unset by Windows Installer's definition, thus not needing all the explicit ...=1 conditions below.

It's also fine to just bind the actual property to the RegistrySearch. See https://robmensching.com/blog/posts/2010/5/2/the-wix-toolsets-remember-property-pattern/.

[reduckted]

The reason for this complexity is explained in the comments above. What that "Remember Property" pattern doesn't handle is allowing the option to be changed via a checkbox in the UI. If a property has a value, the checkbox that uses that property will be initially checked. It doesn't matter whether the property's value is 0 or 1 or something else; if the property exists, the checkbox is checked. So if the option was disabled, we would remember the property value as 0. On the next install, that property would be initialized to the value of 0, but because the property exists, the checkbox would initially be checked.

I'm definitely not a WiX expert, so if there's a simpler way to do this, I'd love to know.

[heaths]

If 0 is checking the check box, it could be that the Integer attribute needs to be set on it. It should be able to bind directly if treated as an integer instead of the default string (which is set if it has any value since 0 is actually "0" in that case).

[reduckted]

😞 Unfortunately that didn't help. I tried it out on the checkbox for ADD_FILE_CONTEXT_MENU_RUNPOWERSHELL like so:

<Control Id="ContextMenuRunPowerShell" Type="CheckBox" X="20" Y="160" Width="290" Height="17" Property="ADD_FILE_CONTEXT_MENU_RUNPOWERSHELL" Integer="yes" CheckBoxValue="1" Text="Add '$(var.PowerShellFileContextSubMenuDialogText)' context menu for PowerShell files"/>

And I reduced the property initialization back to just getting the previous value and restoring the previous value:

<Property Id="PREVIOUS_ADD_FILE_CONTEXT_MENU_RUNPOWERSHELL">
  <RegistrySearch Id="GetSavedAddFileContextMenuRunPowerShell" Root="HKLM" Key="$(var.InstallerPropertiesKey)" Name="AddFileContextMenuRunPowerShell" Type="raw" />
</Property>
<SetProperty Id="ADD_FILE_CONTEXT_MENU_RUNPOWERSHELL" Action="AddFileContextMenuRunPowerShell_SetFromSaved" After="AppSearch" Value="[PREVIOUS_ADD_FILE_CONTEXT_MENU_RUNPOWERSHELL]" Sequence="first">
  PREVIOUS_ADD_FILE_CONTEXT_MENU_RUNPOWERSHELL
</SetProperty>

Running the installer like this...

msiexec /i PowerShell-7.4.2-test-win-x64.msi ADD_FILE_CONTEXT_MENU_RUNPOWERSHELL=0

... causes the checkbox to be initially checked. So does:

• Running with ADD_FILE_CONTEXT_MENU_RUNPOWERSHELL=1.
• Having the registry value AddFileContextMenuRunPowerShell equal to 0.
• Having the registry value AddFileContextMenuRunPowerShell equal to 1.

[heaths]

That's unfortunate. I'm reaching out to a colleague who has done more Windows Installer UI, but it could be that the extra CAs are needed. It'd be nice to avoid, though. I'll flag this for follow up.

[barnson]

MSI, both in general and specifically for check boxes, uses the logic that a null string is false and anything else is true.

The selected state of the box sets the property either to the value specified in the Value column of the CheckBox table or to the initial value of the property specified in the Property table.

So this behavior seems consistent.

[heaths]

I wonder how the Integer flag is actually supposed to work then.

I'll re-review this, but might play around a bit with a scratch package. The number of CAs necessary to make this work seem like overkill, but maybe not. What you have certainly works.

[pull-request-quantifier-deprecated]

This PR has 205 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

---

Quantification details

Label      : Large
Size       : +192 -13
Percentile : 60.5%

Total files changed: 2

Change summary by file extension:
.wxs : +174 -13
.ps1 : +18 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

• Fast and predictable releases to production:
  • Optimal size changes are more likely to be reviewed faster with fewer
    iterations.
  • Similarity in low PR complexity drives similar review times.
• Review quality is likely higher as complexity is lower:
  • Bugs are more likely to be detected.
  • Code inconsistencies are more likely to be detected.
• Knowledge sharing is improved within the participants:
  • Small portions can be assimilated better.
• Better engineering practices are exercised:
  • Solving big problems by dividing them in well contained, smaller problems.
  • Exercising separation of concerns within the code changes.

What can I do to optimize my changes

• Use the PullRequestQuantifier to quantify your PR accurately
  • Create a context profile for your repo using the context generator
  • Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
  • Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
  • Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
• Change your engineering behaviors
  • For PRs that fall outside of the desired spectrum, review the details and check if:
    • Your PR could be split in smaller, self-contained PRs instead
    • Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

• One line was added: +1 -0
• One line was deleted: +0 -1
• One line was modified: +1 -1 (git diff doesn't know about modified, it will
  interpret that line like one addition plus one deletion)
• Change percentiles: Change characteristics (addition, deletion, modification)
  of this PR in relation to all other PRs within the repository.

---

Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

[StevenBucher98]

@heaths if you have time for a re-review that would be awesome 😄

[heaths]

probably also related to the installation options: the installer wipes and replaces the PS7 shortcut in the start menu even if nothing was changed in the executable path name or arguments, but when it replaces the shortcut it does not re-create the advanced properties that the old shortcut had been assigned by the system administrator. (run as administrator, file classification infrastructure data stream)

#21466

@R-Adrian in short, that's how Windows Installer works but there's an easy (and typical) workaround. See #21466 (comment).

[floh96]

@StevenBucher98 @heaths approved the pr. Can you help to speed up the review from the powershell team?

[SteveL-MSFT]

Based on the discussion and the resulting changes, LGTM

[TravisEz13]

/azp run PowerShell-CI-macos

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[microsoft-github-policy-service]

📣 Hey @reduckted, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗 https://aka.ms/PSRepoFeedback

Microsoft Forms