Skip to content

Commit

Permalink
Introduce strong and weak comparisons between installers (#3956)
Browse files Browse the repository at this point in the history 
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).
  • Loading branch information
@JohnMcPMS @yao-msft
JohnMcPMS authored and yao-msft committed Dec 15, 2023
1 parent 280f5ff commit 258444d
Show file tree
Hide file tree
Showing 4 changed files with 119 additions and 24 deletions.
113 changes: 91 additions & 22 deletions src/AppInstallerCLICore/Workflows/ManifestComparator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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<Utility::Architecture> m_allowedArchitectures;
};

Expand Down Expand Up @@ -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)
Expand All @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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)
Expand All @@ -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:
Expand Down Expand Up @@ -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<InapplicabilityFlags> inapplicabilitiesInstallers;
Expand Down Expand Up @@ -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);
}
}
Expand All @@ -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<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
Expand Down
14 changes: 13 additions & 1 deletion src/AppInstallerCLICore/Workflows/ManifestComparator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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;
};
}

Expand Down
14 changes: 14 additions & 0 deletions src/AppInstallerCLITests/ManifestComparator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
2 changes: 1 addition & 1 deletion src/AppInstallerCommonCore/AppInstallerTelemetry.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 258444d

Please sign in to comment.