Add support for DisplayInstallWarnings #2217

ryfu-msft · 2022-06-03T22:30:40Z

Resolves #1786

Changes:

Adds support for prompting the user with a warning message if the DisplayInstallWarnings field is set to true. The message notifies the user that the installer may interfere with running applications
Added --ignore-warnings flag to allow users to override the default behavior of warning prompts

Tests:

Added unit tests to verify that prompts are called correctly with the right message.

Microsoft Reviewers: Open in CodeFlow

yao-msft · 2022-06-03T23:30:49Z

This change will probably block com callers with packages having warnings. We need to expose something in InstallOptions in Com interface

yao-msft · 2022-06-03T23:32:37Z

src/AppInstallerCLICore/Argument.cpp

@@ -97,6 +97,8 @@ namespace AppInstaller::CLI
            return Argument{ "wait", NoAlias, Args::Type::Wait, Resource::String::WaitArgumentDescription, ArgumentType::Flag, false };
        case Args::Type::ProductCode:
            return Argument{ "product-code", NoAlias, Args::Type::ProductCode, Resource::String::ProductCodeArgumentDescription, ArgumentType::Standard, false };
+        case Args::Type::IgnoreWarnings:
+            return Argument{ "ignore-warnings", NoAlias, Args::Type::IgnoreWarnings, Resource::String::IgnoreWarningsArgumentDescription, ArgumentType::Flag, false };


nit: I think "ignore-install-warnings" would be more accurate, "ignore-warnings" seems too generic. Also please update the variable name to leave room for future warnings

nit: I think "ignore-install-warnings" would be more accurate, "ignore-warnings" seems too generic. Also please update the variable name to leave room for future warnings

Also you'll need to expose them in each applicable command.

I think this might be getting back more to the root issue that --force (specifically for Hash Overrides and other actions with security concerns) should be changed to be less generic. As a user, I would expect that --force would act as a general ignore warnings, especially so in this case.

I know that it would likely be a breaking change, but just throwing my thoughts here so @denelon can assess if / when it may be appropriate to make that change, given that it has implications on implementation of other features such as this one

I've seen a couple of features reported with similar concerns like:

Cannot override installer hash as admin. Even if EnableLua = 0 (UAC OFF) #715

I just added it to:

Breaking changes to properly justify a 2.0 release #2128

Breaking or not, we should change --force per this discussion: #2112

Changed to ignore-install-warnings and expose the option in the install and upgrade commands

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

yao-msft · 2022-06-04T00:43:44Z

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

@@ -1349,4 +1349,10 @@ Please specify one of them using the `--source` option to proceed.</value>
  <data name="ShowLabelInstallationNotes" xml:space="preserve">
    <value>InstallationNotes:</value>
  </data>
+  <data name="IgnoreWarningsArgumentDescription" xml:space="preserve">
+    <value>Disables warning prompts</value>


Disables warning prompts

nit: Disables install warning prompts

Changed to Disables install warnings

yao-msft

🕐

yao-msft · 2022-06-06T22:13:20Z

Since this field is per installer level, maybe for better user experience, shall we change the installer selection logic to pick installers without warnings over warnings?

Trenly · 2022-06-07T00:47:00Z

Since this field is per installer level, maybe for better user experience, shall we change the installer selection logic to pick installers without warnings over warnings?

I would say no - I wouldn't want winget to select an x86 installer for a x64 system if the x64 installer had installer warnings. Similarly, I wouldn't want the warnings to change a user scoped installation to a machine scoped installation, just because the machine scope didn't have warnings. I could see adding a setting to always suppress the warnings, but I don't think that changing the selection behavior based on a warning is a good idea, since it isn't a guarantee that the application is running when the user attempts the upgrade

yao-msft · 2022-06-07T04:20:20Z

Since this field is per installer level, maybe for better user experience, shall we change the installer selection logic to pick installers without warnings over warnings?

I would say no - I wouldn't want winget to select an x86 installer for a x64 system if the x64 installer had installer warnings. Similarly, I wouldn't want the warnings to change a user scoped installation to a machine scoped installation, just because the machine scope didn't have warnings. I could see adding a setting to always suppress the warnings, but I don't think that changing the selection behavior based on a warning is a good idea, since it isn't a guarantee that the application is running when the user attempts the upgrade

I agree, the installer selection logic actually has different levels of priority when determining which installer to pick, the priority of the InstallWarnings should always be in the lowest. Only when 2 installers have same scope, arch etc would the InstallWarnings be considered a factor. But given how rare this would be, I'm ok with not implementing anything. Just throwing some thought out.

JohnMcPMS

I don't think that this feature has been though through enough. I would expect that many upgrades affect their own package, which is in line with the current message of "interfering with running applications". So the flag is added, then nearly every package prompts you?

I get that it is painful if running an installer messes with active processes, but I don't think this is the right way to deal with that.

JohnMcPMS · 2022-06-07T18:14:34Z

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

