-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Added settings to select preferred/required architecture. #1727
Changes from 1 commit
d2805fa
01d1029
7293c6e
a7286dc
f535556
0384fcd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,17 +46,19 @@ namespace AppInstaller::CLI::Workflow | |
|
||
struct MachineArchitectureComparator : public details::ComparisonField | ||
{ | ||
MachineArchitectureComparator() : details::ComparisonField("Machine Architecture") {} | ||
MachineArchitectureComparator(Utility::Architecture preference) : | ||
details::ComparisonField("Machine Architecture"), m_preference(preference) {} | ||
|
||
MachineArchitectureComparator(std::vector<Utility::Architecture> allowedArchitectures) : | ||
details::ComparisonField("Machine Architecture"), m_allowedArchitectures(std::move(allowedArchitectures)) | ||
MachineArchitectureComparator(std::vector<Utility::Architecture> allowedArchitectures, Utility::Architecture preference) : | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shouldn't have needed to change to fully support preferences and requirements. An entry of |
||
details::ComparisonField("Machine Architecture"), m_allowedArchitectures(std::move(allowedArchitectures)), m_preference(preference) | ||
{ | ||
AICLI_LOG(CLI, Verbose, << "Architecture Comparator created with allowed architectures: " << GetAllowedArchitecturesString()); | ||
} | ||
|
||
// TODO: At some point we can do better about matching the currently installed architecture | ||
static std::unique_ptr<MachineArchitectureComparator> Create(const Execution::Context& context, const Repository::IPackageVersion::Metadata&) | ||
{ | ||
Utility::Architecture preference = Settings::User().Get<Settings::Setting::InstallArchitecturePreference>(); | ||
if (context.Contains(Execution::Data::AllowedArchitectures)) | ||
{ | ||
const std::vector<Utility::Architecture>& allowedArchitectures = context.Get<Execution::Data::AllowedArchitectures>(); | ||
|
@@ -95,11 +97,11 @@ namespace AppInstaller::CLI::Workflow | |
} | ||
} | ||
|
||
return std::make_unique<MachineArchitectureComparator>(std::move(result)); | ||
return std::make_unique<MachineArchitectureComparator>(std::move(result), preference); | ||
} | ||
} | ||
|
||
return std::make_unique<MachineArchitectureComparator>(); | ||
return std::make_unique<MachineArchitectureComparator>(preference); | ||
} | ||
|
||
InapplicabilityFlags IsApplicable(const Manifest::ManifestInstaller& installer) override | ||
|
@@ -129,6 +131,10 @@ namespace AppInstaller::CLI::Workflow | |
|
||
bool IsFirstBetter(const Manifest::ManifestInstaller& first, const Manifest::ManifestInstaller& second) override | ||
{ | ||
if (m_preference != Utility::Architecture::Neutral) | ||
{ | ||
return (first.Arch == m_preference && second.Arch != m_preference); | ||
} | ||
auto arch1 = CheckAllowedArchitecture(first.Arch); | ||
auto arch2 = CheckAllowedArchitecture(second.Arch); | ||
|
||
|
@@ -170,6 +176,7 @@ namespace AppInstaller::CLI::Workflow | |
} | ||
|
||
std::vector<Utility::Architecture> m_allowedArchitectures; | ||
Utility::Architecture m_preference; | ||
}; | ||
|
||
struct InstalledTypeComparator : public details::ComparisonField | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -962,14 +962,20 @@ namespace AppInstaller::CLI::Workflow | |
bool isUpdate = WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::InstallerExecutionUseUpdate); | ||
|
||
IPackageVersion::Metadata installationMetadata; | ||
Utility::Architecture requiredArchitecture = Settings::User().Get<Settings::Setting::InstallArchitectureRequirement>(); | ||
if (isUpdate) | ||
{ | ||
installationMetadata = context.Get<Execution::Data::InstalledPackageVersion>()->GetMetadata(); | ||
} | ||
if (context.Args.Contains(Execution::Args::Type::InstallArchitecture)) | ||
{ | ||
// arguments override settings. | ||
context.Add<Execution::Data::AllowedArchitectures>({ Utility::ConvertToArchitectureEnum(std::string(context.Args.GetArg(Execution::Args::Type::InstallArchitecture))) }); | ||
} | ||
else if (requiredArchitecture != Utility::Architecture::Neutral) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would make this an unconditional |
||
{ | ||
context.Add<Execution::Data::AllowedArchitectures>({ requiredArchitecture }); | ||
} | ||
ManifestComparator manifestComparator(context, installationMetadata); | ||
auto [installer, inapplicabilities] = manifestComparator.GetPreferredInstaller(context.Get<Execution::Data::Manifest>()); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we consider making it a list to be more flexible(like locale above) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ticket made it sound like you’d select a preferred (single) architecture instead of multiple, but I can do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also thinking we would have a "single" preferred or required architecture. I can't think of a valid scenario where there would be three architectures that could all work and you would prefer (a), accept (b), and reject (c). With locale, I could see a set you would like in some kind of order so you could essentially have a bit of control when one of the languages you used was available, but it wasn't a logical close match otherwise. Let's keep this as a singleton for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just after I hit send, I remembered some emulation might be valid, but I think the "requires" forces it to be what I want and "prefers" will take anything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is how it works. Prefers will fallback to the normal logic if the architecture you prefer is not available. If your required architecture is not available you get an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was just not completing thoughts last night. Days have bee feeling like weeks lately. I'm trying to close a lot of gaps. We have some heavy investments coming in related to Intune integration for the replacement of store for business, and Win32 support for the store. The next couple of quarters are going to be busy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather have it be an array to start. At some point someone will want an array and then we will have two settings (or have to implement it allowing both scalar and array). Even requirements makes sense to support multiple, just like locales. I might be an ARM purist on my ARM64 machine and so require
{ arm64, arm }
.