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 5 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`. Note that if multiple preferred installer types are available for installation, the order the installer types are specified will dictate which installer type is more preferred.

```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
53 changes: 43 additions & 10 deletions src/AppInstallerCLICore/Workflows/ManifestComparator.cpp
Expand Up @@ -247,26 +247,34 @@ 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
{
Expand Down Expand Up @@ -306,15 +314,40 @@ namespace AppInstaller::CLI::Workflow

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;
}

InstallerTypeEnum firstBaseInstallerType = first.BaseInstallerType;
InstallerTypeEnum firstEffectiveInstallerType = first.EffectiveInstallerType();

InstallerTypeEnum secondBaseInstallerType = second.BaseInstallerType;
InstallerTypeEnum secondEffectiveInstallerType = second.EffectiveInstallerType();

for (auto const& preferredInstallerType : m_preference)
{
bool isFirstInstallerTypePreferred = (preferredInstallerType == firstBaseInstallerType) || (preferredInstallerType == firstEffectiveInstallerType);
bool isSecondInstallerTypePreferred = (preferredInstallerType == secondBaseInstallerType) || (preferredInstallerType == secondEffectiveInstallerType);

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;
}
}

return false;
}

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
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
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
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
25 changes: 25 additions & 0 deletions src/AppInstallerCLIE2ETests/Interop/InstallInterop.cs
Expand Up @@ -597,6 +597,31 @@ public async Task InstallWithSkipDependencies()
TestCommon.VerifyPortablePackage(Path.Combine(installDir, Constants.PortableExePackageDirName), commandAlias, fileName, productCode, false);
}

/// <summary>
/// Test installing a package with a specific installer type install option.
/// </summary>
/// <returns>A <see cref="Task"/> representing the asynchronous unit test.</returns>
[Test]
public async Task InstallWithInstallerType()
{
// Find package
var searchResult = this.FindOnePackage(this.testSource, PackageMatchField.Id, PackageFieldMatchOption.Equals, "AppInstallerTest.TestMultipleInstallers");

// Configure installation
var installOptions = this.TestFactory.CreateInstallOptions();
installOptions.PackageInstallMode = PackageInstallMode.Silent;
installOptions.PreferredInstallLocation = this.installDir;
installOptions.InstallerType = PackageInstallerType.Msi;
installOptions.AcceptPackageAgreements = true;

// Install
var installResult = await this.packageManager.InstallPackageAsync(searchResult.CatalogPackage, installOptions);

// Assert
Assert.AreEqual(InstallResultStatus.Ok, installResult.Status);
Assert.True(TestCommon.VerifyTestMsiInstalledAndCleanup(this.installDir));
}

/// <summary>
/// Test to verify the GetApplicableInstaller() COM call returns the correct manifest installer metadata.
/// </summary>
Expand Down
24 changes: 24 additions & 0 deletions src/AppInstallerCLIE2ETests/ShowCommand.cs
Expand Up @@ -124,5 +124,29 @@ public void ShowWithExactArgCaseSensitivity()
Assert.AreEqual(Constants.ErrorCode.ERROR_NO_APPLICATIONS_FOUND, result.ExitCode);
Assert.True(result.StdOut.Contains("No package found matching input criteria."));
}

/// <summary>
/// Test show with installer type.
/// </summary>
[Test]
public void ShowWithInstallerTypeArg()
{
var result = TestCommon.RunAICLICommand("show", $"--id AppInstallerTest.TestMultipleInstallers --installer-type msi");
Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);
Assert.True(result.StdOut.Contains("Found TestMultipleInstallers [AppInstallerTest.TestMultipleInstallers]"));
Assert.True(result.StdOut.Contains("Installer Type: msi"));
}

/// <summary>
/// Test show with an archive installer type.
/// </summary>
[Test]
public void ShowWithZipInstallerTypeArg()
{
var result = TestCommon.RunAICLICommand("show", $"--id AppInstallerTest.TestMultipleInstallers --installer-type zip");
Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);
Assert.True(result.StdOut.Contains("Found TestMultipleInstallers [AppInstallerTest.TestMultipleInstallers]"));
Assert.True(result.StdOut.Contains("Installer Type: exe (zip)"));
}
}
}