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.

Sign up for GitHub

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

Add support for --Installer-Type argument for commands #3516

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3ca8814
Add support for installertype argument
Aug 10, 2023
51e1b61
fix e2e test
Aug 10, 2023
a40a6ef
fix installerType comparison
Aug 10, 2023
e2bbad8
add extra condition
Aug 10, 2023
a155cc1
return false
Aug 10, 2023
12b467f
remove preference order
Aug 11, 2023
da47fb5
address comments and clean up tests
Aug 15, 2023
5ed23fc
add order preference to installer type preference
Aug 28, 2023
3bbad1e
fixE2Etest
Aug 28, 2023
08cfe54
fix test again
Aug 29, 2023
b95a65c
try again
Aug 29, 2023
7d64507
address PR feedback
Aug 30, 2023
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
12 changes: 12 additions & 0 deletions doc/Settings.md
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
82 changes: 57 additions & 25 deletions src/AppInstallerCLICore/Workflows/ManifestComparator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,44 +247,64 @@ 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 does not match required type: ";
result += InstallerTypeToString(installer.EffectiveInstallerType());
result += "Required InstallerTypes: ";
ryfu-msft marked this conversation as resolved.
Show resolved Hide resolved
result += m_requirementAsString;
return result;
}

bool ContainsInstallerType(const std::vector<InstallerTypeEnum>& selection, InstallerTypeEnum installerType)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: move to private

return std::find(selection.begin(), selection.end(), installerType) != selection.end();
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, thinking more about this, I wonder if it would make sense to have a templated utility function for checking if an object is contained within a container.

Although, considering that this PR is required for 1.6, it might make sense not to create a utility function.

I'm thinking something like this might work, but I'm not certain

template <template<typename, typename> typename Container, typename Allocator, typename Value>
bool ContainsObject(Container<Value, Allocator> container, Value value) {
  return std::find(container.begin(), container.end(), value) != container.end();
}

}

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,26 +315,38 @@ 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;
}

bool isFirstInstallerTypePreferred =
ContainsInstallerType(m_preference, first.EffectiveInstallerType()) ||
ContainsInstallerType(m_preference, first.BaseInstallerType);

bool isSecondInstallerTypePreferred =
ContainsInstallerType(m_preference, second.EffectiveInstallerType()) ||
ContainsInstallerType(m_preference, second.BaseInstallerType);

if (isFirstInstallerTypePreferred && isSecondInstallerTypePreferred)
{
return false;
}

if (isFirstInstallerTypePreferred != isSecondInstallerTypePreferred)
{
return isFirstInstallerTypePreferred;
}

return false;
ryfu-msft marked this conversation as resolved.
Show resolved Hide resolved
}

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

Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCLIE2ETests/Constants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
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
73 changes: 49 additions & 24 deletions src/AppInstallerCLIE2ETests/Helpers/WinGetSettingsHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,22 +134,25 @@ 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));

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

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

if (installBehavior["preferences"] == null)
{
installBehavior["preferences"] = new JObject();
}
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", "preferences");
var installBehavior = settingsJson["installBehavior"];
var preferences = installBehavior["preferences"];
preferences[settingName] = value;
preferences[settingName] = new JArray(value);

File.WriteAllText(TestSetup.Parameters.SettingsJsonFilePath, settingsJson.ToString());
}
Expand All @@ -161,22 +164,25 @@ 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));

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

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

if (installBehavior["requirements"] == null)
{
installBehavior["requirements"] = new JObject();
}
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", "requirements");
var installBehavior = settingsJson["installBehavior"];
var requirements = installBehavior["requirements"];
requirements[settingName] = value;
requirements[settingName] = new JArray(value);

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

private static JObject GetJsonSettingsObject(string objectName, string propertyName)
Copy link
Member

Choose a reason for hiding this comment

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

What does this function do?
It seems like it parses the settings file, then adds an object with a single empty property?
That is weird to me, and the method name doesn't help.

Copy link
Member

Choose a reason for hiding this comment

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

What about something like

