Added argument to control whether to upgrade packages if they have "unknown" versions #1765

jedieaston · 2021-12-02T16:56:09Z

Have you signed the Contributor License Agreement?
Are you working against an Issue?

Related to #1692.

This PR adds a argument, --include-unknown, to winget upgrade. This argument controls whether to evaluate packages for upgrade if they have "Unknown" versions (basically, if the version is not set in the ARP table). This resolves a lot of user confusion where every time they run winget upgrade --all, packages will be upgraded regardless of whether they actually need it because winget is not sure if it needs an upgrade or not.

I know nobody from MS commented on that issue, so if anyone has issues with this solution, I'm happy to redo this. The issue also has some discussion on whether or not upgrades for unknown packages should be shown by default in the upgrade table. The way I have it, the argument must be used for the upgrade to show in the table.

Tested: manually. As always, I'm happy to write unit tests if needed.

Microsoft Reviewers: Open in CodeFlow

src/AppInstallerCLICore/Workflows/UpdateFlow.cpp

JohnMcPMS

@denelon , does this seem like fine behavior to you? Nothing springs to mind as to why we wouldn't want it.

@lechacon , does there need to be additional work done for import? It does upgrade-like things but I don't think it uses this code path. If it would only attempt to upgrade in the event that a specific version was given in the import file, then I think it is probably ok to leave as is, even if that means attempting to re-install.

src/AppInstallerCLICore/Commands/UpgradeCommand.cpp

JohnMcPMS · 2021-12-02T21:46:04Z

src/AppInstallerCLICore/Commands/UpgradeCommand.cpp

@@ -46,6 +47,7 @@ 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 }


@denelon for argument name review.

Personally, I find the argument name somewhat awkward for a single app: winget upgrade SomeApp --include-unknown
For upgrade --all I understand it as "include apps with unknown version", but I don't quite get what we are "including" for a single app.

I think if the user explicitly specifies an app, --include-unknown would/should be redundant.

I also think winget upgrade AwesomePackage --all would be similarly strange.

The current behavior is that upgrade <ID> doesn’t upgrade a package if already at latest. I had the argument behavior stay the same so the expectation of winget not doing an upgrade unless it has to holds.

Community opinion here; I would think that if I specify an ID, it should attempt to be upgraded even if the version is unknown. I would think that even if the version doesn't change, from a user perspective, an upgrade was at least attempted. It could be very confusing if the user knows there is an update, and they try to upgrade it only to be told that no applicable update was found.

There's also a message that prints:

PS C:\Users\easton> wingetdev upgrade GOG.Galaxy This package's version number cannot be determined. To upgrade it anyway, add the argument --include-unknown to your previous command. PS C:\Users\easton>

There was some discussion in the issue about whether or not there should be a setting to restore the current behavior (when in doubt, upgrade), but by default I think it should prompt the user to make a decision. The infinite reupgrading is causing more confusion than this is, IMO.

Ahh, I must have missed that. I would be fine with that approach, since it is clear and informative

src/AppInstallerCLICore/Workflows/UpdateFlow.cpp

florelis · 2021-12-02T22:41:49Z

@lechacon , does there need to be additional work done for import? It does upgrade-like things but I don't think it uses this code path. If it would only attempt to upgrade in the event that a specific version was given in the import file, then I think it is probably ok to leave as is, even if that means attempting to re-install.

Yes, there needs to be some work done for import as it doesn't use this code path. If there is no specific version in the file, I think import will try to update to the latest.

Import does call EnsureUpdateVersionApplicable, so we could do the unknown version check there and import would also pick it up. There are also some upgrade flows that don't use SelectLatestApplicableUpdate but do use EnsureUpdateVersionApplicable, like updating from a manifest or to a specific version; so maybe we need it there regardless.

denelon · 2021-12-02T22:42:17Z

The current behavior lists everything.

This PR could be considered a breaking change. I'm leaning in the direction of agreeing to do this since most of the packages listed from the "msstore" source today have version: "Unknown" and are expected to upgrade themselves. We are working towards getting versions from the Microsoft Store, so this will improve over time.

@JohnMcPMS does this code path get used for winget list? We should be consistent in both places.

I'm ok with "--include-unknown". I think this will help much more than hurt or confuse.

It might help to include a count for the number of packages with an unknown version.

PS C:\Users\denelon>winget upgrade
No installed package found matching input criteria. <n> packages specify "Unknown" version. Use "--include unknown" to see all results.

I would add the " packages... Use..." at the end of the winget upgrade --all as well. That will help make this option more discoverable. If we do this for winget list it should be there as well.

Note, we should ensure the value "Unknown" is appropriately defined for a private REST source so the behavior extends there as well @RDMacLachlan.

denelon · 2021-12-02T22:43:50Z

@lechacon we should consider this for import as well.

Import command option to not upgrade installed packages #1763

jedieaston · 2021-12-02T23:23:50Z

does this code path get used for winget list? We should be consistent in both places.

