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

Invoke ShellExecute on dism.exe for enabling Windows Features #3659

Merged
merged 7 commits into from
Oct 11, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
1 change: 1 addition & 0 deletions .github/actions/spelling/allow.txt
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ deserializing
dest
devblogs
differentpath
DISMAPI
DIRECTONLY
distro
dll
Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/Resources.h
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ namespace AppInstaller::CLI::Resource
WINGET_DEFINE_RESOURCE_STRINGID(DownloadDirectoryArgumentDescription);
WINGET_DEFINE_RESOURCE_STRINGID(Downloading);
WINGET_DEFINE_RESOURCE_STRINGID(EnableAdminSettingFailed);
WINGET_DEFINE_RESOURCE_STRINGID(EnableWindowsFeaturesSuccess);
WINGET_DEFINE_RESOURCE_STRINGID(EnablingWindowsFeature);
WINGET_DEFINE_RESOURCE_STRINGID(ErrorCommandLongDescription);
WINGET_DEFINE_RESOURCE_STRINGID(ErrorCommandShortDescription);
Expand Down
91 changes: 5 additions & 86 deletions src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,11 @@
#include "ManifestComparator.h"
#include "InstallFlow.h"
#include "winget\DependenciesGraph.h"
#include "winget\WindowsFeature.h"
#include "DependencyNodeProcessor.h"
#include "ShellExecuteInstallerHandler.h"

using namespace AppInstaller::Repository;
using namespace AppInstaller::Manifest;
using namespace AppInstaller::WindowsFeature;

namespace AppInstaller::CLI::Workflow
{
Expand Down Expand Up @@ -136,94 +135,14 @@ namespace AppInstaller::CLI::Workflow

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

if (rootDependencies.Empty())
if (rootDependencies.Empty() || !rootDependencies.HasAnyOf(DependencyType::WindowsFeature))
{
return;
}

if (rootDependencies.HasAnyOf(DependencyType::WindowsFeature))
{
context << Workflow::EnsureRunningAsAdmin;
if (context.IsTerminated())
{
return;
}

HRESULT hr = S_OK;
std::shared_ptr<DismHelper> dismHelper = std::make_shared<DismHelper>();

bool force = context.Args.Contains(Execution::Args::Type::Force);
bool rebootRequired = false;

rootDependencies.ApplyToType(DependencyType::WindowsFeature, [&context, &hr, &dismHelper, &force, &rebootRequired](Dependency dependency)
{
if (SUCCEEDED(hr) || force)
{
auto featureName = dependency.Id();
AICLI_LOG(Core, Verbose, << "Processing Windows Feature dependency [" << featureName << "]");
WindowsFeature::WindowsFeature windowsFeature = dismHelper->GetWindowsFeature(featureName);

if (windowsFeature.DoesExist())
{
if (!windowsFeature.IsEnabled())
{
Utility::LocIndString featureDisplayName = windowsFeature.GetDisplayName();
Utility::LocIndView locIndFeatureName{ featureName };

context.Reporter.Info() << Resource::String::EnablingWindowsFeature(featureDisplayName, locIndFeatureName) << std::endl;

AICLI_LOG(Core, Info, << "Enabling Windows Feature [" << featureName << "] returned with HRESULT: " << hr);
auto enableFeatureFunction = [&](IProgressCallback& progress)->HRESULT { return windowsFeature.Enable(progress); };
hr = context.Reporter.ExecuteWithProgress(enableFeatureFunction, true);

if (FAILED(hr))
{
AICLI_LOG(Core, Error, << "Failed to enable Windows Feature " << featureDisplayName << " [" << locIndFeatureName << "] with exit code: " << hr);
context.Reporter.Warn() << Resource::String::FailedToEnableWindowsFeature(featureDisplayName, locIndFeatureName) << std::endl
<< GetUserPresentableMessage(hr) << std::endl;
}

if (hr == ERROR_SUCCESS_REBOOT_REQUIRED || windowsFeature.GetRestartRequiredStatus() == DismRestartType::DismRestartRequired)
{
rebootRequired = true;
}
}
}
else
{
// Note: If a feature is not found, continue enabling the rest of the dependencies but block immediately after unless force arg is present.
AICLI_LOG(Core, Info, << "Windows Feature [" << featureName << "] does not exist");
hr = APPINSTALLER_CLI_ERROR_INSTALL_MISSING_DEPENDENCY;
context.Reporter.Warn() << Resource::String::WindowsFeatureNotFound(Utility::LocIndView{ featureName }) << std::endl;
}
}
});

if (FAILED(hr))
{
if (force)
{
context.Reporter.Warn() << Resource::String::FailedToEnableWindowsFeatureOverridden << std::endl;
}
else
{
context.Reporter.Error() << Resource::String::FailedToEnableWindowsFeatureOverrideRequired << std::endl;
AICLI_TERMINATE_CONTEXT(hr);
}
}
else if (rebootRequired)
{
if (force)
{
context.Reporter.Warn() << Resource::String::RebootRequiredToEnableWindowsFeatureOverridden << std::endl;
}
else
{
context.Reporter.Error() << Resource::String::RebootRequiredToEnableWindowsFeatureOverrideRequired << std::endl;
AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL);
}
}
}
context <<
Workflow::EnsureRunningAsAdmin <<
Workflow::ShellExecuteEnableWindowsFeatures;
}

