Skip to content

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

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.

Sign up for GitHub

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

Merged
florelis merged 12 commits into from
Dec 9, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
56f1e9f
Added --include-unknown command to `upgrade`.
jedieaston Nov 29, 2021
892a5e5
Merged upstream.
jedieaston Dec 2, 2021
eb3d2f0
Added documentation of new arg, and moved if.
jedieaston Dec 2, 2021
f80b365
Fixed grammar for comment.
jedieaston Dec 2, 2021
ae9ffc4
Added messaging/loop suggestions from code review.
jedieaston Dec 2, 2021
061a201
Merge branch 'benefit-of-the-doubt' of github.com:jedieaston/winget-c…
jedieaston Dec 2, 2021
488f810
Apply suggestions from code review (localization)
jedieaston Dec 2, 2021
825dac3
Added count of unknown packages.
jedieaston Dec 3, 2021
ebd54f9
Added count to upgrade --all too.
jedieaston Dec 3, 2021
841e769
Attempted to fix comments/phrasing for localization.
jedieaston Dec 3, 2021
6dc0982
Undid change to AppInstallerCLICore project -- sorry!
jedieaston Dec 7, 2021
2d49a69
Removed unnecessary if statement for ShouldListUpgrade.
jedieaston Dec 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions doc/windows/package-manager/winget/upgrade.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,6 @@ The **upgrade** command requires that you specify the exact string to upgrade. I

![upgrade command](images/upgrade.png)

## Arguments

The following arguments are available.

| Argument | Description |
|-------------|-------------|
| **-q,--query** | The query used to search for an app. |
| **-?, --help** | Get additional help on this command. |

## Options

The options allow you to customize the upgrade experience to meet your needs.
Expand All @@ -47,6 +38,7 @@ The options allow you to customize the upgrade experience to meet your needs.
| **-l, --location** | Location to upgrade to (if supported). |
| **--force** | When a hash mismatch is discovered will ignore the error and attempt to install the package. |
| **--all** | Updates all available packages to the latest application. |
| **--include-unknown** | Attempt to upgrade a package even if the package's current version is unknown. |
### Example queries

The following example upgrades a specific version of an application.
Expand Down
11 changes: 9 additions & 2 deletions src/AppInstallerCLICore/Commands/UpgradeCommand.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,14 @@ namespace AppInstaller::CLI
{
bool ShouldListUpgrade(Context& context)
{
return context.Args.Empty() ||
(context.Args.GetArgsCount() == 1 && context.Args.Contains(Execution::Args::Type::Source));
for (Execution::Args::Type type : context.Args.GetTypes())
{
if (type != Execution::Args::Type::Source && type != Execution::Args::Type::IncludeUnknown)
{
return false;
}
}
return true;
}
}

Expand All @@ -46,6 +52,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 }
Copy link
Copy Markdown
Member

@JohnMcPMS JohnMcPMS Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@denelon for argument name review.

Copy link
Copy Markdown
Member

@florelis florelis Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

@denelon denelon Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@jedieaston jedieaston Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@Trenly Trenly Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@jedieaston jedieaston Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@Trenly Trenly Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};
}

Expand Down
13 changes: 13 additions & 0 deletions src/AppInstallerCLICore/ExecutionArgs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ namespace AppInstaller::CLI::Execution
DependencySource, // Index source to be queried against for finding dependencies
CustomHeader, // Optional Rest source header
AcceptSourceAgreements, // Accept all source agreements
IncludeUnknown, // Used in Upgrade command to allow upgrades of packages with unknown versions

// Used for demonstration purposes
ExperimentalArg,
Expand Down Expand Up @@ -141,6 +142,18 @@ namespace AppInstaller::CLI::Execution
return m_parsedArgs.size();
}

std::vector<Type> GetTypes()
{
std::vector<Type> types;

for (auto const& i : m_parsedArgs)
{
types.emplace_back(i.first);
}

return types;
}

