Add support for `RequireExplicitUpgrade` manifest element #1795

florelis · 2021-12-13T22:52:24Z

Adding support for skipping packages marked as RequireExplicitUpgrade during an upgrade --all, and an argument --include-explicit to upgrade these packages. This is similar to what #1765 did for unknown installed versions.

Updated argument validation for upgrade to prevent some argument combinations that don't work together (e.g. --manifest with --header) and allow some that should work together (e.g. winget upgrade with --accept-source-agreements). This is related to what I mentioned in Implement better validation of how CLI arguments are used together #1788, but other commands probably still accept weird combinations.
Added logic to upgrade --all to skip packages marked as requiring explicit upgrade unless the argument was specified. Did not do the same for upgrade (list available) as this requires having identified the installer to use, which we don't currently do in that flow.
Added unit tests for the new --include-explicit argument.

Closes #1163

Microsoft Reviewers: Open in CodeFlow

florelis · 2021-12-13T23:38:55Z

src/AppInstallerCLICore/Commands/UpgradeCommand.cpp

@@ -52,7 +103,8 @@ namespace AppInstaller::CLI
            Argument::ForType(Args::Type::AcceptSourceAgreements),
            Argument::ForType(Execution::Args::Type::CustomHeader),
            Argument{ "all", Argument::NoAlias, Args::Type::All, Resource::String::UpdateAllArgumentDescription, ArgumentType::Flag },
-            Argument{ "include-unknown", Argument::NoAlias, Args::Type::IncludeUnknown, Resource::String::IncludeUnknownArgumentDescription, ArgumentType::Flag }
+            Argument{ "include-unknown", Argument::NoAlias, Args::Type::IncludeUnknown, Resource::String::IncludeUnknownArgumentDescription, ArgumentType::Flag },
+            Argument{ "include-explicit", Argument::NoAlias, Args::Type::IncludeExplicit, Resource::String::IncludeExplicitArgumentDescription, ArgumentType::Flag },


@denelon Is this argument name okay?

I don't know of anything better. Can you think of a better term for something that represents packages that update themselves?

--include-pinned? I'd imagine this argument would be used for user pinned packages later, no? And UpgradeRequireExplicitCount uses the word "pinned" to describe packages which use RequireExplicitUpgrade in the manifest.

src/AppInstallerCLICore/Commands/UpgradeCommand.cpp

jedieaston · 2021-12-14T13:12:54Z

src/AppInstallerCLICore/Commands/UpgradeCommand.cpp

@@ -52,7 +103,8 @@ namespace AppInstaller::CLI
            Argument::ForType(Args::Type::AcceptSourceAgreements),
            Argument::ForType(Execution::Args::Type::CustomHeader),
            Argument{ "all", Argument::NoAlias, Args::Type::All, Resource::String::UpdateAllArgumentDescription, ArgumentType::Flag },
-            Argument{ "include-unknown", Argument::NoAlias, Args::Type::IncludeUnknown, Resource::String::IncludeUnknownArgumentDescription, ArgumentType::Flag }
+            Argument{ "include-unknown", Argument::NoAlias, Args::Type::IncludeUnknown, Resource::String::IncludeUnknownArgumentDescription, ArgumentType::Flag },
+            Argument{ "include-explicit", Argument::NoAlias, Args::Type::IncludeExplicit, Resource::String::IncludeExplicitArgumentDescription, ArgumentType::Flag },


--include-pinned? I'd imagine this argument would be used for user pinned packages later, no? And UpgradeRequireExplicitCount uses the word "pinned" to describe packages which use RequireExplicitUpgrade in the manifest.

JohnMcPMS

The determination of whether explicit upgrade is required needs to be sorted out. I would lean towards using the tracking catalog because it makes the delta to having user driven pinning very small. The drawback being that if the package isn't installed through us, or was installed prior to this feature, or an update to the manifest changes it, we won't have the correct information. But I think those are acceptable, and even recoverable by the user with pinning, situations.

src/AppInstallerCLICore/Workflows/UpdateFlow.cpp

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

glen-84 · 2022-01-29T08:05:08Z

Did not do the same for upgrade (list available) as this requires having identified the installer to use, which we don't currently do in that flow.

Is there an issue to fix this that I can follow? I usually use upgrade to see what upgrades are available, and then update applications one by one. It'll be annoying to have to keep ignoring certain packages like Microsoft Edge WebView2 Runtime.

denelon · 2022-01-31T17:25:05Z

@glen-84 the issue is linked in this PR unless I'm misunderstanding what you're asking. It's #1163