void CreateDependencySubContexts::operator()(Execution::Context& context) const
Expand Down
181 changes: 178 additions & 3 deletions src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ namespace AppInstaller::CLI::Workflow
namespace
{
// ShellExecutes the given path.
std::optional<DWORD> InvokeShellExecuteEx(const std::filesystem::path& filePath, const std::string& args, bool useRunAs, IProgressCallback& progress)
std::optional<DWORD> InvokeShellExecuteEx(const std::filesystem::path& filePath, const std::string& args, bool useRunAs, int show, IProgressCallback& progress)
{
AICLI_LOG(CLI, Info, << "Starting: '" << filePath.u8string() << "' with arguments '" << args << '\'');

Expand All @@ -27,7 +27,7 @@ namespace AppInstaller::CLI::Workflow
execInfo.lpParameters = argsUtf16.c_str();
// Some installers force UI. Setting to SW_HIDE will hide installer UI and installation will never complete.
// Verified setting to SW_SHOW does not hurt silent mode since no UI will be shown.
ryfu-msft marked this conversation as resolved.
Show resolved Hide resolved
execInfo.nShow = SW_SHOW;
execInfo.nShow = show;

// This installer must be run elevated, but we are not currently.
// Have ShellExecute elevate the installer since it won't do so itself.
Expand Down Expand Up @@ -68,7 +68,7 @@ namespace AppInstaller::CLI::Workflow

std::optional<DWORD> InvokeShellExecute(const std::filesystem::path& filePath, const std::string& args, IProgressCallback& progress)
{
return InvokeShellExecuteEx(filePath, args, false, progress);
return InvokeShellExecuteEx(filePath, args, false, SW_SHOW, progress);
}

// Gets the escaped installer args.
Expand Down Expand Up @@ -201,6 +201,45 @@ namespace AppInstaller::CLI::Workflow

return args;
}

std::filesystem::path GetDismExecutablePath()
{
return { ExpandEnvironmentVariables(L"%windir%\\system32\\dism.exe") };
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: there is a GetExpandedPath function in winget/Filesystem.cpp which is specifically created for getting an expanded path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to calling GetExpandedPath from Filesystem.h

}

std::optional<DWORD> DoesWindowsFeatureExist(Execution::Context& context, const std::string& featureName)
{
std::string args = "/Online /Get-FeatureInfo /FeatureName:" + featureName;
const auto& dismExecPath = GetDismExecutablePath();
ryfu-msft marked this conversation as resolved.
Show resolved Hide resolved

auto getFeatureInfoResult = context.Reporter.ExecuteWithProgress(
std::bind(InvokeShellExecuteEx,
dismExecPath,
args,
false,
SW_HIDE,
std::placeholders::_1));

return getFeatureInfoResult;
}

std::optional<DWORD> EnableWindowsFeature(Execution::Context& context, const std::string& featureName)
{
std::string args = "/Online /Enable-Feature /NoRestart /FeatureName:" + featureName;
const auto& dismExecPath = GetDismExecutablePath();
ryfu-msft marked this conversation as resolved.
Show resolved Hide resolved

AICLI_LOG(Core, Info, << "Enabling Windows Feature [" << featureName << "]");

auto enableFeatureResult = context.Reporter.ExecuteWithProgress(
std::bind(InvokeShellExecuteEx,
dismExecPath,
args,
false,
SW_HIDE,
std::placeholders::_1));

return enableFeatureResult;
}
}

