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 --Installer-Type argument for commands #3516

Merged
merged 12 commits into from Aug 30, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 12 additions & 0 deletions doc/Settings.md
Expand Up @@ -140,6 +140,18 @@ The `architectures` behavior affects what architectures will be selected when in
},
```

### Installer Types

The `installerTypes` behavior affects what installer types will be selected when installing a package. The matching parameter is `--installer-type`.
Copy link
Member

Choose a reason for hiding this comment

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

How does this work for scenarios like "zip containing exe" or "exe that installs an msi"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Zip uses the EffectiveInstallerType, but that brings up a good point - if someone has {zip, exe} as their preferences, because the check is against both base and effective type, a zip->msi could still be chosen even if a zip->exe exists

And another great point about the exe that installs an msi or msix; AppsAndFeaturesEntries->InstallerType might need to be considered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For zip containing exe, I check both the baseInstallerType (for zip) and effective installer type to determine if an installer satisfies the preference/requirement so I believe that scenario is covered. I didn't do anything different for "exe that installs an msi" as I only consider what is shown in the manifest


```json
"installBehavior": {
"preferences": {
"installerTypes": ["msi", "msix"]
}
},
```

### Default install root

The `defaultInstallRoot` affects the install location when a package requires one. This can be overridden by the `--location` parameter. This setting is only used when a package manifest includes `InstallLocationRequired`, and the actual location is obtained by appending the package ID to the root.
Expand Down
22 changes: 22 additions & 0 deletions schemas/JSON/settings/settings.schema.0.2.json
Expand Up @@ -93,6 +93,28 @@
"minItems": 1,
"maxItems": 4
}
},
"installerTypes": {
"description": "The installerType(s) to use for a package install",
"type": "array",
"items": {
"uniqueItems": "true",
"type": "string",
"enum": [
"inno",
"wix",
"msi",
"nullsoft",
"zip",
Copy link
Member

Choose a reason for hiding this comment

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

Is zip an installer type users would care about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it will be a common scenario, but it should still be supported if people prefer that installer type.

"msix",
"exe",
"burn",
Comment on lines +104 to +111
Copy link
Member

Choose a reason for hiding this comment

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

Would a user really care about whether something is an msi or an msi made with some-tool?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would this essentially mean that we would need an "InstallerTechnologyType" for each Installer? Effectively reducing it to portable, exe, msi, msix, and msstore?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's what I'm suggesting. But I don't know if it would be a good idea. I don't think most people would care about the difference between a wix and an msi installer, but for people who do care I think it would be confusing if the set of types here is different than in the manifest.

"msstore",
"portable"
],
"minItems": 1,
"maxItems": 9
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I don't see much point in having maxItems set to n-1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the other settings arrays declare a maxItems so I followed it just to be consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's weird but ok, Anyway this settings schema is not used in code for enforcement, it's just informational only for now.

}
}
}
},
Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/Commands/InstallCommand.cpp
Expand Up @@ -30,6 +30,7 @@ namespace AppInstaller::CLI
Argument::ForType(Args::Type::Source),
Argument{ Args::Type::InstallScope, Resource::String::InstallScopeDescription, ArgumentType::Standard, Argument::Visibility::Help },
Argument::ForType(Args::Type::InstallArchitecture),
Argument::ForType(Args::Type::InstallerType),
Argument::ForType(Args::Type::Exact),
Argument::ForType(Args::Type::Interactive),
Argument::ForType(Args::Type::Silent),
Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/Commands/ShowCommand.cpp
Expand Up @@ -27,6 +27,7 @@ namespace AppInstaller::CLI
Argument::ForType(Execution::Args::Type::Exact),
Argument{ Args::Type::InstallScope, Resource::String::InstallScopeDescription, ArgumentType::Standard, Argument::Visibility::Help },
Argument::ForType(Execution::Args::Type::InstallArchitecture),
Argument::ForType(Execution::Args::Type::InstallerType),
Argument::ForType(Execution::Args::Type::Locale),
Argument::ForType(Execution::Args::Type::ListVersions),
Argument::ForType(Execution::Args::Type::CustomHeader),
Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/Commands/UpgradeCommand.cpp
Expand Up @@ -57,6 +57,7 @@ namespace AppInstaller::CLI
Argument::ForType(Args::Type::InstallLocation), // -l
Argument{ Execution::Args::Type::InstallScope, Resource::String::InstalledScopeArgumentDescription, ArgumentType::Standard, Argument::Visibility::Help },
Argument::ForType(Args::Type::InstallArchitecture), // -a
Argument::ForType(Args::Type::InstallerType),
Argument::ForType(Args::Type::Locale),
Argument::ForType(Args::Type::HashOverride),
Argument::ForType(Args::Type::SkipDependencies),
Expand Down
87 changes: 61 additions & 26 deletions src/AppInstallerCLICore/Workflows/ManifestComparator.cpp
Expand Up @@ -247,44 +247,59 @@ namespace AppInstaller::CLI::Workflow

struct InstallerTypeComparator : public details::ComparisonField
{
InstallerTypeComparator(std::vector<InstallerTypeEnum> requirement) :
details::ComparisonField("Installer Type"), m_requirement(std::move(requirement))
InstallerTypeComparator(std::vector<InstallerTypeEnum> preference, std::vector<InstallerTypeEnum> requirement) :
details::ComparisonField("Installer Type"), m_preference(std::move(preference)), m_requirement(std::move(requirement))
{
m_preferenceAsString = Utility::ConvertContainerToString(m_preference, InstallerTypeToString);
m_requirementAsString = Utility::ConvertContainerToString(m_requirement, InstallerTypeToString);
AICLI_LOG(CLI, Verbose,
<< "InstallerType Comparator created with Required InstallerTypes: " << m_requirementAsString);
<< "InstallerType Comparator created with Required InstallerTypes: " << m_requirementAsString
<< " , Preferred InstallerTypes: " << m_preferenceAsString);
}

static std::unique_ptr<InstallerTypeComparator> Create(const Execution::Args& args)
{
std::vector<InstallerTypeEnum> preference;
std::vector<InstallerTypeEnum> requirement;

if (args.Contains(Execution::Args::Type::InstallerType))
{
requirement.emplace_back(Manifest::ConvertToInstallerTypeEnum(std::string(args.GetArg(Execution::Args::Type::InstallerType))));
}
else
{
preference = Settings::User().Get<Settings::Setting::InstallerTypePreference>();
requirement = Settings::User().Get<Settings::Setting::InstallerTypeRequirement>();
}

if (!requirement.empty())
if (!preference.empty() || !requirement.empty())
{
return std::make_unique<InstallerTypeComparator>(requirement);
return std::make_unique<InstallerTypeComparator>(preference, requirement);
}
else
{
return {};
}
}

std::string ExplainInapplicable(const Manifest::ManifestInstaller& installer) override
{
std::string result = "InstallerType [";
result += InstallerTypeToString(installer.EffectiveInstallerType());
result += "] does not match required InstallerTypes: ";
result += m_requirementAsString;
return result;
}

InapplicabilityFlags IsApplicable(const Manifest::ManifestInstaller& installer) override
{
if (!m_requirement.empty())
{
for (auto requiredInstallerType : m_requirement)
// The installer is applicable if the effective or base installer type matches.
if (ContainsInstallerType(m_requirement, installer.EffectiveInstallerType()) ||
ContainsInstallerType(m_requirement, installer.BaseInstallerType))
{
// The installer is applicable if the installer type or nested installer type matches. (User should be allowed to specify 'zip')
if (installer.EffectiveInstallerType() == requiredInstallerType || installer.BaseInstallerType == requiredInstallerType)
{
return InapplicabilityFlags::None;
}
return InapplicabilityFlags::None;
}

return InapplicabilityFlags::InstallerType;
Expand All @@ -295,27 +310,47 @@ namespace AppInstaller::CLI::Workflow
}
}

std::string ExplainInapplicable(const Manifest::ManifestInstaller& installer) override
{
std::string result = "InstallerType does not match required type: ";
result += InstallerTypeToString(installer.EffectiveInstallerType());
result += "Required InstallerTypes: ";
result += m_requirementAsString;
return result;
}

bool IsFirstBetter(const Manifest::ManifestInstaller& first, const Manifest::ManifestInstaller& second) override
{
// TODO: Current implementation assumes there is only a single installer type requirement. This needs to be updated
// once multiple installerType requirements and preferences are accepted.
UNREFERENCED_PARAMETER(first);
UNREFERENCED_PARAMETER(second);
return true;
if (m_preference.empty())
{
return false;
}

for (Manifest::InstallerTypeEnum installerTypePreference : m_preference)
{
bool isFirstInstallerTypePreferred =
first.EffectiveInstallerType() == installerTypePreference ||
first.BaseInstallerType == installerTypePreference;

bool isSecondInstallerTypePreferred =
second.EffectiveInstallerType() == installerTypePreference ||
second.BaseInstallerType == installerTypePreference;

if (isFirstInstallerTypePreferred && isSecondInstallerTypePreferred)
{
return false;
ryfu-msft marked this conversation as resolved.
Show resolved Hide resolved
}

if (isFirstInstallerTypePreferred != isSecondInstallerTypePreferred)
ryfu-msft marked this conversation as resolved.
Show resolved Hide resolved
{
return isFirstInstallerTypePreferred > isSecondInstallerTypePreferred;
ryfu-msft marked this conversation as resolved.
Show resolved Hide resolved
}
}

return false;
}

private:
std::vector<InstallerTypeEnum> m_preference;
std::vector<InstallerTypeEnum> m_requirement;
std::string m_preferenceAsString;
std::string m_requirementAsString;

bool ContainsInstallerType(const std::vector<InstallerTypeEnum>& selection, InstallerTypeEnum installerType)
{
return std::find(selection.begin(), selection.end(), installerType) != selection.end();
}
};

struct InstalledTypeComparator : public details::ComparisonField
Expand Down Expand Up @@ -728,11 +763,11 @@ namespace AppInstaller::CLI::Workflow

// 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.
AddComparator(InstallerTypeComparator::Create(context.Args));
AddComparator(InstalledTypeComparator::Create(installationMetadata));
AddComparator(LocaleComparator::Create(context.Args, installationMetadata));
AddComparator(ScopeComparator::Create(context));
AddComparator(MachineArchitectureComparator::Create(context, installationMetadata));
AddComparator(InstallerTypeComparator::Create(context.Args));
}

InstallerAndInapplicabilities ManifestComparator::GetPreferredInstaller(const Manifest::Manifest& manifest)
Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCLIE2ETests/Constants.cs
Expand Up @@ -117,6 +117,7 @@ public class Constants
public const string PortablePackageUserRoot = "portablePackageUserRoot";
public const string PortablePackageMachineRoot = "portablePackageMachineRoot";
public const string InstallBehaviorScope = "scope";
public const string InstallerTypes = "installerTypes";

// Configuration
public const string PSGalleryName = "PSGallery";
Expand Down
74 changes: 52 additions & 22 deletions src/AppInstallerCLIE2ETests/Helpers/WinGetSettingsHelper.cs
Expand Up @@ -94,13 +94,7 @@ public static void SetWingetSettings(string settings)
/// <param name="status">Status.</param>
public static void ConfigureFeature(string featureName, bool status)
{
JObject settingsJson = JObject.Parse(File.ReadAllText(TestSetup.Parameters.SettingsJsonFilePath));

if (!settingsJson.ContainsKey("experimentalFeatures"))
{
settingsJson["experimentalFeatures"] = new JObject();
}

JObject settingsJson = GetJsonSettingsObject("experimentalFeatures");
var experimentalFeatures = settingsJson["experimentalFeatures"];
experimentalFeatures[featureName] = status;

Expand All @@ -114,13 +108,7 @@ public static void ConfigureFeature(string featureName, bool status)
/// <param name="value">Setting value.</param>
public static void ConfigureInstallBehavior(string settingName, string value)
{
JObject settingsJson = JObject.Parse(File.ReadAllText(TestSetup.Parameters.SettingsJsonFilePath));

if (!settingsJson.ContainsKey("installBehavior"))
{
settingsJson["installBehavior"] = new JObject();
}

JObject settingsJson = GetJsonSettingsObject("installBehavior");
var installBehavior = settingsJson["installBehavior"];
installBehavior[settingName] = value;

Expand All @@ -134,13 +122,28 @@ public static void ConfigureInstallBehavior(string settingName, string value)
/// <param name="value">Setting value.</param>
public static void ConfigureInstallBehaviorPreferences(string settingName, string value)
{
JObject settingsJson = JObject.Parse(File.ReadAllText(TestSetup.Parameters.SettingsJsonFilePath));
JObject settingsJson = GetJsonSettingsObject("installBehavior");
var installBehavior = settingsJson["installBehavior"];

if (!settingsJson.ContainsKey("installBehavior"))
if (installBehavior["preferences"] == null)
{
settingsJson["installBehavior"] = new JObject();
installBehavior["preferences"] = new JObject();
}

var preferences = installBehavior["preferences"];
preferences[settingName] = value;

File.WriteAllText(TestSetup.Parameters.SettingsJsonFilePath, settingsJson.ToString());
}

/// <summary>
/// Configure the install behavior preferences.
/// </summary>
/// <param name="settingName">Setting name.</param>
/// <param name="value">Setting value array.</param>
public static void ConfigureInstallBehaviorPreferences(string settingName, string[] value)
{
JObject settingsJson = GetJsonSettingsObject("installBehavior");
var installBehavior = settingsJson["installBehavior"];

if (installBehavior["preferences"] == null)
Expand All @@ -149,7 +152,7 @@ public static void ConfigureInstallBehaviorPreferences(string settingName, strin
}

var preferences = installBehavior["preferences"];
preferences[settingName] = value;
preferences[settingName] = new JArray(value);

File.WriteAllText(TestSetup.Parameters.SettingsJsonFilePath, settingsJson.ToString());
}
Expand All @@ -161,13 +164,28 @@ public static void ConfigureInstallBehaviorPreferences(string settingName, strin
/// <param name="value">Setting value.</param>
public static void ConfigureInstallBehaviorRequirements(string settingName, string value)
{
JObject settingsJson = JObject.Parse(File.ReadAllText(TestSetup.Parameters.SettingsJsonFilePath));
JObject settingsJson = GetJsonSettingsObject("installBehavior");
var installBehavior = settingsJson["installBehavior"];

if (!settingsJson.ContainsKey("installBehavior"))
if (installBehavior["requirements"] == null)
{
settingsJson["installBehavior"] = new JObject();
installBehavior["requirements"] = new JObject();
}

var requirements = installBehavior["requirements"];
requirements[settingName] = value;

File.WriteAllText(TestSetup.Parameters.SettingsJsonFilePath, settingsJson.ToString());
}

/// <summary>
/// Configure the install behavior requirements.
/// </summary>
/// <param name="settingName">Setting name.</param>
/// <param name="value">Setting value array.</param>
public static void ConfigureInstallBehaviorRequirements(string settingName, string[] value)
{
JObject settingsJson = GetJsonSettingsObject("installBehavior");
var installBehavior = settingsJson["installBehavior"];

if (installBehavior["requirements"] == null)
Expand All @@ -176,7 +194,7 @@ public static void ConfigureInstallBehaviorRequirements(string settingName, stri
}

var requirements = installBehavior["requirements"];
requirements[settingName] = value;
requirements[settingName] = new JArray(value);

File.WriteAllText(TestSetup.Parameters.SettingsJsonFilePath, settingsJson.ToString());
}
Expand All @@ -196,5 +214,17 @@ public static void InitializeAllFeatures(bool status)
ConfigureFeature("windowsFeature", status);
ConfigureFeature("download", status);
}

private static JObject GetJsonSettingsObject(string objectName)
{
JObject settingsJson = JObject.Parse(File.ReadAllText(TestSetup.Parameters.SettingsJsonFilePath));

if (!settingsJson.ContainsKey(objectName))
{
settingsJson[objectName] = new JObject();
}

return settingsJson;
}
}
}