Invoke ShellExecute on dism.exe for enabling Windows Features #3659
Conversation
|
I disabled the tests Please re-enable them in your branch. |
|
|
||
| std::filesystem::path GetDismExecutablePath() | ||
| { | ||
| return { ExpandEnvironmentVariables(L"%windir%\\system32\\dism.exe") }; |
There was a problem hiding this comment.
nit: there is a GetExpandedPath function in winget/Filesystem.cpp which is specifically created for getting an expanded path
There was a problem hiding this comment.
Changed to calling GetExpandedPath from Filesystem.h
src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp
Outdated
Show resolved
Hide resolved
src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp
Outdated
Show resolved
Hide resolved
src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| #ifndef AICLI_DISABLE_TEST_HOOKS | ||
| std::optional<DWORD> s_EnableWindowsFeatureResult_Override{}; |
There was a problem hiding this comment.
nit: should we use the pointer pattern for this one and below like other test hooks so the codes have consistency?
There was a problem hiding this comment.
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.
|
|
||
| rootDependencies.ApplyToType(DependencyType::WindowsFeature, [&context, &result, &force, &rebootRequired](Dependency dependency) | ||
| { | ||
| if (FAILED(result) && !force || context.IsTerminated()) |
There was a problem hiding this comment.
result does not seem to be an HResult, we would not use FAILED to check non hresult type
There was a problem hiding this comment.
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.
src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp
Outdated
Show resolved
Hide resolved
| bool force = context.Args.Contains(Execution::Args::Type::Force); | ||
| bool rebootRequired = false; | ||
|
|
||
| rootDependencies.ApplyToType(DependencyType::WindowsFeature, [&context, &result, &force, &rebootRequired](Dependency dependency) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp
Outdated
Show resolved
Hide resolved
src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp
Outdated
Show resolved
Hide resolved
src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.h
Outdated
Show resolved
Hide resolved
Changes:
Microsoft Reviewers: Open in CodeFlow