glen-84 · 2022-01-31T18:38:56Z

@denelon I'm referring specifically to the quoted text. Closing this PR will close #1163, but it won't be applied to winget upgrade, only winget upgrade --all. I wanted to track when it might be supported there as well.

denelon · 2022-01-31T18:45:22Z

I believe winget upgrade will show "everything" that would be upgraded if you ran winget upgrade --all. The exception here is for packages that upgrade themselves like Visual Studio Code.

We were thinking that "pin" would exclude packages from being upgraded even if an upgrade was available unless you unpinned them. Users would explicitly pin things they don't want upgraded.

The "Unknown" version problem is being handled slightly differently. We're adding additional arguments to be able to display packages that don't report a version to Windows Apps & Features, and to go ahead and "try" to upgrade them. We still wouldn't know if it was an upgrade, so it would be more like just running winget upgrade on those specific packages and hoping the version we have in the source is newer. There really isn't any way to know that until the installers actually report the version installed.

glen-84 · 2022-01-31T18:56:09Z

I believe winget upgrade will show "everything" that would be upgraded if you ran winget upgrade --all. The exception here is for packages that upgrade themselves like Visual Studio Code.

I understood @lechacon's description to mean that winget upgrade will still list packages marked as RequireExplicitUpgrade.

It might be useful to have the option to filter those out. For example, if VSC or WebView2 will be upgraded separately, you don't necessarily need to see them listed as requiring an upgrade. They could still be listed by default though, but perhaps it should indicate which packages require an explicit upgrade, so that you can at least mentally ignore them when looking at the list.

JohnMcPMS · 2022-01-31T19:28:23Z

I agree with @glen-84 , we should still show them by default with winget upgrade but separate them to indicate that they won't be affected by upgrade --all. I would rather split them out than mark them with a * or something. My suggestion is something like:

> winget upgrade
Name                                          Id                                     Version       Available     Source
-----------------------------------------------------------------------------------------------------------------------
Windows Package Manager Manifest Creator      Microsoft.WingetCreate                 0.1.0.6       0.4.4.1       winget
Visual Studio Enterprise 2019                 Microsoft.VisualStudio.2019.Enterprise 16.11.7       16.11.9       winget
Microsoft Visual C++ 2015-2019 Redistributab… Microsoft.VC++2015-2019Redist-x64      14.29.30133.0 14.29.30139.0 winget
Microsoft Visual C++ 2015-2019 Redistributab… Microsoft.VC++2015-2019Redist-x86      14.29.30133.0 14.29.30139.0 winget
Microsoft Visual Studio Code                  Microsoft.VisualStudioCode             1.62.3        1.63.2        winget
Adobe Acrobat Reader DC                       Adobe.Acrobat.Reader.32-bit            21.007.20091  21.011.20039  winget
PowerToys (Preview)                           Microsoft.PowerToys                    0.45.0        0.53.3        winget

The following packages have an upgrade available, but require explicit targeting for upgrade:
Name                                          Id                                     Version       Available     Source
-----------------------------------------------------------------------------------------------------------------------
Microsoft Azure CLI                           Microsoft.AzureCLI                     2.27.0        2.32.0        winget

denelon · 2022-01-31T19:34:08Z

We should add another table for "pinned" packages along with that feature.

#476

JohnMcPMS · 2022-02-01T01:05:56Z

We should add another table for "pinned" packages along with that feature.

#476

I would prefer to treat manually pinned and default pinned through RequiresExplicitUpgrade the same to the user. Thus they would both appear in the lower section and could all be "unpinned" if desired.

denelon · 2022-02-01T01:18:58Z

We should add another table for "pinned" packages along with that feature.
#476

I would prefer to treat manually pinned and default pinned through RequiresExplicitUpgrade the same to the user. Thus they would both appear in the lower section and could all be "unpinned" if desired.

I think of "pinning" an explicit request made by a user. They are saying "don't" upgrade this package. If the package updates itself as specified in the manifest which is what this is covering, the user wouldn't be able to "pin" it since we couldn't guarantee to honor the request. Visual Studio Code is the example for this. It doesn't make sense to have users upgrade it with the Windows Package Manager since the program will update itself the next time they launch.

There may be some confusion in using the term "pin" in the description.

JohnMcPMS · 2022-02-01T01:57:09Z

I think of "pinning" an explicit request made by a user. They are saying "don't" upgrade this package. If the package updates itself as specified in the manifest which is what this is covering, the user wouldn't be able to "pin" it since we couldn't guarantee to honor the request. Visual Studio Code is the example for this. It doesn't make sense to have users upgrade it with the Windows Package Manager since the program will update itself the next time they launch.

