commit 4dc7647644d4c0a2176a466832f8cd486ac30e27
parent 829705ac9ee882c67dc66326eef5ec1c1094af82
Author: JohnMcPMS <johnmcp@microsoft.com>
Date: Wed, 13 Dec 2023 08:58:08 -0800
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).
Diffstat:
4 files changed, 119 insertions(+), 24 deletions(-)
diff --git a/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp b/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp
@@ -210,17 +210,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:
@@ -245,6 +246,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<Utility::Architecture> m_allowedArchitectures;
};
@@ -313,11 +321,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)
@@ -332,15 +340,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:
@@ -414,9 +423,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:
@@ -540,9 +557,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:
@@ -669,11 +692,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)
@@ -683,13 +706,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:
@@ -774,7 +798,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<InapplicabilityFlags> inapplicabilitiesInstallers;
@@ -813,7 +837,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);
}
}
@@ -825,19 +849,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<std::string_view> 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
@@ -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
@@ -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
@@ -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);