private:
std::map<Type, std::vector<std::string>> m_parsedArgs;
};
Expand Down
4 changes: 4 additions & 0 deletions src/AppInstallerCLICore/Resources.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ namespace AppInstaller::CLI::Resource
WINGET_DEFINE_RESOURCE_STRINGID(ImportPackageAlreadyInstalled);
WINGET_DEFINE_RESOURCE_STRINGID(ImportSearchFailed);
WINGET_DEFINE_RESOURCE_STRINGID(ImportSourceNotInstalled);
WINGET_DEFINE_RESOURCE_STRINGID(IncludeUnknownArgumentDescription);
WINGET_DEFINE_RESOURCE_STRINGID(InstallAndUpgradeCommandsReportDependencies);
WINGET_DEFINE_RESOURCE_STRINGID(InstallArchitectureArgumentDescription);
WINGET_DEFINE_RESOURCE_STRINGID(InstallationAbandoned);
Expand Down Expand Up @@ -340,6 +341,9 @@ namespace AppInstaller::CLI::Resource
WINGET_DEFINE_RESOURCE_STRINGID(UpgradeCommandShortDescription);
WINGET_DEFINE_RESOURCE_STRINGID(UpgradeDifferentInstallTechnology);
WINGET_DEFINE_RESOURCE_STRINGID(UpgradeDifferentInstallTechnologyInNewerVersions);
WINGET_DEFINE_RESOURCE_STRINGID(UpgradeUnknownCount);
WINGET_DEFINE_RESOURCE_STRINGID(UpgradeUnknownCountSingle);
WINGET_DEFINE_RESOURCE_STRINGID(UpgradeUnknownVersionExplanation);
WINGET_DEFINE_RESOURCE_STRINGID(Usage);
WINGET_DEFINE_RESOURCE_STRINGID(ValidateCommandLongDescription);
WINGET_DEFINE_RESOURCE_STRINGID(ValidateCommandReportDependencies);
Expand Down
95 changes: 62 additions & 33 deletions src/AppInstallerCLICore/Workflows/UpdateFlow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,30 +44,32 @@ namespace AppInstaller::CLI::Workflow
bool updateFound = false;
bool installedTypeInapplicable = false;

// The version keys should have already been sorted by version
const auto& versionKeys = package->GetAvailableVersionKeys();
for (const auto& key : versionKeys)
if (!installedVersion.IsUnknown() || context.Args.Contains(Execution::Args::Type::IncludeUnknown))
{
// Check Update Version
if (IsUpdateVersionApplicable(installedVersion, Utility::Version(key.Version)))
// The version keys should have already been sorted by version
const auto& versionKeys = package->GetAvailableVersionKeys();
for (const auto& key : versionKeys)
{
auto packageVersion = package->GetAvailableVersion(key);
auto manifest = packageVersion->GetManifest();

// Check applicable Installer
auto [installer, inapplicabilities] = manifestComparator.GetPreferredInstaller(manifest);
if (!installer.has_value())
// Check Update Version
if (IsUpdateVersionApplicable(installedVersion, Utility::Version(key.Version)))
{
// If there is at least one installer whose only reason is InstalledType.
auto onlyInstalledType = std::find(inapplicabilities.begin(), inapplicabilities.end(), InapplicabilityFlags::InstalledType);
if (onlyInstalledType != inapplicabilities.end())
auto packageVersion = package->GetAvailableVersion(key);
auto manifest = packageVersion->GetManifest();

// Check applicable Installer
auto [installer, inapplicabilities] = manifestComparator.GetPreferredInstaller(manifest);
if (!installer.has_value())
{
installedTypeInapplicable = true;
// If there is at least one installer whose only reason is InstalledType.
auto onlyInstalledType = std::find(inapplicabilities.begin(), inapplicabilities.end(), InapplicabilityFlags::InstalledType);
if (onlyInstalledType != inapplicabilities.end())
{
installedTypeInapplicable = true;
}

continue;
}

continue;
}

Logging::Telemetry().LogSelectedInstaller(
static_cast<int>(installer->Arch),
installer->Url,
Expand All @@ -86,16 +88,25 @@ namespace AppInstaller::CLI::Workflow
context.Add<Execution::Data::PackageVersion>(std::move(packageVersion));
context.Add<Execution::Data::Installer>(std::move(installer));

updateFound = true;
break;
updateFound = true;
break;
}
else
{
// Any following versions are not applicable
break;
}
}
else
}
else
{
// the package has an unknown version and the user did not request to upgrade it anyway.
if (m_reportUpdateNotFound)
{
// Any following versions are not applicable
break;
context.Reporter.Info() << Resource::String::UpgradeUnknownVersionExplanation << std::endl;
}
AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE);
}