void ShellExecuteInstallImpl(Execution::Context& context)
Expand All @@ -226,6 +265,7 @@ namespace AppInstaller::CLI::Workflow
context.Get<Execution::Data::InstallerPath>(),
installerArgs,
installer->ElevationRequirement == ElevationRequirementEnum::ElevationRequired && !isElevated,
SW_SHOW,
std::placeholders::_1));

if (!installResult)
Expand Down Expand Up @@ -307,7 +347,142 @@ namespace AppInstaller::CLI::Workflow
else
{
context.Add<Execution::Data::OperationReturnCode>(uninstallResult.value());
}
}
}

#ifndef AICLI_DISABLE_TEST_HOOKS
std::optional<DWORD> s_EnableWindowsFeatureResult_Override{};
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should we use the pointer pattern for this one and below like other test hooks so the codes have consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is another test hook that also passes an optional value like so:
void TestHook_SetPinningIndex_Override(std::optional<std::filesystem::path>&& indexPath);

Since I am also passing an optional DWORD value, I just did something similar to that.


void TestHook_SetEnableWindowsFeatureResult_Override(std::optional<DWORD>&& result)
{
s_EnableWindowsFeatureResult_Override = std::move(result);
}

std::optional<DWORD> s_DoesWindowsFeatureExistResult_Override{};

void TestHook_SetDoesWindowsFeatureExistResult_Override(std::optional<DWORD>&& result)
{
s_DoesWindowsFeatureExistResult_Override = std::move(result);
}
#endif

void ShellExecuteEnableWindowsFeatures(Execution::Context& context)
{
if (!Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::WindowsFeature))
{
return;
}

const auto& rootDependencies = context.Get<Execution::Data::Installer>()->Dependencies;
DWORD result = 0;
bool force = context.Args.Contains(Execution::Args::Type::Force);
bool rebootRequired = false;

