Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for markets #1806

Merged
merged 5 commits into from
Jan 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 47 additions & 31 deletions src/AppInstallerCLICore/Workflows/ManifestComparator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ namespace AppInstaller::CLI::Workflow
MachineArchitectureComparator(std::vector<Utility::Architecture> allowedArchitectures) :
details::ComparisonField("Machine Architecture"), m_allowedArchitectures(std::move(allowedArchitectures))
{
AICLI_LOG(CLI, Verbose, << "Architecture Comparator created with allowed architectures: " << GetAllowedArchitecturesString());
AICLI_LOG(CLI, Verbose, << "Architecture Comparator created with allowed architectures: " << Utility::ConvertContainerToString(m_allowedArchitectures, Utility::ToString));
}

// TODO: At some point we can do better about matching the currently installed architecture
Expand Down Expand Up @@ -169,22 +169,6 @@ namespace AppInstaller::CLI::Workflow
return unsupportedItr != installer.UnsupportedOSArchitectures.end();
}

std::string GetAllowedArchitecturesString()
{
std::stringstream result;
bool prependComma = false;
for (Utility::Architecture architecture : m_allowedArchitectures)
{
if (prependComma)
{
result << ", ";
}
result << Utility::ToString(architecture);
prependComma = true;
}
return result.str();
}

std::vector<Utility::Architecture> m_allowedArchitectures;
};

Expand Down Expand Up @@ -290,7 +274,7 @@ namespace AppInstaller::CLI::Workflow

std::string ExplainInapplicable(const Manifest::ManifestInstaller& installer) override
{
std::string result = "Installer scope does not matched currently installed scope: ";
std::string result = "Installer scope does not match currently installed scope: ";
result += Manifest::ScopeToString(installer.Scope);
result += " != ";
result += Manifest::ScopeToString(m_requirement);
Expand Down Expand Up @@ -429,8 +413,8 @@ namespace AppInstaller::CLI::Workflow
LocaleComparator(std::vector<std::string> preference, std::vector<std::string> requirement) :
details::ComparisonField("Locale"), m_preference(std::move(preference)), m_requirement(std::move(requirement))
{
m_requirementAsString = GetLocalesListAsString(m_requirement);
m_preferenceAsString = GetLocalesListAsString(m_preference);
m_requirementAsString = Utility::ConvertContainerToString(m_requirement);
m_preferenceAsString = Utility::ConvertContainerToString(m_preference);
AICLI_LOG(CLI, Verbose, << "Locale Comparator created with Required Locales: " << m_requirementAsString << " , Preferred Locales: " << m_preferenceAsString);
}

Expand Down Expand Up @@ -521,37 +505,69 @@ namespace AppInstaller::CLI::Workflow
std::vector<std::string> m_requirement;
std::string m_requirementAsString;
std::string m_preferenceAsString;
};

std::string GetLocalesListAsString(const std::vector<std::string>& locales)
struct MarketFilter : public details::FilterField
{
MarketFilter(Manifest::string_t market) : details::FilterField("Market"), m_market(market)
{
std::string result = "[";
AICLI_LOG(CLI, Verbose, << "Market Filter created with market: " << m_market);
}

bool first = true;
for (auto const& locale : locales)
static std::unique_ptr<MarketFilter> Create()
{
return std::make_unique<MarketFilter>(Runtime::GetOSRegion());
}

InapplicabilityFlags IsApplicable(const Manifest::ManifestInstaller& installer) override
{
// If both allowed and excluded lists are provided, we only need to check the allowed markets.
if (!installer.Markets.AllowedMarkets.empty())
{
if (first)
// Inapplicable if NOT found
if (!IsMarketInList(installer.Markets.AllowedMarkets))
{
first = false;
return InapplicabilityFlags::Market;
}
else
}
else if (!installer.Markets.ExcludedMarkets.empty())
{
// Inapplicable if found
if (IsMarketInList(installer.Markets.ExcludedMarkets))
{
result += ", ";
return InapplicabilityFlags::Market;
}

result += locale;
}

result += ']';
return InapplicabilityFlags::None;
}

std::string ExplainInapplicable(const Manifest::ManifestInstaller& installer) override
{
std::string result = "Current market '" + m_market + "' does not match installer markets." +
" Allowed markets: " + Utility::ConvertContainerToString(installer.Markets.AllowedMarkets) +
" Excluded markets: " + Utility::ConvertContainerToString(installer.Markets.ExcludedMarkets);
return result;
}

private:
bool IsMarketInList(const std::vector<Manifest::string_t> markets)
{
return markets.end() != std::find_if(
markets.begin(),
markets.end(),
[&](const auto& m) { return Utility::CaseInsensitiveEquals(m, m_market); });
}

Manifest::string_t m_market;
};
}

