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);