+
+        if (displayInstallWarnings && !context.Args.Contains(Execution::Args::Type::IgnoreWarnings) && !Settings::User().Get<Settings::Setting::InstallIgnoreWarnings>())
+        {
+            bool ignoreWarning = context.Reporter.PromptForBoolResponse(Resource::String::InstallWarning);


I'm not sure what the use case is here, but I don't like prompting on a warning. At a minimum I would think that the setting should be ignore/display/prompt with the default being display.

ignore = Don't show them at all
display = Display the warning once the installer is chosen (before download) which gives an interactive user a fair amount of time to react with CTRL+C
prompt = This behavior of prompting after displaying the warnings.

I agree that not prompting is better, especially knowing that WinGet is used in many autonomous install/upgrade scenarios

Changed settings to take in an enum for installWarning level with ignore, display, and prompt as the possible values. Changed install warning behavior as suggested

JohnMcPMS · 2022-06-07T18:19:29Z

src/AppInstallerCLICore/Argument.cpp

@@ -97,6 +97,8 @@ namespace AppInstaller::CLI
            return Argument{ "wait", NoAlias, Args::Type::Wait, Resource::String::WaitArgumentDescription, ArgumentType::Flag, false };
        case Args::Type::ProductCode:
            return Argument{ "product-code", NoAlias, Args::Type::ProductCode, Resource::String::ProductCodeArgumentDescription, ArgumentType::Standard, false };
+        case Args::Type::IgnoreWarnings:
+            return Argument{ "ignore-warnings", NoAlias, Args::Type::IgnoreWarnings, Resource::String::IgnoreWarningsArgumentDescription, ArgumentType::Flag, false };


Breaking or not, we should change --force per this discussion: #2112

ghost · 2022-06-14T21:03:57Z

@ryfu-msft this pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

Ryan Fu added 2 commits June 3, 2022 11:37

initial commit for display warning

ce49746

add support for displaying install warnings

46b5aa7

ryfu-msft requested a review from a team as a code owner June 3, 2022 22:30

ghost added Area-Manifest This may require a change to the manifest Issue-Feature This is a feature request for the Windows Package Manager client. labels Jun 3, 2022

yao-msft reviewed Jun 3, 2022

View reviewed changes

src/AppInstallerCLICore/Workflows/InstallFlow.cpp Show resolved Hide resolved

yao-msft reviewed Jun 4, 2022

View reviewed changes

yao-msft requested changes Jun 4, 2022

View reviewed changes

ghost added the Needs-Author-Feedback Issue needs attention from issue or PR author label Jun 4, 2022

respond to initial feedback

a8c35a3

JohnMcPMS requested changes Jun 7, 2022

View reviewed changes

add initial com support

6aea4b6

ghost added the No-Recent-Activity Issue has no recent activity label Jun 14, 2022

Improve display warning feature

67caccc

ghost removed Needs-Author-Feedback Issue needs attention from issue or PR author No-Recent-Activity Issue has no recent activity labels Jun 16, 2022

resolve merge conflicts

c221e8f

ryfu-msft requested review from JohnMcPMS and yao-msft June 16, 2022 16:25

resolve merge conflicts

cd01d61

ryfu-msft marked this pull request as draft June 28, 2022 16:56

Trenly mentioned this pull request Jul 28, 2022

Spotify.Spotify does not install or update if winget is running in admin mode #1452

Closed

Trenly mentioned this pull request Aug 8, 2022

discord upgrade #2006

Closed

ryfu-msft closed this Feb 13, 2023

ryfu-msft deleted the displayWarning branch February 29, 2024 18:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for DisplayInstallWarnings #2217

Add support for DisplayInstallWarnings #2217

ryfu-msft commented Jun 3, 2022 •

edited by ghost

Loading

yao-msft commented Jun 3, 2022

yao-msft Jun 3, 2022 •

edited

Loading

yao-msft Jun 3, 2022

Trenly Jun 6, 2022

denelon Jun 7, 2022

JohnMcPMS Jun 7, 2022

ryfu-msft Jun 16, 2022

yao-msft Jun 4, 2022

ryfu-msft Jun 16, 2022

yao-msft left a comment

yao-msft commented Jun 6, 2022

Trenly commented Jun 7, 2022

yao-msft commented Jun 7, 2022

JohnMcPMS left a comment

JohnMcPMS Jun 7, 2022

Trenly Jun 7, 2022

ryfu-msft Jun 16, 2022

JohnMcPMS Jun 7, 2022

ghost commented Jun 14, 2022

Add support for DisplayInstallWarnings #2217

Add support for DisplayInstallWarnings #2217

Conversation

ryfu-msft commented Jun 3, 2022 • edited by ghost Loading

Microsoft Reviewers: Open in CodeFlow

yao-msft commented Jun 3, 2022

yao-msft Jun 3, 2022 • 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

Choose a reason for hiding this comment

yao-msft left a comment

Choose a reason for hiding this comment

yao-msft commented Jun 6, 2022

Trenly commented Jun 7, 2022

yao-msft commented Jun 7, 2022

JohnMcPMS 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

ghost commented Jun 14, 2022

ryfu-msft commented Jun 3, 2022 •

edited by ghost

Loading

yao-msft Jun 3, 2022 •

edited

Loading