Add experimental feature support for enabling Windows Feature dependencies #3005

ryfu-msft · 2023-02-23T17:38:23Z

Related to: #556

Description:
This PR adds experimental feature support for enabling Windows Features prior to installation. There are known packages such as Wix toolkit that require certain optional Windows Features to be enabled before installation can run to completion.

Changes:

Added new windowsFeature experimental feature to the settings schema.
Enabling windows feature is done by loading the DismApi.dll and calling those dll functions.

Implementation flow:

Checks if experimental feature is enabled
Checks if admin mode is enabled in order to support calling DismApis
Retrieves the WindowsFeatures dependencies and verifies that they all exist, if they do not, winget exits with a friendly error message. A user can add the --ignore-missing-dependencies flag to bypass this check and continue with the installation.
Each valid Windows Feature dependency is enabled if not already enabled. If any of the dependencies fail to be enabled, an error message with the corresponding HRESULT is displayed to the user. The user can add --force to bypass this failure and continue with installation. Often times, a reboot will be required to fully enable a feature, in this scenario we warn the user but continue with the installation as it is possible that the installation may still succeed.

Tests:

Added unit tests for verifying the behavior of the CLI output based on the exit codes from enabling/disabling Windows Features using TestHooks
Added tests enabling netfx3 and disabling netfx3 and verifying the output is successful.

Microsoft Reviewers: Open in CodeFlow

doc/Settings.md

src/AppInstallerCLICore/ExecutionArgs.h

Trenly · 2023-02-23T18:01:10Z

src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp

+
+        const auto& rootDependencies = context.Get<Execution::Data::Installer>()->Dependencies;
+
+        if (rootDependencies.Empty())


Is this already handled by HasAnyOf ? If not, should it be?

JohnMcPMS · 2023-02-23T22:30:19Z

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

+    <comment>"Windows Feature" is the name of the options Windows features setting.</comment>
+  </data>
+  <data name="EnablingWindowsFeatures" xml:space="preserve">
+    <value>Enabling Windows Feature dependencies...</value>


There is a symbolic name and a friendly (localized) name. I don't know how easy it is to get the friendly name from the symbolic name. Which do you expect to see (or both)?

I would think either would work; If it's easy to get the friendly name, that would be great, but I would expect most users could google the symbolic name to find the friendly name

Changed to showing friendly name as well as the symbolic name such as [netfx3]

Trenly · 2023-02-23T18:37:06Z

src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp

+
+        if (rootDependencies.HasAnyOf(DependencyType::WindowsFeature))
+        {
+            context << Workflow::EnsureRunningAsAdmin;


Installer dependencies are known before this workflow is ever executed; Specific dependency types are also checked for in the ReportDependencies workflow.

I'm wondering, now that multiple dependency types will be supported, if it makes more sense to set an execution flag for "Contains Package Dependencies" or "Contains Windows Features Dependencies" in the ReportDependencies flow.

With the flags, then would be possible to have a separate workflow around CheckSupportForWindowsFeatures that checks the flag ensures the experimental feature is enabled and user is running as admin, to avoid creating copies of the dependencies data in memory unnecessarily

src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp

+            bool continueOnFailure = context.Args.Contains(Execution::Args::Type::Force);
+            bool rebootRequired = false;
+
+            context.Reporter.Info() << Resource::String::EnablingWindowsFeatures << std::endl;


ryfu-msft · 2023-03-21T20:13:45Z

src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp

+                    WindowsFeature::WindowsFeature windowsFeature{ name };
+                    if (!windowsFeature.DoesExist())
+                    {
+                        invalidFeatures.emplace_back(name);


no longer create vector to aggregate missing dependencies, they will be reported at the end after each windows feature is processed

ryfu-msft · 2023-03-21T17:53:36Z

src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp

+            {
+                if (continueOnFailure)
+                {
+                    context.Reporter.Warn() << Resource::String::FailedToEnableWindowsFeatureOverridden << std::endl;


Changed to report each feature that failed to enable

src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml

Trenly · 2023-02-23T21:29:04Z

src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp

+                }
+                else
+                {
+                    context.Reporter.Error() << Resource::String::FailedToEnableWindowsFeatureOverrideRequired << std::endl;


@denelon - I may be asking the wrong (scope creep) questions here, so forgive me, but would we want any sort of telemetry around these?

Which features are requested to be enabled

The success / fail rates of enabling features

I think we should be showing each feature as its own item and not aggregating them as well.

Changed so that each windows feature is represented as its own item.

Sounds good; I'm presuming any telemetry needs can be added once support is no longer experimental

JohnMcPMS · 2023-02-23T22:59:22Z

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

@@ -495,6 +495,7 @@ namespace AppInstaller::CLI::Workflow
            Workflow::GetDependenciesFromInstaller <<
            Workflow::ReportDependencies(Resource::String::InstallAndUpgradeCommandsReportDependencies) <<
            Workflow::ManagePackageDependencies(Resource::String::InstallAndUpgradeCommandsReportDependencies) <<
+            Workflow::EnableWindowsFeaturesDependencies <<


This whole thing doesn't seem like the right way to handle dependencies. At least, "download" is not consistent with "and also, handle getting all dependencies squared away". So at least maybe change the name of this task?

Moved this out of "download" to the step right before InstallPackageInstaller.

In reality, this is a larger issue than your feature work, and I think it really needs to be addressed before we can enable dependencies on the whole. Neither of us considered that in retrospect.

You should move the workflow task back here (for now); and probably put it before package dependencies. I say that because one of the packages may actually be dependent on the Windows feature (although it really should have declared it). But maybe it is an optional dependency that affects the install time behavior. It is not likely the case that the Windows feature is dependent on any package though.

The issue is that multiple "downloads" are currently allowed via COM, but if dependencies are actually installed as part of "download", then multiple installs are allowed concurrently. So either we need to change the download/install phase split to download everything, then install everything, or we need to support going back and forth between download and install phases as we handle each dependency.

Thanks for the explanation, I didn't know that COM allowed for multiple downloads. I moved the task back to the suggested location and added a TODO comment explaining the issue that needs to be addressed.

src/AppInstallerCLICore/ExecutionArgs.h

JohnMcPMS · 2023-02-23T23:38:29Z

src/AppInstallerCommonCore/Public/winget/WindowsFeature.h

+        HRESULT DisableFeature();
+
+    private:
+        DismApiHelper m_dismApiHelper;


There is a lot of repeated loading and probing for the functions using this pattern. It would be much better to just have this created one time.

If you want dism to be able to unload (which seems reasonable), then I would design this differently as well. I would then create a single DismHelper object in the workflow and have it create WindowsFeature objects for you.

Redesigned the implementation so that a single DismHelper object is initialized and it creates a WindowsFeature object for each feature dependency.

JohnMcPMS · 2023-02-23T23:42:17Z

src/AppInstallerCommonCore/Public/winget/WindowsFeature.h

+
+        wil::unique_hmodule m_module;
+        DismSession m_session = DISM_SESSION_DEFAULT;
+        DismFeatureInfo* m_featureInfo = nullptr;


This shouldn't be holding state that is feature specific.
You shouldn't be using a raw pointer if you can help it, use a smart pointer type instead. Both std::unique_ptr and wil::unique_any can handle this.

I tried using a unique_ptr but I don't think it's possible to access the memory address of a unique_ptr which is what I need for the **FeatureInfo parameter in DismFeatureInfo. I believe this is okay though because DismDelete should handle the cleanup of this pointer.

I think you could do this with wil::unique_any, but I'm not sure that it supports a dynamic delete pointer. But we can live without it.

JohnMcPMS · 2023-02-23T23:51:17Z

src/AppInstallerCommonCore/WindowsFeature.cpp

+        DismFeatureInfo* featureInfo = m_dismApiHelper.GetFeatureInfo(m_featureName);
+        DismPackageFeatureState featureState = featureInfo->FeatureState;
+        AICLI_LOG(Core, Verbose, << "Feature state of " << m_featureName << " is " << featureState);
+        return (featureState == DismStateInstalled || featureState == DismStateInstallPending);


Do you have more information on why DismStateInstallPending should be included but DismStatePartiallyInstalled is not? My gut would be that only DismStateInstalled would be installed and everything else should require us to do something.

I changed the condition so that only DismStateInstalled is considered as enabled.

src/AppInstallerCommonCore/FileLogger.cpp

Trenly · 2023-03-21T18:48:03Z

src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp

+                    AICLI_TERMINATE_CONTEXT(hr);
+                }
+            }
+            else if (rebootRequired)


I think this neglects the case where one feature fails to enable, and another has a reboot required. I don't know if we care about that case or not, or if that's even a possibility with the way hr gets handled.

If --force isn't present, I don't think it matters, since both the reboot and the fail would cause the context to terminate, and the termination reason doesn't make much of a difference. If --force is present, the user would only be informed they are bypassing the failed install, and they wouldn't be informed they are also bypassing the reboot requirement.

Trenly · 2023-03-21T18:52:35Z

src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp

+
+                        if (windowsFeature.DoesExist())
+                        {
+                            if (!windowsFeature.IsEnabled())


I think there's a missing check for a pending reboot if the feature is already enabled?

Thinking of the case where a user attempts an install and gets a failure that a reboot is required. If they attempt to run the install command again without rebooting, the Windows Feature will be shown as enabled(?) and therefore won't trigger the rebootRequired, effectively negating the need to use --force by running the command twice.

I changed the condition so that a reboot required is no longer considered as enabled. That means if they run the install command again, the feature would show up as needing a reboot and wouldn't show up as being enabled and a reboot required message would be the shown requiring --force to proceed.

src/AppInstallerCommonCore/Public/winget/WindowsFeature.h

JohnMcPMS · 2023-03-21T20:50:12Z

src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp

+
+                                info << Resource::String::EnablingWindowsFeature(Utility::LocIndView{ featureDisplayName }, locIndFeatureName) << std::endl;
+
+                                hr = windowsFeature.Enable();


This should be using the reporter's "execute with progress" system, even if it doesn't have any actual progress. That way it at least shows the spinner while it is doing a presumably long running task.

Changed Enable to take in an IProgressCallback parameter and declared it as an unreferenced parameter. Verified spinner works and is shown manually.

src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp

src/AppInstallerCLICore/Workflows/DependenciesFlow.h

JohnMcPMS · 2023-03-21T21:08:17Z

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

@@ -495,6 +495,7 @@ namespace AppInstaller::CLI::Workflow
            Workflow::GetDependenciesFromInstaller <<
            Workflow::ReportDependencies(Resource::String::InstallAndUpgradeCommandsReportDependencies) <<
            Workflow::ManagePackageDependencies(Resource::String::InstallAndUpgradeCommandsReportDependencies) <<
+            Workflow::EnableWindowsFeaturesDependencies <<


In reality, this is a larger issue than your feature work, and I think it really needs to be addressed before we can enable dependencies on the whole. Neither of us considered that in retrospect.

You should move the workflow task back here (for now); and probably put it before package dependencies. I say that because one of the packages may actually be dependent on the Windows feature (although it really should have declared it). But maybe it is an optional dependency that affects the install time behavior. It is not likely the case that the Windows feature is dependent on any package though.

The issue is that multiple "downloads" are currently allowed via COM, but if dependencies are actually installed as part of "download", then multiple installs are allowed concurrently. So either we need to change the download/install phase split to download everything, then install everything, or we need to support going back and forth between download and install phases as we handle each dependency.

JohnMcPMS · 2023-03-21T21:30:53Z

src/AppInstallerCommonCore/Public/winget/WindowsFeature.h

+
+            std::string m_featureName;
+            DismFeatureInfo* m_featureInfo = nullptr;
+            DismGetFeatureInfoPtr m_getFeatureInfoPtr = nullptr;


The correct way to do this would be using a shared_ptr<DismHelper> that the features would hold to keep the helper from being destroyed before them. Then the features would call through that to the functions that it holds.

I moved WindowsFeature out of DismHelper and created a shared_ptr<DismHelper> member. WindowsFeature now calls the functions through the dismhelper shared pointer.

JohnMcPMS · 2023-03-21T21:35:47Z

src/AppInstallerCommonCore/WindowsFeature.cpp

+    DismHelper::DismHelper()
+    {
+#ifndef AICLI_DISABLE_TEST_HOOKS
+        // The entire DismHelper class needs to be mocked since DismHost.exe inherits log file handles.


Doesn't need to change, but it would be a lot cleaner:

Given that this is almost exclusively a function pointer holder, I would have used a derived type for the test override and create function that enabled me to inject the mock (otherwise creating the base type). That would have required a lot fewer test hook points.

src/AppInstallerCommonCore/WindowsFeature.cpp

JohnMcPMS · 2023-03-21T21:58:27Z

src/AppInstallerCommonCore/WindowsFeature.cpp

+        if (!m_dismInitialize)
+        {
+            AICLI_LOG(Core, Error, << "Could not get proc address of DismInitialize");
+            return;


RE: The other comment:
This should throw, not return an object that can't perform it's duties (unless there is a set of these functions that isn't needed in a specific case).

Changed to using suggested helper function. I chose to use THROW_LAST_ERROR since that returns the error code returned from GetProcAddress

JohnMcPMS · 2023-03-21T21:59:26Z

src/AppInstallerCommonCore/WindowsFeature.cpp

+    {
+        if (m_dismInitialize)
+        {
+            LOG_IF_FAILED(m_dismInitialize(2, NULL, NULL));


2? Comment or constant that indicates what/why that is being used.

Changed to using enum value DismLogErrorsWarningsInfo

JohnMcPMS · 2023-03-23T20:47:34Z

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

@@ -1804,8 +1804,9 @@ Please specify one of them using the --source option to proceed.</value>
    <comment>{Locked="{0}","{1}"} Message displayed to the user regarding which Windows Feature is being enabled.</comment>
  </data>
  <data name="FailedToEnableWindowsFeature" xml:space="preserve">
-    <value>Failed to enable [{0}] feature.</value>
-    <comment>{Locked="{0}"} Error message displayed when a failure was encountered when enabling a Windows Feature.</comment>
+    <value>Failed to enable [{0}] feature with exit code: {1}</value>


Suggested change

<value>Failed to enable [{0}] feature with exit code: {1}</value>

<value>Failed to enable {0} [{1}] feature.

{2}: {3}</value>

I was looking for something with more information directly, showing the message as well. Example:

Failed to enable Windows Sandbox [windows-sandbox] feature. 0x80001234: The system error message for the HRESULT.

0 = Display Name
1 = Feature Name
2 = HRESULT
3 = HRESULT description

Changed to suggested, I used GetPresentableUserMessage to handle {2}: {3}

src/AppInstallerCommonCore/FileLogger.cpp

JohnMcPMS · 2023-03-23T20:59:25Z

src/AppInstallerCommonCore/Public/winget/WindowsFeature.h


    private:
+        friend WindowsFeature;


This doesn't look like it needs to be a friend if this object simply exposed public methods for calling into the function pointers.

removed and exposed public functions

JohnMcPMS · 2023-03-23T21:04:19Z

src/AppInstallerCommonCore/Public/winget/WindowsFeature.h

+    {
+        // This default constructor is only used for mocking unit tests.
+        WindowsFeature() = default;
+        WindowsFeature(std::shared_ptr<DismHelper> dismHelper, const std::string& name);


nit: I find this awkward and would have a helper on DismHelper (that also requires that DismHelper derive from std::enable_shared_from_this).

I left the constructor as is to be able to get a shared_ptr to DismHelper. I changed DismHelper to derive from enable_shared_from_this so that we can safely get another shared_ptr as suggested.

The awkward part is the call site; the need to pass the shared_ptr yourself. (I should have put the comment there I suppose...) I wasn't suggesting that you should take the constructor away (although you could make it protected and friend the other way). I was suggesting that you add a function like:

WindowsFeature DismHelper::GetFeature(std::string featureName);

So that the call site was less awkward, like:

WindowsFeature::WindowsFeature windowsFeature = dismHelper->GetFeature(featureName};

If you did make the constructor protected, you then don't have to worry about the shared_ptr being null.

I agree the call site is weird passing shared ptrs around. I feel that this is important if dismhelper were to be extended to other scenarios so I changed it based on your suggestions. I created a helper function within dismhelper that creates the windows object for you, calling the constructor with the shared ptr. I made the WindowsFeature constructor protected and made dismHelper a friend so that only the dismhelper can create the WindowsFeature object.

src/AppInstallerCommonCore/Public/winget/WindowsFeature.h

Ryan Fu added 7 commits February 13, 2023 11:03

add project files

a6ec947

save work

cea7e0d

add arguments, unit tests, and test hooks

8000470

add strings and fix CLI output

f66dd54

add experimental feature for windows feature

ba74875

fix spelling and test

6d8ca3e

add extra warning message during override

5d048d4

ryfu-msft requested a review from a team as a code owner February 23, 2023 17:38

Merge branch 'master' into windowsFeatures

98b44d7

denelon mentioned this pull request Feb 23, 2023

Turn Windows features on and off #556

Closed

Trenly reviewed Feb 23, 2023

View reviewed changes

remove extra file from project files

8ebe76b

Trenly reviewed Feb 23, 2023

View reviewed changes

JohnMcPMS reviewed Feb 23, 2023

View reviewed changes

Ryan Fu added 10 commits February 27, 2023 16:51

refactor dismhelper

d59eb0e

finish addressing feedback

8c8a7cf

add close session

d4d9430

set all dependencies to false

a4e0eee

extend sleep time

b5b2ae4

move to E2E tests

61a4143

fix tests

3ba134a

comment out windows feature tests

f23ccb7

resolve merge conflicts

6711502

block file handle inheritance

427cc0a

This comment has been minimized.

Sign in to view

JohnMcPMS reviewed Mar 9, 2023

View reviewed changes

src/AppInstallerCommonCore/FileLogger.cpp Outdated Show resolved Hide resolved

src/AppInstallerCommonCore/FileLogger.cpp Outdated Show resolved Hide resolved

fix logging stream and add error handling

c28f5e4

This comment has been minimized.

Sign in to view

JohnMcPMS reviewed Mar 10, 2023

View reviewed changes

src/AppInstallerCommonCore/FileLogger.cpp Outdated Show resolved Hide resolved

src/AppInstallerCommonCore/FileLogger.cpp Outdated Show resolved Hide resolved

src/AppInstallerCommonCore/FileLogger.cpp Show resolved Hide resolved

mock everything

fa20945

This comment has been minimized.

Sign in to view

address comments

fb1a034

This comment has been minimized.

Sign in to view

Ryan Fu added 2 commits March 10, 2023 14:39

fix spelling

0e027e8

fix test hooks

09b1a0f

Trenly reviewed Mar 21, 2023

View reviewed changes

JohnMcPMS requested changes Mar 21, 2023

View reviewed changes

microsoft-github-policy-service bot added the Needs-Author-Feedback Issue needs attention from issue or PR author label Mar 21, 2023

address PR comments

12c4cf5

microsoft-github-policy-service bot removed the Needs-Author-Feedback Issue needs attention from issue or PR author label Mar 23, 2023

Ryan Fu added 3 commits March 23, 2023 10:41

add period to strings

e1dd564

add todo comment

fdc86bb

fix e2e test

918875f

JohnMcPMS requested changes Mar 23, 2023

View reviewed changes

microsoft-github-policy-service bot added Needs-Author-Feedback Issue needs attention from issue or PR author and removed Needs-Author-Feedback Issue needs attention from issue or PR author labels Mar 23, 2023

Ryan Fu added 2 commits March 23, 2023 17:57

save work

0f36ef9

fix issues

13200c3

JohnMcPMS approved these changes Mar 24, 2023

View reviewed changes

fix call site for getwindowsfeature

db3c8ac

ryfu-msft merged commit b95f9ca into microsoft:master Mar 24, 2023

ryfu-msft deleted the windowsFeatures branch March 24, 2023 19:55


		const auto& rootDependencies = context.Get<Execution::Data::Installer>()->Dependencies;

		if (rootDependencies.Empty())


		info << Resource::String::EnablingWindowsFeature(Utility::LocIndView{ featureDisplayName }, locIndFeatureName) << std::endl;

		hr = windowsFeature.Enable();

	<value>Failed to enable [{0}] feature with exit code: {1}</value>
	<value>Failed to enable {0} [{1}] feature.
	{2}: {3}</value>

Add experimental feature support for enabling Windows Feature dependencies #3005

Add experimental feature support for enabling Windows Feature dependencies #3005

Conversation

ryfu-msft commented Feb 23, 2023 • edited by microsoft-github-policy-service bot Loading

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

This comment was marked as resolved.

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

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

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

ryfu-msft Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryfu-msft Mar 21, 2023 • edited Loading

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

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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

Choose a reason for hiding this comment

ryfu-msft commented Feb 23, 2023 •

edited by microsoft-github-policy-service bot

Loading

ryfu-msft Mar 21, 2023 •

edited

Loading

ryfu-msft Mar 21, 2023 •

edited

Loading