rootDependencies.ApplyToType(DependencyType::WindowsFeature, [&context, &result, &force, &rebootRequired](Dependency dependency)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep this ApplyToType part in DependenciesFLow.cpp so all dependencies related logic are in that flow. ShellExecuteInstallerHandler.cpp will only expose EnableWindowsFeature flow for enabling only 1 feature, so in the future if we are to implement winget features --enable --disable, we can reuse the same code.

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 moved the ApplyToType part back to the DependenciesFlow as suggested. I created a separate workflow just for enabling a single feature that way it can be reused in the future.

{
if (FAILED(result) && !force || context.IsTerminated())
Copy link
Contributor

Choose a reason for hiding this comment

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

result does not seem to be an HResult, we would not use FAILED to check non hresult type

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 replaced the FAILED(result) check with a boolean value that is only set to true if we encounter an error. Since there are only a few error codes that we are looking for that change our output behavior to the user, I felt that this was an easier approach rather than checking for failed exit codes.

{
return;
}

auto featureName = dependency.Id();
Utility::LocIndView locIndFeatureName{ featureName };

// Check if Windows Feature exists.
#ifndef AICLI_DISABLE_TEST_HOOKS
auto doesFeatureExistResult = s_DoesWindowsFeatureExistResult_Override.has_value() ?
s_DoesWindowsFeatureExistResult_Override.value() :
DoesWindowsFeatureExist(context, featureName);
#else
auto doesFeatureExistResult = DoesWindowsFeatureExist(context, featureName);
#endif

if (!doesFeatureExistResult)
{
// User aborted the operation.
AICLI_TERMINATE_CONTEXT(E_ABORT);
}

auto doesFeatureExistExitCode = doesFeatureExistResult.value();
if (doesFeatureExistExitCode != ERROR_SUCCESS)
{
AICLI_LOG(Core, Info, << "Windows Feature [" << featureName << "] does not exist");
ryfu-msft marked this conversation as resolved.
Show resolved Hide resolved
context.Reporter.Warn() << Resource::String::WindowsFeatureNotFound(locIndFeatureName) << std::endl;
result = doesFeatureExistExitCode;
return;
}

// Enable Windows Feature.
context.Reporter.Info() << Resource::String::EnablingWindowsFeature(locIndFeatureName) << std::endl;

#ifndef AICLI_DISABLE_TEST_HOOKS
auto enableFeatureResult = s_EnableWindowsFeatureResult_Override.has_value() ?
s_EnableWindowsFeatureResult_Override.value() :
EnableWindowsFeature(context, featureName);
#else
auto enableFeatureResult = EnableWindowsFeature(context, featureName);
#endif
if (!enableFeatureResult)
{
// User aborted the operation.
AICLI_TERMINATE_CONTEXT(E_ABORT);
}

auto enableFeatureExitCode = enableFeatureResult.value();
if (enableFeatureExitCode == ERROR_SUCCESS_REBOOT_REQUIRED)
{
AICLI_LOG(Core, Info, << "Reboot required for [" << featureName << "] with exit code: " << result);
ryfu-msft marked this conversation as resolved.
Show resolved Hide resolved
rebootRequired = true;
}
else if (FAILED(enableFeatureExitCode))
{
AICLI_LOG(Core, Error, << "Failed to enable Windows Feature [" << featureName << "] with exit code: " << enableFeatureExitCode);
context.Reporter.Warn() << Resource::String::FailedToEnableWindowsFeature(locIndFeatureName) << std::endl
<< GetUserPresentableMessage(enableFeatureExitCode) << std::endl;
result = enableFeatureExitCode;
}
else
{
AICLI_LOG(Core, Info, << "Successfully enabled [" << featureName << "] with exit code: " << result);
ryfu-msft marked this conversation as resolved.
Show resolved Hide resolved
}
});

if (context.IsTerminated())
{
// This will only occur if the user aborts when finding or enabling a feature.
return;
}

if (FAILED(result))
{
if (force)
{
context.Reporter.Warn() << Resource::String::FailedToEnableWindowsFeatureOverridden << std::endl;
}
else
{
context.Reporter.Error() << Resource::String::FailedToEnableWindowsFeatureOverrideRequired << std::endl;
AICLI_TERMINATE_CONTEXT(HRESULT_FROM_WIN32(result));
}
}
else
{
if (rebootRequired)
{
if (force)
{
context.Reporter.Warn() << Resource::String::RebootRequiredToEnableWindowsFeatureOverridden << std::endl;
}
else
{
context.Reporter.Error() << Resource::String::RebootRequiredToEnableWindowsFeatureOverrideRequired << std::endl;
AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL);
}
}
else
{
context.Reporter.Info() << Resource::String::EnableWindowsFeaturesSuccess << std::endl;
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,10 @@ namespace AppInstaller::CLI::Workflow
// Inputs: Manifest?, Installer, InstallerPath
// Outputs: InstallerArgs
void GetInstallerArgs(Execution::Context& context);

// Enables the Windows Features dependencies by invoking ShellExecute on the dism executable.
// Required Args: None
// Inputs: Dependencies
// Output: None
void ShellExecuteEnableWindowsFeatures(Execution::Context& context);
}
1 change: 1 addition & 0 deletions src/AppInstallerCLIE2ETests/Constants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ public class ErrorCode
public const int OPC_E_ZIP_MISSING_END_OF_CENTRAL_DIRECTORY = unchecked((int)0x8051100F);
public const int ERROR_OLD_WIN_VERSION = unchecked((int)0x8007047E);
public const int HTTP_E_STATUS_NOT_FOUND = unchecked((int)0x80190194);
public const int DISMAPI_E_UNKNOWN_FEATURE = unchecked((int)0x800f080c);

// AICLI custom HRESULTs
public const int ERROR_INTERNAL_ERROR = unchecked((int)0x8A150001);
Expand Down
Loading