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

ryfu-msft · 2023-08-10T18:51:07Z

Resolves #1166

Changes:

Adds the --installer-type argument to the Install, Upgrade, and Show command.
Modifies settings schema to allow for installer type preferences and requirements.
Added the corresponding argument to the COM InstallOptions.
Updated the manifest installer comparator to handle both installer type preferences and requirements.

Tests:

Unit tests to verify the manifest comparator when providing the installerType arg, preference, and requirements.
E2E tests to verify the behavior for COM install, Install, and Show.

Microsoft Reviewers: codeflow:open?pullrequest=https://github.com/microsoft/winget-cli/pull/3516&drop=dogfoodAlpha

src/AppInstallerCLICore/Workflows/ManifestComparator.cpp

src/Microsoft.Management.Deployment/InstallOptions.h

src/AppInstallerCLICore/Workflows/ManifestComparator.cpp

Trenly · 2023-08-11T19:11:58Z

src/AppInstallerCLICore/Workflows/ManifestComparator.cpp

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


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

florelis · 2023-08-11T19:43:48Z

schemas/JSON/settings/settings.schema.0.2.json

+              "portable"
+            ],
+            "minItems": 1,
+            "maxItems": 9


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

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

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

florelis · 2023-08-11T19:46:15Z

schemas/JSON/settings/settings.schema.0.2.json

+              "inno",
+              "wix",
+              "msi",
+              "nullsoft",
+              "zip",
+              "msix",
+              "exe",
+              "burn",


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

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

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.

florelis · 2023-08-11T19:47:23Z

doc/Settings.md

