Skip to content
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

Jump to bottom

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

Merged
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 6 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
16 changes: 14 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,19 @@ namespace AppInstaller::CLI
{
bool ShouldListUpgrade(Context& context)
{
return context.Args.Empty() ||
(context.Args.GetArgsCount() == 1 && context.Args.Contains(Execution::Args::Type::Source));
if (!context.Args.Empty())
jedieaston marked this conversation as resolved.
Show resolved Hide resolved
{
for (Execution::Args::Type type : context.Args.GetTypes())
{
if (type != Execution::Args::Type::Source && type != Execution::Args::Type::IncludeUnknown)
{
return false;
}
}
return true;
}

return true;
}
}

Expand All @@ -46,6 +57,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
Member

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
Member

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
Contributor

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
Contributor Author

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
Contributor

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
Contributor Author

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
Contributor

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
2 changes: 2 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,7 @@ namespace AppInstaller::CLI::Resource
WINGET_DEFINE_RESOURCE_STRINGID(UpgradeCommandShortDescription);
WINGET_DEFINE_RESOURCE_STRINGID(UpgradeDifferentInstallTechnology);
WINGET_DEFINE_RESOURCE_STRINGID(UpgradeDifferentInstallTechnologyInNewerVersions);
WINGET_DEFINE_RESOURCE_STRINGID(UpgradeUnknownVersionExplanation);
WINGET_DEFINE_RESOURCE_STRINGID(Usage);
WINGET_DEFINE_RESOURCE_STRINGID(ValidateCommandLongDescription);
WINGET_DEFINE_RESOURCE_STRINGID(ValidateCommandReportDependencies);
Expand Down
67 changes: 42 additions & 25 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,33 @@ 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)))
{
auto packageVersion = package->GetAvailableVersion(key);
auto manifest = packageVersion->GetManifest();

// Check applicable Installer
auto [installer, inapplicabilities] = manifestComparator.GetPreferredInstaller(manifest);
if (!installer.has_value())
// The version keys should have already been sorted by version
const auto& versionKeys = package->GetAvailableVersionKeys();
for (const auto& key : versionKeys)
{
// 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 +89,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 @@ -141,6 +153,11 @@ namespace AppInstaller::CLI::Workflow
auto previousThreadGlobals = updateContext.SetForCurrentThread();

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

if (context.Args.Contains(Execution::Args::Type::IncludeUnknown))
{
updateContext.Args.AddArg(Execution::Args::Type::IncludeUnknown);
}

updateContext <<
Workflow::GetInstalledPackageVersion <<
Expand Down Expand Up @@ -170,4 +187,4 @@ namespace AppInstaller::CLI::Workflow
APPINSTALLER_CLI_ERROR_UPDATE_ALL_HAS_FAILURE,
{ APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE });
}
}
}
6 changes: 6 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 @@ -720,6 +720,12 @@ 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.
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
6 changes: 6 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,10 @@ 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>
jedieaston marked this conversation as resolved.
Show resolved Hide resolved
</data>
</root>