if (!updateFound)
{
if (m_reportUpdateNotFound)
Expand Down Expand Up @@ -132,15 +143,28 @@ namespace AppInstaller::CLI::Workflow
const auto& matches = context.Get<Execution::Data::SearchResult>().Matches;
std::vector<std::unique_ptr<Execution::Context>> packagesToInstall;
bool updateAllFoundUpdate = false;
int unknownPackagesCount = 0;

for (const auto& match : matches)
{
// We want to do best effort to update all applicable updates regardless on previous update failure
auto updateContextPtr = context.CreateSubContext();
Execution::Context& updateContext = *updateContextPtr;
auto previousThreadGlobals = updateContext.SetForCurrentThread();
auto installedVersion = match.Package->GetInstalledVersion();

updateContext.Add<Execution::Data::Package>(match.Package);

if (context.Args.Contains(Execution::Args::Type::IncludeUnknown))
{
updateContext.Args.AddArg(Execution::Args::Type::IncludeUnknown);
}
else if (Utility::Version(installedVersion->GetProperty(PackageVersionProperty::Version)).IsUnknown())
{
// we don't know what the package's version is and the user didn't ask to upgrade it anyway.
unknownPackagesCount++;
continue;
}

updateContext <<
Workflow::GetInstalledPackageVersion <<
Expand All @@ -160,14 +184,19 @@ namespace AppInstaller::CLI::Workflow
if (!updateAllFoundUpdate)
{
context.Reporter.Info() << Resource::String::UpdateNotApplicable << std::endl;
return;
}

context.Add<Execution::Data::PackagesToInstall>(std::move(packagesToInstall));
context <<
InstallMultiplePackages(
Resource::String::InstallAndUpgradeCommandsReportDependencies,
APPINSTALLER_CLI_ERROR_UPDATE_ALL_HAS_FAILURE,
{ APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE });
else
{
context.Add<Execution::Data::PackagesToInstall>(std::move(packagesToInstall));
context <<
InstallMultiplePackages(
Resource::String::InstallAndUpgradeCommandsReportDependencies,
APPINSTALLER_CLI_ERROR_UPDATE_ALL_HAS_FAILURE,
{ APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE });
}
if (unknownPackagesCount > 0 && !context.Args.Contains(Execution::Args::Type::IncludeUnknown))
{
context.Reporter.Info() << unknownPackagesCount << " " << (unknownPackagesCount == 1 ? Resource::String::UpgradeUnknownCountSingle : Resource::String::UpgradeUnknownCount) << std::endl;
}
}
}
}
12 changes: 12 additions & 0 deletions src/AppInstallerCLICore/Workflows/WorkflowBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,7 @@ namespace AppInstaller::CLI::Workflow
});

int availableUpgradesCount = 0;
int unknownPackagesCount = 0;
auto &source = context.Get<Execution::Data::Source>();
bool shouldShowSource = source.IsComposite() && source.GetAvailableSources().size() > 1;

Expand All @@ -720,6 +721,13 @@ namespace AppInstaller::CLI::Workflow
auto latestVersion = match.Package->GetLatestAvailableVersion();
bool updateAvailable = match.Package->IsUpdateAvailable();

if (m_onlyShowUpgrades && !context.Args.Contains(Execution::Args::Type::IncludeUnknown) && Utility::Version(installedVersion->GetProperty(PackageVersionProperty::Version)).IsUnknown())
{
// We are only showing upgrades, and the user did not request to include packages with unknown versions.
unknownPackagesCount++;
continue;
}

// The only time we don't want to output a line is when filtering and no update is available.
if (updateAvailable || !m_onlyShowUpgrades)
{
Expand Down Expand Up @@ -766,6 +774,10 @@ namespace AppInstaller::CLI::Workflow
context.Reporter.Info() << availableUpgradesCount << ' ' << Resource::String::AvailableUpgrades << std::endl;
Copy link
Copy Markdown
Member

@florelis florelis Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@florelis florelis Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@jedieaston jedieaston Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@JohnMcPMS JohnMcPMS Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@jedieaston jedieaston Dec 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.")...

Copy link
Copy Markdown
Member

@florelis florelis Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good to me.

}
}
if (m_onlyShowUpgrades && unknownPackagesCount > 0 && !context.Args.Contains(Execution::Args::Type::IncludeUnknown))
{
context.Reporter.Info() << unknownPackagesCount << " " << (unknownPackagesCount == 1 ? Resource::String::UpgradeUnknownCountSingle : Resource::String::UpgradeUnknownCount) << std::endl;
}

}

Expand Down
15 changes: 15 additions & 0 deletions src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1250,4 +1250,19 @@ Please specify one of them using the `--source` option to proceed.</value>
<data name="InstallArchitectureArgumentDescription" xml:space="preserve">
<value>Select the architecture to install</value>
</data>
<data name="IncludeUnknownArgumentDescription" xml:space="preserve">
<value>Upgrade packages even if their current version cannot be determined</value>
</data>
<data name="UpgradeUnknownVersionExplanation" xml:space="preserve">
<value>This package's version number cannot be determined. To upgrade it anyway, add the argument --include-unknown to your previous command.</value>
Comment thread
jedieaston marked this conversation as resolved.
<comment>{Locked="--include-unknown"}</comment>
</data>
<data name="UpgradeUnknownCount" xml:space="preserve">
<value>packages have version numbers that cannot be determined. Use "--include-unknown" to see all 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="UpgradeUnknownCountSingle" xml:space="preserve">
<value>package has a version number that cannot be determined. Use "--include-unknown" to see all results.</value>
<comment>{Locked="--include-unknown"} This string is preceded by a (integer) number of packages that do not have notated versions.</comment>
</data>
</root>