private static JObject GetJsonSettingsObjectAndCreateEmptyProperty(string[] propertyPath...)
{
    JObject settingsJson = JObject.Parse(File.ReadAllText(TestSetup.Parameters.SettingsJsonFilePath));
    var currentNode = settings.Json;
    foreach (var key in propertyPath)
    {
        if (!currentNode.ContainsKey(key))
        {
            currentNode[key] = new JObject();
        }
        currentNode = currentNode[key];
    }

    return settingsJson;
}

(Not sure if this even compiles...)

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 changed the helper method to be a little more generic so that it only returns the json settings object that you specify and doesn't do anything with creating a single empty property (which I also agree is kind of weird).

{
JObject settingsJson = JObject.Parse(File.ReadAllText(TestSetup.Parameters.SettingsJsonFilePath));

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

var installBehavior = settingsJson[objectName];
ryfu-msft marked this conversation as resolved.
Show resolved Hide resolved

if (installBehavior[propertyName] == null)
{
installBehavior[propertyName] = new JObject();
}

return settingsJson;
}
}
}
53 changes: 53 additions & 0 deletions src/AppInstallerCLIE2ETests/InstallCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

namespace AppInstallerCLIE2ETests
{
using System;
using System.IO;
using AppInstallerCLIE2ETests.Helpers;
using NUnit.Framework;
Expand Down Expand Up @@ -673,6 +674,58 @@ public void InstallWithPackageDependency_RefreshPathVariable()
Assert.True(TestCommon.VerifyTestExeInstalledAndCleanup(testDir));
}

/// <summary>
/// Test install a package using a specific installer type.
/// </summary>
[Test]
public void InstallWithInstallerTypeArgument()
{
var installDir = TestCommon.GetRandomTestDir();
var result = TestCommon.RunAICLICommand("install", $"AppInstallerTest.TestMultipleInstallers --silent -l {installDir} --installer-type exe");
Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);
Assert.True(result.StdOut.Contains("Successfully installed"));
Assert.True(TestCommon.VerifyTestExeInstalledAndCleanup(installDir, "/execustom"));
}

/// <summary>
/// Test install package with installer type preference settings.
/// </summary>
[Test]
public void InstallWithInstallerTypePreference()
{
string[] installerTypePreference = { "msi" };
WinGetSettingsHelper.ConfigureInstallBehaviorPreferences(Constants.InstallerTypes, installerTypePreference);

string installDir = TestCommon.GetRandomTestDir();
var result = TestCommon.RunAICLICommand("install", $"AppInstallerTest.TestMultipleInstallers --silent -l {installDir}");

// Reset installer type preferences.
WinGetSettingsHelper.ConfigureInstallBehaviorPreferences(Constants.InstallerTypes, Array.Empty<string>());

Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);
Assert.True(result.StdOut.Contains("Successfully installed"));
Assert.True(TestCommon.VerifyTestMsiInstalledAndCleanup(installDir));
}

/// <summary>
/// Test install package with installer type requirement settings.
/// </summary>
[Test]
public void InstallWithInstallerTypeRequirement()
{
string[] installerTypeRequirement = { "inno" };
WinGetSettingsHelper.ConfigureInstallBehaviorRequirements(Constants.InstallerTypes, installerTypeRequirement);

string installDir = TestCommon.GetRandomTestDir();
var result = TestCommon.RunAICLICommand("install", $"AppInstallerTest.TestMultipleInstallers --silent -l {installDir}");

// Reset installer type requirements.
WinGetSettingsHelper.ConfigureInstallBehaviorRequirements(Constants.InstallerTypes, Array.Empty<string>());

Assert.AreEqual(Constants.ErrorCode.ERROR_NO_APPLICABLE_INSTALLER, result.ExitCode);
Assert.True(result.StdOut.Contains("No applicable installer found; see logs for more details."));
}

/// <summary>
/// This test flow is intended to test an EXE that actually installs an MSIX internally, and whose name+publisher
/// information resembles an existing installation. Given this, the goal is to get correlation to stick to the
Expand Down
Loading