ManifestComparator::ManifestComparator(const Execution::Context& context, const Repository::IPackageVersion::Metadata& installationMetadata)
{
AddFilter(std::make_unique<OSVersionFilter>());
AddFilter(InstalledScopeFilter::Create(installationMetadata));
AddFilter(MarketFilter::Create());

// Filter order is not important, but comparison order determines priority.
// TODO: There are improvements to be made here around ordering, especially in the context of implicit vs explicit vs command line preferences.
Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/Workflows/ManifestComparator.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ namespace AppInstaller::CLI::Workflow
Locale = 0x10,
Scope = 0x20,
MachineArchitecture = 0x40,
Market = 0x80,
};

DEFINE_ENUM_FLAG_OPERATORS(InapplicabilityFlags);
Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/Workflows/WorkflowBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -999,6 +999,7 @@ namespace AppInstaller::CLI::Workflow
context.Add<Execution::Data::AllowedArchitectures>({ optionalArchitectures.begin(), optionalArchitectures.end() });
}
}

ManifestComparator manifestComparator(context, installationMetadata);
auto [installer, inapplicabilities] = manifestComparator.GetPreferredInstaller(context.Get<Execution::Data::Manifest>());

Expand Down
74 changes: 67 additions & 7 deletions src/AppInstallerCLITests/ManifestComparator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ const ManifestInstaller& AddInstaller(
ScopeEnum scope = ScopeEnum::Unknown,
std::string minOSVersion = {},
std::string locale = {},
std::vector<Architecture> unsupportedOSArchitectures = {})
std::vector<Architecture> unsupportedOSArchitectures = {},
MarketsInfo markets = {})
{
ManifestInstaller toAdd;
toAdd.Arch = architecture;
Expand All @@ -40,12 +41,24 @@ const ManifestInstaller& AddInstaller(
toAdd.MinOSVersion = minOSVersion;
toAdd.Locale = locale;
toAdd.UnsupportedOSArchitectures = unsupportedOSArchitectures;
toAdd.Markets = markets;

manifest.Installers.emplace_back(std::move(toAdd));

return manifest.Installers.back();
}

template<typename T>
void RequireVectorsEqual(const std::vector<T>& actual, const std::vector<T>& expected)
{
REQUIRE(actual.size() == expected.size());

for (std::size_t i = 0; i < actual.size(); ++i)
{
REQUIRE(actual[i] == expected[i]);
}
}

void RequireInstaller(const std::optional<ManifestInstaller>& actual, const ManifestInstaller& expected)
{
REQUIRE(actual);
Expand All @@ -54,16 +67,14 @@ void RequireInstaller(const std::optional<ManifestInstaller>& actual, const Mani
REQUIRE(actual->Scope == expected.Scope);
REQUIRE(actual->MinOSVersion == expected.MinOSVersion);
REQUIRE(actual->Locale == expected.Locale);

RequireVectorsEqual(actual->Markets.AllowedMarkets, expected.Markets.AllowedMarkets);
RequireVectorsEqual(actual->Markets.ExcludedMarkets, expected.Markets.ExcludedMarkets);
}

void RequireInapplicabilities(const std::vector<InapplicabilityFlags>& inapplicabilities, const std::vector<InapplicabilityFlags>& expected)
{
REQUIRE(inapplicabilities.size() == expected.size());

for (std::size_t i = 0; i < inapplicabilities.size(); i++)
{
REQUIRE(inapplicabilities[i] == expected[i]);
}
RequireVectorsEqual(inapplicabilities, expected);
}

TEST_CASE("ManifestComparator_OSFilter_Low", "[manifest_comparator]")
Expand Down Expand Up @@ -580,3 +591,52 @@ TEST_CASE("ManifestComparator_Inapplicabilities", "[manifest_comparator]")
inapplicabilities,
{ InapplicabilityFlags::OSVersion | InapplicabilityFlags::InstalledType | InapplicabilityFlags::Locale | InapplicabilityFlags::Scope | InapplicabilityFlags::MachineArchitecture });
}