It appears to, but since the argument doesn't exist for winget list it will not show an available upgrade for applications with no version number. The app still is shown in the list, it just doesn't check for an upgrade. I can add the argument there too if you want to keep it consistent.

…li into benefit-of-the-doubt

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

Co-authored-by: Chacón <lechacon@users.noreply.github.com>

denelon · 2021-12-03T00:14:45Z

"Unknown" is considered to be a sentinel value meaning the lowest version of a package. When the installed version is also Unknown, no upgrade is shown as available.

So, there shouldn't be a case where an available package with version "Unknown" is displayed as a potential upgrade in list when there is a version present in Apps & Features.

I am just trying to make sure we capture all of the scenarios:

An installed version is known, and the only available package version is "Unknown"
An installed version is known, and the latest version of an available package is lower.
An installed version is known, and the latest version of an available package is higher.
An installed version is Unknown.

I believe the work here is for the last scenario.

@JohnMcPMS to keep me honest, or at least correct me if I'm mistaken.

jedieaston · 2021-12-03T16:21:04Z

@denelon, does this look good to you?

Edit: note that the last commit doesn't output this message on upgrade --all, so I'll do that this afternoon when I have some time.

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

florelis · 2021-12-03T19:20:12Z

src/AppInstallerCLICore/Workflows/WorkflowBase.cpp

@@ -766,6 +774,10 @@ namespace AppInstaller::CLI::Workflow
                context.Reporter.Info() << availableUpgradesCount << ' ' << Resource::String::AvailableUpgrades << std::endl;


I see that you are using different strings to report the count depending on whether there is a single package or multiple, but here we were using the same string for both cases. I think we should be doing the same for both messages; although I'm not sure which one would be better. I would lean towards using a single string regardless of the number of packages as I don't know if there are any languages that make any distinction beyond 1/many or that don't make distinctions at all.

I looked a bit into pluralization and it's a whole rabbit hole one can go into. I found it interesting to look at the rules for all languages...
I found some internal references to string formats for putting all variants in a single string and using libraries for handling that. @JohnMcPMS does the current setup support something like that? @JohnMcPMS , @denelon , is this something we would want to handle?

I saw that too, but figured it was a separate PR... thanks for pointing it out :D

If we wanted to do something to handle plurals programmatically, then we could probably do it at the same time as the dynamic string replacement mentioned above. I don't see it as important as the dynamic strings though, so depending on the complexity we might just leave a nice place for it. I don't know if the english standard of making it "optional" like:

N installed package version(s) cannot be determined.

applies to the other supported languages. But it might be an option.

Should I go ahead and use a optional plural for now? SourceOpenFailedSuggestion uses it ("Failed when opening source(s); try the 'source reset' command if the problem persists.")...

That sounds good to me.

jedieaston · 2021-12-08T17:02:48Z

@lechacon Do you want me to add the argument to import as well? The only case where upgrade only uses EnsureUpdateVersionApplicable and not SelectLatestApplicableUpdate is where the user specifies a version, and in that scenario, they probably want to get to that version and winget telling them "I don't know if that's an upgrade or not..." wouldn't be helpful. But if we need the logic for import, might as well make it work the same way in both places...

src/AppInstallerCLICore/Commands/UpgradeCommand.cpp

florelis · 2021-12-08T22:44:40Z

@lechacon Do you want me to add the argument to import as well? The only case where upgrade only uses EnsureUpdateVersionApplicable and not SelectLatestApplicableUpdate is where the user specifies a version, and in that scenario, they probably want to get to that version and winget telling them "I don't know if that's an upgrade or not..." wouldn't be helpful. But if we need the logic for import, might as well make it work the same way in both places...

It makes sense to me to not consider if the current version is unknown when updating to a specific version.

If the installed version is unknown and we are installing specific versions from the json, I would agree with making it work the same as upgrade, which I'm fine if that is always updating from unknown versions. If we are installing the latest, I think the behavior would be better driven by whether the package is already installed (#1763) and not by whether it has unknown version.

So I don't think we need the --include-unknown argument for import.

florelis · 2021-12-08T23:02:32Z

/azp run

azure-pipelines · 2021-12-08T23:02:42Z

Azure Pipelines successfully started running 1 pipeline(s).

florelis

jedieaston added 3 commits November 29, 2021 13:44

Added --include-unknown command to upgrade.

56f1e9f

Merged upstream.

892a5e5

Added documentation of new arg, and moved if.

eb3d2f0

jedieaston requested a review from a team as a code owner December 2, 2021 16:56

florelis reviewed Dec 2, 2021

View reviewed changes

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

Fixed grammar for comment.

f80b365

JohnMcPMS reviewed Dec 2, 2021

View reviewed changes

florelis reviewed Dec 2, 2021

View reviewed changes

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

jedieaston added 2 commits December 2, 2021 18:38

Added messaging/loop suggestions from code review.

ae9ffc4

Merge branch 'benefit-of-the-doubt' of github.com:jedieaston/winget-c…

061a201

…li into benefit-of-the-doubt