There may be some confusion in using the term "pin" in the description.

We don't know or control what happens after installation begins. We can't honor any request to pin because of this. Thus, semantically these are the same thing; don't attempt to upgrade this package unless the user explicitly tells us to. But I agree that we shouldn't use the term "pin" in our usage anywhere except the command as a verb. You pin a package, but it is not necessarily pinned. It could just as well be:

winget pin <ID>
Pin a package.  This will exclude the package from winget upgrade --all, requiring an explicit
winget upgrade <ID> command in order for an upgrade to be attempted.

florelis · 2022-07-29T22:29:25Z

Reviving this PR with changes per the comments..

Changed the parameter to --include-pinned
Included the list of pinned packages in the list of available upgrades
Changed the logic for determining whether a package requires an explicit upgrade to use a value from the metadata in the tracking catalog instead of the manifest being installed. I made the flag thinking of being able to add a state "pinned by user"

denelon · 2022-07-29T22:34:58Z

We don't have pinning, and this isn't the same thing. I would use --include-pinned in this use case.

src/AppInstallerCLICore/Workflows/WorkflowBase.cpp

JohnMcPMS · 2022-08-01T19:04:28Z

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

@@ -1272,6 +1272,17 @@ Please specify one of them using the `--source` option to proceed.</value>
    <value>package has a version number that cannot be determined. Using "--include-unknown" may show more results.</value>
    <comment>{Locked="--include-unknown"} This string is preceded by a (integer) number of packages that do not have notated versions.</comment>
  </data>
+  <data name="UpgradeUnknownVersionCount" xml:space="preserve">
+    <value>package(s) have version numbers that cannot be determined. Use "--include-unknown" to see all results.</value>


@AmelBawa-msft is working to fix our lack of placeholders in localization strings. This family of strings will need to be updated and this comment is mostly a note for him.

src/AppInstallerRepositoryCore/Public/winget/RepositorySearch.h

JohnMcPMS · 2022-08-01T19:12:10Z

We don't have pinning, and this isn't the same thing. I would use --include-pinned in this use case.

I assume you meant "I would not use...".

But the manifest entry is implicit pinning, and I don't think we want to create a parallel feature. Adding end-user controlled pinning should simply allow the default to be overridden. The default is currently "not pinned ever", this PR enables the default to be package/installer specific.

denelon · 2022-08-01T20:09:23Z

@JohnMcPMS you are correct. I think the confusion would come from using "--include-pinned" for winget upgrade --all. Truly pinned packages wouldn't be upgraded unless the pin were removed. These packages could be included for upgrades, we're just excluding them from winget upgrade --all when they are known to upgrade themselves.

denelon · 2022-08-01T20:21:27Z

The example packages I was thinking about are Visual Studio Code and Edge Web View 2 Runtime. These packages upgrade themselves and may not work and play well when manually upgraded. I'm sure there are others. I've also seen some cases with other browsers and tools that have more than one package being upgraded with automatic updates.

JohnMcPMS · 2022-08-01T20:33:22Z

Truly pinned packages wouldn't be upgraded unless the pin were removed.

I disagree, manually pinned packages should be able to be explicitly upgraded. I don't see pinning as setting read-only on the package, just moving to a separate list to prevent normal flow from upgrading it.

We could make an additional level of pinning that prevents any modifying interaction, but how far does that guarantee need to go? I don't think it is actually necessary.

Back to --include-pinned; I would have no problem simply removing the entire concept from the PR. I'm not sure that there is actually a large use case for it, since blindly upgrading any "pinned" package defeats the entire point of the pinning. Forcing every package to be named individually would prevent a new package with RequireExplicitUpgrade (or manually pinned) from suddenly being included and breaking things.

florelis requested a review from a team as a code owner December 13, 2021 22:52

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 Dec 13, 2021

florelis commented Dec 13, 2021

View reviewed changes

jedieaston reviewed Dec 14, 2021

View reviewed changes

florelis marked this pull request as draft January 5, 2022 19:05

JohnMcPMS requested changes Jan 19, 2022

View reviewed changes

src/AppInstallerCLICore/Workflows/UpdateFlow.cpp Outdated Show resolved Hide resolved

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw Outdated Show resolved Hide resolved

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

florelis marked this pull request as ready for review January 22, 2022 06:09

ghost removed the Needs-Author-Feedback Issue needs attention from issue or PR author label Jan 22, 2022

florelis mentioned this pull request Jan 24, 2022

