From 258444d1b4eba82805879f1beab150991cfa29b7 Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Wed, 13 Dec 2023 08:58:08 -0800 Subject: [PATCH] Introduce strong and weak comparisons between installers (#3956) Rather than a `bool`, the installer comparison function now returns one of three values: negative, weak positive, and strong positive. The comparison functions are updated to choose between weak and strong positive results as appropriate. For instance, matching the system architecture when the alternative does not is a strong positive result. Simply being "better" in the list of emulated architectures is a weak positive result. The function that used these comparators in our priority order is updated to effectively do multiple passes, one looking for strong results and then one looking for weak results (in reality it is implemented with one pass as an optimization). --- .../Workflows/ManifestComparator.cpp | 113 ++++++++++++++---- .../Workflows/ManifestComparator.h | 14 ++- .../ManifestComparator.cpp | 14 +++ .../AppInstallerTelemetry.cpp | 2 +- 4 files changed, 119 insertions(+), 24 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp b/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp index cd6e1b37ae..0d83c4e92b 100644 --- a/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp +++ b/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp @@ -207,17 +207,18 @@ namespace AppInstaller::CLI::Workflow return result; } - bool IsFirstBetter(const Manifest::ManifestInstaller& first, const Manifest::ManifestInstaller& second) override + details::ComparisonResult IsFirstBetter(const Manifest::ManifestInstaller& first, const Manifest::ManifestInstaller& second) override { auto arch1 = CheckAllowedArchitecture(first.Arch); auto arch2 = CheckAllowedArchitecture(second.Arch); if (arch1 > arch2) { - return true; + // A match with the primary architecture is strong + return (first.Arch == GetStrongArchitectureMatch() ? details::ComparisonResult::StrongPositive : details::ComparisonResult::WeakPositive); } - return false; + return details::ComparisonResult::Negative; } private: @@ -242,6 +243,13 @@ namespace AppInstaller::CLI::Workflow return unsupportedItr != installer.UnsupportedOSArchitectures.end(); } + Utility::Architecture GetStrongArchitectureMatch() + { + // If we have a preferential order, treat the first entry as strong. + // Otherwise, treat the system architecture as strong (which is always first in the default order). + return m_allowedArchitectures.empty() ? Utility::GetSystemArchitecture() : m_allowedArchitectures.front(); + } + std::vector m_allowedArchitectures; }; @@ -310,11 +318,11 @@ namespace AppInstaller::CLI::Workflow } } - bool IsFirstBetter(const Manifest::ManifestInstaller& first, const Manifest::ManifestInstaller& second) override + details::ComparisonResult IsFirstBetter(const Manifest::ManifestInstaller& first, const Manifest::ManifestInstaller& second) override { if (m_preference.empty()) { - return false; + return details::ComparisonResult::Negative; } for (Manifest::InstallerTypeEnum installerTypePreference : m_preference) @@ -329,15 +337,16 @@ namespace AppInstaller::CLI::Workflow if (isFirstInstallerTypePreferred && isSecondInstallerTypePreferred) { - return false; + return details::ComparisonResult::Negative; } else if (isFirstInstallerTypePreferred != isSecondInstallerTypePreferred) { - return isFirstInstallerTypePreferred; + // Treating this as a weak positive because one can use requirements to guarantee the installer type if necessary. + return (isFirstInstallerTypePreferred ? details::ComparisonResult::WeakPositive : details::ComparisonResult::Negative); } } - return false; + return details::ComparisonResult::Negative; } private: @@ -411,9 +420,17 @@ namespace AppInstaller::CLI::Workflow return result; } - bool IsFirstBetter(const Manifest::ManifestInstaller& first, const Manifest::ManifestInstaller& second) override + details::ComparisonResult IsFirstBetter(const Manifest::ManifestInstaller& first, const Manifest::ManifestInstaller& second) override { - return (first.EffectiveInstallerType() == m_installedType && second.EffectiveInstallerType() != m_installedType); + if (first.EffectiveInstallerType() == m_installedType && second.EffectiveInstallerType() != m_installedType) + { + // Installed type matching is intended to be sticky, so make this a strong match. + return details::ComparisonResult::StrongPositive; + } + else + { + return details::ComparisonResult::Negative; + } } private: @@ -537,9 +554,15 @@ namespace AppInstaller::CLI::Workflow return result; } - bool IsFirstBetter(const Manifest::ManifestInstaller& first, const Manifest::ManifestInstaller& second) override + details::ComparisonResult IsFirstBetter(const Manifest::ManifestInstaller& first, const Manifest::ManifestInstaller& second) override { - return m_preference != Manifest::ScopeEnum::Unknown && (first.Scope == m_preference && second.Scope != m_preference); + if (m_preference != Manifest::ScopeEnum::Unknown && first.Scope == m_preference && second.Scope != m_preference) + { + // When the second input is unknown, this is a weak result. If it is not (and therefore the opposite of the preference), this is strong. + return (second.Scope == Manifest::ScopeEnum::Unknown ? details::ComparisonResult::WeakPositive : details::ComparisonResult::StrongPositive); + } + + return details::ComparisonResult::Negative; } private: @@ -666,11 +689,11 @@ namespace AppInstaller::CLI::Workflow return result; } - bool IsFirstBetter(const Manifest::ManifestInstaller& first, const Manifest::ManifestInstaller& second) override + details::ComparisonResult IsFirstBetter(const Manifest::ManifestInstaller& first, const Manifest::ManifestInstaller& second) override { if (m_preference.empty()) { - return false; + return details::ComparisonResult::Negative; } for (auto const& preferredLocale : m_preference) @@ -680,13 +703,14 @@ namespace AppInstaller::CLI::Workflow if (firstScore >= Locale::MinimumDistanceScoreAsCompatibleMatch || secondScore >= Locale::MinimumDistanceScoreAsCompatibleMatch) { - return firstScore > secondScore; + // This could probably be enriched to always check all locales and determine strong/weak based off of the MinimumDistanceScoreAsCompatibleMatch. + return (firstScore > secondScore ? details::ComparisonResult::StrongPositive : details::ComparisonResult::Negative); } } // At this point, the installer locale matches no preference. // if first is unknown and second is no match for sure, we might prefer unknown one. - return first.Locale.empty() && !second.Locale.empty(); + return (first.Locale.empty() && !second.Locale.empty() ? details::ComparisonResult::WeakPositive : details::ComparisonResult::Negative); } private: @@ -771,7 +795,7 @@ namespace AppInstaller::CLI::Workflow InstallerAndInapplicabilities ManifestComparator::GetPreferredInstaller(const Manifest::Manifest& manifest) { - AICLI_LOG(CLI, Info, << "Starting installer selection."); + AICLI_LOG(CLI, Verbose, << "Starting installer selection."); const Manifest::ManifestInstaller* result = nullptr; std::vector inapplicabilitiesInstallers; @@ -810,7 +834,7 @@ namespace AppInstaller::CLI::Workflow auto inapplicability = filter->IsApplicable(installer); if (inapplicability != InapplicabilityFlags::None) { - AICLI_LOG(CLI, Info, << "Installer " << installer << " not applicable: " << filter->ExplainInapplicable(installer)); + AICLI_LOG(CLI, Verbose, << "Installer " << installer << " not applicable: " << filter->ExplainInapplicable(installer)); WI_SetAllFlags(inapplicabilityResult, inapplicability); } } @@ -822,19 +846,64 @@ namespace AppInstaller::CLI::Workflow const Manifest::ManifestInstaller& first, const Manifest::ManifestInstaller& second) { + // The priority will still be used as a tie-break between weak results. + std::optional firstWeakComparator; + bool firstWeakComparatorResult = false; + for (auto comparator : m_comparators) { - if (comparator->IsFirstBetter(first, second)) + details::ComparisonResult forwardCompare = comparator->IsFirstBetter(first, second); + details::ComparisonResult reverseCompare = comparator->IsFirstBetter(second, first); + + // Should not happen, but if it does it points at a serious bug that should be fixed. + if (forwardCompare != details::ComparisonResult::Negative && reverseCompare != details::ComparisonResult::Negative) + { + AICLI_LOG(CLI, Error, << "Installer " << first << " and " << second << " are both better than each other?"); + THROW_HR(E_UNEXPECTED); + } + + if (forwardCompare == details::ComparisonResult::StrongPositive) { - AICLI_LOG(CLI, Verbose, << "Installer " << first << " is better than " << second << " due to: " << comparator->Name()); + AICLI_LOG(CLI, Verbose, << "Installer " << first << " is better [strong] than " << second << " due to: " << comparator->Name()); return true; } - else if (comparator->IsFirstBetter(second, first)) + + if (reverseCompare == details::ComparisonResult::StrongPositive) { // Second is better by this comparator, don't allow a lower priority one to override that. - AICLI_LOG(CLI, Verbose, << "Installer " << second << " is better than " << first << " due to: " << comparator->Name()); + AICLI_LOG(CLI, Verbose, << "Installer " << second << " is better [strong] than " << first << " due to: " << comparator->Name()); return false; } + + // Save the first weak result that we get + if (!firstWeakComparator) + { + if (forwardCompare == details::ComparisonResult::WeakPositive) + { + firstWeakComparator = comparator->Name(); + firstWeakComparatorResult = true; + } + else if (reverseCompare == details::ComparisonResult::WeakPositive) + { + firstWeakComparator = comparator->Name(); + firstWeakComparatorResult = false; + } + } + } + + // If we found a weak result (and no strong result because we made it here), return it. + if (firstWeakComparator) + { + if (firstWeakComparatorResult) + { + AICLI_LOG(CLI, Verbose, << "Installer " << first << " is better [weak] than " << second << " due to: " << *firstWeakComparator); + } + else + { + AICLI_LOG(CLI, Verbose, << "Installer " << second << " is better [weak] than " << first << " due to: " << *firstWeakComparator); + } + + return firstWeakComparatorResult; } // Equal, and thus not better diff --git a/src/AppInstallerCLICore/Workflows/ManifestComparator.h b/src/AppInstallerCLICore/Workflows/ManifestComparator.h index 3f1b0f9b3b..68ad24c4bd 100644 --- a/src/AppInstallerCLICore/Workflows/ManifestComparator.h +++ b/src/AppInstallerCLICore/Workflows/ManifestComparator.h @@ -53,6 +53,18 @@ namespace AppInstaller::CLI::Workflow std::string_view m_name; }; + // The result of ComparisonField::IsFirstBetter + enum class ComparisonResult + { + // The first input is not better than the second input. + Negative, + // The first input is somewhat better than the second input. + // If another comparison has a strong positive result, it will override a weak result. + WeakPositive, + // The first input is definitely better than the second input. + StrongPositive, + }; + // An interface for defining new comparisons based on user inputs. struct ComparisonField : public FilterField { @@ -61,7 +73,7 @@ namespace AppInstaller::CLI::Workflow virtual ~ComparisonField() = default; // Determines if the first installer is a better choice based on this field alone. - virtual bool IsFirstBetter(const Manifest::ManifestInstaller& first, const Manifest::ManifestInstaller& second) = 0; + virtual ComparisonResult IsFirstBetter(const Manifest::ManifestInstaller& first, const Manifest::ManifestInstaller& second) = 0; }; } diff --git a/src/AppInstallerCLITests/ManifestComparator.cpp b/src/AppInstallerCLITests/ManifestComparator.cpp index 74171ac44b..0a0f125384 100644 --- a/src/AppInstallerCLITests/ManifestComparator.cpp +++ b/src/AppInstallerCLITests/ManifestComparator.cpp @@ -846,3 +846,17 @@ TEST_CASE("ManifestComparator_InstallerType", "[manifest_comparator]") RequireInapplicabilities(inapplicabilities, { InapplicabilityFlags::InstallerType, InapplicabilityFlags::InstallerType, InapplicabilityFlags::InstallerType }); } } + +TEST_CASE("ManifestComparator_MachineArchitecture_Strong_Scope_Weak", "[manifest_comparator]") +{ + Manifest manifest; + ManifestInstaller system = AddInstaller(manifest, GetSystemArchitecture(), InstallerTypeEnum::Msi, ScopeEnum::Unknown, "", ""); + ManifestInstaller user = AddInstaller(manifest, Architecture::Neutral, InstallerTypeEnum::Msi, ScopeEnum::User, "", ""); + + ManifestComparatorTestContext context; + + ManifestComparator mc(context, {}); + auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest); + + RequireInstaller(result, system); +} diff --git a/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp b/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp index 2f149a6353..5b3420ba0c 100644 --- a/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp +++ b/src/AppInstallerCommonCore/AppInstallerTelemetry.cpp @@ -469,7 +469,7 @@ namespace AppInstaller::Logging } } - AICLI_LOG(CLI, Info, << "Completed installer selection."); + AICLI_LOG(CLI, Verbose, << "Completed installer selection."); AICLI_LOG(CLI, Verbose, << "Selected installer Architecture: " << arch); AICLI_LOG(CLI, Verbose, << "Selected installer URL: " << url); AICLI_LOG(CLI, Verbose, << "Selected installer InstallerType: " << installerType);