From 6c6ccd3103ac1fc823e718d229f438faedc5691e Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Fri, 10 Dec 2021 09:21:42 -0800 Subject: [PATCH 1/9] Respect RequireExplicitUpgrade manifest entry --- .../JSON/settings/settings.schema.0.2.json | 2 +- .../Commands/UpgradeCommand.cpp | 19 ++- src/AppInstallerCLICore/ExecutionArgs.h | 7 +- src/AppInstallerCLICore/Resources.h | 5 +- .../Workflows/UpdateFlow.cpp | 137 +++++++++++------- .../Workflows/WorkflowBase.cpp | 14 +- .../Shared/Strings/en-us/winget.resw | 11 ++ 7 files changed, 125 insertions(+), 70 deletions(-) diff --git a/schemas/JSON/settings/settings.schema.0.2.json b/schemas/JSON/settings/settings.schema.0.2.json index 6f9d1aa0b8..d19f3fd0d0 100644 --- a/schemas/JSON/settings/settings.schema.0.2.json +++ b/schemas/JSON/settings/settings.schema.0.2.json @@ -1,6 +1,6 @@ { "$id": "https://aka.ms/winget-settings.schema.json", - "$schema": "https://json-schema.org/draft/2019-09/schema#", + "$schema": "https://json-schema.org/draft/2019-09/schema", "title": "Microsoft's Windows Package Manager Settings Profile Schema", "definitions": { "Source": { diff --git a/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp b/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp index 0cb1915c6f..a0a525bf68 100644 --- a/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp +++ b/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp @@ -43,7 +43,9 @@ namespace AppInstaller::CLI // either for upgrading or for listing available upgrades. bool HasArgumentsForMultiplePackages(Execution::Args& execArgs) { - return execArgs.Contains(Args::Type::All); + return execArgs.Contains(Args::Type::All) || + execArgs.Contains(Args::Type::IncludeUnknown) || + execArgs.Contains(Args::Type::IncludeExplicit); } // Determines whether there are any arguments only used as options during an upgrade, @@ -102,7 +104,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 }, }; } @@ -158,10 +161,16 @@ namespace AppInstaller::CLI void UpgradeCommand::ValidateArgumentsInternal(Execution::Args& execArgs) const { - if (execArgs.Contains(Execution::Args::Type::Manifest) && + if (execArgs.Contains(Execution::Args::Type::Manifest) && (HasSearchQueryArguments(execArgs) || - HasArgumentsForMultiplePackages(execArgs) || - HasArgumentsForSource(execArgs))) + HasArgumentsForMultiplePackages(execArgs) || + HasArgumentsForSource(execArgs))) + { + throw CommandException(Resource::String::BothManifestAndSearchQueryProvided); + } + + // TODO: Message + if (HasArgumentsForSinglePackage(execArgs) && HasArgumentsForMultiplePackages(execArgs)) { throw CommandException(Resource::String::BothManifestAndSearchQueryProvided); } diff --git a/src/AppInstallerCLICore/ExecutionArgs.h b/src/AppInstallerCLICore/ExecutionArgs.h index c160cb1688..e7482edc07 100644 --- a/src/AppInstallerCLICore/ExecutionArgs.h +++ b/src/AppInstallerCLICore/ExecutionArgs.h @@ -79,8 +79,12 @@ namespace AppInstaller::CLI::Execution AdminSettingEnable, AdminSettingDisable, + // Upgrade Command + All, // Update all installed packages to latest + IncludeUnknown, // Allow upgrades of packages with unknown versions + IncludeExplicit, // Allow upgrades of packages pinned by default + // Other - All, // Used in Update command to update all installed packages to latest ListVersions, // Used in Show command to list all available versions of an app NoVT, // Disable VirtualTerminal outputs RetroStyle, // Makes progress display as retro @@ -91,7 +95,6 @@ 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 Wait, // Prompts the user to press any key before exiting // Used for demonstration purposes diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index 20b807a135..a06164d8bf 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -107,6 +107,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(ImportPackageAlreadyInstalled); WINGET_DEFINE_RESOURCE_STRINGID(ImportSearchFailed); WINGET_DEFINE_RESOURCE_STRINGID(ImportSourceNotInstalled); + WINGET_DEFINE_RESOURCE_STRINGID(IncludeExplicitArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(IncludeUnknownArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(InstallAndUpgradeCommandsReportDependencies); WINGET_DEFINE_RESOURCE_STRINGID(InstallArchitectureArgumentDescription); @@ -376,8 +377,8 @@ 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(UpgradeRequireExplicitCount); + WINGET_DEFINE_RESOURCE_STRINGID(UpgradeUnknownVersionCount); WINGET_DEFINE_RESOURCE_STRINGID(UpgradeUnknownVersionExplanation); WINGET_DEFINE_RESOURCE_STRINGID(Usage); WINGET_DEFINE_RESOURCE_STRINGID(ValidateCommandLongDescription); diff --git a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp index 4491f9190b..d5a1049b22 100644 --- a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp @@ -44,69 +44,69 @@ namespace AppInstaller::CLI::Workflow bool updateFound = false; bool installedTypeInapplicable = false; - if (!installedVersion.IsUnknown() || context.Args.Contains(Execution::Args::Type::IncludeUnknown)) + if (installedVersion.IsUnknown() && !context.Args.Contains(Execution::Args::Type::IncludeUnknown)) { - // The version keys should have already been sorted by version - const auto& versionKeys = package->GetAvailableVersionKeys(); - for (const auto& key : versionKeys) + // the package has an unknown version and the user did not request to upgrade it anyway. + if (m_reportUpdateNotFound) { - // Check Update Version - if (IsUpdateVersionApplicable(installedVersion, Utility::Version(key.Version))) - { - auto packageVersion = package->GetAvailableVersion(key); - auto manifest = packageVersion->GetManifest(); + context.Reporter.Info() << Resource::String::UpgradeUnknownVersionExplanation << std::endl; + } - // Check applicable Installer - auto [installer, inapplicabilities] = manifestComparator.GetPreferredInstaller(manifest); - if (!installer.has_value()) + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE); + } + + // 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))) + { + auto packageVersion = package->GetAvailableVersion(key); + auto manifest = packageVersion->GetManifest(); + + // Check applicable Installer + auto [installer, inapplicabilities] = manifestComparator.GetPreferredInstaller(manifest); + if (!installer.has_value()) + { + // 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()) { - // 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; + installedTypeInapplicable = true; } - Logging::Telemetry().LogSelectedInstaller( - static_cast(installer->Arch), - installer->Url, - Manifest::InstallerTypeToString(installer->InstallerType), - Manifest::ScopeToString(installer->Scope), - installer->Locale); - - Logging::Telemetry().LogManifestFields( - manifest.Id, - manifest.DefaultLocalization.Get(), - manifest.Version); - - // Since we already did installer selection, just populate the context Data - manifest.ApplyLocale(installer->Locale); - context.Add(std::move(manifest)); - context.Add(std::move(packageVersion)); - context.Add(std::move(installer)); - - updateFound = true; - break; - } - else - { - // Any following versions are not applicable - break; + continue; } + + Logging::Telemetry().LogSelectedInstaller( + static_cast(installer->Arch), + installer->Url, + Manifest::InstallerTypeToString(installer->InstallerType), + Manifest::ScopeToString(installer->Scope), + installer->Locale); + + Logging::Telemetry().LogManifestFields( + manifest.Id, + manifest.DefaultLocalization.Get(), + manifest.Version); + + // Since we already did installer selection, just populate the context Data + manifest.ApplyLocale(installer->Locale); + context.Add(std::move(manifest)); + context.Add(std::move(packageVersion)); + context.Add(std::move(installer)); + + updateFound = true; + break; } - } - else - { - // the package has an unknown version and the user did not request to upgrade it anyway. - if (m_reportUpdateNotFound) + else { - context.Reporter.Info() << Resource::String::UpgradeUnknownVersionExplanation << std::endl; + // Any following versions are not applicable + break; } - AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE); } + if (!updateFound) { if (m_reportUpdateNotFound) @@ -143,7 +143,8 @@ namespace AppInstaller::CLI::Workflow const auto& matches = context.Get().Matches; std::vector> packagesToInstall; bool updateAllFoundUpdate = false; - int unknownPackagesCount = 0; + int packagesWithUnknownVersionSkipped = 0; + int packagesThatRequireExplicitSkipped = 0; for (const auto& match : matches) { @@ -154,7 +155,8 @@ namespace AppInstaller::CLI::Workflow auto installedVersion = match.Package->GetInstalledVersion(); updateContext.Add(match.Package); - + + // Filter out packages with unknown installed versions if (context.Args.Contains(Execution::Args::Type::IncludeUnknown)) { updateContext.Args.AddArg(Execution::Args::Type::IncludeUnknown); @@ -162,7 +164,8 @@ namespace AppInstaller::CLI::Workflow 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++; + AICLI_LOG(CLI, Info, << "Skipping " << match.Package->GetProperty(PackageProperty::Id) << " as it has unknown installed version"); + ++packagesWithUnknownVersionSkipped; continue; } @@ -176,6 +179,18 @@ namespace AppInstaller::CLI::Workflow continue; } + // Filter out packages that require explicit upgrades. + // Note that this is filtering based on the updated installer, not the installed version. So there + // is an edge case where if an app that does not require explicit update changes to require it + // (e.g. it adds auto-updates) we will need to do explicit update to that version. + if (!context.Args.Contains(Execution::Args::Type::IncludeExplicit) && + updateContext.Get()->RequireExplicitUpgrade) + { + AICLI_LOG(CLI, Info, << "Skipping " << match.Package->GetProperty(PackageProperty::Id) << " as it requires explicit upgrade"); + ++packagesThatRequireExplicitSkipped; + continue; + } + updateAllFoundUpdate = true; AddToPackagesToInstallIfNotPresent(packagesToInstall, std::move(updateContextPtr)); @@ -191,5 +206,17 @@ namespace AppInstaller::CLI::Workflow APPINSTALLER_CLI_ERROR_UPDATE_ALL_HAS_FAILURE, { APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE }); } + + if (packagesWithUnknownVersionSkipped > 0) + { + AICLI_LOG(CLI, Info, << packagesWithUnknownVersionSkipped << " package(s) skipped due to unknown installed version"); + context.Reporter.Info() << packagesWithUnknownVersionSkipped << " " << Resource::String::UpgradeUnknownVersionCount << std::endl; + } + + if (packagesThatRequireExplicitSkipped > 0) + { + AICLI_LOG(CLI, Info, << packagesThatRequireExplicitSkipped << " package(s) skipped due to requiring explicit upgrade"); + context.Reporter.Info() << packagesThatRequireExplicitSkipped << " " << Resource::String::UpgradeRequireExplicitCount << std::endl; + } } } diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index b13e612a33..4642d140a4 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -711,7 +711,7 @@ namespace AppInstaller::CLI::Workflow }); int availableUpgradesCount = 0; - int unknownPackagesCount = 0; + int packagesWithUnknownVersionSkipped = 0; auto &source = context.Get(); bool shouldShowSource = source.IsComposite() && source.GetAvailableSources().size() > 1; @@ -727,7 +727,7 @@ namespace AppInstaller::CLI::Workflow if (m_onlyShowUpgrades && !context.Args.Contains(Execution::Args::Type::IncludeUnknown) && Utility::Version(installedVersion->GetProperty(PackageVersionProperty::Version)).IsUnknown() && updateAvailable) { // We are only showing upgrades, and the user did not request to include packages with unknown versions. - unknownPackagesCount++; + ++packagesWithUnknownVersionSkipped; continue; } @@ -782,11 +782,15 @@ namespace AppInstaller::CLI::Workflow context.Reporter.Info() << availableUpgradesCount << ' ' << Resource::String::AvailableUpgrades << std::endl; } } - if (m_onlyShowUpgrades && unknownPackagesCount > 0 && !context.Args.Contains(Execution::Args::Type::IncludeUnknown)) + + if (m_onlyShowUpgrades) { - context.Reporter.Info() << unknownPackagesCount << " " << (unknownPackagesCount == 1 ? Resource::String::UpgradeUnknownCountSingle : Resource::String::UpgradeUnknownCount) << std::endl; + if (packagesWithUnknownVersionSkipped > 0) + { + AICLI_LOG(CLI, Info, << packagesWithUnknownVersionSkipped << " package(s) skipped due to unknown installed version"); + context.Reporter.Info() << packagesWithUnknownVersionSkipped << " " << Resource::String::UpgradeUnknownVersionCount << std::endl; + } } - } void EnsureMatchesFromSearchResult::operator()(Execution::Context& context) const diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index 583db2ef73..17630fcf30 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -1272,6 +1272,17 @@ Please specify one of them using the `--source` option to proceed. package has a version number that cannot be determined. Using "--include-unknown" may show more results. {Locked="--include-unknown"} This string is preceded by a (integer) number of packages that do not have notated versions. + + package(s) have version numbers that cannot be determined. Use "--include-unknown" to see all results. + {Locked="--include-unknown"} This string is preceded by a (integer) number of packages that do not have notated versions. + + + Upgrade packages that are pinned by default + + + package(s) is pinned by default and needs to be explicitly upgraded. Use "--include-explicit" to see all results. + {Locked="--include-explicit"} This string is preceded by a (integer) number of packages that require explicit upgrades. + The arguments provided can only be used with a query. From 2f599cb34cf4c0e21cdb57cd3382f7d045edca7e Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Mon, 13 Dec 2021 13:57:02 -0800 Subject: [PATCH 2/9] Add tests --- .../JSON/settings/settings.schema.0.2.json | 2 +- .../Commands/UpgradeCommand.cpp | 14 +- src/AppInstallerCLICore/Resources.h | 1 + .../Workflows/UpdateFlow.cpp | 34 ++-- .../Shared/Strings/en-us/winget.resw | 3 + .../AppInstallerCLITests.vcxproj | 3 + .../AppInstallerCLITests.vcxproj.filters | 3 + .../UpdateFlowTest_Exe_2_RequireExplicit.yaml | 21 +++ src/AppInstallerCLITests/WorkFlow.cpp | 146 ++++++++++++++++-- 9 files changed, 190 insertions(+), 37 deletions(-) create mode 100644 src/AppInstallerCLITests/TestData/UpdateFlowTest_Exe_2_RequireExplicit.yaml diff --git a/schemas/JSON/settings/settings.schema.0.2.json b/schemas/JSON/settings/settings.schema.0.2.json index d19f3fd0d0..6f9d1aa0b8 100644 --- a/schemas/JSON/settings/settings.schema.0.2.json +++ b/schemas/JSON/settings/settings.schema.0.2.json @@ -1,6 +1,6 @@ { "$id": "https://aka.ms/winget-settings.schema.json", - "$schema": "https://json-schema.org/draft/2019-09/schema", + "$schema": "https://json-schema.org/draft/2019-09/schema#", "title": "Microsoft's Windows Package Manager Settings Profile Schema", "definitions": { "Source": { diff --git a/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp b/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp index a0a525bf68..113c652af6 100644 --- a/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp +++ b/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp @@ -169,14 +169,7 @@ namespace AppInstaller::CLI throw CommandException(Resource::String::BothManifestAndSearchQueryProvided); } - // TODO: Message - if (HasArgumentsForSinglePackage(execArgs) && HasArgumentsForMultiplePackages(execArgs)) - { - throw CommandException(Resource::String::BothManifestAndSearchQueryProvided); - } - - - else if (!ShouldListUpgrade(execArgs) + if (!ShouldListUpgrade(execArgs) && !HasSearchQueryArguments(execArgs) && (execArgs.Contains(Args::Type::Log) || execArgs.Contains(Args::Type::Override) || @@ -186,6 +179,11 @@ namespace AppInstaller::CLI { throw CommandException(Resource::String::InvalidArgumentWithoutQueryError); } + + if (HasArgumentsForSinglePackage(execArgs) && HasArgumentsForMultiplePackages(execArgs)) + { + throw CommandException(Resource::String::IncompatibleArgumentsProvided); + } } void UpgradeCommand::ExecuteInternal(Execution::Context& context) const diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index a06164d8bf..78ebce2c59 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -109,6 +109,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(ImportSourceNotInstalled); WINGET_DEFINE_RESOURCE_STRINGID(IncludeExplicitArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(IncludeUnknownArgumentDescription); + WINGET_DEFINE_RESOURCE_STRINGID(IncompatibleArgumentsProvided); WINGET_DEFINE_RESOURCE_STRINGID(InstallAndUpgradeCommandsReportDependencies); WINGET_DEFINE_RESOURCE_STRINGID(InstallArchitectureArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(InstallationAbandoned); diff --git a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp index d5a1049b22..4696c2ca17 100644 --- a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp @@ -79,23 +79,23 @@ namespace AppInstaller::CLI::Workflow continue; } - Logging::Telemetry().LogSelectedInstaller( - static_cast(installer->Arch), - installer->Url, - Manifest::InstallerTypeToString(installer->InstallerType), - Manifest::ScopeToString(installer->Scope), - installer->Locale); - - Logging::Telemetry().LogManifestFields( - manifest.Id, - manifest.DefaultLocalization.Get(), - manifest.Version); - - // Since we already did installer selection, just populate the context Data - manifest.ApplyLocale(installer->Locale); - context.Add(std::move(manifest)); - context.Add(std::move(packageVersion)); - context.Add(std::move(installer)); + Logging::Telemetry().LogSelectedInstaller( + static_cast(installer->Arch), + installer->Url, + Manifest::InstallerTypeToString(installer->InstallerType), + Manifest::ScopeToString(installer->Scope), + installer->Locale); + + Logging::Telemetry().LogManifestFields( + manifest.Id, + manifest.DefaultLocalization.Get(), + manifest.Version); + + // Since we already did installer selection, just populate the context Data + manifest.ApplyLocale(installer->Locale); + context.Add(std::move(manifest)); + context.Add(std::move(packageVersion)); + context.Add(std::move(installer)); updateFound = true; break; diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index 17630fcf30..7fd563c22e 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -1390,4 +1390,7 @@ Please specify one of them using the `--source` option to proceed. Only one non-portable nested installer can be specified for an archive installer + + Incompatible command line arguments provided + \ No newline at end of file diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj index 5bd30e611c..61d657502a 100644 --- a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj +++ b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj @@ -611,6 +611,9 @@ true + + true + true diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters index 9e45f86551..09b7116a4a 100644 --- a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters +++ b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters @@ -552,6 +552,9 @@ TestData + + TestData + TestData diff --git a/src/AppInstallerCLITests/TestData/UpdateFlowTest_Exe_2_RequireExplicit.yaml b/src/AppInstallerCLITests/TestData/UpdateFlowTest_Exe_2_RequireExplicit.yaml new file mode 100644 index 0000000000..65491a1351 --- /dev/null +++ b/src/AppInstallerCLITests/TestData/UpdateFlowTest_Exe_2_RequireExplicit.yaml @@ -0,0 +1,21 @@ +# Similar content to UpdateFlowTest_Exe_2.yaml but requiring explicit upgrade +PackageIdentifier: AppInstallerCliTest.TestExeInstaller +PackageVersion: 3.0.0.0 +PackageLocale: en-US +PackageName: AppInstaller Test Installer +Publisher: Microsoft Corporation +Moniker: AICLITestExe +License: Test +InstallerSwitches: + Custom: /custom /ver3.0.0.0 + SilentWithProgress: /silentwithprogress + Silent: /silence + Update: /update +RequireExplicitUpgrade: true +Installers: + - Architecture: x86 + InstallerUrl: https://ThisIsNotUsed + InstallerType: exe + InstallerSha256: 65DB2F2AC2686C7F2FD69D4A4C6683B888DC55BFA20A0E32CA9F838B51689A3B +ManifestType: singleton +ManifestVersion: 1.1.0 \ No newline at end of file diff --git a/src/AppInstallerCLITests/WorkFlow.cpp b/src/AppInstallerCLITests/WorkFlow.cpp index 8107d80d5c..f364c3bdea 100644 --- a/src/AppInstallerCLITests/WorkFlow.cpp +++ b/src/AppInstallerCLITests/WorkFlow.cpp @@ -105,9 +105,16 @@ namespace } }; + enum TestSourceSearchOptions + { + None = 0, + UpgradeUsesAgreements, + UpgradeUsesRequireExplicit, + }; + struct WorkflowTestCompositeSource : public TestSource { - WorkflowTestCompositeSource(bool upgradeUsesLicenses) : m_upgradeUsesLicenses(upgradeUsesLicenses) {} + WorkflowTestCompositeSource(TestSourceSearchOptions searchOptions = TestSourceSearchOptions::None) : m_searchOptions(searchOptions) {} SearchResult Search(const SearchRequest& request) const override { @@ -133,7 +140,21 @@ namespace { auto manifest = YamlParser::CreateFromPath(TestDataFile("InstallFlowTest_Exe.yaml")); auto manifest2 = YamlParser::CreateFromPath(TestDataFile("UpdateFlowTest_Exe.yaml")); - auto manifest3 = YamlParser::CreateFromPath(TestDataFile(m_upgradeUsesLicenses ? "UpdateFlowTest_Exe_2_LicenseAgreement.yaml" : "UpdateFlowTest_Exe_2.yaml")); + Manifest manifest3; + switch (m_searchOptions) + { + case TestSourceSearchOptions::UpgradeUsesAgreements: + manifest3 = YamlParser::CreateFromPath(TestDataFile("UpdateFlowTest_Exe_2_LicenseAgreement.yaml")); + break; + case TestSourceSearchOptions::UpgradeUsesRequireExplicit: + manifest3 = YamlParser::CreateFromPath(TestDataFile("UpdateFlowTest_Exe_2_RequireExplicit.yaml")); + break; + case TestSourceSearchOptions::None: + default: + manifest3 = YamlParser::CreateFromPath(TestDataFile("UpdateFlowTest_Exe_2.yaml")); + break; + } + auto testPackage = TestPackage::Make( manifest, @@ -171,7 +192,18 @@ namespace if (input.empty() || input == "AppInstallerCliTest.TestMsixInstaller") { auto manifest = YamlParser::CreateFromPath(TestDataFile("InstallFlowTest_Msix_StreamingFlow.yaml")); - auto manifest2 = YamlParser::CreateFromPath(TestDataFile(m_upgradeUsesLicenses ? "UpdateFlowTest_Msix_LicenseAgreement.yaml" : "UpdateFlowTest_Msix.yaml")); + Manifest manifest2; + switch (m_searchOptions) + { + case TestSourceSearchOptions::UpgradeUsesAgreements: + manifest2 = YamlParser::CreateFromPath(TestDataFile("UpdateFlowTest_Msix_LicenseAgreement.yaml")); + break; + case TestSourceSearchOptions::None: + default: + manifest2 = YamlParser::CreateFromPath(TestDataFile("UpdateFlowTest_Msix.yaml")); + break; + } + result.Matches.emplace_back( ResultMatch( TestPackage::Make( @@ -338,7 +370,7 @@ namespace { auto manifest = YamlParser::CreateFromPath(TestDataFile("InstallFlowTest_Exe.yaml")); auto manifest2 = YamlParser::CreateFromPath(TestDataFile("UpdateFlowTest_Exe.yaml")); - auto manifest3 = YamlParser::CreateFromPath(TestDataFile(m_upgradeUsesLicenses ? "UpdateFlowTest_Exe_2_LicenseAgreement.yaml" : "UpdateFlowTest_Exe_2.yaml")); + auto manifest3 = YamlParser::CreateFromPath(TestDataFile("UpdateFlowTest_Exe_2.yaml")); result.Matches.emplace_back( ResultMatch( TestPackage::Make( @@ -373,7 +405,7 @@ namespace } private: - bool m_upgradeUsesLicenses; + TestSourceSearchOptions m_searchOptions; }; struct TestContext; @@ -479,7 +511,7 @@ void OverrideForOpenSource(TestContext& context) } }); } -void OverrideForCompositeInstalledSource(TestContext& context, bool upgradeUsesLicenses = false) +void OverrideForCompositeInstalledSource(TestContext& context, TestSourceSearchOptions searchOptions = TestSourceSearchOptions::None) { context.Override({ "OpenSource", [](TestContext&) { @@ -487,7 +519,7 @@ void OverrideForCompositeInstalledSource(TestContext& context, bool upgradeUsesL context.Override({ "OpenCompositeSource", [=](TestContext& context) { - context.Add(Source{ std::make_shared(upgradeUsesLicenses) }); + context.Add(Source{ std::make_shared(searchOptions) }); } }); } @@ -495,13 +527,13 @@ void OverrideForImportSource(TestContext& context, bool useTestCompositeSource = { context.Override({ "OpenPredefinedSource", [=](TestContext& context) { - auto installedSource = useTestCompositeSource? std::make_shared(false) : std::make_shared(); + auto installedSource = useTestCompositeSource ? std::make_shared() : std::make_shared(); context.Add(Source{ installedSource }); } }); context.Override({ Workflow::OpenSourcesForImport, [](TestContext& context) { - context.Add(std::vector{ Source{ std::make_shared(false) } }); + context.Add(std::vector{ Source{ std::make_shared() } }); } }); } @@ -2176,7 +2208,7 @@ TEST_CASE("UpdateFlow_All_LicenseAgreement", "[UpdateFlow][workflow]") std::ostringstream updateOutput; TestContext context{ updateOutput, std::cin }; auto previousThreadGlobals = context.SetForCurrentThread(); - OverrideForCompositeInstalledSource(context, /* upgradeUsesLicenses */ true); + OverrideForCompositeInstalledSource(context, TestSourceSearchOptions::UpgradeUsesAgreements); OverrideForShellExecute(context); OverrideForMSIX(context); OverrideForMSStore(context, true); @@ -2213,7 +2245,7 @@ TEST_CASE("UpdateFlow_All_LicenseAgreement_NotAccepted", "[UpdateFlow][workflow] std::ostringstream updateOutput; TestContext context{ updateOutput, updateInput }; auto previousThreadGlobals = context.SetForCurrentThread(); - OverrideForCompositeInstalledSource(context, /* upgradeUsesLicenses */ true); + OverrideForCompositeInstalledSource(context, TestSourceSearchOptions::UpgradeUsesAgreements); context.Args.AddArg(Execution::Args::Type::All); UpgradeCommand update({}); @@ -2250,6 +2282,98 @@ TEST_CASE("UninstallFlow_UninstallPortable", "[UninstallFlow][workflow]") REQUIRE(std::filesystem::exists(uninstallResultPath.GetPath())); } +TEST_CASE("UpdateFlow_All_RequireExplicit", "[UpdateFlow][workflow]") +{ + TestCommon::TempFile updateExeResultPath("TestExeInstalled.txt"); + TestCommon::TempFile updateMsixResultPath("TestMsixInstalled.txt"); + + std::ostringstream updateOutput; + TestContext context{ updateOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + + // Exe package has an update that requires explicit upgrade. + // Msix is also listed with an available upgrade. + OverrideForCompositeInstalledSource(context, TestSourceSearchOptions::UpgradeUsesRequireExplicit); + + SECTION("Skip package with explicit upgrade") + { + SECTION("List available upgrades") + { + UpgradeCommand update({}); + update.Execute(context); + INFO(updateOutput.str()); + + // The package that requires explicit upgrade is still listed, as we don't select + // the installer yet so we don't know if it will require it. + // TODO: Should we select the installer when listing so we can skip it? + REQUIRE(updateOutput.str().find(Resource::LocString(Resource::String::UpgradeRequireExplicitCount)) == std::string::npos); + REQUIRE(updateOutput.str().find("AppInstallerCliTest.TestExeInstaller") != std::string::npos); + REQUIRE(updateOutput.str().find("AppInstallerCliTest.TestMsixInstaller") != std::string::npos); + } + + SECTION("Upgrade all") + { + context.Args.AddArg(Args::Type::All); + OverrideForMSIX(context); + + UpgradeCommand update({}); + update.Execute(context); + INFO(updateOutput.str()); + + auto s = updateOutput.str(); + + // Verify message is printed for skipped package + REQUIRE(updateOutput.str().find(Resource::LocString(Resource::String::UpgradeRequireExplicitCount)) != std::string::npos); + + // Verify package is not installed, but all others are + REQUIRE(!std::filesystem::exists(updateExeResultPath.GetPath())); + REQUIRE(std::filesystem::exists(updateMsixResultPath.GetPath())); + } + } + + SECTION("Include package with explicit upgrade") + { + context.Args.AddArg(Args::Type::IncludeExplicit); + + SECTION("List available upgrades") + { + UpgradeCommand update({}); + update.Execute(context); + INFO(updateOutput.str()); + + + auto s = updateOutput.str(); + + // Verify all packages with updates are listed. + REQUIRE(updateOutput.str().find("AppInstallerCliTest.TestExeInstaller") != std::string::npos); + REQUIRE(updateOutput.str().find("AppInstallerCliTest.TestMsixInstaller") != std::string::npos); + } + + SECTION("Upgrade all") + { + context.Args.AddArg(Args::Type::All); + OverrideForShellExecute(context); + OverrideForMSIX(context); + + UpgradeCommand update({}); + update.Execute(context); + INFO(updateOutput.str()); + + auto s = updateOutput.str(); + + // Verify all packages are updated + REQUIRE(std::filesystem::exists(updateExeResultPath.GetPath())); + REQUIRE(std::filesystem::exists(updateMsixResultPath.GetPath())); + } + + // Verify that skipped package message is not printed + REQUIRE(updateOutput.str().find(Resource::LocString(Resource::String::UpgradeRequireExplicitCount)) == std::string::npos); + } + + // Command should always succeed + REQUIRE(context.GetTerminationHR() == S_OK); +} + TEST_CASE("UninstallFlow_UninstallExe", "[UninstallFlow][workflow]") { TestCommon::TempFile uninstallResultPath("TestExeUninstalled.txt"); From b62d2bc50c64da7a48287fb8b78b67cca5ec7d8b Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Wed, 27 Jul 2022 17:38:21 -0700 Subject: [PATCH 3/9] Show separate table for explicit upgrade --- .../Commands/UpgradeCommand.cpp | 14 ++-- .../Workflows/UpdateFlow.cpp | 2 +- .../Workflows/WorkflowBase.cpp | 76 +++++++++++++++---- 3 files changed, 69 insertions(+), 23 deletions(-) diff --git a/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp b/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp index 113c652af6..aeb7723b4b 100644 --- a/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp +++ b/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp @@ -169,13 +169,13 @@ namespace AppInstaller::CLI throw CommandException(Resource::String::BothManifestAndSearchQueryProvided); } - if (!ShouldListUpgrade(execArgs) - && !HasSearchQueryArguments(execArgs) - && (execArgs.Contains(Args::Type::Log) || - execArgs.Contains(Args::Type::Override) || - execArgs.Contains(Args::Type::InstallLocation) || - execArgs.Contains(Args::Type::HashOverride) || - execArgs.Contains(Args::Type::AcceptPackageAgreements))) + if (!ShouldListUpgrade(execArgs) + && !HasSearchQueryArguments(execArgs) + && (execArgs.Contains(Args::Type::Log) || + execArgs.Contains(Args::Type::Override) || + execArgs.Contains(Args::Type::InstallLocation) || + execArgs.Contains(Args::Type::HashOverride) || + execArgs.Contains(Args::Type::AcceptPackageAgreements))) { throw CommandException(Resource::String::InvalidArgumentWithoutQueryError); } diff --git a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp index 4696c2ca17..4e9b17bd2c 100644 --- a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp @@ -44,7 +44,7 @@ namespace AppInstaller::CLI::Workflow bool updateFound = false; bool installedTypeInapplicable = false; - if (installedVersion.IsUnknown() && !context.Args.Contains(Execution::Args::Type::IncludeUnknown)) + if (installedVersion.IsUnknown() && !context.Args.Contains(Execution::Args::Type::IncludeUnknown)) { // the package has an unknown version and the user did not request to upgrade it anyway. if (m_reportUpdateNotFound) diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index 4642d140a4..df18338252 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -218,6 +218,44 @@ namespace AppInstaller::CLI::Workflow return accepted; } + + // Data shown on a line of a table displaying installed packages + struct InstalledPackagesTableLine + { + InstalledPackagesTableLine(Utility::LocIndString name, Utility::LocIndString id, Utility::LocIndString installedVersion, Utility::LocIndString availableVersion, Utility::LocIndString source) + : Name(name), Id(id), InstalledVersion(installedVersion), AvailableVersion(availableVersion), Source(source) {} + + Utility::LocIndString Name; + Utility::LocIndString Id; + Utility::LocIndString InstalledVersion; + Utility::LocIndString AvailableVersion; + Utility::LocIndString Source; + }; + + void OutputInstalledPackagesTable(Execution::Context& context, const std::vector& lines) + { + Execution::TableOutput<5> table(context.Reporter, + { + Resource::String::SearchName, + Resource::String::SearchId, + Resource::String::SearchVersion, + Resource::String::AvailableHeader, + Resource::String::SearchSource + }); + + for (const auto& line : lines) + { + table.OutputLine({ + line.Name, + line.Id, + line.InstalledVersion, + line.AvailableVersion, + line.Source + }); + } + + table.Complete(); + } } bool WorkflowTask::operator==(const WorkflowTask& other) const @@ -701,14 +739,8 @@ namespace AppInstaller::CLI::Workflow { auto& searchResult = context.Get(); - Execution::TableOutput<5> table(context.Reporter, - { - Resource::String::SearchName, - Resource::String::SearchId, - Resource::String::SearchVersion, - Resource::String::AvailableHeader, - Resource::String::SearchSource - }); + std::vector lines; + std::vector linesForExplicitUpgrade; int availableUpgradesCount = 0; int packagesWithUnknownVersionSkipped = 0; @@ -751,22 +783,30 @@ namespace AppInstaller::CLI::Workflow // Output using the local PackageName instead of the name in the manifest, to prevent confusion for packages that add multiple // Add/Remove Programs entries. // TODO: De-duplicate this list, and only show (by default) one entry per matched package. - table.OutputLine({ + InstalledPackagesTableLine line( installedVersion->GetProperty(PackageVersionProperty::Name), match.Package->GetProperty(PackageProperty::Id), installedVersion->GetProperty(PackageVersionProperty::Version), availableVersion, - shouldShowSource ? sourceName : ""s - }); - - + shouldShowSource ? sourceName : Utility::LocIndString() + ); + + bool requiresExplicitUpgrade = m_onlyShowUpgrades && false; + if (requiresExplicitUpgrade) + { + lines.push_back(std::move(line)); + } + else + { + linesForExplicitUpgrade.push_back(std::move(line)); + } } } } - table.Complete(); + OutputInstalledPackagesTable(context, lines); - if (table.IsEmpty()) + if (lines.empty()) { context.Reporter.Info() << Resource::String::NoInstalledPackageFound << std::endl; } @@ -783,6 +823,12 @@ namespace AppInstaller::CLI::Workflow } } + if (!linesForExplicitUpgrade.empty()) + { + context.Reporter.Info() << std::endl << "The following packages have an upgrade available, but require explicit targeting for upgrade:"_liv << std::endl; /* TODO */ + OutputInstalledPackagesTable(context, linesForExplicitUpgrade); + } + if (m_onlyShowUpgrades) { if (packagesWithUnknownVersionSkipped > 0) From ae07b139cf8d7c4fd5488184821d05bbd4621ebe Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Thu, 28 Jul 2022 13:32:06 -0700 Subject: [PATCH 4/9] Rename include-explicit to include-pinned --- src/AppInstallerCLICore/Commands/UpgradeCommand.cpp | 4 ++-- src/AppInstallerCLICore/ExecutionArgs.h | 2 +- src/AppInstallerCLICore/Resources.h | 2 +- src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw | 6 +++--- src/AppInstallerCLITests/WorkFlow.cpp | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp b/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp index aeb7723b4b..f1edd3a0bf 100644 --- a/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp +++ b/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp @@ -45,7 +45,7 @@ namespace AppInstaller::CLI { return execArgs.Contains(Args::Type::All) || execArgs.Contains(Args::Type::IncludeUnknown) || - execArgs.Contains(Args::Type::IncludeExplicit); + execArgs.Contains(Args::Type::IncludePinned); } // Determines whether there are any arguments only used as options during an upgrade, @@ -105,7 +105,7 @@ namespace AppInstaller::CLI 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-explicit", Argument::NoAlias, Args::Type::IncludeExplicit, Resource::String::IncludeExplicitArgumentDescription, ArgumentType::Flag }, + Argument{ "include-pinned", Argument::NoAlias, Args::Type::IncludePinned, Resource::String::IncludePinnedArgumentDescription, ArgumentType::Flag }, }; } diff --git a/src/AppInstallerCLICore/ExecutionArgs.h b/src/AppInstallerCLICore/ExecutionArgs.h index e7482edc07..201e3b4ee4 100644 --- a/src/AppInstallerCLICore/ExecutionArgs.h +++ b/src/AppInstallerCLICore/ExecutionArgs.h @@ -82,7 +82,7 @@ namespace AppInstaller::CLI::Execution // Upgrade Command All, // Update all installed packages to latest IncludeUnknown, // Allow upgrades of packages with unknown versions - IncludeExplicit, // Allow upgrades of packages pinned by default + IncludePinned, // Allow upgrades of packages pinned by default // Other ListVersions, // Used in Show command to list all available versions of an app diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index 78ebce2c59..ff9b0fe90d 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -107,7 +107,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(ImportPackageAlreadyInstalled); WINGET_DEFINE_RESOURCE_STRINGID(ImportSearchFailed); WINGET_DEFINE_RESOURCE_STRINGID(ImportSourceNotInstalled); - WINGET_DEFINE_RESOURCE_STRINGID(IncludeExplicitArgumentDescription); + WINGET_DEFINE_RESOURCE_STRINGID(IncludePinnedArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(IncludeUnknownArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(IncompatibleArgumentsProvided); WINGET_DEFINE_RESOURCE_STRINGID(InstallAndUpgradeCommandsReportDependencies); diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index 7fd563c22e..f972a546c4 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -1276,12 +1276,12 @@ Please specify one of them using the `--source` option to proceed. package(s) have version numbers that cannot be determined. Use "--include-unknown" to see all results. {Locked="--include-unknown"} This string is preceded by a (integer) number of packages that do not have notated versions. - + Upgrade packages that are pinned by default - package(s) is pinned by default and needs to be explicitly upgraded. Use "--include-explicit" to see all results. - {Locked="--include-explicit"} This string is preceded by a (integer) number of packages that require explicit upgrades. + package(s) is pinned and needs to be explicitly upgraded. Use "--include-pinned" to see all results. + {Locked="--include-pinned"} This string is preceded by a (integer) number of packages that require explicit upgrades. The arguments provided can only be used with a query. diff --git a/src/AppInstallerCLITests/WorkFlow.cpp b/src/AppInstallerCLITests/WorkFlow.cpp index f364c3bdea..e88bda9f56 100644 --- a/src/AppInstallerCLITests/WorkFlow.cpp +++ b/src/AppInstallerCLITests/WorkFlow.cpp @@ -2333,7 +2333,7 @@ TEST_CASE("UpdateFlow_All_RequireExplicit", "[UpdateFlow][workflow]") SECTION("Include package with explicit upgrade") { - context.Args.AddArg(Args::Type::IncludeExplicit); + context.Args.AddArg(Args::Type::IncludePinned); SECTION("List available upgrades") { From 3c2546a6f5cfec449934475d4b71c28eb45a6eee Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Thu, 28 Jul 2022 13:32:18 -0700 Subject: [PATCH 5/9] Store state in tracking catalog --- .../Workflows/UpdateFlow.cpp | 24 ++++++++++++------- .../Workflows/WorkflowBase.cpp | 3 ++- .../PackageTrackingCatalog.cpp | 6 ++++- .../Public/winget/RepositorySearch.h | 2 ++ 4 files changed, 25 insertions(+), 10 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp index 4e9b17bd2c..048ecebd3e 100644 --- a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp @@ -180,15 +180,23 @@ namespace AppInstaller::CLI::Workflow } // Filter out packages that require explicit upgrades. - // Note that this is filtering based on the updated installer, not the installed version. So there - // is an edge case where if an app that does not require explicit update changes to require it - // (e.g. it adds auto-updates) we will need to do explicit update to that version. - if (!context.Args.Contains(Execution::Args::Type::IncludeExplicit) && - updateContext.Get()->RequireExplicitUpgrade) + if (!context.Args.Contains(Execution::Args::Type::IncludePinned)) { - AICLI_LOG(CLI, Info, << "Skipping " << match.Package->GetProperty(PackageProperty::Id) << " as it requires explicit upgrade"); - ++packagesThatRequireExplicitSkipped; - continue; + // We require explicit upgrades only if the installed version is pinned, + // either because it was manually pinned or because the manifest indicated + // RequireExplicitUpgrade. + // Note that this does not consider whether the update to be installed has + // RequireExplicitUpgrade. While this has the downside of not working with + // packages installed from another source, it ensures consistency with the + // list of available updates (there we don't have the selected installer) + // and at most we will update each package like this once. + auto installedMetadata = updateContext.Get()->GetMetadata(); + if (installedMetadata[PackageVersionMetadata::IsPinned] == "1") + { + AICLI_LOG(CLI, Info, << "Skipping " << match.Package->GetProperty(PackageProperty::Id) << " as it requires explicit upgrade"); + ++packagesThatRequireExplicitSkipped; + continue; + } } updateAllFoundUpdate = true; diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index df18338252..7b7bc8d581 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -791,7 +791,8 @@ namespace AppInstaller::CLI::Workflow shouldShowSource ? sourceName : Utility::LocIndString() ); - bool requiresExplicitUpgrade = m_onlyShowUpgrades && false; + bool requiresExplicitUpgrade = m_onlyShowUpgrades && + installedVersion->GetMetadata()[PackageVersionMetadata::IsPinned] == "1"; if (requiresExplicitUpgrade) { lines.push_back(std::move(line)); diff --git a/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp b/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp index a05e3a4527..ec8186b238 100644 --- a/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp +++ b/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp @@ -215,7 +215,6 @@ namespace AppInstaller::Repository bool isUpgrade) { // TODO: Store additional information from these if needed - UNREFERENCED_PARAMETER(installer); UNREFERENCED_PARAMETER(isUpgrade); auto& index = m_implementation->Source->GetIndex(); @@ -239,6 +238,11 @@ namespace AppInstaller::Repository strstr << Utility::GetCurrentUnixEpoch(); index.SetMetadataByManifestId(manifestId, PackageVersionMetadata::TrackingWriteTime, strstr.str()); + if (installer.RequireExplicitUpgrade) + { + index.SetMetadataByManifestId(manifestId, PackageVersionMetadata::IsPinned, "1"sv); + } + std::shared_ptr result = std::make_shared(); result->Id = manifestId; return { std::move(result) }; diff --git a/src/AppInstallerRepositoryCore/Public/winget/RepositorySearch.h b/src/AppInstallerRepositoryCore/Public/winget/RepositorySearch.h index 6e9fe6a8fe..40c985beb4 100644 --- a/src/AppInstallerRepositoryCore/Public/winget/RepositorySearch.h +++ b/src/AppInstallerRepositoryCore/Public/winget/RepositorySearch.h @@ -178,6 +178,8 @@ namespace AppInstaller::Repository TrackingWriteTime, // The Architecture of an installed package InstalledArchitecture, + // Whether the package is pinned and should only be updated if explicitly targeted + IsPinned, }; // Convert a PackageVersionMetadata to a string. From bf1ca331c522994fea5348093c04db9cc53ffb27 Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Fri, 29 Jul 2022 15:15:21 -0700 Subject: [PATCH 6/9] Update tests --- .../AppInstallerCLITests.vcxproj | 7 +- .../AppInstallerCLITests.vcxproj.filters | 3 - .../UpdateFlowTest_Exe_2_RequireExplicit.yaml | 21 --- src/AppInstallerCLITests/WorkFlow.cpp | 139 ++++++++++-------- 4 files changed, 76 insertions(+), 94 deletions(-) delete mode 100644 src/AppInstallerCLITests/TestData/UpdateFlowTest_Exe_2_RequireExplicit.yaml diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj index 61d657502a..6c92d38159 100644 --- a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj +++ b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj @@ -295,7 +295,7 @@ true - + true @@ -611,9 +611,6 @@ true - - true - true @@ -625,7 +622,7 @@ true - + true diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters index 09b7116a4a..9e45f86551 100644 --- a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters +++ b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters @@ -552,9 +552,6 @@ TestData - - TestData - TestData diff --git a/src/AppInstallerCLITests/TestData/UpdateFlowTest_Exe_2_RequireExplicit.yaml b/src/AppInstallerCLITests/TestData/UpdateFlowTest_Exe_2_RequireExplicit.yaml deleted file mode 100644 index 65491a1351..0000000000 --- a/src/AppInstallerCLITests/TestData/UpdateFlowTest_Exe_2_RequireExplicit.yaml +++ /dev/null @@ -1,21 +0,0 @@ -# Similar content to UpdateFlowTest_Exe_2.yaml but requiring explicit upgrade -PackageIdentifier: AppInstallerCliTest.TestExeInstaller -PackageVersion: 3.0.0.0 -PackageLocale: en-US -PackageName: AppInstaller Test Installer -Publisher: Microsoft Corporation -Moniker: AICLITestExe -License: Test -InstallerSwitches: - Custom: /custom /ver3.0.0.0 - SilentWithProgress: /silentwithprogress - Silent: /silence - Update: /update -RequireExplicitUpgrade: true -Installers: - - Architecture: x86 - InstallerUrl: https://ThisIsNotUsed - InstallerType: exe - InstallerSha256: 65DB2F2AC2686C7F2FD69D4A4C6683B888DC55BFA20A0E32CA9F838B51689A3B -ManifestType: singleton -ManifestVersion: 1.1.0 \ No newline at end of file diff --git a/src/AppInstallerCLITests/WorkFlow.cpp b/src/AppInstallerCLITests/WorkFlow.cpp index e88bda9f56..0e6db25d12 100644 --- a/src/AppInstallerCLITests/WorkFlow.cpp +++ b/src/AppInstallerCLITests/WorkFlow.cpp @@ -109,7 +109,7 @@ namespace { None = 0, UpgradeUsesAgreements, - UpgradeUsesRequireExplicit, + UpgradeRequiresExplicit, }; struct WorkflowTestCompositeSource : public TestSource @@ -146,10 +146,6 @@ namespace case TestSourceSearchOptions::UpgradeUsesAgreements: manifest3 = YamlParser::CreateFromPath(TestDataFile("UpdateFlowTest_Exe_2_LicenseAgreement.yaml")); break; - case TestSourceSearchOptions::UpgradeUsesRequireExplicit: - manifest3 = YamlParser::CreateFromPath(TestDataFile("UpdateFlowTest_Exe_2_RequireExplicit.yaml")); - break; - case TestSourceSearchOptions::None: default: manifest3 = YamlParser::CreateFromPath(TestDataFile("UpdateFlowTest_Exe_2.yaml")); break; @@ -204,11 +200,21 @@ namespace break; } + TestPackage::MetadataMap packageMetadata + { + { PackageVersionMetadata::InstalledType, "Msix" }, + }; + + if (m_searchOptions == TestSourceSearchOptions::UpgradeRequiresExplicit) + { + packageMetadata[PackageVersionMetadata::PinnedState] = "PinnedByManifest"; + } + result.Matches.emplace_back( ResultMatch( TestPackage::Make( manifest, - TestPackage::MetadataMap{ { PackageVersionMetadata::InstalledType, "Msix" } }, + packageMetadata, std::vector{ manifest2, manifest }, shared_from_this() ), @@ -2282,6 +2288,18 @@ TEST_CASE("UninstallFlow_UninstallPortable", "[UninstallFlow][workflow]") REQUIRE(std::filesystem::exists(uninstallResultPath.GetPath())); } +TEST_CASE("UpdateFlow_RequireExplicit", "[UpdateFlow][workflow]") +{ + // Install a manifest with "RequiresExplicitUpgrade" and verify that the flag is respected. + std::ostringstream updateOutput; + TestContext context{ updateOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + + // Exe package has an update that requires explicit upgrade. + // Msix is also listed with an available upgrade. + OverrideForOpenSource(context); +} + TEST_CASE("UpdateFlow_All_RequireExplicit", "[UpdateFlow][workflow]") { TestCommon::TempFile updateExeResultPath("TestExeInstalled.txt"); @@ -2291,80 +2309,71 @@ TEST_CASE("UpdateFlow_All_RequireExplicit", "[UpdateFlow][workflow]") TestContext context{ updateOutput, std::cin }; auto previousThreadGlobals = context.SetForCurrentThread(); - // Exe package has an update that requires explicit upgrade. - // Msix is also listed with an available upgrade. - OverrideForCompositeInstalledSource(context, TestSourceSearchOptions::UpgradeUsesRequireExplicit); + // Msix package has an update that requires explicit upgrade. + // Exe, Portable, MSStore, Zip are also listed with an available upgrade. + OverrideForCompositeInstalledSource(context, TestSourceSearchOptions::UpgradeRequiresExplicit); - SECTION("Skip package with explicit upgrade") + SECTION("List available upgrades") { - SECTION("List available upgrades") - { - UpgradeCommand update({}); - update.Execute(context); - INFO(updateOutput.str()); - - // The package that requires explicit upgrade is still listed, as we don't select - // the installer yet so we don't know if it will require it. - // TODO: Should we select the installer when listing so we can skip it? - REQUIRE(updateOutput.str().find(Resource::LocString(Resource::String::UpgradeRequireExplicitCount)) == std::string::npos); - REQUIRE(updateOutput.str().find("AppInstallerCliTest.TestExeInstaller") != std::string::npos); - REQUIRE(updateOutput.str().find("AppInstallerCliTest.TestMsixInstaller") != std::string::npos); - } + UpgradeCommand update({}); + update.Execute(context); + INFO(updateOutput.str()); + + // The package that requires explicit upgrade is listed below the header for pinned packages + REQUIRE(updateOutput.str().find("AppInstallerCliTest.TestExeInstaller") != std::string::npos); + + auto pinnedPackagesHeaderPosition = updateOutput.str().find(Resource::LocString(Resource::String::UpgradeAvailableForPinned)); + auto pinnedPackageLinePosition = updateOutput.str().find("AppInstallerCliTest.TestMsixInstaller"); + REQUIRE(pinnedPackagesHeaderPosition != std::string::npos); + REQUIRE(pinnedPackageLinePosition != std::string::npos); + REQUIRE(pinnedPackagesHeaderPosition < pinnedPackageLinePosition); + REQUIRE(updateOutput.str().find(Resource::LocString(Resource::String::UpgradeRequireExplicitCount)) == std::string::npos); + } - SECTION("Upgrade all") - { - context.Args.AddArg(Args::Type::All); - OverrideForMSIX(context); + SECTION("Upgrade all except pinned") + { + context.Args.AddArg(Args::Type::All); + OverrideForMSStore(context, true); + OverrideForPortableInstall(context); + OverrideForShellExecute(context); + OverrideForExtractInstallerFromArchive(context); + OverrideForVerifyAndSetNestedInstaller(context); - UpgradeCommand update({}); - update.Execute(context); - INFO(updateOutput.str()); + UpgradeCommand update({}); + update.Execute(context); + INFO(updateOutput.str()); - auto s = updateOutput.str(); + auto s = updateOutput.str(); - // Verify message is printed for skipped package - REQUIRE(updateOutput.str().find(Resource::LocString(Resource::String::UpgradeRequireExplicitCount)) != std::string::npos); + // Verify message is printed for skipped package + REQUIRE(updateOutput.str().find(Resource::LocString(Resource::String::UpgradeRequireExplicitCount)) != std::string::npos); - // Verify package is not installed, but all others are - REQUIRE(!std::filesystem::exists(updateExeResultPath.GetPath())); - REQUIRE(std::filesystem::exists(updateMsixResultPath.GetPath())); - } + // Verify package is not installed, but all others are + REQUIRE(std::filesystem::exists(updateExeResultPath.GetPath())); + REQUIRE(!std::filesystem::exists(updateMsixResultPath.GetPath())); } - SECTION("Include package with explicit upgrade") + SECTION("Upgrade all including pinned") { context.Args.AddArg(Args::Type::IncludePinned); - SECTION("List available upgrades") - { - UpgradeCommand update({}); - update.Execute(context); - INFO(updateOutput.str()); - - - auto s = updateOutput.str(); - - // Verify all packages with updates are listed. - REQUIRE(updateOutput.str().find("AppInstallerCliTest.TestExeInstaller") != std::string::npos); - REQUIRE(updateOutput.str().find("AppInstallerCliTest.TestMsixInstaller") != std::string::npos); - } - - SECTION("Upgrade all") - { - context.Args.AddArg(Args::Type::All); - OverrideForShellExecute(context); - OverrideForMSIX(context); + context.Args.AddArg(Args::Type::All); + OverrideForShellExecute(context); + OverrideForMSIX(context); + OverrideForMSStore(context, true); + OverrideForPortableInstall(context); + OverrideForExtractInstallerFromArchive(context); + OverrideForVerifyAndSetNestedInstaller(context); - UpgradeCommand update({}); - update.Execute(context); - INFO(updateOutput.str()); + UpgradeCommand update({}); + update.Execute(context); + INFO(updateOutput.str()); - auto s = updateOutput.str(); + auto s = updateOutput.str(); - // Verify all packages are updated - REQUIRE(std::filesystem::exists(updateExeResultPath.GetPath())); - REQUIRE(std::filesystem::exists(updateMsixResultPath.GetPath())); - } + // Verify all packages are updated + REQUIRE(std::filesystem::exists(updateExeResultPath.GetPath())); + REQUIRE(std::filesystem::exists(updateMsixResultPath.GetPath())); // Verify that skipped package message is not printed REQUIRE(updateOutput.str().find(Resource::LocString(Resource::String::UpgradeRequireExplicitCount)) == std::string::npos); From aaf60a98674251d856191592f790aefdc63c8907 Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Fri, 29 Jul 2022 15:15:38 -0700 Subject: [PATCH 7/9] Use enum in tracking catalog --- src/AppInstallerCLICore/Resources.h | 1 + .../Workflows/UpdateFlow.cpp | 3 ++- .../Workflows/WorkflowBase.cpp | 9 ++++---- .../Shared/Strings/en-us/winget.resw | 4 ++++ src/AppInstallerCLITests/WorkFlow.cpp | 12 ---------- .../PackageTrackingCatalog.cpp | 2 +- .../Public/winget/RepositorySearch.h | 15 +++++++++++-- .../RepositorySearch.cpp | 22 +++++++++++++++++++ 8 files changed, 48 insertions(+), 20 deletions(-) diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index ff9b0fe90d..6ddfcc6b21 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -374,6 +374,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(UnsupportedArgument); WINGET_DEFINE_RESOURCE_STRINGID(UpdateAllArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(UpdateNotApplicable); + WINGET_DEFINE_RESOURCE_STRINGID(UpgradeAvailableForPinned); WINGET_DEFINE_RESOURCE_STRINGID(UpgradeCommandLongDescription); WINGET_DEFINE_RESOURCE_STRINGID(UpgradeCommandShortDescription); WINGET_DEFINE_RESOURCE_STRINGID(UpgradeDifferentInstallTechnology); diff --git a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp index 048ecebd3e..e20eb1f83a 100644 --- a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp @@ -191,7 +191,8 @@ namespace AppInstaller::CLI::Workflow // list of available updates (there we don't have the selected installer) // and at most we will update each package like this once. auto installedMetadata = updateContext.Get()->GetMetadata(); - if (installedMetadata[PackageVersionMetadata::IsPinned] == "1") + auto pinnedState = ConvertToPackagePinnedStateEnum(installedMetadata[PackageVersionMetadata::PinnedState]); + if (pinnedState == PackagePinnedState::PinnedByManifest) { AICLI_LOG(CLI, Info, << "Skipping " << match.Package->GetProperty(PackageProperty::Id) << " as it requires explicit upgrade"); ++packagesThatRequireExplicitSkipped; diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index 7b7bc8d581..047337ee94 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -791,15 +791,16 @@ namespace AppInstaller::CLI::Workflow shouldShowSource ? sourceName : Utility::LocIndString() ); + auto pinnedState = ConvertToPackagePinnedStateEnum(installedVersion->GetMetadata()[PackageVersionMetadata::PinnedState]); bool requiresExplicitUpgrade = m_onlyShowUpgrades && - installedVersion->GetMetadata()[PackageVersionMetadata::IsPinned] == "1"; + (pinnedState == PackagePinnedState::PinnedByManifest); if (requiresExplicitUpgrade) { - lines.push_back(std::move(line)); + linesForExplicitUpgrade.push_back(std::move(line)); } else { - linesForExplicitUpgrade.push_back(std::move(line)); + lines.push_back(std::move(line)); } } } @@ -826,7 +827,7 @@ namespace AppInstaller::CLI::Workflow if (!linesForExplicitUpgrade.empty()) { - context.Reporter.Info() << std::endl << "The following packages have an upgrade available, but require explicit targeting for upgrade:"_liv << std::endl; /* TODO */ + context.Reporter.Info() << std::endl << Resource::String::UpgradeAvailableForPinned << std::endl; OutputInstalledPackagesTable(context, linesForExplicitUpgrade); } diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index f972a546c4..8249ef4f7b 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -1393,4 +1393,8 @@ Please specify one of them using the `--source` option to proceed. Incompatible command line arguments provided + + The following packages have an upgrade available, but require explicit targeting for upgrade: + "require explicit targeting for upgrade" means that the package will not be upgraded with all others unless an extra flag is added, or the package is mentioned explicitly + \ No newline at end of file diff --git a/src/AppInstallerCLITests/WorkFlow.cpp b/src/AppInstallerCLITests/WorkFlow.cpp index 0e6db25d12..86d1e5bfe7 100644 --- a/src/AppInstallerCLITests/WorkFlow.cpp +++ b/src/AppInstallerCLITests/WorkFlow.cpp @@ -2288,18 +2288,6 @@ TEST_CASE("UninstallFlow_UninstallPortable", "[UninstallFlow][workflow]") REQUIRE(std::filesystem::exists(uninstallResultPath.GetPath())); } -TEST_CASE("UpdateFlow_RequireExplicit", "[UpdateFlow][workflow]") -{ - // Install a manifest with "RequiresExplicitUpgrade" and verify that the flag is respected. - std::ostringstream updateOutput; - TestContext context{ updateOutput, std::cin }; - auto previousThreadGlobals = context.SetForCurrentThread(); - - // Exe package has an update that requires explicit upgrade. - // Msix is also listed with an available upgrade. - OverrideForOpenSource(context); -} - TEST_CASE("UpdateFlow_All_RequireExplicit", "[UpdateFlow][workflow]") { TestCommon::TempFile updateExeResultPath("TestExeInstalled.txt"); diff --git a/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp b/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp index ec8186b238..74084fba8f 100644 --- a/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp +++ b/src/AppInstallerRepositoryCore/PackageTrackingCatalog.cpp @@ -240,7 +240,7 @@ namespace AppInstaller::Repository if (installer.RequireExplicitUpgrade) { - index.SetMetadataByManifestId(manifestId, PackageVersionMetadata::IsPinned, "1"sv); + index.SetMetadataByManifestId(manifestId, PackageVersionMetadata::PinnedState, ToString(PackagePinnedState::PinnedByManifest)); } std::shared_ptr result = std::make_shared(); diff --git a/src/AppInstallerRepositoryCore/Public/winget/RepositorySearch.h b/src/AppInstallerRepositoryCore/Public/winget/RepositorySearch.h index 40c985beb4..247d8027ba 100644 --- a/src/AppInstallerRepositoryCore/Public/winget/RepositorySearch.h +++ b/src/AppInstallerRepositoryCore/Public/winget/RepositorySearch.h @@ -178,13 +178,24 @@ namespace AppInstaller::Repository TrackingWriteTime, // The Architecture of an installed package InstalledArchitecture, - // Whether the package is pinned and should only be updated if explicitly targeted - IsPinned, + // The PackagePinnedState of the installed package + PinnedState, }; // Convert a PackageVersionMetadata to a string. std::string_view ToString(PackageVersionMetadata pvm); + // Possible pinned states for a package. + // Pinned packages need to be explicitly updated (i.e., are not included in `upgrade --all`) + enum class PackagePinnedState + { + Unknown, + PinnedByManifest, + }; + + std::string_view ToString(PackagePinnedState state); + PackagePinnedState ConvertToPackagePinnedStateEnum(std::string_view in); + // A single package version. struct IPackageVersion { diff --git a/src/AppInstallerRepositoryCore/RepositorySearch.cpp b/src/AppInstallerRepositoryCore/RepositorySearch.cpp index f27c687c55..c067954bb7 100644 --- a/src/AppInstallerRepositoryCore/RepositorySearch.cpp +++ b/src/AppInstallerRepositoryCore/RepositorySearch.cpp @@ -88,6 +88,28 @@ namespace AppInstaller::Repository } } + std::string_view ToString(PackagePinnedState state) + { + switch (state) + { + case PackagePinnedState::PinnedByManifest: return "PinnedByManifest"sv; + default: return "Unknown"; + } + } + + PackagePinnedState ConvertToPackagePinnedStateEnum(std::string_view in) + { + if (Utility::CaseInsensitiveEquals(in, "PinnedByManifest"sv)) + { + return PackagePinnedState::PinnedByManifest; + } + else + { + return PackagePinnedState::Unknown; + } + } + + const char* UnsupportedRequestException::what() const noexcept { if (m_whatMessage.empty()) From a130778707d412bf12152927a7261ad12518bffc Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Mon, 1 Aug 2022 16:09:29 -0700 Subject: [PATCH 8/9] PR comments --- src/AppInstallerCLICore/Workflows/UpdateFlow.cpp | 2 +- src/AppInstallerCLICore/Workflows/WorkflowBase.cpp | 3 +-- .../Public/winget/RepositorySearch.h | 2 +- src/AppInstallerRepositoryCore/RepositorySearch.cpp | 6 ++++-- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp index e20eb1f83a..eb012f57d5 100644 --- a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp @@ -192,7 +192,7 @@ namespace AppInstaller::CLI::Workflow // and at most we will update each package like this once. auto installedMetadata = updateContext.Get()->GetMetadata(); auto pinnedState = ConvertToPackagePinnedStateEnum(installedMetadata[PackageVersionMetadata::PinnedState]); - if (pinnedState == PackagePinnedState::PinnedByManifest) + if (pinnedState != PackagePinnedState::NotPinned) { AICLI_LOG(CLI, Info, << "Skipping " << match.Package->GetProperty(PackageProperty::Id) << " as it requires explicit upgrade"); ++packagesThatRequireExplicitSkipped; diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index 047337ee94..65dd2928fd 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -792,8 +792,7 @@ namespace AppInstaller::CLI::Workflow ); auto pinnedState = ConvertToPackagePinnedStateEnum(installedVersion->GetMetadata()[PackageVersionMetadata::PinnedState]); - bool requiresExplicitUpgrade = m_onlyShowUpgrades && - (pinnedState == PackagePinnedState::PinnedByManifest); + bool requiresExplicitUpgrade = m_onlyShowUpgrades && pinnedState != PackagePinnedState::NotPinned; if (requiresExplicitUpgrade) { linesForExplicitUpgrade.push_back(std::move(line)); diff --git a/src/AppInstallerRepositoryCore/Public/winget/RepositorySearch.h b/src/AppInstallerRepositoryCore/Public/winget/RepositorySearch.h index 247d8027ba..9dd72d70e5 100644 --- a/src/AppInstallerRepositoryCore/Public/winget/RepositorySearch.h +++ b/src/AppInstallerRepositoryCore/Public/winget/RepositorySearch.h @@ -189,7 +189,7 @@ namespace AppInstaller::Repository // Pinned packages need to be explicitly updated (i.e., are not included in `upgrade --all`) enum class PackagePinnedState { - Unknown, + NotPinned, PinnedByManifest, }; diff --git a/src/AppInstallerRepositoryCore/RepositorySearch.cpp b/src/AppInstallerRepositoryCore/RepositorySearch.cpp index c067954bb7..0bd637785e 100644 --- a/src/AppInstallerRepositoryCore/RepositorySearch.cpp +++ b/src/AppInstallerRepositoryCore/RepositorySearch.cpp @@ -93,7 +93,9 @@ namespace AppInstaller::Repository switch (state) { case PackagePinnedState::PinnedByManifest: return "PinnedByManifest"sv; - default: return "Unknown"; + case PackagePinnedState::NotPinned: + default: + return "Unknown"; } } @@ -105,7 +107,7 @@ namespace AppInstaller::Repository } else { - return PackagePinnedState::Unknown; + return PackagePinnedState::NotPinned; } } From 9ac8336d9d1ec42bd1829b33a8e9ac34c3a68e9d Mon Sep 17 00:00:00 2001 From: Flor Elisa Chacon Ochoa Date: Tue, 2 Aug 2022 17:08:32 -0700 Subject: [PATCH 9/9] Remove --include-pinned arg --- .../Commands/UpgradeCommand.cpp | 4 +-- src/AppInstallerCLICore/ExecutionArgs.h | 1 - src/AppInstallerCLICore/Resources.h | 1 - .../Workflows/UpdateFlow.cpp | 31 +++++++++---------- .../Shared/Strings/en-us/winget.resw | 7 ++--- src/AppInstallerCLITests/WorkFlow.cpp | 20 ++---------- 6 files changed, 20 insertions(+), 44 deletions(-) diff --git a/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp b/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp index f1edd3a0bf..530b091dc5 100644 --- a/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp +++ b/src/AppInstallerCLICore/Commands/UpgradeCommand.cpp @@ -44,8 +44,7 @@ namespace AppInstaller::CLI bool HasArgumentsForMultiplePackages(Execution::Args& execArgs) { return execArgs.Contains(Args::Type::All) || - execArgs.Contains(Args::Type::IncludeUnknown) || - execArgs.Contains(Args::Type::IncludePinned); + execArgs.Contains(Args::Type::IncludeUnknown); } // Determines whether there are any arguments only used as options during an upgrade, @@ -105,7 +104,6 @@ namespace AppInstaller::CLI 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-pinned", Argument::NoAlias, Args::Type::IncludePinned, Resource::String::IncludePinnedArgumentDescription, ArgumentType::Flag }, }; } diff --git a/src/AppInstallerCLICore/ExecutionArgs.h b/src/AppInstallerCLICore/ExecutionArgs.h index 201e3b4ee4..80568e1b89 100644 --- a/src/AppInstallerCLICore/ExecutionArgs.h +++ b/src/AppInstallerCLICore/ExecutionArgs.h @@ -82,7 +82,6 @@ namespace AppInstaller::CLI::Execution // Upgrade Command All, // Update all installed packages to latest IncludeUnknown, // Allow upgrades of packages with unknown versions - IncludePinned, // Allow upgrades of packages pinned by default // Other ListVersions, // Used in Show command to list all available versions of an app diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index 6ddfcc6b21..d369b4cb99 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -107,7 +107,6 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(ImportPackageAlreadyInstalled); WINGET_DEFINE_RESOURCE_STRINGID(ImportSearchFailed); WINGET_DEFINE_RESOURCE_STRINGID(ImportSourceNotInstalled); - WINGET_DEFINE_RESOURCE_STRINGID(IncludePinnedArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(IncludeUnknownArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(IncompatibleArgumentsProvided); WINGET_DEFINE_RESOURCE_STRINGID(InstallAndUpgradeCommandsReportDependencies); diff --git a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp index eb012f57d5..2d75611b11 100644 --- a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp @@ -180,24 +180,21 @@ namespace AppInstaller::CLI::Workflow } // Filter out packages that require explicit upgrades. - if (!context.Args.Contains(Execution::Args::Type::IncludePinned)) + // We require explicit upgrades only if the installed version is pinned, + // either because it was manually pinned or because the manifest indicated + // RequireExplicitUpgrade. + // Note that this does not consider whether the update to be installed has + // RequireExplicitUpgrade. While this has the downside of not working with + // packages installed from another source, it ensures consistency with the + // list of available updates (there we don't have the selected installer) + // and at most we will update each package like this once. + auto installedMetadata = updateContext.Get()->GetMetadata(); + auto pinnedState = ConvertToPackagePinnedStateEnum(installedMetadata[PackageVersionMetadata::PinnedState]); + if (pinnedState != PackagePinnedState::NotPinned) { - // We require explicit upgrades only if the installed version is pinned, - // either because it was manually pinned or because the manifest indicated - // RequireExplicitUpgrade. - // Note that this does not consider whether the update to be installed has - // RequireExplicitUpgrade. While this has the downside of not working with - // packages installed from another source, it ensures consistency with the - // list of available updates (there we don't have the selected installer) - // and at most we will update each package like this once. - auto installedMetadata = updateContext.Get()->GetMetadata(); - auto pinnedState = ConvertToPackagePinnedStateEnum(installedMetadata[PackageVersionMetadata::PinnedState]); - if (pinnedState != PackagePinnedState::NotPinned) - { - AICLI_LOG(CLI, Info, << "Skipping " << match.Package->GetProperty(PackageProperty::Id) << " as it requires explicit upgrade"); - ++packagesThatRequireExplicitSkipped; - continue; - } + AICLI_LOG(CLI, Info, << "Skipping " << match.Package->GetProperty(PackageProperty::Id) << " as it requires explicit upgrade"); + ++packagesThatRequireExplicitSkipped; + continue; } updateAllFoundUpdate = true; diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index 8249ef4f7b..4fed42652f 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -1276,12 +1276,9 @@ Please specify one of them using the `--source` option to proceed. package(s) have version numbers that cannot be determined. Use "--include-unknown" to see all results. {Locked="--include-unknown"} This string is preceded by a (integer) number of packages that do not have notated versions. - - Upgrade packages that are pinned by default - - package(s) is pinned and needs to be explicitly upgraded. Use "--include-pinned" to see all results. - {Locked="--include-pinned"} This string is preceded by a (integer) number of packages that require explicit upgrades. + package(s) is pinned and needs to be explicitly upgraded. + This string is preceded by a (integer) number of packages that require explicit upgrades. The arguments provided can only be used with a query. diff --git a/src/AppInstallerCLITests/WorkFlow.cpp b/src/AppInstallerCLITests/WorkFlow.cpp index 86d1e5bfe7..6e8411c71b 100644 --- a/src/AppInstallerCLITests/WorkFlow.cpp +++ b/src/AppInstallerCLITests/WorkFlow.cpp @@ -2288,7 +2288,7 @@ TEST_CASE("UninstallFlow_UninstallPortable", "[UninstallFlow][workflow]") REQUIRE(std::filesystem::exists(uninstallResultPath.GetPath())); } -TEST_CASE("UpdateFlow_All_RequireExplicit", "[UpdateFlow][workflow]") +TEST_CASE("UpdateFlow_RequireExplicit", "[UpdateFlow][workflow]") { TestCommon::TempFile updateExeResultPath("TestExeInstalled.txt"); TestCommon::TempFile updateMsixResultPath("TestMsixInstalled.txt"); @@ -2341,30 +2341,16 @@ TEST_CASE("UpdateFlow_All_RequireExplicit", "[UpdateFlow][workflow]") REQUIRE(!std::filesystem::exists(updateMsixResultPath.GetPath())); } - SECTION("Upgrade all including pinned") + SECTION("Upgrade explicitly") { - context.Args.AddArg(Args::Type::IncludePinned); - - context.Args.AddArg(Args::Type::All); - OverrideForShellExecute(context); + context.Args.AddArg(Execution::Args::Type::Query, "AppInstallerCliTest.TestMsixInstaller"sv); OverrideForMSIX(context); - OverrideForMSStore(context, true); - OverrideForPortableInstall(context); - OverrideForExtractInstallerFromArchive(context); - OverrideForVerifyAndSetNestedInstaller(context); UpgradeCommand update({}); update.Execute(context); INFO(updateOutput.str()); - auto s = updateOutput.str(); - - // Verify all packages are updated - REQUIRE(std::filesystem::exists(updateExeResultPath.GetPath())); REQUIRE(std::filesystem::exists(updateMsixResultPath.GetPath())); - - // Verify that skipped package message is not printed - REQUIRE(updateOutput.str().find(Resource::LocString(Resource::String::UpgradeRequireExplicitCount)) == std::string::npos); } // Command should always succeed