Added extra check for valid arguments in upgrade. #1874

Merged

2 tasks

jedieaston added a commit to jedieaston/winget-cli that referenced this pull request Jan 25, 2022

Merged updated validation logic from microsoft#1795.

77df490

werrnerben approved these changes Feb 16, 2022

View reviewed changes

florelis marked this pull request as draft February 17, 2022 19:20

Flor Elisa Chacon Ochoa and others added 6 commits July 27, 2022 14:33

Respect RequireExplicitUpgrade manifest entry

6c6ccd3

Add tests

2f599cb

Show separate table for explicit upgrade

b62d2bc

Rename include-explicit to include-pinned

ae07b13

Store state in tracking catalog

3c2546a

Update tests

bf1ca33

Use enum in tracking catalog

aaf60a9

florelis force-pushed the explicitUpgrade branch from 6290869 to aaf60a9 Compare July 29, 2022 22:23

florelis marked this pull request as ready for review July 29, 2022 22:29

JohnMcPMS approved these changes Aug 1, 2022

View reviewed changes

florelis added 2 commits August 1, 2022 16:09

PR comments

a130778

Remove --include-pinned arg

9ac8336

florelis merged commit d0ff81e into microsoft:master Aug 3, 2022

florelis deleted the explicitUpgrade branch August 3, 2022 21:47

Trenly mentioned this pull request Sep 29, 2022

RequireExplicitUpgrade not requiring explicit upgrade #2559

Closed

o-l-a-v mentioned this pull request Feb 21, 2023

Get-WinGetPackageUpdate if results contains packages that require explicit targeting for upgrade ethanbergstrom/Cobalt#22

Closed

Romanitho mentioned this pull request Mar 18, 2023

[Bug]: Notification text is empty Romanitho/Winget-AutoUpdate#276

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `RequireExplicitUpgrade` manifest element #1795

Add support for `RequireExplicitUpgrade` manifest element #1795

florelis commented Dec 13, 2021 •

edited by ghost

Loading

florelis Dec 13, 2021

denelon Dec 14, 2021

jedieaston Dec 14, 2021

jedieaston Dec 14, 2021

JohnMcPMS left a comment

glen-84 commented Jan 29, 2022

denelon commented Jan 31, 2022

glen-84 commented Jan 31, 2022

denelon commented Jan 31, 2022

glen-84 commented Jan 31, 2022

JohnMcPMS commented Jan 31, 2022

denelon commented Jan 31, 2022 •

edited

Loading

JohnMcPMS commented Feb 1, 2022

denelon commented Feb 1, 2022

JohnMcPMS commented Feb 1, 2022

florelis commented Jul 29, 2022

denelon commented Jul 29, 2022

JohnMcPMS Aug 1, 2022

JohnMcPMS commented Aug 1, 2022

denelon commented Aug 1, 2022

denelon commented Aug 1, 2022

JohnMcPMS commented Aug 1, 2022

Add support for RequireExplicitUpgrade manifest element #1795

Add support for RequireExplicitUpgrade manifest element #1795

Conversation

florelis commented Dec 13, 2021 • edited by ghost Loading

Microsoft Reviewers: Open in CodeFlow

florelis Dec 13, 2021

Choose a reason for hiding this comment

denelon Dec 14, 2021

Choose a reason for hiding this comment

jedieaston Dec 14, 2021

Choose a reason for hiding this comment

jedieaston Dec 14, 2021

Choose a reason for hiding this comment

JohnMcPMS left a comment

Choose a reason for hiding this comment

glen-84 commented Jan 29, 2022

denelon commented Jan 31, 2022

glen-84 commented Jan 31, 2022

denelon commented Jan 31, 2022

glen-84 commented Jan 31, 2022

JohnMcPMS commented Jan 31, 2022

denelon commented Jan 31, 2022 • edited Loading

JohnMcPMS commented Feb 1, 2022

denelon commented Feb 1, 2022

JohnMcPMS commented Feb 1, 2022

florelis commented Jul 29, 2022

denelon commented Jul 29, 2022

JohnMcPMS Aug 1, 2022

Choose a reason for hiding this comment

JohnMcPMS commented Aug 1, 2022

denelon commented Aug 1, 2022

denelon commented Aug 1, 2022

JohnMcPMS commented Aug 1, 2022

Add support for `RequireExplicitUpgrade` manifest element #1795

Add support for `RequireExplicitUpgrade` manifest element #1795

florelis commented Dec 13, 2021 •

edited by ghost

Loading

denelon commented Jan 31, 2022 •

edited

Loading