Add experimental feature support for enabling Windows Feature dependencies #3005
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,7 +66,7 @@ namespace AppInstaller | |
| void TestHook_SetEnableWindowsFeatureResult_Override(HRESULT* result); | ||
| void TestHook_SetIsWindowsFeatureEnabledResult_Override(bool* status); | ||
| void TestHook_SetDoesWindowsFeatureExistResult_Override(bool* status); | ||
| void TestHook_SetWindowsFeatureGetDisplayNameResult_Override(Utility::LocIndString* displayName); | ||
| void TestHook_SetWindowsFeatureGetRestartStatusResult_Override(AppInstaller::WindowsFeature::DismRestartType* restartType); | ||
| } | ||
| } | ||
|
|
@@ -181,7 +181,7 @@ namespace TestHook | |
|
|
||
| struct SetWindowsFeatureGetDisplayNameResult_Override | ||
| { | ||
| SetWindowsFeatureGetDisplayNameResult_Override(AppInstaller::Utility::LocIndString displayName) : m_displayName(displayName) | ||
| { | ||
| AppInstaller::WindowsFeature::TestHook_SetWindowsFeatureGetDisplayNameResult_Override(&m_displayName); | ||
| } | ||
|
|
@@ -192,7 +192,7 @@ namespace TestHook | |
| } | ||
|
|
||
| private: | ||
| AppInstaller::Utility::LocIndString m_displayName; | ||
| }; | ||
|
|
||
| struct SetWindowsFeatureGetRestartStatusResult_Override | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,6 @@ TEST_CASE("InstallFlow_WindowsFeatureDoesNotExist", "[windowsFeature]") | |
| std::ostringstream installOutput; | ||
| TestContext context{ installOutput, std::cin }; | ||
| auto previousThreadGlobals = context.SetForCurrentThread(); | ||
| context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string()); | ||
|
|
||
| auto mockDismHelperOverride = TestHook::MockDismHelper_Override(); | ||
|
|
@@ -65,16 +64,15 @@ TEST_CASE("InstallFlow_FailedToEnableWindowsFeature", "[windowsFeature]") | |
| std::ostringstream installOutput; | ||
| TestContext context{ installOutput, std::cin }; | ||
| auto previousThreadGlobals = context.SetForCurrentThread(); | ||
| context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string()); | ||
|
|
||
| // Override with arbitrary DISM api error (DISMAPI_E_DISMAPI_NOT_INITIALIZED) and make windows feature discoverable. | ||
| HRESULT dismErrorResult = 0xc0040001; | ||
| auto mockDismHelperOverride = TestHook::MockDismHelper_Override(); | ||
| auto doesFeatureExistOverride = TestHook::SetDoesWindowsFeatureExistResult_Override(true); | ||
| auto setIsFeatureEnabledOverride = TestHook::SetIsWindowsFeatureEnabledResult_Override(false); | ||
| auto setEnableFeatureOverride = TestHook::SetEnableWindowsFeatureResult_Override(dismErrorResult); | ||
| auto getDisplayNameOverride = TestHook::SetWindowsFeatureGetDisplayNameResult_Override(LocIndString{ "Test Windows Feature"_liv }); | ||
| auto getRestartStatusOverride = TestHook::SetWindowsFeatureGetRestartStatusResult_Override(DismRestartNo); | ||
|
|
||
| InstallCommand install({}); | ||
|
|
@@ -105,7 +103,7 @@ TEST_CASE("InstallFlow_FailedToEnableWindowsFeature_Force", "[windowsFeature]") | |
| auto setEnableFeatureOverride = TestHook::SetEnableWindowsFeatureResult_Override(dismErrorResult); | ||
| auto doesFeatureExistOverride = TestHook::SetDoesWindowsFeatureExistResult_Override(true); | ||
| auto setIsFeatureEnabledOverride = TestHook::SetIsWindowsFeatureEnabledResult_Override(false); | ||
| auto getDisplayNameOverride = TestHook::SetWindowsFeatureGetDisplayNameResult_Override(LocIndString{ "Test Windows Feature"_liv }); | ||
| auto getRestartStatusOverride = TestHook::SetWindowsFeatureGetRestartStatusResult_Override(DismRestartNo); | ||
|
|
||
| std::ostringstream installOutput; | ||
|
|
@@ -121,8 +119,8 @@ TEST_CASE("InstallFlow_FailedToEnableWindowsFeature_Force", "[windowsFeature]") | |
|
|
||
| // Verify Installer is called and parameters are passed in. | ||
| REQUIRE(context.GetTerminationHR() == ERROR_SUCCESS); | ||
| REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeature(LocIndView{ "testFeature1" }, dismErrorResult)).get()) != std::string::npos); | ||
| REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeature(LocIndView{ "testFeature2" }, dismErrorResult)).get()) != std::string::npos); | ||
| REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeatureOverridden).get()) != std::string::npos); | ||
| REQUIRE(std::filesystem::exists(installResultPath.GetPath())); | ||
| std::ifstream installResultFile(installResultPath.GetPath()); | ||
|
|
@@ -151,13 +149,12 @@ TEST_CASE("InstallFlow_RebootRequired", "[windowsFeature]") | |
| auto setEnableFeatureOverride = TestHook::SetEnableWindowsFeatureResult_Override(ERROR_SUCCESS_REBOOT_REQUIRED); | ||
| auto setIsFeatureEnabledOverride = TestHook::SetIsWindowsFeatureEnabledResult_Override (false); | ||
| auto doesFeatureExistOverride = TestHook::SetDoesWindowsFeatureExistResult_Override(true); | ||
| auto getDisplayNameOverride = TestHook::SetWindowsFeatureGetDisplayNameResult_Override(LocIndString{ "Test Windows Feature"_liv }); | ||
| auto getRestartStatusOverride = TestHook::SetWindowsFeatureGetRestartStatusResult_Override(DismRestartRequired); | ||
|
|
||
| std::ostringstream installOutput; | ||
| TestContext context{ installOutput, std::cin }; | ||
| auto previousThreadGlobals = context.SetForCurrentThread(); | ||
| context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string()); | ||
|
|
||
| InstallCommand install({}); | ||
|
|
@@ -187,7 +184,7 @@ TEST_CASE("InstallFlow_RebootRequired_Force", "[windowsFeature]") | |
| auto setEnableFeatureOverride = TestHook::SetEnableWindowsFeatureResult_Override(ERROR_SUCCESS_REBOOT_REQUIRED); | ||
| auto setIsFeatureEnabledOverride = TestHook::SetIsWindowsFeatureEnabledResult_Override(false); | ||
| auto doesFeatureExistOverride = TestHook::SetDoesWindowsFeatureExistResult_Override(true); | ||
| auto getDisplayNameOverride = TestHook::SetWindowsFeatureGetDisplayNameResult_Override(LocIndString{ "Test Windows Feature"_liv }); | ||
| auto getRestartStatusOverride = TestHook::SetWindowsFeatureGetRestartStatusResult_Override(DismRestartRequired); | ||
|
|
||
| std::ostringstream installOutput; | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -137,6 +137,7 @@ namespace AppInstaller::Logging | |
| } | ||
| else | ||
| { | ||
| AICLI_LOG(Core, Error, << "Failed to open log file"); | ||
ryfu-msft marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| throw std::system_error(fopenError, std::generic_category()); | ||
ryfu-msft marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
| #pragma once | ||
| #include <AppInstallerProgress.h> | ||
| #include <winget/LocIndependent.h> | ||
|
|
||
| namespace AppInstaller::WindowsFeature | ||
| { | ||
|
|
@@ -113,69 +115,17 @@ namespace AppInstaller::WindowsFeature | |
| using DismDisableFeaturePtr = HRESULT(WINAPI*)(UINT, PCWSTR, PCWSTR, BOOL, HANDLE, DISM_PROGRESS_CALLBACK, PVOID); | ||
| using DismDeletePtr = HRESULT(WINAPI*)(VOID*); | ||
|
|
||
| // forward declaration | ||
| struct WindowsFeature; | ||
|
|
||
| struct DismHelper | ||
| { | ||
| DismHelper(); | ||
|
|
||
| ~DismHelper(); | ||
|
|
||
| private: | ||
| friend WindowsFeature; | ||
|
There was a problem hiding this comment. This doesn't look like it needs to be a friend if this object simply exposed public methods for calling into the function pointers. There was a problem hiding this comment. removed and exposed public functions |
||
| typedef UINT DismSession; | ||
|
|
||
| wil::unique_hmodule m_module; | ||
|
|
@@ -194,5 +144,46 @@ namespace AppInstaller::WindowsFeature | |
| void OpenSession(); | ||
| void CloseSession(); | ||
| void Shutdown(); | ||
|
|
||
| template<typename FuncType> | ||
| FuncType GetProcAddressHelper(HMODULE module, LPCSTR functionName); | ||
| }; | ||
|
|
||
| /// <summary> | ||
| /// Struct representation of a single Windows Feature. | ||
| /// </summary> | ||
| struct WindowsFeature | ||
| { | ||
| // This default constructor is only used for mocking unit tests. | ||
| WindowsFeature() = default; | ||
| WindowsFeature(std::shared_ptr<DismHelper> dismHelper, const std::string& name); | ||
|
There was a problem hiding this comment. nit: I find this awkward and would have a helper on There was a problem hiding this comment. I left the constructor as is to be able to get a shared_ptr to DismHelper. I changed DismHelper to derive from There was a problem hiding this comment. The awkward part is the call site; the need to pass the So that the call site was less awkward, like: If you did make the constructor protected, you then don't have to worry about the There was a problem hiding this comment. 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. |
||
|
|
||
| HRESULT Enable(IProgressCallback& progress); | ||
ryfu-msft marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| HRESULT Disable(); | ||
| bool DoesExist(); | ||
| bool IsEnabled(); | ||
| Utility::LocIndString GetDisplayName(); | ||
| DismRestartType GetRestartRequiredStatus(); | ||
|
|
||
| DismPackageFeatureState GetState() | ||
| { | ||
| return m_featureInfo->FeatureState; | ||
| } | ||
|
|
||
| ~WindowsFeature() | ||
| { | ||
| if (m_featureInfo) | ||
| { | ||
| m_dismHelper->m_dismDelete(m_featureInfo); | ||
| } | ||
| } | ||
|
|
||
| private: | ||
| void GetFeatureInfo(); | ||
|
|
||
| std::string m_featureName; | ||
| std::shared_ptr<DismHelper> m_dismHelper; | ||
| DismFeatureInfo* m_featureInfo = nullptr; | ||
| DismSession m_session = DISM_SESSION_DEFAULT; | ||
| }; | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
--forceby running the command twice.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
--forceto proceed.