-
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
Added settings to select preferred/required architecture. #1727
Conversation
}, | ||
"architecture": { | ||
"description": "The architecture for a package install", | ||
"type": "string", |
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 }
.
|
||
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 comment
The 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 Utility::Architecture::Unknown
in Execution::Data::AllowedArchitectures
indicates "and all the rest"; aka that the previous architectures are preferences. Thus the only difference between the preferences and requirements settings are whether to append Unknown
.
}, | ||
"architecture": { | ||
"description": "The architecture for a package install", | ||
"type": "string", |
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 }
.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
I would make this an unconditional else
and the check the settings (both once you revert the ManifestComparator
changes) inside it. That way we don't do unnecessary work getting the setting(s) if there is an argument passed in.
WINGET_VALIDATE_SIGNATURE(InstallArchitecturePreference) | ||
{ | ||
Utility::Architecture arch = Utility::ConvertToArchitectureEnum(value); | ||
if (Utility::IsApplicableArchitecture(arch) != Utility::InapplicableArchitecture) |
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.
You have tabs here; please convert to 4 spaces.
WINGET_VALIDATE_PASS_THROUGH(TelemetryDisable) | ||
WINGET_VALIDATE_PASS_THROUGH(EFDirectMSI) | ||
WINGET_VALIDATE_PASS_THROUGH(EnableSelfInitiatedMinidump) | ||
|
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.
What did whitespace do to you?
I fixed all of the complaints (array is used, ManifestComparator isn't messed with, whitespace fixed). All of the changes to Sorry for the delay, I was traveling for Thanksgiving. |
I noticed something when working on that last commit, which is that adding the Unknown architecture to AllowedArchitectures makes the ManifestComparator use its traditional logic instead of accepting the user's preference above all else: settings.json:
Relevant logs from
Am I missing something here or do I still need to do something to ManifestComparator to make sure that the preferences are honored above the traditional logic? |
Are you putting the |
That was exactly it, thanks! Now it's using emplace_back, and all is right with the world. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
…crosoft#1727)" This reverts commit f1c2358.
Resolves #906.
This is the other half of #906, adding settings to select a preferred or required architecture. The required architecture behaves exactly like the
--architecture
flag, whereAllowedArchitectures
only contains the architecture that is in the setting. The preferred architecture behaves much like the preferred scope setting, where the installer entry with the preferred architecture is selected over all others, but if there isn't an installer entry with the preferred architecture, the normal rationale is used.The fields are validated by determining if the architecture selected is compatible with the current system. By default the setting is set to
neutral
. (although if there is a reason we need to allow users to require neutral manifests, that could just as easily beunknown
. I figured there were few scenarios where a user wouldn't want a installer that only had the relevant bits for their system over a neutral one with all of the bits.)The only other note I have is that I decided to make these just strings instead of arrays since that's what it sounded like in the original ticket. If there's a usecase where someone wants to prefer multiple architectures, that's a quick fix (although there's only ever at most three options, so I doubt that comes up much?).
Tested: manually. I can write unit tests if necessary.
Edit: also, sorry for the big diff on UserSettings.cpp. Visual Studio got upset that the line endings were mixed.
Microsoft Reviewers: Open in CodeFlow