From 88637a46482c5f21ed6d3b56f4508f6b474fb3a6 Mon Sep 17 00:00:00 2001 From: Ruben Guerrero Samaniego Date: Tue, 26 Oct 2021 18:39:58 -0700 Subject: [PATCH 1/6] Add inapplicability flag --- .../Workflows/ManifestComparator.cpp | 44 +++--- .../Workflows/ManifestComparator.h | 24 +++- .../Workflows/UpdateFlow.cpp | 10 +- .../Workflows/WorkflowBase.cpp | 3 +- .../ManifestComparator.cpp | 134 +++++++++--------- 5 files changed, 128 insertions(+), 87 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp b/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp index 4deaaa054e..4bf0a08e90 100644 --- a/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp +++ b/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp @@ -24,7 +24,7 @@ namespace AppInstaller::CLI::Workflow { struct OSVersionFilter : public details::FilterField { - OSVersionFilter() : details::FilterField("OS Version") {} + OSVersionFilter() : details::FilterField("OS Version", InapplicabilityFlags::OSVersion) {} bool IsApplicable(const Manifest::ManifestInstaller& installer) override { @@ -41,10 +41,10 @@ namespace AppInstaller::CLI::Workflow struct MachineArchitectureComparator : public details::ComparisonField { - MachineArchitectureComparator() : details::ComparisonField("Machine Architecture") {} + MachineArchitectureComparator() : details::ComparisonField("Machine Architecture", InapplicabilityFlags::MachineArchitecture) {} MachineArchitectureComparator(std::vector allowedArchitectures) : - details::ComparisonField("Machine Architecture"), m_allowedArchitectures(std::move(allowedArchitectures)) + details::ComparisonField("Machine Architecture", InapplicabilityFlags::MachineArchitecture), m_allowedArchitectures(std::move(allowedArchitectures)) { AICLI_LOG(CLI, Verbose, << "Architecture Comparator created with allowed architectures: " << GetAllowedArchitecturesString()); } @@ -165,7 +165,7 @@ namespace AppInstaller::CLI::Workflow struct InstalledTypeComparator : public details::ComparisonField { InstalledTypeComparator(Manifest::InstallerTypeEnum installedType) : - details::ComparisonField("Installed Type"), m_installedType(installedType) {} + details::ComparisonField("Installed Type", InapplicabilityFlags::InstalledType), m_installedType(installedType) {} static std::unique_ptr Create(const Repository::IPackageVersion::Metadata& installationMetadata) { @@ -206,7 +206,7 @@ namespace AppInstaller::CLI::Workflow struct InstalledScopeFilter : public details::FilterField { InstalledScopeFilter(Manifest::ScopeEnum requirement) : - details::FilterField("Installed Scope"), m_requirement(requirement) {} + details::FilterField("Installed Scope", InapplicabilityFlags::InstalledScope), m_requirement(requirement) {} static std::unique_ptr Create(const Repository::IPackageVersion::Metadata& installationMetadata) { @@ -246,7 +246,7 @@ namespace AppInstaller::CLI::Workflow struct ScopeComparator : public details::ComparisonField { ScopeComparator(Manifest::ScopeEnum preference, Manifest::ScopeEnum requirement) : - details::ComparisonField("Scope"), m_preference(preference), m_requirement(requirement) {} + details::ComparisonField("Scope", InapplicabilityFlags::Scope), m_preference(preference), m_requirement(requirement) {} static std::unique_ptr Create(const Execution::Args& args) { @@ -314,7 +314,7 @@ namespace AppInstaller::CLI::Workflow struct InstalledLocaleComparator : public details::ComparisonField { InstalledLocaleComparator(std::string installedLocale) : - details::ComparisonField("Installed Locale"), m_installedLocale(std::move(installedLocale)) {} + details::ComparisonField("Installed Locale", InapplicabilityFlags::InstalledLocale), m_installedLocale(std::move(installedLocale)) {} static std::unique_ptr Create(const Repository::IPackageVersion::Metadata& installationMetadata) { @@ -358,7 +358,7 @@ namespace AppInstaller::CLI::Workflow struct LocaleComparator : public details::ComparisonField { LocaleComparator(std::vector preference, std::vector requirement) : - details::ComparisonField("Locale"), m_preference(std::move(preference)), m_requirement(std::move(requirement)) + details::ComparisonField("Locale", InapplicabilityFlags::Locale), m_preference(std::move(preference)), m_requirement(std::move(requirement)) { m_requirementAsString = GetLocalesListAsString(m_requirement); m_preferenceAsString = GetLocalesListAsString(m_preference); @@ -502,24 +502,33 @@ namespace AppInstaller::CLI::Workflow AddComparator(MachineArchitectureComparator::Create(context, installationMetadata)); } - std::optional ManifestComparator::GetPreferredInstaller(const Manifest::Manifest& manifest) + std::tuple, InapplicabilityFlags> ManifestComparator::GetPreferredInstaller(const Manifest::Manifest& manifest) { AICLI_LOG(CLI, Info, << "Starting installer selection."); const Manifest::ManifestInstaller* result = nullptr; + InapplicabilityFlags inapplicabilityInstallers = InapplicabilityFlags::None; for (const auto& installer : manifest.Installers) { - if (IsApplicable(installer) && (!result || IsFirstBetter(installer, *result))) + auto inapplicabilityInstaller = IsApplicable(installer); + if (inapplicabilityInstaller == InapplicabilityFlags::None) { - AICLI_LOG(CLI, Verbose, << "Installer " << installer << " is current best choice"); - result = &installer; + if (!result || IsFirstBetter(installer, *result)) + { + AICLI_LOG(CLI, Verbose, << "Installer " << installer << " is current best choice"); + result = &installer; + } + } + else + { + WI_SetAllFlags(inapplicabilityInstallers, inapplicabilityInstaller); } } if (!result) { - return {}; + return { {}, inapplicabilityInstallers }; } Logging::Telemetry().LogSelectedInstaller( @@ -529,22 +538,21 @@ namespace AppInstaller::CLI::Workflow Manifest::ScopeToString(result->Scope), result->Locale); - return *result; + return { *result, InapplicabilityFlags::None }; } - // TODO: Implement a mechanism for better error messaging for no applicable installer scenario - bool ManifestComparator::IsApplicable(const Manifest::ManifestInstaller& installer) + InapplicabilityFlags ManifestComparator::IsApplicable(const Manifest::ManifestInstaller& installer) { for (const auto& filter : m_filters) { if (!filter->IsApplicable(installer)) { AICLI_LOG(CLI, Info, << "Installer " << installer << " not applicable: " << filter->ExplainInapplicable(installer)); - return false; + return filter->InapplicableReason(); } } - return true; + return InapplicabilityFlags::None; } bool ManifestComparator::IsFirstBetter( diff --git a/src/AppInstallerCLICore/Workflows/ManifestComparator.h b/src/AppInstallerCLICore/Workflows/ManifestComparator.h index 9c91294520..b8df075809 100644 --- a/src/AppInstallerCLICore/Workflows/ManifestComparator.h +++ b/src/AppInstallerCLICore/Workflows/ManifestComparator.h @@ -13,16 +13,33 @@ namespace AppInstaller::CLI::Workflow { + // Flags to indicate why an installer was not applicable + enum class InapplicabilityFlags : int + { + None = 0x0, + OSVersion = 0x1, + InstalledScope = 0x2, + InstalledType = 0x4, + InstalledLocale = 0x8, + Locale = 0x10, + Scope = 0x20, + MachineArchitecture = 0x40, + }; + + DEFINE_ENUM_FLAG_OPERATORS(InapplicabilityFlags); + namespace details { // An interface for defining new filters based on user inputs. struct FilterField { - FilterField(std::string_view name) : m_name(name) {} + FilterField(std::string_view name, InapplicabilityFlags installerApplicability) : + m_name(name), m_installerApplicability(installerApplicability) {} virtual ~FilterField() = default; std::string_view Name() const { return m_name; } + InapplicabilityFlags InapplicableReason() const { return m_installerApplicability; } // Determines if the installer is applicable based on this field alone. virtual bool IsApplicable(const Manifest::ManifestInstaller& installer) = 0; @@ -33,6 +50,7 @@ namespace AppInstaller::CLI::Workflow private: std::string_view m_name; + InapplicabilityFlags m_installerApplicability; }; // An interface for defining new comparisons based on user inputs. @@ -53,10 +71,10 @@ namespace AppInstaller::CLI::Workflow ManifestComparator(const Execution::Context& context, const Repository::IPackageVersion::Metadata& installationMetadata); // Gets the best installer from the manifest, if at least one is applicable. - std::optional GetPreferredInstaller(const Manifest::Manifest& manifest); + std::tuple, InapplicabilityFlags> GetPreferredInstaller(const Manifest::Manifest& manifest); // Determines if an installer is applicable. - bool IsApplicable(const Manifest::ManifestInstaller& installer); + InapplicabilityFlags IsApplicable(const Manifest::ManifestInstaller& installer); // Determines if the first installer is a better choice. bool IsFirstBetter( diff --git a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp index c214bc4971..3cdd56f3f4 100644 --- a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp @@ -27,6 +27,7 @@ namespace AppInstaller::CLI::Workflow Utility::Version installedVersion = Utility::Version(installedPackage->GetProperty(PackageVersionProperty::Version)); ManifestComparator manifestComparator(context, installedPackage->GetMetadata()); bool updateFound = false; + InapplicabilityFlags inapplicabilityVersions = InapplicabilityFlags::None; // The version keys should have already been sorted by version const auto& versionKeys = package->GetAvailableVersionKeys(); @@ -39,9 +40,10 @@ namespace AppInstaller::CLI::Workflow auto manifest = packageVersion->GetManifest(); // Check applicable Installer - auto installer = manifestComparator.GetPreferredInstaller(manifest); + auto [installer, inapplicability] = manifestComparator.GetPreferredInstaller(manifest); if (!installer.has_value()) { + WI_SetAllFlags(inapplicabilityVersions, inapplicability); continue; } @@ -66,7 +68,13 @@ namespace AppInstaller::CLI::Workflow if (m_reportUpdateNotFound) { context.Reporter.Info() << Resource::String::UpdateNotApplicable << std::endl; + + if (WI_IsSingleFlagSetInMask(inapplicabilityVersions, InapplicabilityFlags::InstalledType)) + { + context.Reporter.Info() << "TODO CHANGE" << std::endl; + } } + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE); } } diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index 14603ae67b..f9bd822168 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -917,7 +917,8 @@ namespace AppInstaller::CLI::Workflow } ManifestComparator manifestComparator(context, installationMetadata); - context.Add(manifestComparator.GetPreferredInstaller(context.Get())); + auto [installer, inapplicability] = manifestComparator.GetPreferredInstaller(context.Get()); + context.Add(installer); } void EnsureRunningAsAdmin(Execution::Context& context) diff --git a/src/AppInstallerCLITests/ManifestComparator.cpp b/src/AppInstallerCLITests/ManifestComparator.cpp index d5e1037bd6..9334fe368f 100644 --- a/src/AppInstallerCLITests/ManifestComparator.cpp +++ b/src/AppInstallerCLITests/ManifestComparator.cpp @@ -38,7 +38,7 @@ const ManifestInstaller& AddInstaller(Manifest& manifest, Architecture architect return manifest.Installers.back(); } -void RequireInstaller(const std::optional& actual, const ManifestInstaller& expected) +void RequireInstaller(const std::optional& actual, const ManifestInstaller& expected, InapplicabilityFlags inapplicability) { REQUIRE(actual); REQUIRE(actual->Arch == expected.Arch); @@ -46,6 +46,7 @@ void RequireInstaller(const std::optional& actual, const Mani REQUIRE(actual->Scope == expected.Scope); REQUIRE(actual->MinOSVersion == expected.MinOSVersion); REQUIRE(actual->Locale == expected.Locale); + REQUIRE(inapplicability == InapplicabilityFlags::None); } TEST_CASE("ManifestComparator_OSFilter_Low", "[manifest_comparator]") @@ -54,9 +55,10 @@ TEST_CASE("ManifestComparator_OSFilter_Low", "[manifest_comparator]") AddInstaller(manifest, Architecture::Neutral, InstallerTypeEnum::Exe, ScopeEnum::Unknown, "10.0.99999.0"); ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); REQUIRE(!result); + REQUIRE(WI_IsSingleFlagSetInMask(inapplicability, InapplicabilityFlags::OSVersion)); } TEST_CASE("ManifestComparator_OSFilter_High", "[manifest_comparator]") @@ -65,9 +67,9 @@ TEST_CASE("ManifestComparator_OSFilter_High", "[manifest_comparator]") ManifestInstaller expected = AddInstaller(manifest, Architecture::Neutral, InstallerTypeEnum::Exe, ScopeEnum::Unknown, "10.0.0.0"); ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, expected); + RequireInstaller(result, expected, inapplicability); } TEST_CASE("ManifestComparator_InstalledScopeFilter_Uknown", "[manifest_comparator]") @@ -78,10 +80,10 @@ TEST_CASE("ManifestComparator_InstalledScopeFilter_Uknown", "[manifest_comparato SECTION("Nothing Installed") { ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); // Only because it is first - RequireInstaller(result, unknown); + RequireInstaller(result, unknown, inapplicability); } SECTION("User Installed") { @@ -89,9 +91,9 @@ TEST_CASE("ManifestComparator_InstalledScopeFilter_Uknown", "[manifest_comparato metadata[PackageVersionMetadata::InstalledScope] = ScopeToString(ScopeEnum::User); ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, unknown); + RequireInstaller(result, unknown, inapplicability); } SECTION("Machine Installed") { @@ -99,9 +101,9 @@ TEST_CASE("ManifestComparator_InstalledScopeFilter_Uknown", "[manifest_comparato metadata[PackageVersionMetadata::InstalledScope] = ScopeToString(ScopeEnum::Machine); ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, unknown); + RequireInstaller(result, unknown, inapplicability); } } @@ -114,10 +116,10 @@ TEST_CASE("ManifestComparator_InstalledScopeFilter", "[manifest_comparator]") SECTION("Nothing Installed") { ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); // Only because it is first - RequireInstaller(result, user); + RequireInstaller(result, user, inapplicability); } SECTION("User Installed") { @@ -125,9 +127,9 @@ TEST_CASE("ManifestComparator_InstalledScopeFilter", "[manifest_comparator]") metadata[PackageVersionMetadata::InstalledScope] = ScopeToString(ScopeEnum::User); ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, user); + RequireInstaller(result, user, inapplicability); } SECTION("Machine Installed") { @@ -135,9 +137,9 @@ TEST_CASE("ManifestComparator_InstalledScopeFilter", "[manifest_comparator]") metadata[PackageVersionMetadata::InstalledScope] = ScopeToString(ScopeEnum::Machine); ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, machine); + RequireInstaller(result, machine, inapplicability); } } @@ -150,10 +152,10 @@ TEST_CASE("ManifestComparator_InstalledTypeFilter", "[manifest_comparator]") SECTION("Nothing Installed") { ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); // Only because it is first - RequireInstaller(result, msi); + RequireInstaller(result, msi, inapplicability); } SECTION("MSI Installed") { @@ -161,9 +163,9 @@ TEST_CASE("ManifestComparator_InstalledTypeFilter", "[manifest_comparator]") metadata[PackageVersionMetadata::InstalledType] = InstallerTypeToString(InstallerTypeEnum::Msi); ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, msi); + RequireInstaller(result, msi, inapplicability); } SECTION("MSIX Installed") { @@ -171,9 +173,9 @@ TEST_CASE("ManifestComparator_InstalledTypeFilter", "[manifest_comparator]") metadata[PackageVersionMetadata::InstalledType] = InstallerTypeToString(InstallerTypeEnum::Msix); ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, msix); + RequireInstaller(result, msix, inapplicability); } } @@ -186,10 +188,10 @@ TEST_CASE("ManifestComparator_InstalledTypeCompare", "[manifest_comparator]") SECTION("Nothing Installed") { ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); // Only because it is first - RequireInstaller(result, burn); + RequireInstaller(result, burn, inapplicability); } SECTION("Exe Installed") { @@ -197,9 +199,9 @@ TEST_CASE("ManifestComparator_InstalledTypeCompare", "[manifest_comparator]") metadata[PackageVersionMetadata::InstalledType] = InstallerTypeToString(InstallerTypeEnum::Exe); ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, exe); + RequireInstaller(result, exe, inapplicability); } SECTION("Inno Installed") { @@ -207,9 +209,9 @@ TEST_CASE("ManifestComparator_InstalledTypeCompare", "[manifest_comparator]") metadata[PackageVersionMetadata::InstalledType] = InstallerTypeToString(InstallerTypeEnum::Inno); ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, burn); + RequireInstaller(result, burn, inapplicability); } } @@ -222,10 +224,10 @@ TEST_CASE("ManifestComparator_ScopeFilter", "[manifest_comparator]") SECTION("Nothing Required") { ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); // Only because it is first - RequireInstaller(result, user); + RequireInstaller(result, user, inapplicability); } SECTION("User Required") { @@ -233,9 +235,9 @@ TEST_CASE("ManifestComparator_ScopeFilter", "[manifest_comparator]") context.Args.AddArg(Args::Type::InstallScope, ScopeToString(ScopeEnum::User)); ManifestComparator mc(context, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, user); + RequireInstaller(result, user, inapplicability); } SECTION("Machine Required") { @@ -243,9 +245,9 @@ TEST_CASE("ManifestComparator_ScopeFilter", "[manifest_comparator]") context.Args.AddArg(Args::Type::InstallScope, ScopeToString(ScopeEnum::Machine)); ManifestComparator mc(context, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, machine); + RequireInstaller(result, machine, inapplicability); } } @@ -258,10 +260,10 @@ TEST_CASE("ManifestComparator_ScopeCompare", "[manifest_comparator]") SECTION("No Preference") { ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); // The default preference is user - RequireInstaller(result, user); + RequireInstaller(result, user, inapplicability); } SECTION("User Preference") { @@ -269,9 +271,9 @@ TEST_CASE("ManifestComparator_ScopeCompare", "[manifest_comparator]") settings.Set(ScopePreference::User); ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, user); + RequireInstaller(result, user, inapplicability); } SECTION("Machine Preference") { @@ -279,9 +281,9 @@ TEST_CASE("ManifestComparator_ScopeCompare", "[manifest_comparator]") settings.Set(ScopePreference::Machine); ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, machine); + RequireInstaller(result, machine, inapplicability); } } @@ -297,10 +299,10 @@ TEST_CASE("ManifestComparator_InstalledLocaleComparator_Uknown", "[manifest_comp settings.Set({ "en-US" }); ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); // Only because it is first - RequireInstaller(result, enGB); + RequireInstaller(result, enGB, inapplicability); } SECTION("en-US Installed") { @@ -308,9 +310,9 @@ TEST_CASE("ManifestComparator_InstalledLocaleComparator_Uknown", "[manifest_comp metadata[PackageVersionMetadata::InstalledLocale] = "en-US"; ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, enGB); + RequireInstaller(result, enGB, inapplicability); } SECTION("zh-CN Installed") { @@ -318,9 +320,9 @@ TEST_CASE("ManifestComparator_InstalledLocaleComparator_Uknown", "[manifest_comp metadata[PackageVersionMetadata::InstalledLocale] = "zh-CN"; ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, unknown); + RequireInstaller(result, unknown, inapplicability); } } @@ -336,10 +338,10 @@ TEST_CASE("ManifestComparator_InstalledLocaleComparator", "[manifest_comparator] settings.Set({ "en-US" }); ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); // Only because it is first - RequireInstaller(result, enGB); + RequireInstaller(result, enGB, inapplicability); } SECTION("en-US Installed") { @@ -347,9 +349,9 @@ TEST_CASE("ManifestComparator_InstalledLocaleComparator", "[manifest_comparator] metadata[PackageVersionMetadata::InstalledLocale] = "en-US"; ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, enGB); + RequireInstaller(result, enGB, inapplicability); } SECTION("zh-CN Installed") { @@ -357,9 +359,10 @@ TEST_CASE("ManifestComparator_InstalledLocaleComparator", "[manifest_comparator] metadata[PackageVersionMetadata::InstalledLocale] = "zh-CN"; ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); REQUIRE(!result); + REQUIRE(WI_IsSingleFlagSetInMask(inapplicability, InapplicabilityFlags::InstalledLocale)); } } @@ -376,9 +379,9 @@ TEST_CASE("ManifestComparator_LocaleComparator", "[manifest_comparator]") context.Args.AddArg(Args::Type::Locale, "en-GB"s); ManifestComparator mc(context, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, enGB); + RequireInstaller(result, enGB, inapplicability); } SECTION("zh-CN Required") { @@ -386,9 +389,10 @@ TEST_CASE("ManifestComparator_LocaleComparator", "[manifest_comparator]") context.Args.AddArg(Args::Type::Locale, "zh-CN"s); ManifestComparator mc(context, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); REQUIRE(!result); + REQUIRE(WI_IsSingleFlagSetInMask(inapplicability, InapplicabilityFlags::Locale)); } SECTION("en-US Preference") { @@ -396,9 +400,9 @@ TEST_CASE("ManifestComparator_LocaleComparator", "[manifest_comparator]") settings.Set({ "en-US" }); ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, enGB); + RequireInstaller(result, enGB, inapplicability); } SECTION("zh-CN Preference") { @@ -406,9 +410,9 @@ TEST_CASE("ManifestComparator_LocaleComparator", "[manifest_comparator]") settings.Set({ "zh-CN" }); ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, unknown); + RequireInstaller(result, unknown, inapplicability); } } @@ -421,9 +425,10 @@ TEST_CASE("ManifestComparator_AllowedArchitecture_x64_only", "[manifest_comparat context.Add({ Architecture::X64 }); ManifestComparator mc(context, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); REQUIRE(!result); + REQUIRE(WI_IsSingleFlagSetInMask(inapplicability, InapplicabilityFlags::MachineArchitecture)); } TEST_CASE("ManifestComparator_AllowedArchitecture", "[manifest_comparator]") @@ -440,9 +445,9 @@ TEST_CASE("ManifestComparator_AllowedArchitecture", "[manifest_comparator]") context.Add({ Architecture::X86, Architecture::X64 }); ManifestComparator mc(context, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, x86); + RequireInstaller(result, x86, inapplicability); } SECTION("Unknown") { @@ -450,10 +455,11 @@ TEST_CASE("ManifestComparator_AllowedArchitecture", "[manifest_comparator]") context.Add({ Architecture::Unknown }); ManifestComparator mc(context, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); REQUIRE(result); REQUIRE(result->Arch == GetApplicableArchitectures()[0]); + REQUIRE(inapplicability == InapplicabilityFlags::None); } SECTION("x86 and Unknown") { @@ -461,8 +467,8 @@ TEST_CASE("ManifestComparator_AllowedArchitecture", "[manifest_comparator]") context.Add({ Architecture::X86, Architecture::Unknown }); ManifestComparator mc(context, {}); - auto result = mc.GetPreferredInstaller(manifest); + auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, x86); + RequireInstaller(result, x86, inapplicability); } } From b27603395aef231fa7bca2a418914ec981510862 Mon Sep 17 00:00:00 2001 From: Ruben Guerrero Samaniego Date: Wed, 27 Oct 2021 12:34:04 -0700 Subject: [PATCH 2/6] Add new message --- src/AppInstallerCLICore/Resources.h | 1 + src/AppInstallerCLICore/Workflows/UpdateFlow.cpp | 8 +++++--- src/AppInstallerCLICore/Workflows/WorkflowBase.cpp | 8 ++++++++ .../Shared/Strings/en-us/winget.resw | 5 ++++- 4 files changed, 18 insertions(+), 4 deletions(-) diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index 84d3c907ca..08d1fd46fd 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -322,6 +322,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(UninstallFlowUninstallSuccess); WINGET_DEFINE_RESOURCE_STRINGID(UnrecognizedCommand); WINGET_DEFINE_RESOURCE_STRINGID(UpdateAllArgumentDescription); + WINGET_DEFINE_RESOURCE_STRINGID(UpdateDifferentInstallTechnology); WINGET_DEFINE_RESOURCE_STRINGID(UpdateNotApplicable); WINGET_DEFINE_RESOURCE_STRINGID(UpgradeCommandLongDescription); WINGET_DEFINE_RESOURCE_STRINGID(UpgradeCommandShortDescription); diff --git a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp index 3cdd56f3f4..8777d8b46f 100644 --- a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp @@ -67,11 +67,13 @@ namespace AppInstaller::CLI::Workflow { if (m_reportUpdateNotFound) { - context.Reporter.Info() << Resource::String::UpdateNotApplicable << std::endl; - if (WI_IsSingleFlagSetInMask(inapplicabilityVersions, InapplicabilityFlags::InstalledType)) { - context.Reporter.Info() << "TODO CHANGE" << std::endl; + context.Reporter.Info() << Resource::String::UpdateDifferentInstallTechnology << std::endl; + } + else + { + context.Reporter.Info() << Resource::String::UpdateNotApplicable << std::endl; } } diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index f9bd822168..7f20fed8bf 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -918,6 +918,14 @@ namespace AppInstaller::CLI::Workflow ManifestComparator manifestComparator(context, installationMetadata); auto [installer, inapplicability] = manifestComparator.GetPreferredInstaller(context.Get()); + + if (!installer.has_value() && + WI_IsSingleFlagSetInMask(inapplicability, InapplicabilityFlags::InstalledType)) + { + context.Reporter.Info() << Resource::String::UpdateDifferentInstallTechnology << std::endl; + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE); + } + context.Add(installer); } diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index afdfabd198..9c51df9a4b 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -1193,4 +1193,7 @@ Please specify one of them using the `--source` option to proceed. The requested number of results must be between 1 and 1000. - + + At least a newer package was found but the installed technology is different from the current version installed. Please uninstall the package and install a newer version. + + \ No newline at end of file From 4dd9c884eedb42eaf7671120ccae3bd5b74625dd Mon Sep 17 00:00:00 2001 From: Ruben Guerrero Samaniego Date: Wed, 27 Oct 2021 17:58:05 -0700 Subject: [PATCH 3/6] PR comments and new error message --- src/AppInstallerCLICore/Resources.h | 3 +- .../Workflows/DependencyNodeProcessor.cpp | 4 +- .../Workflows/ManifestComparator.cpp | 88 +++++++++++++------ .../Workflows/ManifestComparator.h | 15 ++-- .../Workflows/UpdateFlow.cpp | 4 +- .../Workflows/WorkflowBase.cpp | 4 +- .../Shared/Strings/en-us/winget.resw | 7 +- .../ManifestComparator.cpp | 8 +- 8 files changed, 85 insertions(+), 48 deletions(-) diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index 7fec7d7d88..3eeeb730e0 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -334,10 +334,11 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(UninstallFlowUninstallSuccess); WINGET_DEFINE_RESOURCE_STRINGID(UnrecognizedCommand); WINGET_DEFINE_RESOURCE_STRINGID(UpdateAllArgumentDescription); - WINGET_DEFINE_RESOURCE_STRINGID(UpdateDifferentInstallTechnology); WINGET_DEFINE_RESOURCE_STRINGID(UpdateNotApplicable); WINGET_DEFINE_RESOURCE_STRINGID(UpgradeCommandLongDescription); WINGET_DEFINE_RESOURCE_STRINGID(UpgradeCommandShortDescription); + WINGET_DEFINE_RESOURCE_STRINGID(UpgradeDifferentInstallTechnology); + WINGET_DEFINE_RESOURCE_STRINGID(UpgradeDifferentInstallTechnologyInNewerVersions); WINGET_DEFINE_RESOURCE_STRINGID(Usage); WINGET_DEFINE_RESOURCE_STRINGID(ValidateCommandLongDescription); WINGET_DEFINE_RESOURCE_STRINGID(ValidateCommandReportDependencies); diff --git a/src/AppInstallerCLICore/Workflows/DependencyNodeProcessor.cpp b/src/AppInstallerCLICore/Workflows/DependencyNodeProcessor.cpp index aba098b909..b2f80e4ce5 100644 --- a/src/AppInstallerCLICore/Workflows/DependencyNodeProcessor.cpp +++ b/src/AppInstallerCLICore/Workflows/DependencyNodeProcessor.cpp @@ -72,8 +72,6 @@ namespace AppInstaller::CLI::Workflow return DependencyNodeProcessorResult::Error; } - std::optional installer; - IPackageVersion::Metadata installationMetadata; if (m_nodePackageInstalledVersion) { @@ -81,7 +79,7 @@ namespace AppInstaller::CLI::Workflow } ManifestComparator manifestComparator(m_context, installationMetadata); - installer = manifestComparator.GetPreferredInstaller(m_nodeManifest); + auto [installer, inapplicability] = manifestComparator.GetPreferredInstaller(m_nodeManifest); if (!installer.has_value()) { diff --git a/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp b/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp index 4bf0a08e90..905efbe5dd 100644 --- a/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp +++ b/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp @@ -24,11 +24,16 @@ namespace AppInstaller::CLI::Workflow { struct OSVersionFilter : public details::FilterField { - OSVersionFilter() : details::FilterField("OS Version", InapplicabilityFlags::OSVersion) {} + OSVersionFilter() : details::FilterField("OS Version") {} - bool IsApplicable(const Manifest::ManifestInstaller& installer) override + InapplicabilityFlags IsApplicable(const Manifest::ManifestInstaller& installer) override { - return installer.MinOSVersion.empty() || Runtime::IsCurrentOSVersionGreaterThanOrEqual(Utility::Version(installer.MinOSVersion)); + if (installer.MinOSVersion.empty() || Runtime::IsCurrentOSVersionGreaterThanOrEqual(Utility::Version(installer.MinOSVersion))) + { + return InapplicabilityFlags::None; + } + + return InapplicabilityFlags::OSVersion; } std::string ExplainInapplicable(const Manifest::ManifestInstaller& installer) override @@ -41,10 +46,10 @@ namespace AppInstaller::CLI::Workflow struct MachineArchitectureComparator : public details::ComparisonField { - MachineArchitectureComparator() : details::ComparisonField("Machine Architecture", InapplicabilityFlags::MachineArchitecture) {} + MachineArchitectureComparator() : details::ComparisonField("Machine Architecture") {} MachineArchitectureComparator(std::vector allowedArchitectures) : - details::ComparisonField("Machine Architecture", InapplicabilityFlags::MachineArchitecture), m_allowedArchitectures(std::move(allowedArchitectures)) + details::ComparisonField("Machine Architecture"), m_allowedArchitectures(std::move(allowedArchitectures)) { AICLI_LOG(CLI, Verbose, << "Architecture Comparator created with allowed architectures: " << GetAllowedArchitecturesString()); } @@ -97,9 +102,14 @@ namespace AppInstaller::CLI::Workflow return std::make_unique(); } - bool IsApplicable(const Manifest::ManifestInstaller& installer) override + InapplicabilityFlags IsApplicable(const Manifest::ManifestInstaller& installer) override { - return CheckAllowedArchitecture(installer.Arch) != Utility::InapplicableArchitecture; + if (CheckAllowedArchitecture(installer.Arch) == Utility::InapplicableArchitecture) + { + return InapplicabilityFlags::MachineArchitecture; + } + + return InapplicabilityFlags::None; } std::string ExplainInapplicable(const Manifest::ManifestInstaller& installer) override @@ -165,7 +175,7 @@ namespace AppInstaller::CLI::Workflow struct InstalledTypeComparator : public details::ComparisonField { InstalledTypeComparator(Manifest::InstallerTypeEnum installedType) : - details::ComparisonField("Installed Type", InapplicabilityFlags::InstalledType), m_installedType(installedType) {} + details::ComparisonField("Installed Type"), m_installedType(installedType) {} static std::unique_ptr Create(const Repository::IPackageVersion::Metadata& installationMetadata) { @@ -182,9 +192,14 @@ namespace AppInstaller::CLI::Workflow return {}; } - bool IsApplicable(const Manifest::ManifestInstaller& installer) override + InapplicabilityFlags IsApplicable(const Manifest::ManifestInstaller& installer) override { - return Manifest::IsInstallerTypeCompatible(installer.InstallerType, m_installedType); + if (Manifest::IsInstallerTypeCompatible(installer.InstallerType, m_installedType)) + { + return InapplicabilityFlags::None; + } + + return InapplicabilityFlags::InstalledType; } std::string ExplainInapplicable(const Manifest::ManifestInstaller& installer) override @@ -206,7 +221,7 @@ namespace AppInstaller::CLI::Workflow struct InstalledScopeFilter : public details::FilterField { InstalledScopeFilter(Manifest::ScopeEnum requirement) : - details::FilterField("Installed Scope", InapplicabilityFlags::InstalledScope), m_requirement(requirement) {} + details::FilterField("Installed Scope"), m_requirement(requirement) {} static std::unique_ptr Create(const Repository::IPackageVersion::Metadata& installationMetadata) { @@ -224,10 +239,15 @@ namespace AppInstaller::CLI::Workflow return {}; } - bool IsApplicable(const Manifest::ManifestInstaller& installer) override + InapplicabilityFlags IsApplicable(const Manifest::ManifestInstaller& installer) override { // We have to assume the unknown scope will match our required scope, or the entire catalog would stop working for upgrade. - return installer.Scope == Manifest::ScopeEnum::Unknown || installer.Scope == m_requirement; + if (installer.Scope == Manifest::ScopeEnum::Unknown || installer.Scope == m_requirement) + { + return InapplicabilityFlags::None; + } + + return InapplicabilityFlags::InstalledScope; } std::string ExplainInapplicable(const Manifest::ManifestInstaller& installer) override @@ -246,7 +266,7 @@ namespace AppInstaller::CLI::Workflow struct ScopeComparator : public details::ComparisonField { ScopeComparator(Manifest::ScopeEnum preference, Manifest::ScopeEnum requirement) : - details::ComparisonField("Scope", InapplicabilityFlags::Scope), m_preference(preference), m_requirement(requirement) {} + details::ComparisonField("Scope"), m_preference(preference), m_requirement(requirement) {} static std::unique_ptr Create(const Execution::Args& args) { @@ -275,9 +295,14 @@ namespace AppInstaller::CLI::Workflow } } - bool IsApplicable(const Manifest::ManifestInstaller& installer) override + InapplicabilityFlags IsApplicable(const Manifest::ManifestInstaller& installer) override { - return m_requirement == Manifest::ScopeEnum::Unknown || installer.Scope == m_requirement; + if (m_requirement == Manifest::ScopeEnum::Unknown || installer.Scope == m_requirement) + { + return InapplicabilityFlags::None; + } + + return InapplicabilityFlags::Scope; } std::string ExplainInapplicable(const Manifest::ManifestInstaller& installer) override @@ -314,7 +339,7 @@ namespace AppInstaller::CLI::Workflow struct InstalledLocaleComparator : public details::ComparisonField { InstalledLocaleComparator(std::string installedLocale) : - details::ComparisonField("Installed Locale", InapplicabilityFlags::InstalledLocale), m_installedLocale(std::move(installedLocale)) {} + details::ComparisonField("Installed Locale"), m_installedLocale(std::move(installedLocale)) {} static std::unique_ptr Create(const Repository::IPackageVersion::Metadata& installationMetadata) { @@ -328,10 +353,16 @@ namespace AppInstaller::CLI::Workflow return {}; } - bool IsApplicable(const Manifest::ManifestInstaller& installer) override + InapplicabilityFlags IsApplicable(const Manifest::ManifestInstaller& installer) override { // We have to assume an unknown installer locale will match our installed locale, or the entire catalog would stop working for upgrade. - return installer.Locale.empty() || Locale::GetDistanceOfLanguage(m_installedLocale, installer.Locale) >= Locale::MinimumDistanceScoreAsCompatibleMatch; + if (installer.Locale.empty() || + Locale::GetDistanceOfLanguage(m_installedLocale, installer.Locale) >= Locale::MinimumDistanceScoreAsCompatibleMatch) + { + return InapplicabilityFlags::None; + } + + return InapplicabilityFlags::InstalledLocale; } std::string ExplainInapplicable(const Manifest::ManifestInstaller& installer) override @@ -358,7 +389,7 @@ namespace AppInstaller::CLI::Workflow struct LocaleComparator : public details::ComparisonField { LocaleComparator(std::vector preference, std::vector requirement) : - details::ComparisonField("Locale", InapplicabilityFlags::Locale), m_preference(std::move(preference)), m_requirement(std::move(requirement)) + details::ComparisonField("Locale"), m_preference(std::move(preference)), m_requirement(std::move(requirement)) { m_requirementAsString = GetLocalesListAsString(m_requirement); m_preferenceAsString = GetLocalesListAsString(m_preference); @@ -397,22 +428,22 @@ namespace AppInstaller::CLI::Workflow } } - bool IsApplicable(const Manifest::ManifestInstaller& installer) override + InapplicabilityFlags IsApplicable(const Manifest::ManifestInstaller& installer) override { if (m_requirement.empty()) { - return true; + return InapplicabilityFlags::None; } for (auto const& requiredLocale : m_requirement) { if (Locale::GetDistanceOfLanguage(requiredLocale, installer.Locale) >= Locale::MinimumDistanceScoreAsPerfectMatch) { - return true; + return InapplicabilityFlags::None; } } - return false; + return InapplicabilityFlags::Locale; } std::string ExplainInapplicable(const Manifest::ManifestInstaller& installer) override @@ -502,7 +533,7 @@ namespace AppInstaller::CLI::Workflow AddComparator(MachineArchitectureComparator::Create(context, installationMetadata)); } - std::tuple, InapplicabilityFlags> ManifestComparator::GetPreferredInstaller(const Manifest::Manifest& manifest) + InstallerAndInapplicability ManifestComparator::GetPreferredInstaller(const Manifest::Manifest& manifest) { AICLI_LOG(CLI, Info, << "Starting installer selection."); @@ -517,7 +548,7 @@ namespace AppInstaller::CLI::Workflow if (!result || IsFirstBetter(installer, *result)) { AICLI_LOG(CLI, Verbose, << "Installer " << installer << " is current best choice"); - result = &installer; + result = &installer; } } else @@ -545,10 +576,11 @@ namespace AppInstaller::CLI::Workflow { for (const auto& filter : m_filters) { - if (!filter->IsApplicable(installer)) + auto inapplicability = filter->IsApplicable(installer); + if (inapplicability != InapplicabilityFlags::None) { AICLI_LOG(CLI, Info, << "Installer " << installer << " not applicable: " << filter->ExplainInapplicable(installer)); - return filter->InapplicableReason(); + return inapplicability; } } diff --git a/src/AppInstallerCLICore/Workflows/ManifestComparator.h b/src/AppInstallerCLICore/Workflows/ManifestComparator.h index 25c0cf8377..0024e53a2e 100644 --- a/src/AppInstallerCLICore/Workflows/ManifestComparator.h +++ b/src/AppInstallerCLICore/Workflows/ManifestComparator.h @@ -33,16 +33,14 @@ namespace AppInstaller::CLI::Workflow // An interface for defining new filters based on user inputs. struct FilterField { - FilterField(std::string_view name, InapplicabilityFlags installerApplicability) : - m_name(name), m_installerApplicability(installerApplicability) {} + FilterField(std::string_view name) : m_name(name) {} virtual ~FilterField() = default; std::string_view Name() const { return m_name; } - InapplicabilityFlags InapplicableReason() const { return m_installerApplicability; } // Determines if the installer is applicable based on this field alone. - virtual bool IsApplicable(const Manifest::ManifestInstaller& installer) = 0; + virtual InapplicabilityFlags IsApplicable(const Manifest::ManifestInstaller& installer) = 0; // Explains why the filter regarded this installer as inapplicable. // Will only be called when IsApplicable returns false. @@ -50,7 +48,6 @@ namespace AppInstaller::CLI::Workflow private: std::string_view m_name; - InapplicabilityFlags m_installerApplicability; }; // An interface for defining new comparisons based on user inputs. @@ -65,13 +62,19 @@ namespace AppInstaller::CLI::Workflow }; } + struct InstallerAndInapplicability + { + std::optional installer; + InapplicabilityFlags inapplicabilityFlags; + }; + // Class in charge of comparing manifest entries struct ManifestComparator { ManifestComparator(const Execution::Context& context, const Repository::IPackageVersion::Metadata& installationMetadata); // Gets the best installer from the manifest, if at least one is applicable. - std::tuple, InapplicabilityFlags> GetPreferredInstaller(const Manifest::Manifest& manifest); + InstallerAndInapplicability GetPreferredInstaller(const Manifest::Manifest& manifest); // Determines if an installer is applicable. InapplicabilityFlags IsApplicable(const Manifest::ManifestInstaller& installer); diff --git a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp index 8777d8b46f..3667da6ea5 100644 --- a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp @@ -67,9 +67,9 @@ namespace AppInstaller::CLI::Workflow { if (m_reportUpdateNotFound) { - if (WI_IsSingleFlagSetInMask(inapplicabilityVersions, InapplicabilityFlags::InstalledType)) + if (inapplicabilityVersions == InapplicabilityFlags::InstalledType) { - context.Reporter.Info() << Resource::String::UpdateDifferentInstallTechnology << std::endl; + context.Reporter.Info() << Resource::String::UpgradeDifferentInstallTechnologyInNewerVersions << std::endl; } else { diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index 93048f8754..139b22cbf0 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -971,9 +971,9 @@ namespace AppInstaller::CLI::Workflow auto [installer, inapplicability] = manifestComparator.GetPreferredInstaller(context.Get()); if (!installer.has_value() && - WI_IsSingleFlagSetInMask(inapplicability, InapplicabilityFlags::InstalledType)) + (inapplicability == InapplicabilityFlags::InstalledType)) { - context.Reporter.Info() << Resource::String::UpdateDifferentInstallTechnology << std::endl; + context.Reporter.Info() << Resource::String::UpgradeDifferentInstallTechnology << std::endl; AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE); } diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index fa926322a8..d268d4a89d 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -1236,7 +1236,10 @@ Please specify one of them using the `--source` option to proceed. The requested number of results must be between 1 and 1000. - - At least a newer package was found but the installed technology is different from the current version installed. Please uninstall the package and install a newer version. + + At least a newer package was found but the install technology is different from the current version installed. Please uninstall the package and install a newer version. + + + The install technology is different from the current version installed. Please uninstall the package and the newer version. \ No newline at end of file diff --git a/src/AppInstallerCLITests/ManifestComparator.cpp b/src/AppInstallerCLITests/ManifestComparator.cpp index 9334fe368f..79112ffee2 100644 --- a/src/AppInstallerCLITests/ManifestComparator.cpp +++ b/src/AppInstallerCLITests/ManifestComparator.cpp @@ -58,7 +58,7 @@ TEST_CASE("ManifestComparator_OSFilter_Low", "[manifest_comparator]") auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); REQUIRE(!result); - REQUIRE(WI_IsSingleFlagSetInMask(inapplicability, InapplicabilityFlags::OSVersion)); + REQUIRE(inapplicability == InapplicabilityFlags::OSVersion); } TEST_CASE("ManifestComparator_OSFilter_High", "[manifest_comparator]") @@ -362,7 +362,7 @@ TEST_CASE("ManifestComparator_InstalledLocaleComparator", "[manifest_comparator] auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); REQUIRE(!result); - REQUIRE(WI_IsSingleFlagSetInMask(inapplicability, InapplicabilityFlags::InstalledLocale)); + REQUIRE(inapplicability == InapplicabilityFlags::InstalledLocale); } } @@ -392,7 +392,7 @@ TEST_CASE("ManifestComparator_LocaleComparator", "[manifest_comparator]") auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); REQUIRE(!result); - REQUIRE(WI_IsSingleFlagSetInMask(inapplicability, InapplicabilityFlags::Locale)); + REQUIRE(inapplicability == InapplicabilityFlags::Locale); } SECTION("en-US Preference") { @@ -428,7 +428,7 @@ TEST_CASE("ManifestComparator_AllowedArchitecture_x64_only", "[manifest_comparat auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); REQUIRE(!result); - REQUIRE(WI_IsSingleFlagSetInMask(inapplicability, InapplicabilityFlags::MachineArchitecture)); + REQUIRE(inapplicability == InapplicabilityFlags::MachineArchitecture); } TEST_CASE("ManifestComparator_AllowedArchitecture", "[manifest_comparator]") From 90aa101b30834f7d025552dcd93121d5fb37de42 Mon Sep 17 00:00:00 2001 From: Ruben Guerrero Samaniego Date: Thu, 28 Oct 2021 14:25:38 -0700 Subject: [PATCH 4/6] Inapplicabilities per installer --- .../Workflows/DependencyNodeProcessor.cpp | 2 +- .../Workflows/ManifestComparator.cpp | 16 +- .../Workflows/ManifestComparator.h | 6 +- .../Workflows/UpdateFlow.cpp | 14 +- .../Workflows/WorkflowBase.cpp | 13 +- .../Shared/Strings/en-us/winget.resw | 4 +- .../ManifestComparator.cpp | 200 ++++++++++++------ src/AppInstallerCLITests/WorkFlow.cpp | 22 +- 8 files changed, 184 insertions(+), 93 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/DependencyNodeProcessor.cpp b/src/AppInstallerCLICore/Workflows/DependencyNodeProcessor.cpp index b2f80e4ce5..d13300aee0 100644 --- a/src/AppInstallerCLICore/Workflows/DependencyNodeProcessor.cpp +++ b/src/AppInstallerCLICore/Workflows/DependencyNodeProcessor.cpp @@ -79,7 +79,7 @@ namespace AppInstaller::CLI::Workflow } ManifestComparator manifestComparator(m_context, installationMetadata); - auto [installer, inapplicability] = manifestComparator.GetPreferredInstaller(m_nodeManifest); + auto [installer, inapplicabilities] = manifestComparator.GetPreferredInstaller(m_nodeManifest); if (!installer.has_value()) { diff --git a/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp b/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp index 905efbe5dd..213599b9f8 100644 --- a/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp +++ b/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp @@ -533,12 +533,12 @@ namespace AppInstaller::CLI::Workflow AddComparator(MachineArchitectureComparator::Create(context, installationMetadata)); } - InstallerAndInapplicability ManifestComparator::GetPreferredInstaller(const Manifest::Manifest& manifest) + InstallerAndInapplicabilities ManifestComparator::GetPreferredInstaller(const Manifest::Manifest& manifest) { AICLI_LOG(CLI, Info, << "Starting installer selection."); const Manifest::ManifestInstaller* result = nullptr; - InapplicabilityFlags inapplicabilityInstallers = InapplicabilityFlags::None; + std::vector inapplicabilitiesInstallers; for (const auto& installer : manifest.Installers) { @@ -553,13 +553,13 @@ namespace AppInstaller::CLI::Workflow } else { - WI_SetAllFlags(inapplicabilityInstallers, inapplicabilityInstaller); + inapplicabilitiesInstallers.push_back(inapplicabilityInstaller); } } if (!result) { - return { {}, inapplicabilityInstallers }; + return { {}, std::move(inapplicabilitiesInstallers) }; } Logging::Telemetry().LogSelectedInstaller( @@ -569,22 +569,24 @@ namespace AppInstaller::CLI::Workflow Manifest::ScopeToString(result->Scope), result->Locale); - return { *result, InapplicabilityFlags::None }; + return { *result, std::move(inapplicabilitiesInstallers) }; } InapplicabilityFlags ManifestComparator::IsApplicable(const Manifest::ManifestInstaller& installer) { + InapplicabilityFlags inapplicabilityResult = InapplicabilityFlags::None; + for (const auto& filter : m_filters) { auto inapplicability = filter->IsApplicable(installer); if (inapplicability != InapplicabilityFlags::None) { AICLI_LOG(CLI, Info, << "Installer " << installer << " not applicable: " << filter->ExplainInapplicable(installer)); - return inapplicability; + WI_SetAllFlags(inapplicabilityResult, inapplicability); } } - return InapplicabilityFlags::None; + return inapplicabilityResult; } bool ManifestComparator::IsFirstBetter( diff --git a/src/AppInstallerCLICore/Workflows/ManifestComparator.h b/src/AppInstallerCLICore/Workflows/ManifestComparator.h index 0024e53a2e..d08d5c988b 100644 --- a/src/AppInstallerCLICore/Workflows/ManifestComparator.h +++ b/src/AppInstallerCLICore/Workflows/ManifestComparator.h @@ -62,10 +62,10 @@ namespace AppInstaller::CLI::Workflow }; } - struct InstallerAndInapplicability + struct InstallerAndInapplicabilities { std::optional installer; - InapplicabilityFlags inapplicabilityFlags; + std::vector inapplicabilitiesInstaller; }; // Class in charge of comparing manifest entries @@ -74,7 +74,7 @@ namespace AppInstaller::CLI::Workflow ManifestComparator(const Execution::Context& context, const Repository::IPackageVersion::Metadata& installationMetadata); // Gets the best installer from the manifest, if at least one is applicable. - InstallerAndInapplicability GetPreferredInstaller(const Manifest::Manifest& manifest); + InstallerAndInapplicabilities GetPreferredInstaller(const Manifest::Manifest& manifest); // Determines if an installer is applicable. InapplicabilityFlags IsApplicable(const Manifest::ManifestInstaller& installer); diff --git a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp index 3667da6ea5..54f985b435 100644 --- a/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/UpdateFlow.cpp @@ -27,7 +27,7 @@ namespace AppInstaller::CLI::Workflow Utility::Version installedVersion = Utility::Version(installedPackage->GetProperty(PackageVersionProperty::Version)); ManifestComparator manifestComparator(context, installedPackage->GetMetadata()); bool updateFound = false; - InapplicabilityFlags inapplicabilityVersions = InapplicabilityFlags::None; + bool installedTypeInapplicable = false; // The version keys should have already been sorted by version const auto& versionKeys = package->GetAvailableVersionKeys(); @@ -40,10 +40,16 @@ namespace AppInstaller::CLI::Workflow auto manifest = packageVersion->GetManifest(); // Check applicable Installer - auto [installer, inapplicability] = manifestComparator.GetPreferredInstaller(manifest); + auto [installer, inapplicabilities] = manifestComparator.GetPreferredInstaller(manifest); if (!installer.has_value()) { - WI_SetAllFlags(inapplicabilityVersions, inapplicability); + // 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; } @@ -67,7 +73,7 @@ namespace AppInstaller::CLI::Workflow { if (m_reportUpdateNotFound) { - if (inapplicabilityVersions == InapplicabilityFlags::InstalledType) + if (installedTypeInapplicable) { context.Reporter.Info() << Resource::String::UpgradeDifferentInstallTechnologyInNewerVersions << std::endl; } diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index 139b22cbf0..c43aef9374 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -968,13 +968,16 @@ namespace AppInstaller::CLI::Workflow } ManifestComparator manifestComparator(context, installationMetadata); - auto [installer, inapplicability] = manifestComparator.GetPreferredInstaller(context.Get()); + auto [installer, inapplicabilities] = manifestComparator.GetPreferredInstaller(context.Get()); - if (!installer.has_value() && - (inapplicability == InapplicabilityFlags::InstalledType)) + if (!installer.has_value()) { - context.Reporter.Info() << Resource::String::UpgradeDifferentInstallTechnology << std::endl; - AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE); + auto onlyInstalledType = std::find(inapplicabilities.begin(), inapplicabilities.end(), InapplicabilityFlags::InstalledType); + if (onlyInstalledType != inapplicabilities.end()) + { + context.Reporter.Info() << Resource::String::UpgradeDifferentInstallTechnology << std::endl; + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE); + } } context.Add(installer); diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index d268d4a89d..3ce8b1ec0b 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -1237,9 +1237,9 @@ Please specify one of them using the `--source` option to proceed. The requested number of results must be between 1 and 1000. - At least a newer package was found but the install technology is different from the current version installed. Please uninstall the package and install a newer version. + A newer version was found, but the install technology is different from the current version installed. Please uninstall the package and install the newer version. - The install technology is different from the current version installed. Please uninstall the package and the newer version. + The install technology of the newer version specified is different from the current version installed. Please uninstall the package and install the newer version. \ No newline at end of file diff --git a/src/AppInstallerCLITests/ManifestComparator.cpp b/src/AppInstallerCLITests/ManifestComparator.cpp index 79112ffee2..80372daaf6 100644 --- a/src/AppInstallerCLITests/ManifestComparator.cpp +++ b/src/AppInstallerCLITests/ManifestComparator.cpp @@ -38,7 +38,7 @@ const ManifestInstaller& AddInstaller(Manifest& manifest, Architecture architect return manifest.Installers.back(); } -void RequireInstaller(const std::optional& actual, const ManifestInstaller& expected, InapplicabilityFlags inapplicability) +void RequireInstaller(const std::optional& actual, const ManifestInstaller& expected) { REQUIRE(actual); REQUIRE(actual->Arch == expected.Arch); @@ -46,7 +46,16 @@ void RequireInstaller(const std::optional& actual, const Mani REQUIRE(actual->Scope == expected.Scope); REQUIRE(actual->MinOSVersion == expected.MinOSVersion); REQUIRE(actual->Locale == expected.Locale); - REQUIRE(inapplicability == InapplicabilityFlags::None); +} + +void RequireInapplicabilities(const std::vector& inapplicabilities, const std::vector& expected) +{ + REQUIRE(inapplicabilities.size() == expected.size()); + + for (int i = 0; i < inapplicabilities.size(); i++) + { + REQUIRE(inapplicabilities[i] == expected[i]); + } } TEST_CASE("ManifestComparator_OSFilter_Low", "[manifest_comparator]") @@ -55,10 +64,10 @@ TEST_CASE("ManifestComparator_OSFilter_Low", "[manifest_comparator]") AddInstaller(manifest, Architecture::Neutral, InstallerTypeEnum::Exe, ScopeEnum::Unknown, "10.0.99999.0"); ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); REQUIRE(!result); - REQUIRE(inapplicability == InapplicabilityFlags::OSVersion); + RequireInapplicabilities(inapplicabilities, { InapplicabilityFlags::OSVersion }); } TEST_CASE("ManifestComparator_OSFilter_High", "[manifest_comparator]") @@ -67,9 +76,10 @@ TEST_CASE("ManifestComparator_OSFilter_High", "[manifest_comparator]") ManifestInstaller expected = AddInstaller(manifest, Architecture::Neutral, InstallerTypeEnum::Exe, ScopeEnum::Unknown, "10.0.0.0"); ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, expected, inapplicability); + RequireInstaller(result, expected); + REQUIRE(inapplicabilities.size() == 0); } TEST_CASE("ManifestComparator_InstalledScopeFilter_Uknown", "[manifest_comparator]") @@ -80,10 +90,11 @@ TEST_CASE("ManifestComparator_InstalledScopeFilter_Uknown", "[manifest_comparato SECTION("Nothing Installed") { ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); // Only because it is first - RequireInstaller(result, unknown, inapplicability); + RequireInstaller(result, unknown); + REQUIRE(inapplicabilities.size() == 0); } SECTION("User Installed") { @@ -91,9 +102,10 @@ TEST_CASE("ManifestComparator_InstalledScopeFilter_Uknown", "[manifest_comparato metadata[PackageVersionMetadata::InstalledScope] = ScopeToString(ScopeEnum::User); ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, unknown, inapplicability); + RequireInstaller(result, unknown); + REQUIRE(inapplicabilities.size() == 0); } SECTION("Machine Installed") { @@ -101,9 +113,10 @@ TEST_CASE("ManifestComparator_InstalledScopeFilter_Uknown", "[manifest_comparato metadata[PackageVersionMetadata::InstalledScope] = ScopeToString(ScopeEnum::Machine); ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, unknown, inapplicability); + RequireInstaller(result, unknown); + REQUIRE(inapplicabilities.size() == 0); } } @@ -116,10 +129,11 @@ TEST_CASE("ManifestComparator_InstalledScopeFilter", "[manifest_comparator]") SECTION("Nothing Installed") { ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); // Only because it is first - RequireInstaller(result, user, inapplicability); + RequireInstaller(result, user); + REQUIRE(inapplicabilities.size() == 0); } SECTION("User Installed") { @@ -127,9 +141,10 @@ TEST_CASE("ManifestComparator_InstalledScopeFilter", "[manifest_comparator]") metadata[PackageVersionMetadata::InstalledScope] = ScopeToString(ScopeEnum::User); ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, user, inapplicability); + RequireInstaller(result, user); + RequireInapplicabilities(inapplicabilities, { InapplicabilityFlags::InstalledScope }); } SECTION("Machine Installed") { @@ -137,9 +152,10 @@ TEST_CASE("ManifestComparator_InstalledScopeFilter", "[manifest_comparator]") metadata[PackageVersionMetadata::InstalledScope] = ScopeToString(ScopeEnum::Machine); ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, machine, inapplicability); + RequireInstaller(result, machine); + RequireInapplicabilities(inapplicabilities, { InapplicabilityFlags::InstalledScope }); } } @@ -152,10 +168,11 @@ TEST_CASE("ManifestComparator_InstalledTypeFilter", "[manifest_comparator]") SECTION("Nothing Installed") { ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); // Only because it is first - RequireInstaller(result, msi, inapplicability); + RequireInstaller(result, msi); + REQUIRE(inapplicabilities.size() == 0); } SECTION("MSI Installed") { @@ -163,9 +180,10 @@ TEST_CASE("ManifestComparator_InstalledTypeFilter", "[manifest_comparator]") metadata[PackageVersionMetadata::InstalledType] = InstallerTypeToString(InstallerTypeEnum::Msi); ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, msi, inapplicability); + RequireInstaller(result, msi); + RequireInapplicabilities(inapplicabilities, { InapplicabilityFlags::InstalledType }); } SECTION("MSIX Installed") { @@ -173,9 +191,10 @@ TEST_CASE("ManifestComparator_InstalledTypeFilter", "[manifest_comparator]") metadata[PackageVersionMetadata::InstalledType] = InstallerTypeToString(InstallerTypeEnum::Msix); ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, msix, inapplicability); + RequireInstaller(result, msix); + RequireInapplicabilities(inapplicabilities, { InapplicabilityFlags::InstalledType }); } } @@ -188,10 +207,11 @@ TEST_CASE("ManifestComparator_InstalledTypeCompare", "[manifest_comparator]") SECTION("Nothing Installed") { ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); // Only because it is first - RequireInstaller(result, burn, inapplicability); + RequireInstaller(result, burn); + REQUIRE(inapplicabilities.size() == 0); } SECTION("Exe Installed") { @@ -199,9 +219,10 @@ TEST_CASE("ManifestComparator_InstalledTypeCompare", "[manifest_comparator]") metadata[PackageVersionMetadata::InstalledType] = InstallerTypeToString(InstallerTypeEnum::Exe); ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, exe, inapplicability); + RequireInstaller(result, exe); + REQUIRE(inapplicabilities.size() == 0); } SECTION("Inno Installed") { @@ -209,9 +230,10 @@ TEST_CASE("ManifestComparator_InstalledTypeCompare", "[manifest_comparator]") metadata[PackageVersionMetadata::InstalledType] = InstallerTypeToString(InstallerTypeEnum::Inno); ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, burn, inapplicability); + RequireInstaller(result, burn); + REQUIRE(inapplicabilities.size() == 0); } } @@ -224,10 +246,11 @@ TEST_CASE("ManifestComparator_ScopeFilter", "[manifest_comparator]") SECTION("Nothing Required") { ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); // Only because it is first - RequireInstaller(result, user, inapplicability); + RequireInstaller(result, user); + REQUIRE(inapplicabilities.size() == 0); } SECTION("User Required") { @@ -235,9 +258,10 @@ TEST_CASE("ManifestComparator_ScopeFilter", "[manifest_comparator]") context.Args.AddArg(Args::Type::InstallScope, ScopeToString(ScopeEnum::User)); ManifestComparator mc(context, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, user, inapplicability); + RequireInstaller(result, user); + RequireInapplicabilities(inapplicabilities, { InapplicabilityFlags::Scope }); } SECTION("Machine Required") { @@ -245,9 +269,10 @@ TEST_CASE("ManifestComparator_ScopeFilter", "[manifest_comparator]") context.Args.AddArg(Args::Type::InstallScope, ScopeToString(ScopeEnum::Machine)); ManifestComparator mc(context, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, machine, inapplicability); + RequireInstaller(result, machine); + RequireInapplicabilities(inapplicabilities, { InapplicabilityFlags::Scope }); } } @@ -260,10 +285,11 @@ TEST_CASE("ManifestComparator_ScopeCompare", "[manifest_comparator]") SECTION("No Preference") { ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); // The default preference is user - RequireInstaller(result, user, inapplicability); + RequireInstaller(result, user); + REQUIRE(inapplicabilities.size() == 0); } SECTION("User Preference") { @@ -271,9 +297,10 @@ TEST_CASE("ManifestComparator_ScopeCompare", "[manifest_comparator]") settings.Set(ScopePreference::User); ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, user, inapplicability); + RequireInstaller(result, user); + REQUIRE(inapplicabilities.size() == 0); } SECTION("Machine Preference") { @@ -281,9 +308,10 @@ TEST_CASE("ManifestComparator_ScopeCompare", "[manifest_comparator]") settings.Set(ScopePreference::Machine); ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, machine, inapplicability); + RequireInstaller(result, machine); + REQUIRE(inapplicabilities.size() == 0); } } @@ -299,10 +327,11 @@ TEST_CASE("ManifestComparator_InstalledLocaleComparator_Uknown", "[manifest_comp settings.Set({ "en-US" }); ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); // Only because it is first - RequireInstaller(result, enGB, inapplicability); + RequireInstaller(result, enGB); + REQUIRE(inapplicabilities.size() == 0); } SECTION("en-US Installed") { @@ -310,9 +339,10 @@ TEST_CASE("ManifestComparator_InstalledLocaleComparator_Uknown", "[manifest_comp metadata[PackageVersionMetadata::InstalledLocale] = "en-US"; ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, enGB, inapplicability); + RequireInstaller(result, enGB); + REQUIRE(inapplicabilities.size() == 0); } SECTION("zh-CN Installed") { @@ -320,9 +350,10 @@ TEST_CASE("ManifestComparator_InstalledLocaleComparator_Uknown", "[manifest_comp metadata[PackageVersionMetadata::InstalledLocale] = "zh-CN"; ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, unknown, inapplicability); + RequireInstaller(result, unknown); + RequireInapplicabilities(inapplicabilities, { InapplicabilityFlags::InstalledLocale }); } } @@ -338,10 +369,11 @@ TEST_CASE("ManifestComparator_InstalledLocaleComparator", "[manifest_comparator] settings.Set({ "en-US" }); ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); // Only because it is first - RequireInstaller(result, enGB, inapplicability); + RequireInstaller(result, enGB); + REQUIRE(inapplicabilities.size() == 0); } SECTION("en-US Installed") { @@ -349,9 +381,10 @@ TEST_CASE("ManifestComparator_InstalledLocaleComparator", "[manifest_comparator] metadata[PackageVersionMetadata::InstalledLocale] = "en-US"; ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, enGB, inapplicability); + RequireInstaller(result, enGB); + RequireInapplicabilities(inapplicabilities, { InapplicabilityFlags::InstalledLocale }); } SECTION("zh-CN Installed") { @@ -359,10 +392,10 @@ TEST_CASE("ManifestComparator_InstalledLocaleComparator", "[manifest_comparator] metadata[PackageVersionMetadata::InstalledLocale] = "zh-CN"; ManifestComparator mc(ManifestComparatorTestContext{}, metadata); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); REQUIRE(!result); - REQUIRE(inapplicability == InapplicabilityFlags::InstalledLocale); + RequireInapplicabilities(inapplicabilities, { InapplicabilityFlags::InstalledLocale, InapplicabilityFlags::InstalledLocale }); } } @@ -379,9 +412,10 @@ TEST_CASE("ManifestComparator_LocaleComparator", "[manifest_comparator]") context.Args.AddArg(Args::Type::Locale, "en-GB"s); ManifestComparator mc(context, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, enGB, inapplicability); + RequireInstaller(result, enGB); + RequireInapplicabilities(inapplicabilities, { InapplicabilityFlags::Locale , InapplicabilityFlags::Locale }); } SECTION("zh-CN Required") { @@ -389,10 +423,10 @@ TEST_CASE("ManifestComparator_LocaleComparator", "[manifest_comparator]") context.Args.AddArg(Args::Type::Locale, "zh-CN"s); ManifestComparator mc(context, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); REQUIRE(!result); - REQUIRE(inapplicability == InapplicabilityFlags::Locale); + RequireInapplicabilities(inapplicabilities, { InapplicabilityFlags::Locale , InapplicabilityFlags::Locale, InapplicabilityFlags::Locale }); } SECTION("en-US Preference") { @@ -400,9 +434,10 @@ TEST_CASE("ManifestComparator_LocaleComparator", "[manifest_comparator]") settings.Set({ "en-US" }); ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, enGB, inapplicability); + RequireInstaller(result, enGB); + REQUIRE(inapplicabilities.size() == 0); } SECTION("zh-CN Preference") { @@ -410,9 +445,10 @@ TEST_CASE("ManifestComparator_LocaleComparator", "[manifest_comparator]") settings.Set({ "zh-CN" }); ManifestComparator mc(ManifestComparatorTestContext{}, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, unknown, inapplicability); + RequireInstaller(result, unknown); + REQUIRE(inapplicabilities.size() == 0); } } @@ -425,10 +461,10 @@ TEST_CASE("ManifestComparator_AllowedArchitecture_x64_only", "[manifest_comparat context.Add({ Architecture::X64 }); ManifestComparator mc(context, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); REQUIRE(!result); - REQUIRE(inapplicability == InapplicabilityFlags::MachineArchitecture); + RequireInapplicabilities(inapplicabilities, { InapplicabilityFlags::MachineArchitecture }); } TEST_CASE("ManifestComparator_AllowedArchitecture", "[manifest_comparator]") @@ -445,9 +481,10 @@ TEST_CASE("ManifestComparator_AllowedArchitecture", "[manifest_comparator]") context.Add({ Architecture::X86, Architecture::X64 }); ManifestComparator mc(context, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, x86, inapplicability); + RequireInstaller(result, x86); + RequireInapplicabilities(inapplicabilities, { InapplicabilityFlags::MachineArchitecture, InapplicabilityFlags::MachineArchitecture }); } SECTION("Unknown") { @@ -455,11 +492,11 @@ TEST_CASE("ManifestComparator_AllowedArchitecture", "[manifest_comparator]") context.Add({ Architecture::Unknown }); ManifestComparator mc(context, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); REQUIRE(result); REQUIRE(result->Arch == GetApplicableArchitectures()[0]); - REQUIRE(inapplicability == InapplicabilityFlags::None); + RequireInapplicabilities(inapplicabilities, { InapplicabilityFlags::MachineArchitecture, InapplicabilityFlags::MachineArchitecture }); } SECTION("x86 and Unknown") { @@ -467,8 +504,31 @@ TEST_CASE("ManifestComparator_AllowedArchitecture", "[manifest_comparator]") context.Add({ Architecture::X86, Architecture::Unknown }); ManifestComparator mc(context, {}); - auto [result, inapplicability] = mc.GetPreferredInstaller(manifest); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); - RequireInstaller(result, x86, inapplicability); + RequireInstaller(result, x86); + RequireInapplicabilities(inapplicabilities, { InapplicabilityFlags::MachineArchitecture, InapplicabilityFlags::MachineArchitecture }); } } + +TEST_CASE("ManifestComparator_Inapplicabilities", "[manifest_comparator]") +{ + Manifest manifest; + ManifestInstaller installer = AddInstaller(manifest, Architecture::Arm64, InstallerTypeEnum::Exe, ScopeEnum::User, "10.0.99999.0", "es-MX"); + + ManifestComparatorTestContext context; + context.Add({ Architecture::X86 }); + context.Args.AddArg(Args::Type::InstallScope, ScopeToString(ScopeEnum::Machine)); + context.Args.AddArg(Args::Type::Locale, "en-GB"s); + + IPackageVersion::Metadata metadata; + metadata[PackageVersionMetadata::InstalledType] = InstallerTypeToString(InstallerTypeEnum::Msix); + + ManifestComparator mc(context, metadata); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); + + REQUIRE(!result); + RequireInapplicabilities( + inapplicabilities, + { InapplicabilityFlags::OSVersion | InapplicabilityFlags::InstalledType | InapplicabilityFlags::Locale | InapplicabilityFlags::Scope | InapplicabilityFlags::MachineArchitecture }); +} diff --git a/src/AppInstallerCLITests/WorkFlow.cpp b/src/AppInstallerCLITests/WorkFlow.cpp index c3cbb72d35..a7191bbc34 100644 --- a/src/AppInstallerCLITests/WorkFlow.cpp +++ b/src/AppInstallerCLITests/WorkFlow.cpp @@ -1393,7 +1393,27 @@ TEST_CASE("UpdateFlow_UpdateExeInstallerTypeNotApplicable", "[UpdateFlow][workfl // Verify Installer is not called. REQUIRE(!std::filesystem::exists(updateResultPath.GetPath())); - REQUIRE(updateOutput.str().find(Resource::LocString(Resource::String::UpdateNotApplicable).get()) != std::string::npos); + REQUIRE(updateOutput.str().find(Resource::LocString(Resource::String::UpgradeDifferentInstallTechnologyInNewerVersions).get()) != std::string::npos); + REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE); +} + +TEST_CASE("UpdateFlow_UpdateExeInstallerTypeNotApplicableSpecificVersion", "[UpdateFlow][workflow]") +{ + TestCommon::TempFile updateResultPath("TestExeInstalled.txt"); + + std::ostringstream updateOutput; + TestContext context{ updateOutput, std::cin }; + OverrideForCompositeInstalledSource(context); + context.Args.AddArg(Execution::Args::Type::Query, "TestExeInstallerWithIncompatibleInstallerType"sv); + context.Args.AddArg(Execution::Args::Type::Version, "2.0.0.0"sv); + + UpgradeCommand update({}); + update.Execute(context); + INFO(updateOutput.str()); + + // Verify Installer is not called. + REQUIRE(!std::filesystem::exists(updateResultPath.GetPath())); + REQUIRE(updateOutput.str().find(Resource::LocString(Resource::String::UpgradeDifferentInstallTechnology).get()) != std::string::npos); REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_UPDATE_NOT_APPLICABLE); } From 94f24c3fb762677f701b6e88142612d6293660d0 Mon Sep 17 00:00:00 2001 From: Ruben Guerrero Samaniego Date: Thu, 28 Oct 2021 14:30:40 -0700 Subject: [PATCH 5/6] inapplicabilities --- .github/actions/spelling/allow.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/actions/spelling/allow.txt b/.github/actions/spelling/allow.txt index 037a1e6be2..5c6e4f4632 100644 --- a/.github/actions/spelling/allow.txt +++ b/.github/actions/spelling/allow.txt @@ -221,6 +221,7 @@ IIS ILogger IManifest impl +inapplicabilities Inet inheritdoc inno From bfa8ae8cad17723b08802f3fbc1a3d1eadb0c61d Mon Sep 17 00:00:00 2001 From: Ruben Guerrero Samaniego Date: Thu, 28 Oct 2021 15:31:07 -0700 Subject: [PATCH 6/6] size_t --- src/AppInstallerCLITests/ManifestComparator.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/AppInstallerCLITests/ManifestComparator.cpp b/src/AppInstallerCLITests/ManifestComparator.cpp index 80372daaf6..9a41b98a34 100644 --- a/src/AppInstallerCLITests/ManifestComparator.cpp +++ b/src/AppInstallerCLITests/ManifestComparator.cpp @@ -52,7 +52,7 @@ void RequireInapplicabilities(const std::vector& inapplica { REQUIRE(inapplicabilities.size() == expected.size()); - for (int i = 0; i < inapplicabilities.size(); i++) + for (std::size_t i = 0; i < inapplicabilities.size(); i++) { REQUIRE(inapplicabilities[i] == expected[i]); }