@@ -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`.


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

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

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

florelis · 2023-08-11T19:48:24Z

schemas/JSON/settings/settings.schema.0.2.json

+              "wix",
+              "msi",
+              "nullsoft",
+              "zip",


Is zip an installer type users would care about?

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

src/AppInstallerCLICore/Workflows/ManifestComparator.cpp

florelis · 2023-08-11T19:57:59Z

src/AppInstallerCommonCore/UserSettings.cpp

+                Manifest::InstallerTypeEnum installerType = Manifest::ConvertToInstallerTypeEnum(i);
+                if (installerType == Manifest::InstallerTypeEnum::Unknown)
+                {
+                    return {};


Would it make sense to just skip the ones that are not valid instead of dropping everything? That way, if in future versions we add new installer types, people could use those same settings files on this version.

I basically followed how we validate the other settings (like Architecture preference that drop if there is an invalid value) so I left it as is because I don't think installer type should behave any differently.

src/AppInstallerCLIE2ETests/Helpers/WinGetSettingsHelper.cs

florelis · 2023-08-11T20:03:40Z

src/AppInstallerCLIE2ETests/Helpers/WinGetSettingsHelper.cs

@@ -196,5 +202,24 @@ public static void InitializeAllFeatures(bool status)
            ConfigureFeature("windowsFeature", status);
            ConfigureFeature("download", status);
        }
+
+        private static JObject GetJsonSettingsObject(string objectName, string propertyName)


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.

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...)

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).

yao-msft · 2023-08-26T00:55:09Z

schemas/JSON/settings/settings.schema.0.2.json

+              "portable"
+            ],
+            "minItems": 1,
+            "maxItems": 9


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

yao-msft · 2023-08-26T01:06:26Z

src/AppInstallerCLICore/Workflows/ManifestComparator.cpp

+            }
+
+            bool ContainsInstallerType(const std::vector<InstallerTypeEnum>& selection, InstallerTypeEnum installerType)
+            {


nit: move to private

yao-msft · 2023-08-26T01:18:07Z

src/AppInstallerCLICore/Workflows/ManifestComparator.cpp

+                    ContainsInstallerType(m_preference, second.EffectiveInstallerType()) ||
+                    ContainsInstallerType(m_preference, second.BaseInstallerType);
+
+                if (isFirstInstallerTypePreferred == isSecondInstallerTypePreferred)


I'm a bit torn about whether making the preference ordered vs unordered. Making it unordered will limit the user's ability to prefer a specific installer type over another and all other preferences are ordered. Making it ordered seems too strong for installer type though. maybe a middle ground will be making it ordered and put this comparator in the last on line 765 so it's not that significant in the end (btw, putting the comparator to last should be done regardless of whether we decide it ordered or unordered)

src/AppInstallerCLICore/Workflows/ManifestComparator.cpp

Add support for installertype argument

3ca8814

ryfu-msft requested a review from a team as a code owner August 10, 2023 18:51

microsoft-github-policy-service bot added Issue-Feature This is a feature request for the Windows Package Manager client. Area-Settings Issue related to WinGet Settings labels Aug 10, 2023

Trenly reviewed Aug 10, 2023

View reviewed changes

src/AppInstallerCLICore/Workflows/ManifestComparator.cpp Outdated Show resolved Hide resolved

src/Microsoft.Management.Deployment/InstallOptions.h Outdated Show resolved Hide resolved

Ryan Fu added 4 commits August 10, 2023 13:19

fix e2e test

51e1b61

fix installerType comparison

a40a6ef

add extra condition

e2bbad8

return false

a155cc1

Trenly reviewed Aug 10, 2023

View reviewed changes

src/AppInstallerCLICore/Workflows/ManifestComparator.cpp Show resolved Hide resolved

remove preference order

12b467f

ryfu-msft requested a review from Trenly August 11, 2023 17:10

Trenly reviewed Aug 11, 2023

View reviewed changes

src/AppInstallerCLICore/Workflows/ManifestComparator.cpp Outdated Show resolved Hide resolved

src/AppInstallerCLICore/Workflows/ManifestComparator.cpp Outdated Show resolved Hide resolved

Trenly reviewed Aug 11, 2023

View reviewed changes

florelis reviewed Aug 11, 2023

View reviewed changes

address comments and clean up tests

da47fb5

ryfu-msft requested a review from florelis August 15, 2023 22:28

yao-msft previously approved these changes Aug 26, 2023

View reviewed changes

add order preference to installer type preference

5ed23fc

ryfu-msft dismissed yao-msft’s stale review via 5ed23fc August 28, 2023 17:16

Ryan Fu added 3 commits August 28, 2023 14:58

fixE2Etest

3bbad1e

fix test again

08cfe54

try again

b95a65c

yao-msft reviewed Aug 30, 2023

View reviewed changes

src/AppInstallerCLICore/Workflows/ManifestComparator.cpp Outdated Show resolved Hide resolved

yao-msft reviewed Aug 30, 2023

View reviewed changes

src/AppInstallerCLICore/Workflows/ManifestComparator.cpp Outdated Show resolved Hide resolved

address PR feedback

7d64507

yao-msft approved these changes Aug 30, 2023

View reviewed changes

ryfu-msft merged commit 020c2f0 into microsoft:master Aug 30, 2023
8 checks passed

ryfu-msft deleted the installerTypeArg branch August 30, 2023 20:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ryfu-msft commented Aug 10, 2023 •

edited by microsoft-github-policy-service bot

Trenly Aug 11, 2023

florelis Aug 11, 2023

ryfu-msft Aug 15, 2023

yao-msft Aug 26, 2023

florelis Aug 11, 2023

Trenly Aug 12, 2023

florelis Aug 14, 2023

florelis Aug 11, 2023

Trenly Aug 11, 2023

ryfu-msft Aug 15, 2023

florelis Aug 11, 2023

ryfu-msft Aug 15, 2023

florelis Aug 11, 2023

ryfu-msft Aug 15, 2023

florelis Aug 11, 2023

florelis Aug 11, 2023

ryfu-msft Aug 15, 2023

yao-msft Aug 26, 2023

yao-msft Aug 26, 2023

yao-msft Aug 26, 2023

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

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

Conversation

ryfu-msft commented Aug 10, 2023 • edited by microsoft-github-policy-service bot

Microsoft Reviewers: codeflow:open?pullrequest=https://github.com/microsoft/winget-cli/pull/3516&drop=dogfoodAlpha

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

ryfu-msft commented Aug 10, 2023 •

edited by microsoft-github-policy-service bot