florelis reviewed Dec 2, 2021

View reviewed changes

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

Apply suggestions from code review (localization)

488f810

Co-authored-by: Chacón <lechacon@users.noreply.github.com>

Added count of unknown packages.

825dac3

Added count to upgrade --all too.

ebd54f9

florelis reviewed Dec 3, 2021

View reviewed changes

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

florelis reviewed Dec 3, 2021

View reviewed changes

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

florelis reviewed Dec 3, 2021

View reviewed changes

Attempted to fix comments/phrasing for localization.

841e769

jedieaston mentioned this pull request Dec 4, 2021

[Bug]: GOG.Galaxy installer does not register DisplayVersion microsoft/winget-pkgs#30134

Closed

Undid change to AppInstallerCLICore project -- sorry!

6dc0982

florelis reviewed Dec 8, 2021

View reviewed changes

src/AppInstallerCLICore/Commands/UpgradeCommand.cpp Outdated Show resolved Hide resolved

florelis approved these changes Dec 8, 2021

View reviewed changes

Removed unnecessary if statement for ShouldListUpgrade.

2d49a69

jedieaston requested a review from JohnMcPMS December 9, 2021 19:25

florelis merged commit 4968df4 into microsoft:master Dec 9, 2021

jedieaston deleted the benefit-of-the-doubt branch December 9, 2021 19:44

florelis mentioned this pull request Dec 13, 2021

Add support for RequireExplicitUpgrade manifest element #1795

Merged

This was referenced Jan 12, 2022

[Bug]: winget cannot find pokemon showdown version and always thinks there is an available upgrade microsoft/winget-pkgs#40529

Closed

[Bug]: ClipX is reinstalling at each upgrade microsoft/winget-pkgs#40779

Closed

OfficialEsco mentioned this pull request Jan 27, 2022

No version number reported in windows safing/portmaster#503

Closed

ItzLevvie mentioned this pull request Jan 30, 2022

winget upgrade --all shouldn't install the same package more than once #1105

Closed

jedieaston mentioned this pull request Jan 31, 2022

winget doesn't recognizes not the installed version of Toggl #1884

Closed

felipecrs mentioned this pull request Feb 16, 2022

Improve --include-unknown message #1946

Merged

2 tasks

denelon mentioned this pull request Apr 28, 2022

Pin a package #476

Closed

Danik1601 mentioned this pull request May 20, 2022

Winget integration is broken msys2/msys2-installer#47

Closed

jedieaston mentioned this pull request Jul 25, 2022

[Package Issue]: Rustlang.Rustup is outdated and can't detect version. microsoft/winget-pkgs#67393

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added argument to control whether to upgrade packages if they have "unknown" versions #1765

Added argument to control whether to upgrade packages if they have "unknown" versions #1765

jedieaston commented Dec 2, 2021 •

edited by ghost

Loading

JohnMcPMS left a comment

JohnMcPMS Dec 2, 2021

florelis Dec 2, 2021

denelon Dec 3, 2021

jedieaston Dec 3, 2021

Trenly Dec 7, 2021

jedieaston Dec 7, 2021

Trenly Dec 7, 2021

florelis commented Dec 2, 2021

denelon commented Dec 2, 2021

denelon commented Dec 2, 2021

jedieaston commented Dec 2, 2021 •

edited

Loading

denelon commented Dec 3, 2021

jedieaston commented Dec 3, 2021 •

edited

Loading

florelis Dec 3, 2021

florelis Dec 3, 2021

jedieaston Dec 3, 2021

JohnMcPMS Dec 3, 2021

jedieaston Dec 6, 2021

florelis Dec 8, 2021

jedieaston commented Dec 8, 2021

florelis commented Dec 8, 2021

florelis commented Dec 8, 2021

azure-pipelines bot commented Dec 8, 2021

florelis left a comment

		@@ -766,6 +774,10 @@ namespace AppInstaller::CLI::Workflow
		context.Reporter.Info() << availableUpgradesCount << ' ' << Resource::String::AvailableUpgrades << std::endl;

Added argument to control whether to upgrade packages if they have "unknown" versions #1765

Added argument to control whether to upgrade packages if they have "unknown" versions #1765

Conversation

jedieaston commented Dec 2, 2021 • edited by ghost Loading

Microsoft Reviewers: Open in CodeFlow

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florelis commented Dec 2, 2021

denelon commented Dec 2, 2021

denelon commented Dec 2, 2021

jedieaston commented Dec 2, 2021 • edited Loading

denelon commented Dec 3, 2021

jedieaston commented Dec 3, 2021 • 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

jedieaston commented Dec 8, 2021

florelis commented Dec 8, 2021

florelis commented Dec 8, 2021

azure-pipelines bot commented Dec 8, 2021

florelis left a comment

Choose a reason for hiding this comment

jedieaston commented Dec 2, 2021 •

edited by ghost

Loading

jedieaston commented Dec 2, 2021 •

edited

Loading

jedieaston commented Dec 3, 2021 •

edited

Loading