TEST_CASE("ManifestComparator_MarketFilter", "[manifest_comparator]")
{
Manifest manifest;

// Get current market.
winrt::Windows::Globalization::GeographicRegion region;
Manifest::string_t currentMarket{ region.CodeTwoLetter() };

SECTION("Applicable")
{
MarketsInfo markets;
SECTION("Allowed")
{
markets.AllowedMarkets = { currentMarket };
}
SECTION("Not excluded")
{
markets.ExcludedMarkets = { "XX" };
}

ManifestInstaller installer = AddInstaller(manifest, Architecture::X86, InstallerTypeEnum::Exe, {}, {}, {}, {}, markets);
ManifestComparator mc(ManifestComparatorTestContext{}, {});
auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest);

RequireInstaller(result, installer);
REQUIRE(inapplicabilities.empty());
}

SECTION("Inapplicable")
{
MarketsInfo markets;
SECTION("Excluded")
{
markets.ExcludedMarkets = { currentMarket };
}
SECTION("Not allowed")
{
markets.AllowedMarkets = { "XX" };
}

ManifestInstaller installer = AddInstaller(manifest, Architecture::X86, InstallerTypeEnum::Exe, {}, {}, {}, {}, markets);
ManifestComparator mc(ManifestComparatorTestContext{}, {});
auto [result, inapplicabilities] = mc.GetPreferredInstaller(manifest);

REQUIRE(!result);
RequireInapplicabilities(inapplicabilities, { InapplicabilityFlags::Market});
}
}
1 change: 1 addition & 0 deletions src/AppInstallerCLITests/pch.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

#include <winrt/Windows.Foundation.h>
#include <winrt/Windows.Foundation.Collections.h>
#include <winrt/Windows.Globalization.h>
#include <winrt/Windows.Management.Deployment.h>

#include <wil/resource.h>
Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCommonCore/Public/AppInstallerRuntime.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ namespace AppInstaller::Runtime
Utility::LocIndString GetOSVersion();

// Gets the OS region.
// This can be used as the current market.
std::string GetOSRegion();

// A path to be retrieved based on the runtime.
Expand Down
32 changes: 32 additions & 0 deletions src/AppInstallerCommonCore/Public/AppInstallerStrings.h
Original file line number Diff line number Diff line change
Expand Up @@ -145,4 +145,36 @@ namespace AppInstaller::Utility

// Gets the file name part of the given URI.
std::filesystem::path GetFileNameFromURI(std::string_view uri);

// Converts a container to a string representation of it.
template <typename T, typename U>
std::string ConvertContainerToString(const T& container, U toString)
{
std::ostringstream strstr;
strstr << '[';

bool firstItem = true;
for (const auto& item : container)
{
if (firstItem)
{
firstItem = false;
}
else
{
strstr << ", ";
}

strstr << toString(item);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which type(s) were not being output properly by the <<? I would expect any stringish type we use to work without a coercion function.

Not critical, but I like to understand 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Utility::Architecture for the applicable architectures filter. It was coming out as an integer.

}

strstr << ']';
return strstr.str(); // We need C++20 to get std::move(strstr).str() to extract the string from inside the stream
}

template <typename T>
std::string ConvertContainerToString(const T& container)
{
return ConvertContainerToString(container, [](const auto& item) { return item; });
}
}