From a6ec947fadff7562913866a6ad48a48e0b68d59b Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Mon, 13 Feb 2023 11:03:43 -0800 Subject: [PATCH 01/29] add project files --- .../AppInstallerCommonCore.vcxproj | 2 ++ .../AppInstallerCommonCore.vcxproj.filters | 6 ++++ .../Public/winget/WindowsFeature.h | 14 +++++++++ src/AppInstallerCommonCore/WindowsFeature.cpp | 29 +++++++++++++++++++ 4 files changed, 51 insertions(+) create mode 100644 src/AppInstallerCommonCore/Public/winget/WindowsFeature.h create mode 100644 src/AppInstallerCommonCore/WindowsFeature.cpp diff --git a/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj b/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj index 7ab3ba750c..eecdf6ab17 100644 --- a/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj +++ b/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj @@ -329,6 +329,7 @@ + @@ -398,6 +399,7 @@ + diff --git a/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj.filters b/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj.filters index 963f635427..7f467fe178 100644 --- a/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj.filters +++ b/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj.filters @@ -192,6 +192,9 @@ Public\winget + + Public\winget + @@ -341,6 +344,9 @@ Source Files + + Source Files + diff --git a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h new file mode 100644 index 0000000000..1efa2240f8 --- /dev/null +++ b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h @@ -0,0 +1,14 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +#pragma once + +namespace AppInstaller::WindowsFeature +{ + bool EnableWindowsFeature(const std::string& name); + + bool DisableWindowsFeature(const std::string& name); + + bool IsWindowsFeatureEnabled(const std::string& name); + + bool DoesWindowsFeatureExist(const std::string& name); +} \ No newline at end of file diff --git a/src/AppInstallerCommonCore/WindowsFeature.cpp b/src/AppInstallerCommonCore/WindowsFeature.cpp new file mode 100644 index 0000000000..dcefc44606 --- /dev/null +++ b/src/AppInstallerCommonCore/WindowsFeature.cpp @@ -0,0 +1,29 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +#include "pch.h" +#include "winget/WindowsFeature.h" +#include "Public/AppInstallerLogging.h" + +namespace AppInstaller::WindowsFeatures +{ + namespace + { + struct WindowsFeatureHelper + { + WindowsFeatureHelper() + { + m_module.reset(LoadLibraryEx(L"dismapi.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32)); + if (!m_module) + { + AICLI_LOG(Core, Verbose, << "Could not load dismapi.dll"); + return; + } + + + + } + private: + wil::unique_hmodule m_module; + }; + } +} \ No newline at end of file From cea7e0dae095f3732edb7d2d8bb10acf61ea2b2e Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Wed, 15 Feb 2023 12:56:36 -0800 Subject: [PATCH 02/29] save work --- .../Workflows/DependenciesFlow.cpp | 58 +++ .../Workflows/DependenciesFlow.h | 2 + .../Workflows/InstallFlow.cpp | 1 + .../AppInstallerCLITests.vcxproj | 1 + .../AppInstallerCLITests.vcxproj.filters | 3 + src/AppInstallerCLITests/WindowsFeature.cpp | 55 +++ .../Public/winget/WindowsFeature.h | 367 +++++++++++++++++- src/AppInstallerCommonCore/WindowsFeature.cpp | 214 +++++++++- 8 files changed, 682 insertions(+), 19 deletions(-) create mode 100644 src/AppInstallerCLITests/WindowsFeature.cpp diff --git a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp index 2acc0a40a3..3fe41259a3 100644 --- a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp @@ -6,10 +6,12 @@ #include "ManifestComparator.h" #include "InstallFlow.h" #include "winget\DependenciesGraph.h" +#include "winget\WindowsFeature.h" #include "DependencyNodeProcessor.h" using namespace AppInstaller::Repository; using namespace AppInstaller::Manifest; +using namespace AppInstaller::WindowsFeature; namespace AppInstaller::CLI::Workflow { @@ -133,6 +135,62 @@ namespace AppInstaller::CLI::Workflow } } + void EnableWindowsFeaturesDependencies(Execution::Context& context) + { + if (!Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::Dependencies)) + { + return; + } + + // obtain windows features. + // Don't forget to check the root installer node but for now we'll just look at the individual installer root. + + const auto& rootInstaller = context.Get(); + const auto& rootDependencies = rootInstaller->Dependencies; + + if (rootDependencies.Empty()) + { + // If there's no dependencies there's nothing to do aside of logging the outcome + return; + } + + if (rootDependencies.HasAnyOf(DependencyType::WindowsFeature)) + { + context << Workflow::EnsureRunningAsAdmin; + + // May have to refactor out the logic into a separate function + bool errorMessageDisplayed = false; + bool allFeaturesExist = true; + auto error = context.Reporter.Error(); + rootDependencies.ApplyToType(DependencyType::WindowsFeature, [&error, &allFeaturesExist, &errorMessageDisplayed](Dependency dependency) + { + WindowsFeature::WindowsFeature feature{ dependency.Id() }; + if (!feature.DoesFeatureExist()) + { + if (!errorMessageDisplayed) + { + error << "The following dependencies were not found:" << std::endl; + error << " - " << Resource::String::WindowsFeaturesDependencies << std::endl; + errorMessageDisplayed = true; + } + + allFeaturesExist = false; + error << " " << dependency.Id() << std::endl; + } + }); + + if (!allFeaturesExist && context.Args.Contains(Execution::Args::Type::Force)) + { + std::cout << "proceeding due to --force." << std::endl; + } + + //rootDependencies.ApplyToType(DependencyType::WindowsFeature, [&](Dependency dependency) + //{ + // AppInstaller::WindowsFeature::EnableWindowsFeature(dependency.Id()); + //}); + } + } + void ManagePackageDependencies::operator()(Execution::Context& context) const { if (!Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::Dependencies)) diff --git a/src/AppInstallerCLICore/Workflows/DependenciesFlow.h b/src/AppInstallerCLICore/Workflows/DependenciesFlow.h index f6ffa00a19..a55df0c6cc 100644 --- a/src/AppInstallerCLICore/Workflows/DependenciesFlow.h +++ b/src/AppInstallerCLICore/Workflows/DependenciesFlow.h @@ -59,4 +59,6 @@ namespace AppInstaller::CLI::Workflow // Inputs: PackageVersion, Manifest // Outputs: DependencySource void OpenDependencySource(Execution::Context& context); + + void EnableWindowsFeaturesDependencies(Execution::Context& context); } \ No newline at end of file diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index 6a1ce0d66f..01efc8b099 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/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 << Workflow::DownloadInstaller; } diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj index b7fb1bed81..2b456f0364 100644 --- a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj +++ b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj @@ -248,6 +248,7 @@ + diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters index 140f4de563..26123276cd 100644 --- a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters +++ b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters @@ -284,6 +284,9 @@ Source Files\CLI + + Source Files\CLI + diff --git a/src/AppInstallerCLITests/WindowsFeature.cpp b/src/AppInstallerCLITests/WindowsFeature.cpp new file mode 100644 index 0000000000..0c62fef7aa --- /dev/null +++ b/src/AppInstallerCLITests/WindowsFeature.cpp @@ -0,0 +1,55 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +#include "pch.h" +#include "TestCommon.h" +#include +#include + +using namespace AppInstaller::WindowsFeature; +using namespace TestCommon; + +// IMPORTANT: These tests require elevation and will modify your Windows Feature settings. + +const std::string s_featureName = "netfx3"; + +TEST_CASE("GetWindowsFeature", "[windowsFeature]") +{ + if (!AppInstaller::Runtime::IsRunningAsAdmin()) + { + WARN("Test requires admin privilege. Skipped."); + return; + } + + WindowsFeature validFeature{ s_featureName }; + REQUIRE(validFeature.DoesFeatureExist()); + + WindowsFeature invalidFeature{ "invalidFeature" }; + REQUIRE_FALSE(invalidFeature.DoesFeatureExist()); +} + +TEST_CASE("DisableEnableWindowsFeature", "[windowsFeature]") +{ + if (!AppInstaller::Runtime::IsRunningAsAdmin()) + { + WARN("Test requires admin privilege. Skipped."); + return; + } + + WindowsFeature feature{ s_featureName }; + REQUIRE(feature.DisableFeature()); + REQUIRE_FALSE(feature.IsEnabled()); + + REQUIRE(feature.EnableFeature()); + REQUIRE(feature.IsEnabled()); +} + +// Start tests with manifests +// manifest with valid feature +// manifest 2 with 1 valid and 1 invalid feature + + +// Verify behavior of 1 valid should succeed. +// Verify behavior of 2 should fail due to invalid feature +// Verify behavior of 2 should continue if --force is passed +// Verify behavior of 2 with no interactive should error out +// diff --git a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h index 1efa2240f8..b922f6c11c 100644 --- a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h +++ b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h @@ -4,11 +4,370 @@ namespace AppInstaller::WindowsFeature { - bool EnableWindowsFeature(const std::string& name); +/****************************************************************************\ - bool DisableWindowsFeature(const std::string& name); + DismApi.H - bool IsWindowsFeatureEnabled(const std::string& name); + Copyright (c) Microsoft Corporation. + All rights reserved. - bool DoesWindowsFeatureExist(const std::string& name); +\****************************************************************************/ + +#ifndef _DISMAPI_H_ +#define _DISMAPI_H_ + +#include + +#pragma region Desktop Family or DISM Package +#if WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP | WINAPI_PARTITION_PKG_DISM) + +#ifdef __cplusplus + extern "C" + { +#endif + + ////////////////////////////////////////////////////////////////////////////// + // + // Typedefs + // + ////////////////////////////////////////////////////////////////////////////// + + typedef UINT DismSession; + + ////////////////////////////////////////////////////////////////////////////// + // + // Callbacks + // + ////////////////////////////////////////////////////////////////////////////// + + typedef void(CALLBACK* DISM_PROGRESS_CALLBACK)(_In_ UINT Current, _In_ UINT Total, _In_opt_ PVOID UserData); + + ////////////////////////////////////////////////////////////////////////////// + // + // Constants + // + ////////////////////////////////////////////////////////////////////////////// + +#define DISM_ONLINE_IMAGE L"DISM_{53BFAE52-B167-4E2F-A258-0A37B57FF845}" +#define DISM_SESSION_DEFAULT 0 + + ////////////////////////////////////////////////////////////////////////////// + // + // Enums + // + ////////////////////////////////////////////////////////////////////////////// + + typedef enum _DismLogLevel + { + DismLogErrors = 0, + DismLogErrorsWarnings, + DismLogErrorsWarningsInfo, + DismLogErrorsWarningsInfoDebug + } DismLogLevel; + + typedef enum _DismPackageIdentifier + { + DismPackageNone = 0, + DismPackageName, + DismPackagePath + } DismPackageIdentifier; + + typedef enum _DismPackageFeatureState + { + DismStateNotPresent = 0, + DismStateUninstallPending, + DismStateStaged, + DismStateResolved, // For internal use only + DismStateRemoved = DismStateResolved, + DismStateInstalled, + DismStateInstallPending, + DismStateSuperseded, + DismStatePartiallyInstalled + } DismPackageFeatureState; + + typedef enum _DismRestartType + { + DismRestartNo = 0, + DismRestartPossible, + DismRestartRequired + } DismRestartType; + + ////////////////////////////////////////////////////////////////////////////// + // + // Structs + // + ////////////////////////////////////////////////////////////////////////////// +#pragma pack(push, 1) + + typedef struct _DismCustomProperty + { + PCWSTR Name; + PCWSTR Value; + PCWSTR Path; + } DismCustomProperty; + + typedef struct _DismFeature + { + PCWSTR FeatureName; + DismPackageFeatureState State; + } DismFeature; + + typedef struct _DismFeatureInfo + { + PCWSTR FeatureName; + DismPackageFeatureState FeatureState; + PCWSTR DisplayName; + PCWSTR Description; + DismRestartType RestartRequired; + DismCustomProperty* CustomProperty; + UINT CustomPropertyCount; + } DismFeatureInfo; + +#pragma pack(pop) + + ////////////////////////////////////////////////////////////////////////////// + // + // Success Codes + // + ////////////////////////////////////////////////////////////////////////////// + + // For online scenario, computer needs to be restarted when the return value is ERROR_SUCCESS_REBOOT_REQUIRED (3010L). + + // + // MessageId: DISMAPI_S_RELOAD_IMAGE_SESSION_REQUIRED + // + // MessageText: + // + // The DISM session needs to be reloaded. + // +#define DISMAPI_S_RELOAD_IMAGE_SESSION_REQUIRED 0x00000001 + +////////////////////////////////////////////////////////////////////////////// +// +// Error Codes +// +////////////////////////////////////////////////////////////////////////////// + +// +// MessageId: DISMAPI_E_DISMAPI_NOT_INITIALIZED +// +// MessageText: +// +// DISM API was not initialized for this process +// +#define DISMAPI_E_DISMAPI_NOT_INITIALIZED 0xC0040001 + +// +// MessageId: DISMAPI_E_SHUTDOWN_IN_PROGRESS +// +// MessageText: +// +// A DismSession was being shutdown when another operation was called on it +// +#define DISMAPI_E_SHUTDOWN_IN_PROGRESS 0xC0040002 + +// +// MessageId: DISMAPI_E_OPEN_SESSION_HANDLES +// +// MessageText: +// +// A DismShutdown was called while there were open DismSession handles +// +#define DISMAPI_E_OPEN_SESSION_HANDLES 0xC0040003 + +// +// MessageId: DISMAPI_E_INVALID_DISM_SESSION +// +// MessageText: +// +// An invalid DismSession handle was passed into a DISMAPI function +// +#define DISMAPI_E_INVALID_DISM_SESSION 0xC0040004 + +// +// MessageId: DISMAPI_E_INVALID_IMAGE_INDEX +// +// MessageText: +// +// An invalid image index was specified +// +#define DISMAPI_E_INVALID_IMAGE_INDEX 0xC0040005 + +// +// MessageId: DISMAPI_E_INVALID_IMAGE_NAME +// +// MessageText: +// +// An invalid image name was specified +// +#define DISMAPI_E_INVALID_IMAGE_NAME 0xC0040006 + +// +// MessageId: DISMAPI_E_UNABLE_TO_UNMOUNT_IMAGE_PATH +// +// MessageText: +// +// An image that is not a mounted WIM or mounted VHD was attempted to be unmounted +// +#define DISMAPI_E_UNABLE_TO_UNMOUNT_IMAGE_PATH 0xC0040007 + +// +// MessageId: DISMAPI_E_LOGGING_DISABLED +// +// MessageText: +// +// Failed to gain access to the log file user specified. Logging has been disabled.. +// +#define DISMAPI_E_LOGGING_DISABLED 0xC0040009 + +// +// MessageId: DISMAPI_E_OPEN_HANDLES_UNABLE_TO_UNMOUNT_IMAGE_PATH +// +// MessageText: +// +// A DismSession with open handles was attempted to be unmounted +// +#define DISMAPI_E_OPEN_HANDLES_UNABLE_TO_UNMOUNT_IMAGE_PATH 0xC004000A + +// +// MessageId: DISMAPI_E_OPEN_HANDLES_UNABLE_TO_MOUNT_IMAGE_PATH +// +// MessageText: +// +// A DismSession with open handles was attempted to be mounted +// +#define DISMAPI_E_OPEN_HANDLES_UNABLE_TO_MOUNT_IMAGE_PATH 0xC004000B + +// +// MessageId: DISMAPI_E_OPEN_HANDLES_UNABLE_TO_REMOUNT_IMAGE_PATH +// +// MessageText: +// +// A DismSession with open handles was attempted to be remounted +// +#define DISMAPI_E_OPEN_HANDLES_UNABLE_TO_REMOUNT_IMAGE_PATH 0xC004000C + +// +// MessageId: DISMAPI_E_PARENT_FEATURE_DISABLED +// +// MessageText: +// +// One or several parent features are disabled so current feature can not be enabled. +// Solutions: +// 1 Call function DismGetFeatureParent to get all parent features and enable all of them. Or +// 2 Set EnableAll to TRUE when calling function DismEnableFeature. +// +#define DISMAPI_E_PARENT_FEATURE_DISABLED 0xC004000D + +// +// MessageId: DISMAPI_E_MUST_SPECIFY_ONLINE_IMAGE +// +// MessageText: +// +// The offline image specified is the running system. The macro DISM_ONLINE_IMAGE must be +// used instead. +// +#define DISMAPI_E_MUST_SPECIFY_ONLINE_IMAGE 0xC004000E + +// +// MessageId: DISMAPI_E_INVALID_PRODUCT_KEY +// +// MessageText: +// +// The specified product key could not be validated. Check that the specified +// product key is valid and that it matches the target edition. +// +#define DISMAPI_E_INVALID_PRODUCT_KEY 0xC004000F + +// +// MessageId: DISMAPI_E_NEEDS_TO_REMOUNT_THE_IMAGE +// +// MessageText: +// +// The image needs to be remounted before any servicing operation. +// +#define DISMAPI_E_NEEDS_REMOUNT 0XC1510114 + +// +// MessageId: DISMAPI_E_UNKNOWN_FEATURE +// +// MessageText: +// +// The feature is not present in the package. +// +#define DISMAPI_E_UNKNOWN_FEATURE 0x800f080c + +// +// MessageId: DISMAPI_E_BUSY +// +// MessageText: +// +// The current package and feature servicing infrastructure is busy. Wait a +// bit and try the operation again. +// +#define DISMAPI_E_BUSY 0x800f0902 + +#ifdef __cplusplus + } +#endif + +#endif /* WINAPI_FAMILY_PARTITION(WINAPI_PARTITION_DESKTOP | WINAPI_PARTITION_PKG_DISM) */ +#pragma endregion + +#endif // _DISMAPI_H_ + + + struct DismApiHelper + { + DismApiHelper(); + ~DismApiHelper(); + + DismFeatureInfo* GetFeatureInfo(const std::string_view& name); + HRESULT EnableFeature(const std::string_view& name); + HRESULT DisableFeature(const std::string_view& name); + + private: + using DismInitializePtr = HRESULT(WINAPI*)(int, PCWSTR, PCWSTR); + using DismOpenSessionPtr = HRESULT(WINAPI*)(PCWSTR, PCWSTR, PCWSTR, UINT*); + using DismShutdownPtr = HRESULT(WINAPI*)(); + using DismGetFeatureInfoPtr = HRESULT(WINAPI*)(UINT, PCWSTR, PCWSTR, DismPackageIdentifier, DismFeatureInfo**); + using DismEnableFeaturePtr = HRESULT(WINAPI*)(UINT, PCWSTR, PCWSTR, DismPackageIdentifier, BOOL, PCWSTR*, UINT, BOOL, HANDLE, DISM_PROGRESS_CALLBACK, PVOID); + using DismDisableFeaturePtr = HRESULT(WINAPI*)(UINT, PCWSTR, PCWSTR, BOOL, HANDLE, DISM_PROGRESS_CALLBACK, PVOID); + using DismDeletePtr = HRESULT(WINAPI*)(VOID*); + + typedef UINT DismSession; + + wil::unique_hmodule m_module; + DismInitializePtr m_dismInitialize = nullptr; + DismOpenSessionPtr m_dismOpenSession = nullptr; + DismGetFeatureInfoPtr m_dismGetFeatureInfo = nullptr; + DismEnableFeaturePtr m_dismEnableFeature = nullptr; + DismDisableFeaturePtr m_dismDisableFeature = nullptr; + DismShutdownPtr m_dismShutdown = nullptr; + DismSession m_session = 0; // DISM_SESSION_DEFAULT + DismFeatureInfo* m_featureInfo = nullptr; + DismDeletePtr m_dismDelete = nullptr; + + void Initialize(); + void OpenSession(); + void Delete(); + void Shutdown(); + }; + + struct WindowsFeature + { + WindowsFeature(const std::string& name) : m_featureName{ name } {}; + + bool DoesFeatureExist(); + + bool IsEnabled(); + + bool EnableFeature(); + + bool DisableFeature(); + + private: + DismApiHelper m_dismApiHelper; + std::string m_featureName; + }; } \ No newline at end of file diff --git a/src/AppInstallerCommonCore/WindowsFeature.cpp b/src/AppInstallerCommonCore/WindowsFeature.cpp index dcefc44606..5b84e260eb 100644 --- a/src/AppInstallerCommonCore/WindowsFeature.cpp +++ b/src/AppInstallerCommonCore/WindowsFeature.cpp @@ -3,27 +3,211 @@ #include "pch.h" #include "winget/WindowsFeature.h" #include "Public/AppInstallerLogging.h" +#include "Public/AppInstallerStrings.h" -namespace AppInstaller::WindowsFeatures +namespace AppInstaller::WindowsFeature { - namespace + DismApiHelper::DismApiHelper() { - struct WindowsFeatureHelper + m_module.reset(LoadLibraryEx(L"dismapi.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32)); + if (!m_module) { - WindowsFeatureHelper() - { - m_module.reset(LoadLibraryEx(L"dismapi.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32)); - if (!m_module) - { - AICLI_LOG(Core, Verbose, << "Could not load dismapi.dll"); - return; - } + // Do I need special handling here? + AICLI_LOG(Core, Verbose, << "Could not load dismapi.dll"); + return; + } + m_dismInitialize = + reinterpret_cast(GetProcAddress(m_module.get(), "DismInitialize")); + if (!m_dismInitialize) + { + AICLI_LOG(Core, Verbose, << "Could not get proc address of DismInitialize"); + return; + } + + m_dismOpenSession = + reinterpret_cast(GetProcAddress(m_module.get(), "DismOpenSession")); + if (!m_dismOpenSession) + { + AICLI_LOG(Core, Verbose, << "Could not get proc address of DismOpenSession"); + return; + } + + m_dismGetFeatureInfo = + reinterpret_cast(GetProcAddress(m_module.get(), "DismGetFeatureInfo")); + if (!m_dismGetFeatureInfo) + { + AICLI_LOG(Core, Verbose, << "Could not get proc address of DismGetFeatureInfo"); + return; + } + + m_dismEnableFeature = + reinterpret_cast(GetProcAddress(m_module.get(), "DismEnableFeature")); + if (!m_dismEnableFeature) + { + AICLI_LOG(Core, Verbose, << "Could not get proc address of DismEnableFeature"); + return; + } + + m_dismDisableFeature = + reinterpret_cast(GetProcAddress(m_module.get(), "DismDisableFeature")); + if (!m_dismDisableFeature) + { + AICLI_LOG(Core, Verbose, << "Could not get proc address of DismDisableFeature"); + return; + } + + m_dismDelete = + reinterpret_cast(GetProcAddress(m_module.get(), "DismDelete")); + if (!m_dismDelete) + { + AICLI_LOG(Core, Verbose, << "Could not get proc address of DismDelete"); + return; + } + + m_dismShutdown = + reinterpret_cast(GetProcAddress(m_module.get(), "DismShutdown")); + if (!m_dismGetFeatureInfo) + { + AICLI_LOG(Core, Verbose, << "Could not get proc address of DismShutdown"); + return; + } + + Initialize(); + OpenSession(); + } + + DismApiHelper::~DismApiHelper() + { + if (m_featureInfo) + { + Delete(); + } + + Shutdown(); + } + + DismFeatureInfo* DismApiHelper::GetFeatureInfo(const std::string_view& name) + { + if (m_dismGetFeatureInfo) + { + LOG_IF_FAILED(m_dismGetFeatureInfo(m_session, Utility::ConvertToUTF16(name).c_str(), NULL, DismPackageNone, &m_featureInfo)); + } + + return m_featureInfo; + } + + HRESULT DismApiHelper::EnableFeature(const std::string_view& name) + { + HRESULT hr = ERROR_PROC_NOT_FOUND; + if (m_dismEnableFeature) + { + hr = m_dismEnableFeature(m_session, Utility::ConvertToUTF16(name).c_str(), NULL, DismPackageNone, FALSE, NULL, NULL, TRUE, NULL, NULL, NULL); + LOG_IF_FAILED(hr); + } + + return hr; + } + + HRESULT DismApiHelper::DisableFeature(const std::string_view& name) + { + HRESULT hr = ERROR_PROC_NOT_FOUND; + if (m_dismDisableFeature) + { + hr = m_dismDisableFeature(m_session, Utility::ConvertToUTF16(name).c_str(), NULL, FALSE, NULL, NULL, NULL); + LOG_IF_FAILED(hr); + } + + return hr; + } + + void DismApiHelper::Initialize() + { + if (m_dismInitialize) + { + LOG_IF_FAILED(m_dismInitialize(2, nullptr, nullptr)); + } + } + + void DismApiHelper::OpenSession() + { + if (m_dismOpenSession) + { + LOG_IF_FAILED(m_dismOpenSession(L"DISM_{53BFAE52-B167-4E2F-A258-0A37B57FF845}", nullptr, nullptr, &m_session)); + } + } + + void DismApiHelper::Delete() + { + if (m_dismDelete) + { + LOG_IF_FAILED(m_dismDelete(m_featureInfo)); + } + } + + void DismApiHelper::Shutdown() + { + if (m_dismShutdown) + { + LOG_IF_FAILED(m_dismShutdown()); + } + } + bool WindowsFeature::EnableFeature() + { + DismFeatureInfo* featureInfo = m_dismApiHelper.GetFeatureInfo(m_featureName); + if (!featureInfo) + { + AICLI_LOG(Core, Verbose, << "Windows feature " << m_featureName << " does not exist."); + return false; + } + + HRESULT hr = m_dismApiHelper.EnableFeature(m_featureName); + if (SUCCEEDED(hr)) + { + AICLI_LOG(Core, Verbose, << "Windows feature " << m_featureName << " enabled successfully."); + return true; + } + else + { + AICLI_LOG(Core, Verbose, << "Failed to enable Windows Feature " << m_featureName << " with HRESULT " << hr); + return false; + } + } + + bool WindowsFeature::DisableFeature() + { + DismFeatureInfo* featureInfo = m_dismApiHelper.GetFeatureInfo(m_featureName); + if (!featureInfo) + { + AICLI_LOG(Core, Verbose, << "Windows feature " << m_featureName << " does not exist."); + return true; + } - } - private: - wil::unique_hmodule m_module; - }; + HRESULT hr = m_dismApiHelper.DisableFeature(m_featureName); + if (SUCCEEDED(hr)) + { + AICLI_LOG(Core, Verbose, << "Windows feature " << m_featureName << " disabled successfully."); + return true; + } + else + { + AICLI_LOG(Core, Verbose, << "Failed to disable Windows Feature " << m_featureName << " with HRESULT " << hr); + return false; + } + } + + bool WindowsFeature::DoesFeatureExist() + { + DismFeatureInfo* featureInfo = m_dismApiHelper.GetFeatureInfo(m_featureName); + return featureInfo; + } + + bool WindowsFeature::IsEnabled() + { + 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); } } \ No newline at end of file From 8000470688b77cc1dc809867869a92707bde609c Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Wed, 15 Feb 2023 19:01:15 -0800 Subject: [PATCH 03/29] add arguments, unit tests, and test hooks --- src/AppInstallerCLICore/Argument.cpp | 4 + .../Commands/InstallCommand.cpp | 1 + src/AppInstallerCLICore/ExecutionArgs.h | 1 + src/AppInstallerCLICore/Resources.h | 3 + .../Workflows/DependenciesFlow.cpp | 101 ++++++++---- .../Shared/Strings/en-us/winget.resw | 14 ++ .../AppInstallerCLITests.vcxproj | 6 + .../AppInstallerCLITests.vcxproj.filters | 6 + ...nstallFlowTest_InvalidWindowsFeatures.yaml | 25 +++ .../InstallFlowTest_WindowsFeatures.yaml | 23 +++ src/AppInstallerCLITests/TestHooks.h | 38 +++++ src/AppInstallerCLITests/WindowsFeature.cpp | 150 ++++++++++++++++-- .../Public/winget/WindowsFeature.h | 11 +- src/AppInstallerCommonCore/WindowsFeature.cpp | 73 ++++----- 14 files changed, 373 insertions(+), 83 deletions(-) create mode 100644 src/AppInstallerCLITests/TestData/InstallFlowTest_InvalidWindowsFeatures.yaml create mode 100644 src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml diff --git a/src/AppInstallerCLICore/Argument.cpp b/src/AppInstallerCLICore/Argument.cpp index 5a83c6e050..c804ed6b10 100644 --- a/src/AppInstallerCLICore/Argument.cpp +++ b/src/AppInstallerCLICore/Argument.cpp @@ -81,6 +81,8 @@ namespace AppInstaller::CLI return { type, "ignore-security-hash"_liv, ArgTypeCategory::InstallerBehavior | ArgTypeCategory::CopyFlagToSubContext }; case Execution::Args::Type::IgnoreLocalArchiveMalwareScan: return { type, "ignore-local-archive-malware-scan"_liv, ArgTypeCategory::InstallerBehavior | ArgTypeCategory::CopyFlagToSubContext }; + case Execution::Args::Type::IgnoreMissingDependencies: + return { type, "ignore-missing-dependencies"_liv, ArgTypeCategory::InstallerBehavior | ArgTypeCategory::CopyFlagToSubContext }; case Execution::Args::Type::AcceptPackageAgreements: return { type, "accept-package-agreements"_liv, ArgTypeCategory::InstallerBehavior }; case Execution::Args::Type::Rename: @@ -277,6 +279,8 @@ namespace AppInstaller::CLI return Argument{ type, Resource::String::HelpArgumentDescription, ArgumentType::Flag }; case Args::Type::IgnoreLocalArchiveMalwareScan: return Argument{ type, Resource::String::IgnoreLocalArchiveMalwareScanArgumentDescription, ArgumentType::Flag, Settings::TogglePolicy::Policy::LocalArchiveMalwareScanOverride, Settings::AdminSetting::LocalArchiveMalwareScanOverride }; + case Args::Type::IgnoreMissingDependencies: + return Argument{ type, Resource::String::IgnoreMissingDependenciesArgumentDescription, ArgumentType::Flag, false }; case Args::Type::SourceName: return Argument{ type, Resource::String::SourceNameArgumentDescription, ArgumentType::Positional, false }; case Args::Type::SourceArg: diff --git a/src/AppInstallerCLICore/Commands/InstallCommand.cpp b/src/AppInstallerCLICore/Commands/InstallCommand.cpp index 5092fc4b5a..35a973306c 100644 --- a/src/AppInstallerCLICore/Commands/InstallCommand.cpp +++ b/src/AppInstallerCLICore/Commands/InstallCommand.cpp @@ -39,6 +39,7 @@ namespace AppInstaller::CLI Argument::ForType(Args::Type::InstallLocation), Argument::ForType(Args::Type::HashOverride), Argument::ForType(Args::Type::IgnoreLocalArchiveMalwareScan), + Argument::ForType(Args::Type::IgnoreMissingDependencies), Argument::ForType(Args::Type::DependencySource), Argument::ForType(Args::Type::AcceptPackageAgreements), Argument::ForType(Args::Type::NoUpgrade), diff --git a/src/AppInstallerCLICore/ExecutionArgs.h b/src/AppInstallerCLICore/ExecutionArgs.h index 216bed615b..8309086197 100644 --- a/src/AppInstallerCLICore/ExecutionArgs.h +++ b/src/AppInstallerCLICore/ExecutionArgs.h @@ -46,6 +46,7 @@ namespace AppInstaller::CLI::Execution AcceptPackageAgreements, // Accept all license agreements for packages Rename, // Renames the file of the executable. Only applies to the portable installerType NoUpgrade, // Install flow should not try to convert to upgrade flow upon finding existing installed version + IgnoreMissingDependencies, // Ignores missing Windows Feature dependencies. // Uninstall behavior Purge, // Removes all files and directories related to a package during an uninstall. Only applies to the portable installerType. diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index 753a0a3985..c03ad8793e 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -112,6 +112,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(HelpLinkPreamble); WINGET_DEFINE_RESOURCE_STRINGID(IdArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(IgnoreLocalArchiveMalwareScanArgumentDescription); + WINGET_DEFINE_RESOURCE_STRINGID(IgnoreMissingDependenciesArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(ImportCommandLongDescription); WINGET_DEFINE_RESOURCE_STRINGID(ImportCommandReportDependencies); WINGET_DEFINE_RESOURCE_STRINGID(ImportCommandShortDescription); @@ -460,6 +461,8 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(VersionArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(VersionsArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(WaitArgumentDescription); + WINGET_DEFINE_RESOURCE_STRINGID(WindowsFeatureNotFoundOverride); + WINGET_DEFINE_RESOURCE_STRINGID(WindowsFeatureNotFoundOverrideRequired); WINGET_DEFINE_RESOURCE_STRINGID(WindowsFeaturesDependencies); WINGET_DEFINE_RESOURCE_STRINGID(WindowsLibrariesDependencies); WINGET_DEFINE_RESOURCE_STRINGID(WindowsStoreTerms); diff --git a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp index 3fe41259a3..4dad781ae9 100644 --- a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp @@ -142,52 +142,97 @@ namespace AppInstaller::CLI::Workflow return; } - // obtain windows features. - // Don't forget to check the root installer node but for now we'll just look at the individual installer root. - - const auto& rootInstaller = context.Get(); - const auto& rootDependencies = rootInstaller->Dependencies; + const auto& rootDependencies = context.Get()->Dependencies; if (rootDependencies.Empty()) { - // If there's no dependencies there's nothing to do aside of logging the outcome return; } if (rootDependencies.HasAnyOf(DependencyType::WindowsFeature)) { context << Workflow::EnsureRunningAsAdmin; + if (context.IsTerminated()) + { + return; + } + + std::vector invalidFeatures; - // May have to refactor out the logic into a separate function - bool errorMessageDisplayed = false; - bool allFeaturesExist = true; - auto error = context.Reporter.Error(); - rootDependencies.ApplyToType(DependencyType::WindowsFeature, [&error, &allFeaturesExist, &errorMessageDisplayed](Dependency dependency) + rootDependencies.ApplyToType(DependencyType::WindowsFeature, [&invalidFeatures](Dependency dependency) { - WindowsFeature::WindowsFeature feature{ dependency.Id() }; - if (!feature.DoesFeatureExist()) + const auto& name = dependency.Id(); + WindowsFeature::WindowsFeature windowsFeature{ name }; + if (!windowsFeature.DoesExist()) { - if (!errorMessageDisplayed) - { - error << "The following dependencies were not found:" << std::endl; - error << " - " << Resource::String::WindowsFeaturesDependencies << std::endl; - errorMessageDisplayed = true; - } - - allFeaturesExist = false; - error << " " << dependency.Id() << std::endl; + invalidFeatures.emplace_back(name); } }); - if (!allFeaturesExist && context.Args.Contains(Execution::Args::Type::Force)) + if (!invalidFeatures.empty()) { - std::cout << "proceeding due to --force." << std::endl; + bool shouldTerminate = true; + auto warn = context.Reporter.Warn(); + if (context.Args.Contains(Execution::Args::Type::IgnoreMissingDependencies)) + { + warn << Resource::String::WindowsFeatureNotFoundOverride << std::endl; + shouldTerminate = false; + } + else + { + warn << Resource::String::WindowsFeatureNotFoundOverrideRequired << std::endl; + } + + warn << " - " << Resource::String::WindowsFeaturesDependencies << std::endl; + + for (const auto& feature : invalidFeatures) + { + warn << " " << feature << std::endl; + } + + if (shouldTerminate) + { + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALL_MISSING_DEPENDENCY); + } } - //rootDependencies.ApplyToType(DependencyType::WindowsFeature, [&](Dependency dependency) - //{ - // AppInstaller::WindowsFeature::EnableWindowsFeature(dependency.Id()); - //}); + HRESULT hr = S_OK; + bool continueOnFailure = context.Args.Contains(Execution::Args::Type::Force); + rootDependencies.ApplyToType(DependencyType::WindowsFeature, [&hr, &continueOnFailure, &context](Dependency dependency) + { + if (SUCCEEDED(hr) || context.Args.Contains(Execution::Args::Type::Force)) + { + // Use progress callback to report to context for each windows feature. + + auto featureName = dependency.Id(); + WindowsFeature::WindowsFeature windowsFeature{ featureName }; + if (windowsFeature.DoesExist() && !windowsFeature.IsEnabled()) + { + context.Reporter.Info() << "Enabling " << featureName << ": "; + hr = windowsFeature.EnableFeature(); + if (hr == ERROR_SUCCESS_REBOOT_REQUIRED) + { + context.Reporter.Warn() << "Reboot required" << std::endl; + } + else if (FAILED(hr)) + { + AICLI_LOG(CLI, Error, << "Failed to enable Windows Feature '" << featureName << "' with HRESULT " << hr); + context.Reporter.Error() << "Failed" << std::endl; + } + else + { + context.Reporter.Info() << "Succeeded" << std::endl; + } + } + } + }); + + // Need a better way to handle reporting this error. + if (FAILED(hr) && !continueOnFailure) + { + context.Reporter.Error() << "Failed to enable all Windows Feature dependencies. To proceed with installation, use --force" << std::endl; + AICLI_TERMINATE_CONTEXT(hr); + } } } diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index e0b83e4657..2173aac148 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -1677,4 +1677,18 @@ Please specify one of them using the --source option to proceed. {0} package(s) have pins that prevent upgrade. Use the 'winget pin' command to view and edit pins. Using the --include-pinned argument may show more results. {Locked="winget pin","--include-pinned"} {0} is a placeholder replaced by an integer number of packages + + The following Windows Feature(s) were not found; proceeding due to --ignore-missing-dependencies. + "Windows Feature(s)" is the name of the options Windows features setting. +{Locked="--force"} + + + The following Windows Feature(s) were not found. To proceed with installation, use --ignore-missing-dependencies. + "Windows Feature(s)" is the name of the options Windows features setting. +{Locked="--force"} + + + Ignores missing Windows Feature dependencies + "Windows Feature" is the name of the options Windows features setting. + \ No newline at end of file diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj index 2b456f0364..58c95464f6 100644 --- a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj +++ b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj @@ -328,6 +328,12 @@ true + + true + + + true + true diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters index 26123276cd..338c5a66c4 100644 --- a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters +++ b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters @@ -609,6 +609,12 @@ TestData + + TestData + + + TestData + TestData diff --git a/src/AppInstallerCLITests/TestData/InstallFlowTest_InvalidWindowsFeatures.yaml b/src/AppInstallerCLITests/TestData/InstallFlowTest_InvalidWindowsFeatures.yaml new file mode 100644 index 0000000000..7f29afc36e --- /dev/null +++ b/src/AppInstallerCLITests/TestData/InstallFlowTest_InvalidWindowsFeatures.yaml @@ -0,0 +1,25 @@ +PackageIdentifier: AppInstallerCliTest.TestExeInstaller.InvalidWindowsFeatures +PackageVersion: 1.0.0.0 +PackageLocale: en-US +PackageName: AppInstaller Test Installer +ShortDescription: Installs exe installer with invalid Windows Features dependencies +Publisher: Microsoft Corporation +Moniker: AICLITestZip +License: Test +Installers: + - Architecture: x86 + InstallerUrl: https://ThisIsNotUsed + InstallerType: exe + InstallerSha256: 65DB2F2AC2686C7F2FD69D4A4C6683B888DC55BFA20A0E32CA9F838B51689A3B + InstallerSwitches: + Custom: /custom /scope=machine + SilentWithProgress: /silentwithprogress + Silent: /silence + Update: /update + Dependencies: + WindowsFeatures: + - netfx3 + - invalidFeature + - badFeature +ManifestType: singleton +ManifestVersion: 1.4.0 \ No newline at end of file diff --git a/src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml b/src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml new file mode 100644 index 0000000000..02edb938d1 --- /dev/null +++ b/src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml @@ -0,0 +1,23 @@ +PackageIdentifier: AppInstallerCliTest.TestExeInstaller.WindowsFeatures +PackageVersion: 1.0.0.0 +PackageLocale: en-US +PackageName: AppInstaller Test Installer +ShortDescription: Installs exe installer with invalid Windows Features dependencies +Publisher: Microsoft Corporation +Moniker: AICLITestZip +License: Test +Installers: + - Architecture: x86 + InstallerUrl: https://ThisIsNotUsed + InstallerType: exe + InstallerSha256: 65DB2F2AC2686C7F2FD69D4A4C6683B888DC55BFA20A0E32CA9F838B51689A3B + InstallerSwitches: + Custom: /custom /scope=machine + SilentWithProgress: /silentwithprogress + Silent: /silence + Update: /update + Dependencies: + WindowsFeatures: + - netfx3 +ManifestType: singleton +ManifestVersion: 1.4.0 \ No newline at end of file diff --git a/src/AppInstallerCLITests/TestHooks.h b/src/AppInstallerCLITests/TestHooks.h index 0496d4a20f..5c21cb09f8 100644 --- a/src/AppInstallerCLITests/TestHooks.h +++ b/src/AppInstallerCLITests/TestHooks.h @@ -58,6 +58,12 @@ namespace AppInstaller { void TestHook_SetScanArchiveResult_Override(bool* status); } + + namespace WindowsFeature + { + void TestHook_SetEnableWindowsFeatureResult_Override(HRESULT* result); + void TestHook_SetIsWindowsFeatureEnabledResult_Override(bool* status); + } } namespace TestHook @@ -106,4 +112,36 @@ namespace TestHook AppInstaller::Repository::Microsoft::TestHook_SetPinningIndex_Override({}); } }; + + struct SetEnableWindowsFeatureResult_Override + { + SetEnableWindowsFeatureResult_Override(HRESULT result) : m_result(result) + { + AppInstaller::WindowsFeature::TestHook_SetEnableWindowsFeatureResult_Override(&m_result); + } + + ~SetEnableWindowsFeatureResult_Override() + { + AppInstaller::WindowsFeature::TestHook_SetEnableWindowsFeatureResult_Override(nullptr); + } + + private: + HRESULT m_result; + }; + + struct SetIsWindowsFeatureEnabledResult_Override + { + SetIsWindowsFeatureEnabledResult_Override(bool status) : m_status(status) + { + AppInstaller::WindowsFeature::TestHook_SetIsWindowsFeatureEnabledResult_Override(&m_status); + } + + ~SetIsWindowsFeatureEnabledResult_Override() + { + AppInstaller::WindowsFeature::TestHook_SetIsWindowsFeatureEnabledResult_Override(nullptr); + } + + private: + bool m_status; + }; } \ No newline at end of file diff --git a/src/AppInstallerCLITests/WindowsFeature.cpp b/src/AppInstallerCLITests/WindowsFeature.cpp index 0c62fef7aa..ffcbd8ced7 100644 --- a/src/AppInstallerCLITests/WindowsFeature.cpp +++ b/src/AppInstallerCLITests/WindowsFeature.cpp @@ -2,9 +2,14 @@ // Licensed under the MIT License. #include "pch.h" #include "TestCommon.h" -#include +#include "WorkflowCommon.h" #include +#include +#include +#include "TestHooks.h" +using namespace AppInstaller::CLI; +using namespace AppInstaller::Settings; using namespace AppInstaller::WindowsFeature; using namespace TestCommon; @@ -21,10 +26,10 @@ TEST_CASE("GetWindowsFeature", "[windowsFeature]") } WindowsFeature validFeature{ s_featureName }; - REQUIRE(validFeature.DoesFeatureExist()); + REQUIRE(validFeature.DoesExist()); WindowsFeature invalidFeature{ "invalidFeature" }; - REQUIRE_FALSE(invalidFeature.DoesFeatureExist()); + REQUIRE_FALSE(invalidFeature.DoesExist()); } TEST_CASE("DisableEnableWindowsFeature", "[windowsFeature]") @@ -43,13 +48,136 @@ TEST_CASE("DisableEnableWindowsFeature", "[windowsFeature]") REQUIRE(feature.IsEnabled()); } -// Start tests with manifests -// manifest with valid feature -// manifest 2 with 1 valid and 1 invalid feature +TEST_CASE("InstallFlow_ValidWindowsFeature", "[windowsFeature]") +{ + if (!AppInstaller::Runtime::IsRunningAsAdmin()) + { + WARN("Test requires admin privilege. Skipped."); + return; + } + + TestCommon::TempFile installResultPath("TestExeInstalled.txt"); + + TestCommon::TestUserSettings testSettings; + testSettings.Set(true); + + std::ostringstream installOutput; + TestContext context{ installOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + OverrideForShellExecute(context); + context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string()); + InstallCommand install({}); + install.Execute(context); + INFO(installOutput.str()); -// Verify behavior of 1 valid should succeed. -// Verify behavior of 2 should fail due to invalid feature -// Verify behavior of 2 should continue if --force is passed -// Verify behavior of 2 with no interactive should error out -// + // Verify Installer is called and parameters are passed in. + REQUIRE(std::filesystem::exists(installResultPath.GetPath())); + std::ifstream installResultFile(installResultPath.GetPath()); + REQUIRE(installResultFile.is_open()); + std::string installResultStr; + std::getline(installResultFile, installResultStr); + REQUIRE(installResultStr.find("/custom") != std::string::npos); + REQUIRE(installResultStr.find("/silentwithprogress") != std::string::npos); +} + +TEST_CASE("InstallFlow_InvalidWindowsFeature", "[windowsFeature]") +{ + if (!AppInstaller::Runtime::IsRunningAsAdmin()) + { + WARN("Test requires admin privilege. Skipped."); + return; + } + + TestCommon::TempFile installResultPath("TestExeInstalled.txt"); + + TestCommon::TestUserSettings testSettings; + testSettings.Set(true); + + std::ostringstream installOutput; + TestContext context{ installOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); + + InstallCommand install({}); + install.Execute(context); + INFO(installOutput.str()); + + REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_MISSING_DEPENDENCY); + REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::WindowsFeatureNotFoundOverrideRequired).get()) != std::string::npos); + REQUIRE(installOutput.str().find("invalidFeature") != std::string::npos); + REQUIRE(installOutput.str().find("badFeature") != std::string::npos); +} + +TEST_CASE("InstallFlow_FailedToEnableWindowsFeature", "[windowsFeature]") +{ + if (!AppInstaller::Runtime::IsRunningAsAdmin()) + { + WARN("Test requires admin privilege. Skipped."); + return; + } + + TestCommon::TempFile installResultPath("TestExeInstalled.txt"); + + TestCommon::TestUserSettings testSettings; + testSettings.Set(true); + + // Override with arbitrary DISM api error and make windows feature discoverable. + TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(DISMAPI_E_DISMAPI_NOT_INITIALIZED); + TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); + + std::ostringstream installOutput; + TestContext context{ installOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); + context.Args.AddArg(Execution::Args::Type::IgnoreMissingDependencies); + + InstallCommand install({}); + install.Execute(context); + INFO(installOutput.str()); + + REQUIRE(context.GetTerminationHR() == DISMAPI_E_DISMAPI_NOT_INITIALIZED); + REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); + REQUIRE(installOutput.str().find("Failed to enable all Windows Feature dependencies") != std::string::npos); +} + +TEST_CASE("InstallFlow_FailedToEnableWindowsFeature_Force", "[windowsFeature]") +{ + if (!AppInstaller::Runtime::IsRunningAsAdmin()) + { + WARN("Test requires admin privilege. Skipped."); + return; + } + + TestCommon::TempFile installResultPath("TestExeInstalled.txt"); + + TestCommon::TestUserSettings testSettings; + testSettings.Set(true); + + // Override with arbitrary DISM api error and make windows feature discoverable. + TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(DISMAPI_E_DISMAPI_NOT_INITIALIZED); + TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); + + std::ostringstream installOutput; + TestContext context{ installOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + OverrideForShellExecute(context); + context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); + context.Args.AddArg(Execution::Args::Type::IgnoreMissingDependencies); + context.Args.AddArg(Execution::Args::Type::Force); + + InstallCommand install({}); + install.Execute(context); + INFO(installOutput.str()); + + // Verify Installer is called and parameters are passed in. + REQUIRE(context.GetTerminationHR() == ERROR_SUCCESS); + REQUIRE(std::filesystem::exists(installResultPath.GetPath())); + std::ifstream installResultFile(installResultPath.GetPath()); + REQUIRE(installResultFile.is_open()); + std::string installResultStr; + std::getline(installResultFile, installResultStr); + REQUIRE(installResultStr.find("/custom") != std::string::npos); + REQUIRE(installResultStr.find("/silentwithprogress") != std::string::npos); +} diff --git a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h index b922f6c11c..4db5b1fe75 100644 --- a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h +++ b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h @@ -338,14 +338,15 @@ namespace AppInstaller::WindowsFeature typedef UINT DismSession; wil::unique_hmodule m_module; + DismSession m_session = DISM_SESSION_DEFAULT; + DismFeatureInfo* m_featureInfo = nullptr; + DismInitializePtr m_dismInitialize = nullptr; DismOpenSessionPtr m_dismOpenSession = nullptr; DismGetFeatureInfoPtr m_dismGetFeatureInfo = nullptr; DismEnableFeaturePtr m_dismEnableFeature = nullptr; DismDisableFeaturePtr m_dismDisableFeature = nullptr; DismShutdownPtr m_dismShutdown = nullptr; - DismSession m_session = 0; // DISM_SESSION_DEFAULT - DismFeatureInfo* m_featureInfo = nullptr; DismDeletePtr m_dismDelete = nullptr; void Initialize(); @@ -358,13 +359,13 @@ namespace AppInstaller::WindowsFeature { WindowsFeature(const std::string& name) : m_featureName{ name } {}; - bool DoesFeatureExist(); + bool DoesExist(); bool IsEnabled(); - bool EnableFeature(); + HRESULT EnableFeature(); - bool DisableFeature(); + HRESULT DisableFeature(); private: DismApiHelper m_dismApiHelper; diff --git a/src/AppInstallerCommonCore/WindowsFeature.cpp b/src/AppInstallerCommonCore/WindowsFeature.cpp index 5b84e260eb..ca3e74df76 100644 --- a/src/AppInstallerCommonCore/WindowsFeature.cpp +++ b/src/AppInstallerCommonCore/WindowsFeature.cpp @@ -125,7 +125,7 @@ namespace AppInstaller::WindowsFeature { if (m_dismInitialize) { - LOG_IF_FAILED(m_dismInitialize(2, nullptr, nullptr)); + LOG_IF_FAILED(m_dismInitialize(2, NULL, NULL)); } } @@ -133,7 +133,7 @@ namespace AppInstaller::WindowsFeature { if (m_dismOpenSession) { - LOG_IF_FAILED(m_dismOpenSession(L"DISM_{53BFAE52-B167-4E2F-A258-0A37B57FF845}", nullptr, nullptr, &m_session)); + LOG_IF_FAILED(m_dismOpenSession(L"DISM_{53BFAE52-B167-4E2F-A258-0A37B57FF845}", NULL, NULL, &m_session)); } } @@ -153,58 +153,53 @@ namespace AppInstaller::WindowsFeature } } - bool WindowsFeature::EnableFeature() +#ifndef AICLI_DISABLE_TEST_HOOKS + static HRESULT* s_EnableWindowsFeatureResult_TestHook_Override = nullptr; + + void TestHook_SetEnableWindowsFeatureResult_Override(HRESULT* result) { - DismFeatureInfo* featureInfo = m_dismApiHelper.GetFeatureInfo(m_featureName); - if (!featureInfo) - { - AICLI_LOG(Core, Verbose, << "Windows feature " << m_featureName << " does not exist."); - return false; - } + s_EnableWindowsFeatureResult_TestHook_Override = result; + } +#endif - HRESULT hr = m_dismApiHelper.EnableFeature(m_featureName); - if (SUCCEEDED(hr)) - { - AICLI_LOG(Core, Verbose, << "Windows feature " << m_featureName << " enabled successfully."); - return true; - } - else + HRESULT WindowsFeature::EnableFeature() + { +#ifndef AICLI_DISABLE_TEST_HOOKS + if (s_EnableWindowsFeatureResult_TestHook_Override) { - AICLI_LOG(Core, Verbose, << "Failed to enable Windows Feature " << m_featureName << " with HRESULT " << hr); - return false; + return *s_EnableWindowsFeatureResult_TestHook_Override; } +#endif + return m_dismApiHelper.EnableFeature(m_featureName); } - bool WindowsFeature::DisableFeature() + HRESULT WindowsFeature::DisableFeature() { - DismFeatureInfo* featureInfo = m_dismApiHelper.GetFeatureInfo(m_featureName); - if (!featureInfo) - { - AICLI_LOG(Core, Verbose, << "Windows feature " << m_featureName << " does not exist."); - return true; - } + return m_dismApiHelper.DisableFeature(m_featureName); + } - HRESULT hr = m_dismApiHelper.DisableFeature(m_featureName); - if (SUCCEEDED(hr)) - { - AICLI_LOG(Core, Verbose, << "Windows feature " << m_featureName << " disabled successfully."); - return true; - } - else - { - AICLI_LOG(Core, Verbose, << "Failed to disable Windows Feature " << m_featureName << " with HRESULT " << hr); - return false; - } + bool WindowsFeature::DoesExist() + { + return m_dismApiHelper.GetFeatureInfo(m_featureName); } - bool WindowsFeature::DoesFeatureExist() +#ifndef AICLI_DISABLE_TEST_HOOKS + static bool* s_IsWindowsFeatureEnabledResult_TestHook_Override = nullptr; + + void TestHook_SetIsWindowsFeatureEnabledResult_Override(bool* status) { - DismFeatureInfo* featureInfo = m_dismApiHelper.GetFeatureInfo(m_featureName); - return featureInfo; + s_IsWindowsFeatureEnabledResult_TestHook_Override = status; } +#endif bool WindowsFeature::IsEnabled() { +#ifndef AICLI_DISABLE_TEST_HOOKS + if (s_IsWindowsFeatureEnabledResult_TestHook_Override) + { + return *s_IsWindowsFeatureEnabledResult_TestHook_Override; + } +#endif DismFeatureInfo* featureInfo = m_dismApiHelper.GetFeatureInfo(m_featureName); DismPackageFeatureState featureState = featureInfo->FeatureState; AICLI_LOG(Core, Verbose, << "Feature state of " << m_featureName << " is " << featureState); From f66dd54a507783cdcfd18b7aacaac5fac3bae124 Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Tue, 21 Feb 2023 13:28:18 -0800 Subject: [PATCH 04/29] add strings and fix CLI output --- src/AppInstallerCLICore/Resources.h | 3 + .../Workflows/DependenciesFlow.cpp | 29 +-- .../Shared/Strings/en-us/winget.resw | 11 + src/AppInstallerCLITests/WindowsFeature.cpp | 51 +++- .../Public/winget/WindowsFeature.h | 217 +----------------- src/AppInstallerCommonCore/WindowsFeature.cpp | 3 +- 6 files changed, 73 insertions(+), 241 deletions(-) diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index c03ad8793e..e7145a78d1 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -68,6 +68,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(Done); WINGET_DEFINE_RESOURCE_STRINGID(Downloading); WINGET_DEFINE_RESOURCE_STRINGID(EnableAdminSettingFailed); + WINGET_DEFINE_RESOURCE_STRINGID(EnablingWindowsFeatures); WINGET_DEFINE_RESOURCE_STRINGID(ExactArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(ExperimentalArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(ExperimentalCommandLongDescription); @@ -82,6 +83,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(ExtractArchiveSucceeded); WINGET_DEFINE_RESOURCE_STRINGID(ExtractingArchive); WINGET_DEFINE_RESOURCE_STRINGID(ExtraPositionalError); + WINGET_DEFINE_RESOURCE_STRINGID(FailedToEnableWindowsFeatureOverrideRequired); WINGET_DEFINE_RESOURCE_STRINGID(FeatureDisabledByAdminSettingMessage); WINGET_DEFINE_RESOURCE_STRINGID(FeatureDisabledMessage); WINGET_DEFINE_RESOURCE_STRINGID(FeaturesCommandLongDescription); @@ -292,6 +294,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(PurgeInstallDirectory); WINGET_DEFINE_RESOURCE_STRINGID(QueryArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(RainbowArgumentDescription); + WINGET_DEFINE_RESOURCE_STRINGID(RebootRequiredForEnablingWindowsFeature); WINGET_DEFINE_RESOURCE_STRINGID(RelatedLink); WINGET_DEFINE_RESOURCE_STRINGID(RenameArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(ReparsePointsNotSupportedError); diff --git a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp index 4dad781ae9..5ec7ee76f2 100644 --- a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp @@ -198,41 +198,36 @@ namespace AppInstaller::CLI::Workflow HRESULT hr = S_OK; bool continueOnFailure = context.Args.Contains(Execution::Args::Type::Force); - rootDependencies.ApplyToType(DependencyType::WindowsFeature, [&hr, &continueOnFailure, &context](Dependency dependency) + bool rebootRequired = false; + + context.Reporter.Info() << Resource::String::EnablingWindowsFeatures << std::endl; + rootDependencies.ApplyToType(DependencyType::WindowsFeature, [&hr, &continueOnFailure, &rebootRequired](Dependency dependency) { - if (SUCCEEDED(hr) || context.Args.Contains(Execution::Args::Type::Force)) + if (SUCCEEDED(hr) || continueOnFailure) { - // Use progress callback to report to context for each windows feature. - auto featureName = dependency.Id(); WindowsFeature::WindowsFeature windowsFeature{ featureName }; if (windowsFeature.DoesExist() && !windowsFeature.IsEnabled()) { - context.Reporter.Info() << "Enabling " << featureName << ": "; hr = windowsFeature.EnableFeature(); + AICLI_LOG(Core, Info, << "Enabling Windows Feature " << featureName << " returned with HRESULT: " << hr); if (hr == ERROR_SUCCESS_REBOOT_REQUIRED) { - context.Reporter.Warn() << "Reboot required" << std::endl; - } - else if (FAILED(hr)) - { - AICLI_LOG(CLI, Error, << "Failed to enable Windows Feature '" << featureName << "' with HRESULT " << hr); - context.Reporter.Error() << "Failed" << std::endl; - } - else - { - context.Reporter.Info() << "Succeeded" << std::endl; + rebootRequired = true; } } } }); - // Need a better way to handle reporting this error. if (FAILED(hr) && !continueOnFailure) { - context.Reporter.Error() << "Failed to enable all Windows Feature dependencies. To proceed with installation, use --force" << std::endl; + context.Reporter.Error() << Resource::String::FailedToEnableWindowsFeatureOverrideRequired << std::endl; AICLI_TERMINATE_CONTEXT(hr); } + else if (rebootRequired) + { + context.Reporter.Warn() << Resource::String::RebootRequiredForEnablingWindowsFeature << std::endl; + } } } diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index 2173aac148..16fbde201d 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -1691,4 +1691,15 @@ Please specify one of them using the --source option to proceed. Ignores missing Windows Feature dependencies "Windows Feature" is the name of the options Windows features setting. + + Enabling Windows Feature dependencies... + + + Failed to enable Windows Feature dependencies. To proceed with installation, use --force + {Locked="--force"} + + + Reboot required to fully enable the Windows Feature(s) + "Windows Feature(s)" is the name of the options Windows features setting. + \ No newline at end of file diff --git a/src/AppInstallerCLITests/WindowsFeature.cpp b/src/AppInstallerCLITests/WindowsFeature.cpp index ffcbd8ced7..b0e014d931 100644 --- a/src/AppInstallerCLITests/WindowsFeature.cpp +++ b/src/AppInstallerCLITests/WindowsFeature.cpp @@ -123,8 +123,9 @@ TEST_CASE("InstallFlow_FailedToEnableWindowsFeature", "[windowsFeature]") TestCommon::TestUserSettings testSettings; testSettings.Set(true); - // Override with arbitrary DISM api error and make windows feature discoverable. - TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(DISMAPI_E_DISMAPI_NOT_INITIALIZED); + // Override with arbitrary DISM api error (DISMAPI_E_DISMAPI_NOT_INITIALIZED) and make windows feature discoverable. + HRESULT dismErrorResult = 0xc0040001; + TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(dismErrorResult); TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); std::ostringstream installOutput; @@ -137,9 +138,9 @@ TEST_CASE("InstallFlow_FailedToEnableWindowsFeature", "[windowsFeature]") install.Execute(context); INFO(installOutput.str()); - REQUIRE(context.GetTerminationHR() == DISMAPI_E_DISMAPI_NOT_INITIALIZED); + REQUIRE(context.GetTerminationHR() == dismErrorResult); REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); - REQUIRE(installOutput.str().find("Failed to enable all Windows Feature dependencies") != std::string::npos); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeatureOverrideRequired).get()) != std::string::npos); } TEST_CASE("InstallFlow_FailedToEnableWindowsFeature_Force", "[windowsFeature]") @@ -155,8 +156,9 @@ TEST_CASE("InstallFlow_FailedToEnableWindowsFeature_Force", "[windowsFeature]") TestCommon::TestUserSettings testSettings; testSettings.Set(true); - // Override with arbitrary DISM api error and make windows feature discoverable. - TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(DISMAPI_E_DISMAPI_NOT_INITIALIZED); + // Override with arbitrary DISM api error (DISMAPI_E_DISMAPI_NOT_INITIALIZED) and make windows feature discoverable. + HRESULT dismErrorResult = 0xc0040001; + TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(dismErrorResult); TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); std::ostringstream installOutput; @@ -181,3 +183,40 @@ TEST_CASE("InstallFlow_FailedToEnableWindowsFeature_Force", "[windowsFeature]") REQUIRE(installResultStr.find("/custom") != std::string::npos); REQUIRE(installResultStr.find("/silentwithprogress") != std::string::npos); } + +TEST_CASE("InstallFlow_RebootRequired", "[windowsFeature]") +{ + if (!AppInstaller::Runtime::IsRunningAsAdmin()) + { + WARN("Test requires admin privilege. Skipped."); + return; + } + + TestCommon::TempFile installResultPath("TestExeInstalled.txt"); + + TestCommon::TestUserSettings testSettings; + testSettings.Set(true); + + // Override with reboot required HRESULT. + TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(ERROR_SUCCESS_REBOOT_REQUIRED); + TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); + + std::ostringstream installOutput; + TestContext context{ installOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + OverrideForShellExecute(context); + context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string()); + + InstallCommand install({}); + install.Execute(context); + INFO(installOutput.str()); + + REQUIRE(std::filesystem::exists(installResultPath.GetPath())); + std::ifstream installResultFile(installResultPath.GetPath()); + REQUIRE(installResultFile.is_open()); + std::string installResultStr; + std::getline(installResultFile, installResultStr); + REQUIRE(installResultStr.find("/custom") != std::string::npos); + REQUIRE(installResultStr.find("/silentwithprogress") != std::string::npos); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::RebootRequiredForEnablingWindowsFeature).get()) != std::string::npos); +} diff --git a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h index 4db5b1fe75..1b5bfc7c74 100644 --- a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h +++ b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h @@ -6,7 +6,7 @@ namespace AppInstaller::WindowsFeature { /****************************************************************************\ - DismApi.H + Declaration copied from DismApi.H to support enabling Windows Features. Copyright (c) Microsoft Corporation. All rights reserved. @@ -26,37 +26,12 @@ namespace AppInstaller::WindowsFeature { #endif - ////////////////////////////////////////////////////////////////////////////// - // - // Typedefs - // - ////////////////////////////////////////////////////////////////////////////// - typedef UINT DismSession; - - ////////////////////////////////////////////////////////////////////////////// - // - // Callbacks - // - ////////////////////////////////////////////////////////////////////////////// - typedef void(CALLBACK* DISM_PROGRESS_CALLBACK)(_In_ UINT Current, _In_ UINT Total, _In_opt_ PVOID UserData); - ////////////////////////////////////////////////////////////////////////////// - // - // Constants - // - ////////////////////////////////////////////////////////////////////////////// - #define DISM_ONLINE_IMAGE L"DISM_{53BFAE52-B167-4E2F-A258-0A37B57FF845}" #define DISM_SESSION_DEFAULT 0 - ////////////////////////////////////////////////////////////////////////////// - // - // Enums - // - ////////////////////////////////////////////////////////////////////////////// - typedef enum _DismLogLevel { DismLogErrors = 0, @@ -92,11 +67,6 @@ namespace AppInstaller::WindowsFeature DismRestartRequired } DismRestartType; - ////////////////////////////////////////////////////////////////////////////// - // - // Structs - // - ////////////////////////////////////////////////////////////////////////////// #pragma pack(push, 1) typedef struct _DismCustomProperty @@ -125,188 +95,6 @@ namespace AppInstaller::WindowsFeature #pragma pack(pop) - ////////////////////////////////////////////////////////////////////////////// - // - // Success Codes - // - ////////////////////////////////////////////////////////////////////////////// - - // For online scenario, computer needs to be restarted when the return value is ERROR_SUCCESS_REBOOT_REQUIRED (3010L). - - // - // MessageId: DISMAPI_S_RELOAD_IMAGE_SESSION_REQUIRED - // - // MessageText: - // - // The DISM session needs to be reloaded. - // -#define DISMAPI_S_RELOAD_IMAGE_SESSION_REQUIRED 0x00000001 - -////////////////////////////////////////////////////////////////////////////// -// -// Error Codes -// -////////////////////////////////////////////////////////////////////////////// - -// -// MessageId: DISMAPI_E_DISMAPI_NOT_INITIALIZED -// -// MessageText: -// -// DISM API was not initialized for this process -// -#define DISMAPI_E_DISMAPI_NOT_INITIALIZED 0xC0040001 - -// -// MessageId: DISMAPI_E_SHUTDOWN_IN_PROGRESS -// -// MessageText: -// -// A DismSession was being shutdown when another operation was called on it -// -#define DISMAPI_E_SHUTDOWN_IN_PROGRESS 0xC0040002 - -// -// MessageId: DISMAPI_E_OPEN_SESSION_HANDLES -// -// MessageText: -// -// A DismShutdown was called while there were open DismSession handles -// -#define DISMAPI_E_OPEN_SESSION_HANDLES 0xC0040003 - -// -// MessageId: DISMAPI_E_INVALID_DISM_SESSION -// -// MessageText: -// -// An invalid DismSession handle was passed into a DISMAPI function -// -#define DISMAPI_E_INVALID_DISM_SESSION 0xC0040004 - -// -// MessageId: DISMAPI_E_INVALID_IMAGE_INDEX -// -// MessageText: -// -// An invalid image index was specified -// -#define DISMAPI_E_INVALID_IMAGE_INDEX 0xC0040005 - -// -// MessageId: DISMAPI_E_INVALID_IMAGE_NAME -// -// MessageText: -// -// An invalid image name was specified -// -#define DISMAPI_E_INVALID_IMAGE_NAME 0xC0040006 - -// -// MessageId: DISMAPI_E_UNABLE_TO_UNMOUNT_IMAGE_PATH -// -// MessageText: -// -// An image that is not a mounted WIM or mounted VHD was attempted to be unmounted -// -#define DISMAPI_E_UNABLE_TO_UNMOUNT_IMAGE_PATH 0xC0040007 - -// -// MessageId: DISMAPI_E_LOGGING_DISABLED -// -// MessageText: -// -// Failed to gain access to the log file user specified. Logging has been disabled.. -// -#define DISMAPI_E_LOGGING_DISABLED 0xC0040009 - -// -// MessageId: DISMAPI_E_OPEN_HANDLES_UNABLE_TO_UNMOUNT_IMAGE_PATH -// -// MessageText: -// -// A DismSession with open handles was attempted to be unmounted -// -#define DISMAPI_E_OPEN_HANDLES_UNABLE_TO_UNMOUNT_IMAGE_PATH 0xC004000A - -// -// MessageId: DISMAPI_E_OPEN_HANDLES_UNABLE_TO_MOUNT_IMAGE_PATH -// -// MessageText: -// -// A DismSession with open handles was attempted to be mounted -// -#define DISMAPI_E_OPEN_HANDLES_UNABLE_TO_MOUNT_IMAGE_PATH 0xC004000B - -// -// MessageId: DISMAPI_E_OPEN_HANDLES_UNABLE_TO_REMOUNT_IMAGE_PATH -// -// MessageText: -// -// A DismSession with open handles was attempted to be remounted -// -#define DISMAPI_E_OPEN_HANDLES_UNABLE_TO_REMOUNT_IMAGE_PATH 0xC004000C - -// -// MessageId: DISMAPI_E_PARENT_FEATURE_DISABLED -// -// MessageText: -// -// One or several parent features are disabled so current feature can not be enabled. -// Solutions: -// 1 Call function DismGetFeatureParent to get all parent features and enable all of them. Or -// 2 Set EnableAll to TRUE when calling function DismEnableFeature. -// -#define DISMAPI_E_PARENT_FEATURE_DISABLED 0xC004000D - -// -// MessageId: DISMAPI_E_MUST_SPECIFY_ONLINE_IMAGE -// -// MessageText: -// -// The offline image specified is the running system. The macro DISM_ONLINE_IMAGE must be -// used instead. -// -#define DISMAPI_E_MUST_SPECIFY_ONLINE_IMAGE 0xC004000E - -// -// MessageId: DISMAPI_E_INVALID_PRODUCT_KEY -// -// MessageText: -// -// The specified product key could not be validated. Check that the specified -// product key is valid and that it matches the target edition. -// -#define DISMAPI_E_INVALID_PRODUCT_KEY 0xC004000F - -// -// MessageId: DISMAPI_E_NEEDS_TO_REMOUNT_THE_IMAGE -// -// MessageText: -// -// The image needs to be remounted before any servicing operation. -// -#define DISMAPI_E_NEEDS_REMOUNT 0XC1510114 - -// -// MessageId: DISMAPI_E_UNKNOWN_FEATURE -// -// MessageText: -// -// The feature is not present in the package. -// -#define DISMAPI_E_UNKNOWN_FEATURE 0x800f080c - -// -// MessageId: DISMAPI_E_BUSY -// -// MessageText: -// -// The current package and feature servicing infrastructure is busy. Wait a -// bit and try the operation again. -// -#define DISMAPI_E_BUSY 0x800f0902 - #ifdef __cplusplus } #endif @@ -360,11 +148,8 @@ namespace AppInstaller::WindowsFeature WindowsFeature(const std::string& name) : m_featureName{ name } {}; bool DoesExist(); - bool IsEnabled(); - HRESULT EnableFeature(); - HRESULT DisableFeature(); private: diff --git a/src/AppInstallerCommonCore/WindowsFeature.cpp b/src/AppInstallerCommonCore/WindowsFeature.cpp index ca3e74df76..aae0d8aa87 100644 --- a/src/AppInstallerCommonCore/WindowsFeature.cpp +++ b/src/AppInstallerCommonCore/WindowsFeature.cpp @@ -12,7 +12,6 @@ namespace AppInstaller::WindowsFeature m_module.reset(LoadLibraryEx(L"dismapi.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32)); if (!m_module) { - // Do I need special handling here? AICLI_LOG(Core, Verbose, << "Could not load dismapi.dll"); return; } @@ -133,7 +132,7 @@ namespace AppInstaller::WindowsFeature { if (m_dismOpenSession) { - LOG_IF_FAILED(m_dismOpenSession(L"DISM_{53BFAE52-B167-4E2F-A258-0A37B57FF845}", NULL, NULL, &m_session)); + LOG_IF_FAILED(m_dismOpenSession(DISM_ONLINE_IMAGE, NULL, NULL, &m_session)); } } From ba748751a064da156c690aef9ea4cd522df6369e Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Wed, 22 Feb 2023 16:00:56 -0800 Subject: [PATCH 05/29] add experimental feature for windows feature --- doc/Settings.md | 11 +++++++++++ .../Workflows/DependenciesFlow.cpp | 2 +- src/AppInstallerCLITests/WindowsFeature.cpp | 10 +++++----- src/AppInstallerCommonCore/ExperimentalFeature.cpp | 4 ++++ .../Public/winget/ExperimentalFeature.h | 1 + .../Public/winget/UserSettings.h | 2 ++ src/AppInstallerCommonCore/UserSettings.cpp | 1 + 7 files changed, 25 insertions(+), 6 deletions(-) diff --git a/doc/Settings.md b/doc/Settings.md index 5ca6e500a0..1b647e59bb 100644 --- a/doc/Settings.md +++ b/doc/Settings.md @@ -263,3 +263,14 @@ You can enable the feature as shown below. "pinning": true }, ``` + +### windowsFeature + +This feature enables the ability to enable Windows Feature dependencies during installation. +You can enable the feature as shown below. + +```json + "experimentalFeatures": { + "windowsFeature": true + }, +``` \ No newline at end of file diff --git a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp index 5ec7ee76f2..f1ae62d532 100644 --- a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp @@ -137,7 +137,7 @@ namespace AppInstaller::CLI::Workflow void EnableWindowsFeaturesDependencies(Execution::Context& context) { - if (!Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::Dependencies)) + if (!Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::WindowsFeature)) { return; } diff --git a/src/AppInstallerCLITests/WindowsFeature.cpp b/src/AppInstallerCLITests/WindowsFeature.cpp index b0e014d931..fbf2488fd4 100644 --- a/src/AppInstallerCLITests/WindowsFeature.cpp +++ b/src/AppInstallerCLITests/WindowsFeature.cpp @@ -59,7 +59,7 @@ TEST_CASE("InstallFlow_ValidWindowsFeature", "[windowsFeature]") TestCommon::TempFile installResultPath("TestExeInstalled.txt"); TestCommon::TestUserSettings testSettings; - testSettings.Set(true); + testSettings.Set(true); std::ostringstream installOutput; TestContext context{ installOutput, std::cin }; @@ -92,7 +92,7 @@ TEST_CASE("InstallFlow_InvalidWindowsFeature", "[windowsFeature]") TestCommon::TempFile installResultPath("TestExeInstalled.txt"); TestCommon::TestUserSettings testSettings; - testSettings.Set(true); + testSettings.Set(true); std::ostringstream installOutput; TestContext context{ installOutput, std::cin }; @@ -121,7 +121,7 @@ TEST_CASE("InstallFlow_FailedToEnableWindowsFeature", "[windowsFeature]") TestCommon::TempFile installResultPath("TestExeInstalled.txt"); TestCommon::TestUserSettings testSettings; - testSettings.Set(true); + testSettings.Set(true); // Override with arbitrary DISM api error (DISMAPI_E_DISMAPI_NOT_INITIALIZED) and make windows feature discoverable. HRESULT dismErrorResult = 0xc0040001; @@ -154,7 +154,7 @@ TEST_CASE("InstallFlow_FailedToEnableWindowsFeature_Force", "[windowsFeature]") TestCommon::TempFile installResultPath("TestExeInstalled.txt"); TestCommon::TestUserSettings testSettings; - testSettings.Set(true); + testSettings.Set(true); // Override with arbitrary DISM api error (DISMAPI_E_DISMAPI_NOT_INITIALIZED) and make windows feature discoverable. HRESULT dismErrorResult = 0xc0040001; @@ -195,7 +195,7 @@ TEST_CASE("InstallFlow_RebootRequired", "[windowsFeature]") TestCommon::TempFile installResultPath("TestExeInstalled.txt"); TestCommon::TestUserSettings testSettings; - testSettings.Set(true); + testSettings.Set(true); // Override with reboot required HRESULT. TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(ERROR_SUCCESS_REBOOT_REQUIRED); diff --git a/src/AppInstallerCommonCore/ExperimentalFeature.cpp b/src/AppInstallerCommonCore/ExperimentalFeature.cpp index 628285efdd..896b2fedbc 100644 --- a/src/AppInstallerCommonCore/ExperimentalFeature.cpp +++ b/src/AppInstallerCommonCore/ExperimentalFeature.cpp @@ -46,6 +46,8 @@ namespace AppInstaller::Settings return userSettings.Get(); case ExperimentalFeature::Feature::UninstallPreviousArgument: return userSettings.Get(); + case ExperimentalFeature::Feature::WindowsFeature: + return userSettings.Get(); default: THROW_HR(E_UNEXPECTED); } @@ -81,6 +83,8 @@ namespace AppInstaller::Settings return ExperimentalFeature{ "Package Pinning", "pinning", "https://aka.ms/winget-settings", Feature::Pinning}; case Feature::UninstallPreviousArgument: return ExperimentalFeature{ "Uninstall Previous Argument", "uninstallPreviousArgument", "https://aka.ms/winget-settings", Feature::UninstallPreviousArgument }; + case Feature::WindowsFeature: + return ExperimentalFeature{ "Windows Feature Dependencies", "windowsFeature", "https://aka.ms/winget-settings", Feature::WindowsFeature }; default: THROW_HR(E_UNEXPECTED); } diff --git a/src/AppInstallerCommonCore/Public/winget/ExperimentalFeature.h b/src/AppInstallerCommonCore/Public/winget/ExperimentalFeature.h index 94eafe4cbf..b0b500256b 100644 --- a/src/AppInstallerCommonCore/Public/winget/ExperimentalFeature.h +++ b/src/AppInstallerCommonCore/Public/winget/ExperimentalFeature.h @@ -26,6 +26,7 @@ namespace AppInstaller::Settings DirectMSI = 0x2, Pinning = 0x4, UninstallPreviousArgument = 0x8, + WindowsFeature = 0x10, Max, // This MUST always be after all experimental features // Features listed after Max will not be shown with the features command diff --git a/src/AppInstallerCommonCore/Public/winget/UserSettings.h b/src/AppInstallerCommonCore/Public/winget/UserSettings.h index 508be5fe81..451d9f91d6 100644 --- a/src/AppInstallerCommonCore/Public/winget/UserSettings.h +++ b/src/AppInstallerCommonCore/Public/winget/UserSettings.h @@ -72,6 +72,7 @@ namespace AppInstaller::Settings EFDirectMSI, EFPinning, EFUninstallPreviousArgument, + EFWindowsFeature, // Telemetry TelemetryDisable, // Install behavior @@ -141,6 +142,7 @@ namespace AppInstaller::Settings SETTINGMAPPING_SPECIALIZATION(Setting::EFDirectMSI, bool, bool, false, ".experimentalFeatures.directMSI"sv); SETTINGMAPPING_SPECIALIZATION(Setting::EFPinning, bool, bool, false, ".experimentalFeatures.pinning"sv); SETTINGMAPPING_SPECIALIZATION(Setting::EFUninstallPreviousArgument, bool, bool, false, ".experimentalFeatures.uninstallPreviousArgument"sv); + SETTINGMAPPING_SPECIALIZATION(Setting::EFWindowsFeature, bool, bool, false, ".experimentalFeatures.windowsFeature"sv); // Telemetry SETTINGMAPPING_SPECIALIZATION(Setting::TelemetryDisable, bool, bool, false, ".telemetry.disable"sv); // Install behavior diff --git a/src/AppInstallerCommonCore/UserSettings.cpp b/src/AppInstallerCommonCore/UserSettings.cpp index 2299e30501..e1467cf4ee 100644 --- a/src/AppInstallerCommonCore/UserSettings.cpp +++ b/src/AppInstallerCommonCore/UserSettings.cpp @@ -262,6 +262,7 @@ namespace AppInstaller::Settings WINGET_VALIDATE_PASS_THROUGH(EFDirectMSI) WINGET_VALIDATE_PASS_THROUGH(EFPinning) WINGET_VALIDATE_PASS_THROUGH(EFUninstallPreviousArgument) + WINGET_VALIDATE_PASS_THROUGH(EFWindowsFeature) WINGET_VALIDATE_PASS_THROUGH(TelemetryDisable) WINGET_VALIDATE_PASS_THROUGH(InteractivityDisable) WINGET_VALIDATE_PASS_THROUGH(EnableSelfInitiatedMinidump) From 6d8ca3ebf14b97ac8a4d4136a0c3506e6e9bdbdc Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Wed, 22 Feb 2023 23:04:27 -0800 Subject: [PATCH 06/29] fix spelling and test --- .github/actions/spelling/expect.txt | 4 ++++ src/AppInstallerCLITests/WindowsFeature.cpp | 8 ++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt index 8a9c8ba2f2..1e06fdcdec 100644 --- a/.github/actions/spelling/expect.txt +++ b/.github/actions/spelling/expect.txt @@ -72,6 +72,7 @@ contractversion count'th countof countryregion +cplusplus createmanifestmetadata ctc DACL @@ -81,6 +82,7 @@ deigh deleteifnotneeded desktopappinstaller diskfull +dismapi dllimport DMPAs dnld @@ -219,6 +221,7 @@ mysilentwithprogress nameof nativehandle NESTEDINSTALLER +netfx netlify Newtonsoft NOEXPAND @@ -383,6 +386,7 @@ websites WERSJA wesome Whatif +winapifamily windir windowsdeveloper winerror diff --git a/src/AppInstallerCLITests/WindowsFeature.cpp b/src/AppInstallerCLITests/WindowsFeature.cpp index fbf2488fd4..29e36afc46 100644 --- a/src/AppInstallerCLITests/WindowsFeature.cpp +++ b/src/AppInstallerCLITests/WindowsFeature.cpp @@ -41,10 +41,14 @@ TEST_CASE("DisableEnableWindowsFeature", "[windowsFeature]") } WindowsFeature feature{ s_featureName }; - REQUIRE(feature.DisableFeature()); + HRESULT disableResult = feature.DisableFeature(); + bool disableStatus = (disableResult == S_OK || disableResult == ERROR_SUCCESS_REBOOT_REQUIRED); + REQUIRE(disableStatus); REQUIRE_FALSE(feature.IsEnabled()); - REQUIRE(feature.EnableFeature()); + HRESULT enableResult = feature.EnableFeature(); + bool enableStatus = (enableResult == S_OK) || (enableResult == ERROR_SUCCESS_REBOOT_REQUIRED); + REQUIRE(enableStatus); REQUIRE(feature.IsEnabled()); } From 5d048d4e747d3e950311830a399ad867df338e88 Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Thu, 23 Feb 2023 09:37:56 -0800 Subject: [PATCH 07/29] add extra warning message during override --- src/AppInstallerCLICore/Resources.h | 1 + .../Workflows/DependenciesFlow.cpp | 13 ++++++++++--- .../Shared/Strings/en-us/winget.resw | 11 +++++++++-- src/AppInstallerCLITests/WindowsFeature.cpp | 1 + 4 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index e7145a78d1..9e19760c5a 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -83,6 +83,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(ExtractArchiveSucceeded); WINGET_DEFINE_RESOURCE_STRINGID(ExtractingArchive); WINGET_DEFINE_RESOURCE_STRINGID(ExtraPositionalError); + WINGET_DEFINE_RESOURCE_STRINGID(FailedToEnableWindowsFeatureOverridden); WINGET_DEFINE_RESOURCE_STRINGID(FailedToEnableWindowsFeatureOverrideRequired); WINGET_DEFINE_RESOURCE_STRINGID(FeatureDisabledByAdminSettingMessage); WINGET_DEFINE_RESOURCE_STRINGID(FeatureDisabledMessage); diff --git a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp index f1ae62d532..4eb58950fd 100644 --- a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp @@ -219,10 +219,17 @@ namespace AppInstaller::CLI::Workflow } }); - if (FAILED(hr) && !continueOnFailure) + if (FAILED(hr)) { - context.Reporter.Error() << Resource::String::FailedToEnableWindowsFeatureOverrideRequired << std::endl; - AICLI_TERMINATE_CONTEXT(hr); + if (continueOnFailure) + { + context.Reporter.Warn() << Resource::String::FailedToEnableWindowsFeatureOverridden << std::endl; + } + else + { + context.Reporter.Error() << Resource::String::FailedToEnableWindowsFeatureOverrideRequired << std::endl; + AICLI_TERMINATE_CONTEXT(hr); + } } else if (rebootRequired) { diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index 16fbde201d..af9babc84f 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -1680,12 +1680,14 @@ Please specify one of them using the --source option to proceed. The following Windows Feature(s) were not found; proceeding due to --ignore-missing-dependencies. "Windows Feature(s)" is the name of the options Windows features setting. -{Locked="--force"} +{Locked="--force"} +{Locked="--ignore-missing-dependencies"} The following Windows Feature(s) were not found. To proceed with installation, use --ignore-missing-dependencies. "Windows Feature(s)" is the name of the options Windows features setting. -{Locked="--force"} +{Locked="--force"} +{Locked="--ignore-missing-dependencies"} Ignores missing Windows Feature dependencies @@ -1702,4 +1704,9 @@ Please specify one of them using the --source option to proceed. Reboot required to fully enable the Windows Feature(s) "Windows Feature(s)" is the name of the options Windows features setting. + + Failed to enable Windows Feature dependencies; proceeding due to --force + "Windows Feature(s)" is the name of the options Windows features setting. +{Locked="--force"} + \ No newline at end of file diff --git a/src/AppInstallerCLITests/WindowsFeature.cpp b/src/AppInstallerCLITests/WindowsFeature.cpp index 29e36afc46..c80421d5af 100644 --- a/src/AppInstallerCLITests/WindowsFeature.cpp +++ b/src/AppInstallerCLITests/WindowsFeature.cpp @@ -179,6 +179,7 @@ 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::FailedToEnableWindowsFeatureOverridden).get()) != std::string::npos); REQUIRE(std::filesystem::exists(installResultPath.GetPath())); std::ifstream installResultFile(installResultPath.GetPath()); REQUIRE(installResultFile.is_open()); From 8ebe76b5ba9e84e9c6c431a5d472232cdbe7de46 Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Thu, 23 Feb 2023 12:00:17 -0800 Subject: [PATCH 08/29] remove extra file from project files --- src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj | 1 - 1 file changed, 1 deletion(-) diff --git a/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj b/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj index 054b2fd402..3aac7b1ff5 100644 --- a/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj +++ b/src/AppInstallerCommonCore/AppInstallerCommonCore.vcxproj @@ -399,7 +399,6 @@ - From d59eb0ee7f395667d66fb552a1284998c9348b89 Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Mon, 27 Feb 2023 16:51:54 -0800 Subject: [PATCH 09/29] refactor dismhelper --- src/AppInstallerCLICore/Argument.cpp | 4 - .../Commands/InstallCommand.cpp | 1 - src/AppInstallerCLICore/ExecutionArgs.h | 1 - .../Workflows/DependenciesFlow.cpp | 56 +++------ src/AppInstallerCLITests/WindowsFeature.cpp | 19 +-- .../Public/winget/WindowsFeature.h | 97 +++++++++------ src/AppInstallerCommonCore/WindowsFeature.cpp | 113 ++++++++---------- 7 files changed, 136 insertions(+), 155 deletions(-) diff --git a/src/AppInstallerCLICore/Argument.cpp b/src/AppInstallerCLICore/Argument.cpp index c804ed6b10..5a83c6e050 100644 --- a/src/AppInstallerCLICore/Argument.cpp +++ b/src/AppInstallerCLICore/Argument.cpp @@ -81,8 +81,6 @@ namespace AppInstaller::CLI return { type, "ignore-security-hash"_liv, ArgTypeCategory::InstallerBehavior | ArgTypeCategory::CopyFlagToSubContext }; case Execution::Args::Type::IgnoreLocalArchiveMalwareScan: return { type, "ignore-local-archive-malware-scan"_liv, ArgTypeCategory::InstallerBehavior | ArgTypeCategory::CopyFlagToSubContext }; - case Execution::Args::Type::IgnoreMissingDependencies: - return { type, "ignore-missing-dependencies"_liv, ArgTypeCategory::InstallerBehavior | ArgTypeCategory::CopyFlagToSubContext }; case Execution::Args::Type::AcceptPackageAgreements: return { type, "accept-package-agreements"_liv, ArgTypeCategory::InstallerBehavior }; case Execution::Args::Type::Rename: @@ -279,8 +277,6 @@ namespace AppInstaller::CLI return Argument{ type, Resource::String::HelpArgumentDescription, ArgumentType::Flag }; case Args::Type::IgnoreLocalArchiveMalwareScan: return Argument{ type, Resource::String::IgnoreLocalArchiveMalwareScanArgumentDescription, ArgumentType::Flag, Settings::TogglePolicy::Policy::LocalArchiveMalwareScanOverride, Settings::AdminSetting::LocalArchiveMalwareScanOverride }; - case Args::Type::IgnoreMissingDependencies: - return Argument{ type, Resource::String::IgnoreMissingDependenciesArgumentDescription, ArgumentType::Flag, false }; case Args::Type::SourceName: return Argument{ type, Resource::String::SourceNameArgumentDescription, ArgumentType::Positional, false }; case Args::Type::SourceArg: diff --git a/src/AppInstallerCLICore/Commands/InstallCommand.cpp b/src/AppInstallerCLICore/Commands/InstallCommand.cpp index 35a973306c..5092fc4b5a 100644 --- a/src/AppInstallerCLICore/Commands/InstallCommand.cpp +++ b/src/AppInstallerCLICore/Commands/InstallCommand.cpp @@ -39,7 +39,6 @@ namespace AppInstaller::CLI Argument::ForType(Args::Type::InstallLocation), Argument::ForType(Args::Type::HashOverride), Argument::ForType(Args::Type::IgnoreLocalArchiveMalwareScan), - Argument::ForType(Args::Type::IgnoreMissingDependencies), Argument::ForType(Args::Type::DependencySource), Argument::ForType(Args::Type::AcceptPackageAgreements), Argument::ForType(Args::Type::NoUpgrade), diff --git a/src/AppInstallerCLICore/ExecutionArgs.h b/src/AppInstallerCLICore/ExecutionArgs.h index 8309086197..216bed615b 100644 --- a/src/AppInstallerCLICore/ExecutionArgs.h +++ b/src/AppInstallerCLICore/ExecutionArgs.h @@ -46,7 +46,6 @@ namespace AppInstaller::CLI::Execution AcceptPackageAgreements, // Accept all license agreements for packages Rename, // Renames the file of the executable. Only applies to the portable installerType NoUpgrade, // Install flow should not try to convert to upgrade flow upon finding existing installed version - IgnoreMissingDependencies, // Ignores missing Windows Feature dependencies. // Uninstall behavior Purge, // Removes all files and directories related to a package during an uninstall. Only applies to the portable installerType. diff --git a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp index 4eb58950fd..262b536026 100644 --- a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp @@ -157,59 +157,31 @@ namespace AppInstaller::CLI::Workflow return; } - std::vector invalidFeatures; - - rootDependencies.ApplyToType(DependencyType::WindowsFeature, [&invalidFeatures](Dependency dependency) - { - const auto& name = dependency.Id(); - WindowsFeature::WindowsFeature windowsFeature{ name }; - if (!windowsFeature.DoesExist()) - { - invalidFeatures.emplace_back(name); - } - }); - - if (!invalidFeatures.empty()) - { - bool shouldTerminate = true; - auto warn = context.Reporter.Warn(); - if (context.Args.Contains(Execution::Args::Type::IgnoreMissingDependencies)) - { - warn << Resource::String::WindowsFeatureNotFoundOverride << std::endl; - shouldTerminate = false; - } - else - { - warn << Resource::String::WindowsFeatureNotFoundOverrideRequired << std::endl; - } - - warn << " - " << Resource::String::WindowsFeaturesDependencies << std::endl; - - for (const auto& feature : invalidFeatures) - { - warn << " " << feature << std::endl; - } - - if (shouldTerminate) - { - AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALL_MISSING_DEPENDENCY); - } - } - HRESULT hr = S_OK; bool continueOnFailure = context.Args.Contains(Execution::Args::Type::Force); bool rebootRequired = false; + // Do I need warn, info and error reporters for this function?\ + + // Pass info and warn so that we can show enabling and the status. If we don't have continue on failure, we error out and display in a separate. + auto info = context.Reporter.Info(); + auto warn = context.Reporter.Warn(); + + DismHelper dismHelper = DismHelper(); + context.Reporter.Info() << Resource::String::EnablingWindowsFeatures << std::endl; - rootDependencies.ApplyToType(DependencyType::WindowsFeature, [&hr, &continueOnFailure, &rebootRequired](Dependency dependency) + rootDependencies.ApplyToType(DependencyType::WindowsFeature, [&hr, &continueOnFailure, &rebootRequired, &warn, &info, &dismHelper](Dependency dependency) { if (SUCCEEDED(hr) || continueOnFailure) { + info << "Enabling feature"; + auto featureName = dependency.Id(); - WindowsFeature::WindowsFeature windowsFeature{ featureName }; + DismHelper::WindowsFeature windowsFeature = dismHelper.CreateWindowsFeature(featureName); + if (windowsFeature.DoesExist() && !windowsFeature.IsEnabled()) { - hr = windowsFeature.EnableFeature(); + hr = windowsFeature.Enable(); AICLI_LOG(Core, Info, << "Enabling Windows Feature " << featureName << " returned with HRESULT: " << hr); if (hr == ERROR_SUCCESS_REBOOT_REQUIRED) { diff --git a/src/AppInstallerCLITests/WindowsFeature.cpp b/src/AppInstallerCLITests/WindowsFeature.cpp index c80421d5af..32bd04429c 100644 --- a/src/AppInstallerCLITests/WindowsFeature.cpp +++ b/src/AppInstallerCLITests/WindowsFeature.cpp @@ -25,10 +25,16 @@ TEST_CASE("GetWindowsFeature", "[windowsFeature]") return; } - WindowsFeature validFeature{ s_featureName }; + DismHelper dismHelper = DismHelper(); + + DismHelper::WindowsFeature validFeature = dismHelper.CreateWindowsFeature(s_featureName); REQUIRE(validFeature.DoesExist()); - WindowsFeature invalidFeature{ "invalidFeature" }; + DismHelper::WindowsFeature validFeatureWithCasing = dismHelper.CreateWindowsFeature("NeTFX3"); + REQUIRE(validFeatureWithCasing.DoesExist()); + + + DismHelper::WindowsFeature invalidFeature = dismHelper.CreateWindowsFeature("invalidFeature"); REQUIRE_FALSE(invalidFeature.DoesExist()); } @@ -40,13 +46,14 @@ TEST_CASE("DisableEnableWindowsFeature", "[windowsFeature]") return; } - WindowsFeature feature{ s_featureName }; - HRESULT disableResult = feature.DisableFeature(); + DismHelper dismHelper = DismHelper(); + DismHelper::WindowsFeature feature = dismHelper.CreateWindowsFeature(s_featureName); + HRESULT disableResult = feature.Disable(); bool disableStatus = (disableResult == S_OK || disableResult == ERROR_SUCCESS_REBOOT_REQUIRED); REQUIRE(disableStatus); REQUIRE_FALSE(feature.IsEnabled()); - HRESULT enableResult = feature.EnableFeature(); + HRESULT enableResult = feature.Enable(); bool enableStatus = (enableResult == S_OK) || (enableResult == ERROR_SUCCESS_REBOOT_REQUIRED); REQUIRE(enableStatus); REQUIRE(feature.IsEnabled()); @@ -136,7 +143,6 @@ TEST_CASE("InstallFlow_FailedToEnableWindowsFeature", "[windowsFeature]") TestContext context{ installOutput, std::cin }; auto previousThreadGlobals = context.SetForCurrentThread(); context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); - context.Args.AddArg(Execution::Args::Type::IgnoreMissingDependencies); InstallCommand install({}); install.Execute(context); @@ -170,7 +176,6 @@ TEST_CASE("InstallFlow_FailedToEnableWindowsFeature_Force", "[windowsFeature]") auto previousThreadGlobals = context.SetForCurrentThread(); OverrideForShellExecute(context); context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); - context.Args.AddArg(Execution::Args::Type::IgnoreMissingDependencies); context.Args.AddArg(Execution::Args::Type::Force); InstallCommand install({}); diff --git a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h index 1b5bfc7c74..7bf62d1454 100644 --- a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h +++ b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h @@ -4,14 +4,14 @@ namespace AppInstaller::WindowsFeature { -/****************************************************************************\ + /****************************************************************************\ - Declaration copied from DismApi.H to support enabling Windows Features. + Declaration copied from DismApi.H to support enabling Windows Features. - Copyright (c) Microsoft Corporation. - All rights reserved. + Copyright (c) Microsoft Corporation. + All rights reserved. -\****************************************************************************/ + \****************************************************************************/ #ifndef _DISMAPI_H_ #define _DISMAPI_H_ @@ -104,30 +104,72 @@ namespace AppInstaller::WindowsFeature #endif // _DISMAPI_H_ + using DismInitializePtr = HRESULT(WINAPI*)(int, PCWSTR, PCWSTR); + using DismOpenSessionPtr = HRESULT(WINAPI*)(PCWSTR, PCWSTR, PCWSTR, UINT*); + using DismShutdownPtr = HRESULT(WINAPI*)(); + using DismGetFeatureInfoPtr = HRESULT(WINAPI*)(UINT, PCWSTR, PCWSTR, DismPackageIdentifier, DismFeatureInfo**); + using DismEnableFeaturePtr = HRESULT(WINAPI*)(UINT, PCWSTR, PCWSTR, DismPackageIdentifier, BOOL, PCWSTR*, UINT, BOOL, HANDLE, DISM_PROGRESS_CALLBACK, PVOID); + using DismDisableFeaturePtr = HRESULT(WINAPI*)(UINT, PCWSTR, PCWSTR, BOOL, HANDLE, DISM_PROGRESS_CALLBACK, PVOID); + using DismDeletePtr = HRESULT(WINAPI*)(VOID*); - struct DismApiHelper + struct DismHelper { - DismApiHelper(); - ~DismApiHelper(); - DismFeatureInfo* GetFeatureInfo(const std::string_view& name); - HRESULT EnableFeature(const std::string_view& name); - HRESULT DisableFeature(const std::string_view& name); + DismHelper(); + ~DismHelper() + { + Shutdown(); + }; - private: - using DismInitializePtr = HRESULT(WINAPI*)(int, PCWSTR, PCWSTR); - using DismOpenSessionPtr = HRESULT(WINAPI*)(PCWSTR, PCWSTR, PCWSTR, UINT*); - using DismShutdownPtr = HRESULT(WINAPI*)(); - using DismGetFeatureInfoPtr = HRESULT(WINAPI*)(UINT, PCWSTR, PCWSTR, DismPackageIdentifier, DismFeatureInfo**); - using DismEnableFeaturePtr = HRESULT(WINAPI*)(UINT, PCWSTR, PCWSTR, DismPackageIdentifier, BOOL, PCWSTR*, UINT, BOOL, HANDLE, DISM_PROGRESS_CALLBACK, PVOID); - using DismDisableFeaturePtr = HRESULT(WINAPI*)(UINT, PCWSTR, PCWSTR, BOOL, HANDLE, DISM_PROGRESS_CALLBACK, PVOID); - using DismDeletePtr = HRESULT(WINAPI*)(VOID*); + struct WindowsFeature + { + bool DoesExist(); + bool IsEnabled(); + DismPackageFeatureState GetState() { return m_featureInfo->FeatureState; }; + std::wstring GetDisplayName() { return std::wstring { m_featureInfo->DisplayName }; } + DismRestartType GetRestartRequiredStatus() { return m_featureInfo->RestartRequired; } + HRESULT Enable(); + HRESULT Disable(); + + ~WindowsFeature() + { + if (m_featureInfo) + { + m_deletePtr(m_featureInfo); + } + } + + private: + friend DismHelper; + + WindowsFeature(const std::string& name, DismGetFeatureInfoPtr getFeatureInfoPtr, DismEnableFeaturePtr enableFeaturePtr, DismDisableFeaturePtr disableFeaturePtr, DismDeletePtr deletePtr, DismSession session); + + WindowsFeature(const std::string& name) : m_featureName(name) + { + GetFeatureInfo(); + } + + std::string m_featureName; + DismFeatureInfo* m_featureInfo = nullptr; + void GetFeatureInfo(); + + DismGetFeatureInfoPtr m_getFeatureInfoPtr = nullptr; + DismEnableFeaturePtr m_enableFeature = nullptr; + DismDisableFeaturePtr m_disableFeaturePtr = nullptr; + DismDeletePtr m_deletePtr = nullptr; + DismSession m_session; + }; + + WindowsFeature CreateWindowsFeature(const std::string& name) + { + return WindowsFeature(name, m_dismGetFeatureInfo, m_dismEnableFeature, m_dismDisableFeature, m_dismDelete, m_session); + } + private: typedef UINT DismSession; wil::unique_hmodule m_module; DismSession m_session = DISM_SESSION_DEFAULT; - DismFeatureInfo* m_featureInfo = nullptr; DismInitializePtr m_dismInitialize = nullptr; DismOpenSessionPtr m_dismOpenSession = nullptr; @@ -139,21 +181,6 @@ namespace AppInstaller::WindowsFeature void Initialize(); void OpenSession(); - void Delete(); void Shutdown(); }; - - struct WindowsFeature - { - WindowsFeature(const std::string& name) : m_featureName{ name } {}; - - bool DoesExist(); - bool IsEnabled(); - HRESULT EnableFeature(); - HRESULT DisableFeature(); - - private: - DismApiHelper m_dismApiHelper; - std::string m_featureName; - }; } \ No newline at end of file diff --git a/src/AppInstallerCommonCore/WindowsFeature.cpp b/src/AppInstallerCommonCore/WindowsFeature.cpp index aae0d8aa87..d941814b94 100644 --- a/src/AppInstallerCommonCore/WindowsFeature.cpp +++ b/src/AppInstallerCommonCore/WindowsFeature.cpp @@ -7,7 +7,7 @@ namespace AppInstaller::WindowsFeature { - DismApiHelper::DismApiHelper() + DismHelper::DismHelper() { m_module.reset(LoadLibraryEx(L"dismapi.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32)); if (!m_module) @@ -76,51 +76,7 @@ namespace AppInstaller::WindowsFeature OpenSession(); } - DismApiHelper::~DismApiHelper() - { - if (m_featureInfo) - { - Delete(); - } - - Shutdown(); - } - - DismFeatureInfo* DismApiHelper::GetFeatureInfo(const std::string_view& name) - { - if (m_dismGetFeatureInfo) - { - LOG_IF_FAILED(m_dismGetFeatureInfo(m_session, Utility::ConvertToUTF16(name).c_str(), NULL, DismPackageNone, &m_featureInfo)); - } - - return m_featureInfo; - } - - HRESULT DismApiHelper::EnableFeature(const std::string_view& name) - { - HRESULT hr = ERROR_PROC_NOT_FOUND; - if (m_dismEnableFeature) - { - hr = m_dismEnableFeature(m_session, Utility::ConvertToUTF16(name).c_str(), NULL, DismPackageNone, FALSE, NULL, NULL, TRUE, NULL, NULL, NULL); - LOG_IF_FAILED(hr); - } - - return hr; - } - - HRESULT DismApiHelper::DisableFeature(const std::string_view& name) - { - HRESULT hr = ERROR_PROC_NOT_FOUND; - if (m_dismDisableFeature) - { - hr = m_dismDisableFeature(m_session, Utility::ConvertToUTF16(name).c_str(), NULL, FALSE, NULL, NULL, NULL); - LOG_IF_FAILED(hr); - } - - return hr; - } - - void DismApiHelper::Initialize() + void DismHelper::Initialize() { if (m_dismInitialize) { @@ -128,7 +84,7 @@ namespace AppInstaller::WindowsFeature } } - void DismApiHelper::OpenSession() + void DismHelper::OpenSession() { if (m_dismOpenSession) { @@ -136,15 +92,7 @@ namespace AppInstaller::WindowsFeature } } - void DismApiHelper::Delete() - { - if (m_dismDelete) - { - LOG_IF_FAILED(m_dismDelete(m_featureInfo)); - } - } - - void DismApiHelper::Shutdown() + void DismHelper::Shutdown() { if (m_dismShutdown) { @@ -161,7 +109,7 @@ namespace AppInstaller::WindowsFeature } #endif - HRESULT WindowsFeature::EnableFeature() + HRESULT DismHelper::WindowsFeature::Enable() { #ifndef AICLI_DISABLE_TEST_HOOKS if (s_EnableWindowsFeatureResult_TestHook_Override) @@ -169,17 +117,31 @@ namespace AppInstaller::WindowsFeature return *s_EnableWindowsFeatureResult_TestHook_Override; } #endif - return m_dismApiHelper.EnableFeature(m_featureName); + HRESULT hr = ERROR_PROC_NOT_FOUND; + if (m_enableFeature) + { + hr = m_enableFeature(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, DismPackageNone, FALSE, NULL, NULL, TRUE, NULL, NULL, NULL); + LOG_IF_FAILED(hr); + } + + return hr; } - HRESULT WindowsFeature::DisableFeature() + HRESULT DismHelper::WindowsFeature::Disable() { - return m_dismApiHelper.DisableFeature(m_featureName); + HRESULT hr = ERROR_PROC_NOT_FOUND; + if (m_disableFeaturePtr) + { + hr = m_disableFeaturePtr(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, FALSE, NULL, NULL, NULL); + LOG_IF_FAILED(hr); + } + + return hr; } - bool WindowsFeature::DoesExist() + bool DismHelper::WindowsFeature::DoesExist() { - return m_dismApiHelper.GetFeatureInfo(m_featureName); + return m_featureInfo; } #ifndef AICLI_DISABLE_TEST_HOOKS @@ -191,7 +153,7 @@ namespace AppInstaller::WindowsFeature } #endif - bool WindowsFeature::IsEnabled() + bool DismHelper::WindowsFeature::IsEnabled() { #ifndef AICLI_DISABLE_TEST_HOOKS if (s_IsWindowsFeatureEnabledResult_TestHook_Override) @@ -199,9 +161,30 @@ namespace AppInstaller::WindowsFeature return *s_IsWindowsFeatureEnabledResult_TestHook_Override; } #endif - DismFeatureInfo* featureInfo = m_dismApiHelper.GetFeatureInfo(m_featureName); - DismPackageFeatureState featureState = featureInfo->FeatureState; + // Refresh feature info state prior to retrieving state info. + GetFeatureInfo(); + DismPackageFeatureState featureState = GetState(); AICLI_LOG(Core, Verbose, << "Feature state of " << m_featureName << " is " << featureState); return (featureState == DismStateInstalled || featureState == DismStateInstallPending); } + + void DismHelper::WindowsFeature::GetFeatureInfo() + { + if (m_getFeatureInfoPtr) + { + LOG_IF_FAILED(m_getFeatureInfoPtr(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, DismPackageNone, &m_featureInfo)); + } + } + + DismHelper::WindowsFeature::WindowsFeature(const std::string& name, DismGetFeatureInfoPtr getFeatureInfoPtr, DismEnableFeaturePtr enableFeaturePtr, DismDisableFeaturePtr disableFeaturePtr, DismDeletePtr deletePtr, DismSession session) + { + m_featureName = name; + m_getFeatureInfoPtr = getFeatureInfoPtr; + m_enableFeature = enableFeaturePtr; + m_disableFeaturePtr = disableFeaturePtr; + m_deletePtr = deletePtr; + m_session = session; + + GetFeatureInfo(); + } } \ No newline at end of file From 8c8a7cf91e33c61ee235417c8090b56e1bf58298 Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Wed, 1 Mar 2023 09:43:36 -0800 Subject: [PATCH 10/29] finish addressing feedback --- src/AppInstallerCLICore/Resources.h | 10 ++-- .../Workflows/DependenciesFlow.cpp | 59 ++++++++++++------ .../Workflows/InstallFlow.cpp | 2 +- .../Shared/Strings/en-us/winget.resw | 37 ++++++------ src/AppInstallerCLITests/WindowsFeature.cpp | 50 ++++++++++++++-- .../Public/winget/WindowsFeature.h | 60 ++++++++++++------- src/AppInstallerCommonCore/WindowsFeature.cpp | 34 ++++++----- 7 files changed, 166 insertions(+), 86 deletions(-) diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index 9e19760c5a..3dfbf9e15f 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -68,7 +68,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(Done); WINGET_DEFINE_RESOURCE_STRINGID(Downloading); WINGET_DEFINE_RESOURCE_STRINGID(EnableAdminSettingFailed); - WINGET_DEFINE_RESOURCE_STRINGID(EnablingWindowsFeatures); + WINGET_DEFINE_RESOURCE_STRINGID(EnablingWindowsFeature); WINGET_DEFINE_RESOURCE_STRINGID(ExactArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(ExperimentalArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(ExperimentalCommandLongDescription); @@ -83,6 +83,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(ExtractArchiveSucceeded); WINGET_DEFINE_RESOURCE_STRINGID(ExtractingArchive); WINGET_DEFINE_RESOURCE_STRINGID(ExtraPositionalError); + WINGET_DEFINE_RESOURCE_STRINGID(FailedToEnableWindowsFeature); WINGET_DEFINE_RESOURCE_STRINGID(FailedToEnableWindowsFeatureOverridden); WINGET_DEFINE_RESOURCE_STRINGID(FailedToEnableWindowsFeatureOverrideRequired); WINGET_DEFINE_RESOURCE_STRINGID(FeatureDisabledByAdminSettingMessage); @@ -115,7 +116,6 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(HelpLinkPreamble); WINGET_DEFINE_RESOURCE_STRINGID(IdArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(IgnoreLocalArchiveMalwareScanArgumentDescription); - WINGET_DEFINE_RESOURCE_STRINGID(IgnoreMissingDependenciesArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(ImportCommandLongDescription); WINGET_DEFINE_RESOURCE_STRINGID(ImportCommandReportDependencies); WINGET_DEFINE_RESOURCE_STRINGID(ImportCommandShortDescription); @@ -295,7 +295,8 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(PurgeInstallDirectory); WINGET_DEFINE_RESOURCE_STRINGID(QueryArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(RainbowArgumentDescription); - WINGET_DEFINE_RESOURCE_STRINGID(RebootRequiredForEnablingWindowsFeature); + WINGET_DEFINE_RESOURCE_STRINGID(RebootRequiredToEnableWindowsFeatureOverridden); + WINGET_DEFINE_RESOURCE_STRINGID(RebootRequiredToEnableWindowsFeatureOverrideRequired); WINGET_DEFINE_RESOURCE_STRINGID(RelatedLink); WINGET_DEFINE_RESOURCE_STRINGID(RenameArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(ReparsePointsNotSupportedError); @@ -465,8 +466,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(VersionArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(VersionsArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(WaitArgumentDescription); - WINGET_DEFINE_RESOURCE_STRINGID(WindowsFeatureNotFoundOverride); - WINGET_DEFINE_RESOURCE_STRINGID(WindowsFeatureNotFoundOverrideRequired); + WINGET_DEFINE_RESOURCE_STRINGID(WindowsFeatureNotFound); WINGET_DEFINE_RESOURCE_STRINGID(WindowsFeaturesDependencies); WINGET_DEFINE_RESOURCE_STRINGID(WindowsLibrariesDependencies); WINGET_DEFINE_RESOURCE_STRINGID(WindowsStoreTerms); diff --git a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp index 262b536026..fcb164fab5 100644 --- a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp @@ -158,42 +158,55 @@ namespace AppInstaller::CLI::Workflow } HRESULT hr = S_OK; - bool continueOnFailure = context.Args.Contains(Execution::Args::Type::Force); - bool rebootRequired = false; + DismHelper dismHelper = DismHelper(); - // Do I need warn, info and error reporters for this function?\ + bool force = context.Args.Contains(Execution::Args::Type::Force); + bool rebootRequired = false; - // Pass info and warn so that we can show enabling and the status. If we don't have continue on failure, we error out and display in a separate. auto info = context.Reporter.Info(); auto warn = context.Reporter.Warn(); - DismHelper dismHelper = DismHelper(); - - context.Reporter.Info() << Resource::String::EnablingWindowsFeatures << std::endl; - rootDependencies.ApplyToType(DependencyType::WindowsFeature, [&hr, &continueOnFailure, &rebootRequired, &warn, &info, &dismHelper](Dependency dependency) + rootDependencies.ApplyToType(DependencyType::WindowsFeature, [&hr, &dismHelper, &info, &warn, &force, &rebootRequired](Dependency dependency) { - if (SUCCEEDED(hr) || continueOnFailure) + if (SUCCEEDED(hr) || force) { - info << "Enabling feature"; - auto featureName = dependency.Id(); DismHelper::WindowsFeature windowsFeature = dismHelper.CreateWindowsFeature(featureName); - if (windowsFeature.DoesExist() && !windowsFeature.IsEnabled()) + if (windowsFeature.DoesExist()) { - hr = windowsFeature.Enable(); - AICLI_LOG(Core, Info, << "Enabling Windows Feature " << featureName << " returned with HRESULT: " << hr); - if (hr == ERROR_SUCCESS_REBOOT_REQUIRED) + if (!windowsFeature.IsEnabled()) { - rebootRequired = true; + std::string featureDisplayName = AppInstaller::Utility::ConvertToUTF8(windowsFeature.GetDisplayName()); + Utility::LocIndView locIndFeatureName{ featureName }; + + info << Resource::String::EnablingWindowsFeature(Utility::LocIndView{ featureDisplayName }, locIndFeatureName) << std::endl; + + hr = windowsFeature.Enable(); + AICLI_LOG(Core, Info, << "Enabling Windows Feature [" << featureName << "] returned with HRESULT: " << hr); + + if (FAILED(hr)) + { + warn << Resource::String::FailedToEnableWindowsFeature(locIndFeatureName) << std::endl; + } + if (hr == ERROR_SUCCESS_REBOOT_REQUIRED || windowsFeature.GetRestartRequiredStatus() == DismRestartType::DismRestartRequired) + { + rebootRequired = true; + } } } + else + { + AICLI_LOG(Core, Info, << "Windows Feature [" << featureName << "] does not exist"); + hr = APPINSTALLER_CLI_ERROR_INSTALL_MISSING_DEPENDENCY; + warn << Resource::String::WindowsFeatureNotFound(Utility::LocIndView{ featureName }) << std::endl; + } } }); - if (FAILED(hr)) + if (FAILED(hr) || hr == APPINSTALLER_CLI_ERROR_INSTALL_MISSING_DEPENDENCY) { - if (continueOnFailure) + if (force) { context.Reporter.Warn() << Resource::String::FailedToEnableWindowsFeatureOverridden << std::endl; } @@ -205,7 +218,15 @@ namespace AppInstaller::CLI::Workflow } else if (rebootRequired) { - context.Reporter.Warn() << Resource::String::RebootRequiredForEnablingWindowsFeature << std::endl; + 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); + } } } } diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index 01efc8b099..baebf72eef 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -495,7 +495,6 @@ namespace AppInstaller::CLI::Workflow Workflow::GetDependenciesFromInstaller << Workflow::ReportDependencies(Resource::String::InstallAndUpgradeCommandsReportDependencies) << Workflow::ManagePackageDependencies(Resource::String::InstallAndUpgradeCommandsReportDependencies) << - Workflow::EnableWindowsFeaturesDependencies << Workflow::DownloadInstaller; } @@ -504,6 +503,7 @@ namespace AppInstaller::CLI::Workflow context << Workflow::CheckForUnsupportedArgs << Workflow::DownloadSinglePackage << + Workflow::EnableWindowsFeaturesDependencies << Workflow::InstallPackageInstaller; } diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index af9babc84f..077f9b59a6 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -1677,31 +1677,16 @@ Please specify one of them using the --source option to proceed. {0} package(s) have pins that prevent upgrade. Use the 'winget pin' command to view and edit pins. Using the --include-pinned argument may show more results. {Locked="winget pin","--include-pinned"} {0} is a placeholder replaced by an integer number of packages - - The following Windows Feature(s) were not found; proceeding due to --ignore-missing-dependencies. - "Windows Feature(s)" is the name of the options Windows features setting. -{Locked="--force"} -{Locked="--ignore-missing-dependencies"} - - - The following Windows Feature(s) were not found. To proceed with installation, use --ignore-missing-dependencies. - "Windows Feature(s)" is the name of the options Windows features setting. -{Locked="--force"} -{Locked="--ignore-missing-dependencies"} - - - Ignores missing Windows Feature dependencies - "Windows Feature" is the name of the options Windows features setting. - - - Enabling Windows Feature dependencies... + + The feature [{0}] was not found. + {Locked="{0}"} Error message displayed when a Windows feature was not found on the system. Failed to enable Windows Feature dependencies. To proceed with installation, use --force {Locked="--force"} - - Reboot required to fully enable the Windows Feature(s) + + Reboot required to fully enable the Windows Feature(s); to override this check use --force "Windows Feature(s)" is the name of the options Windows features setting. @@ -1709,4 +1694,16 @@ Please specify one of them using the --source option to proceed. "Windows Feature(s)" is the name of the options Windows features setting. {Locked="--force"} + + Enabling {0} [{1}]... + {Locked="{0}","{1}"} Message displayed to the user regarding which Windows Feature is being enabled. + + + Failed to enable [{0}] feature. + {Locked="{0}"} Error message displayed when a failure was encountered when enabling a Windows Feature. + + + Reboot required to fully enable the Windows Feature(s); proceeding due to --force + "Windows Feature(s)" is the name of the options Windows features setting. + \ No newline at end of file diff --git a/src/AppInstallerCLITests/WindowsFeature.cpp b/src/AppInstallerCLITests/WindowsFeature.cpp index 32bd04429c..158b541400 100644 --- a/src/AppInstallerCLITests/WindowsFeature.cpp +++ b/src/AppInstallerCLITests/WindowsFeature.cpp @@ -10,6 +10,7 @@ using namespace AppInstaller::CLI; using namespace AppInstaller::Settings; +using namespace AppInstaller::Utility; using namespace AppInstaller::WindowsFeature; using namespace TestCommon; @@ -108,6 +109,7 @@ TEST_CASE("InstallFlow_InvalidWindowsFeature", "[windowsFeature]") std::ostringstream installOutput; TestContext context{ installOutput, std::cin }; auto previousThreadGlobals = context.SetForCurrentThread(); + OverrideForShellExecute(context); context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); InstallCommand install({}); @@ -116,9 +118,11 @@ TEST_CASE("InstallFlow_InvalidWindowsFeature", "[windowsFeature]") REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_MISSING_DEPENDENCY); REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); - REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::WindowsFeatureNotFoundOverrideRequired).get()) != std::string::npos); - REQUIRE(installOutput.str().find("invalidFeature") != std::string::npos); - REQUIRE(installOutput.str().find("badFeature") != std::string::npos); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::WindowsFeatureNotFound(LocIndView{ "invalidFeature" })).get()) != std::string::npos); + + // "badFeature" should not be displayed as the flow should terminate after failing to find the first feature. + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::WindowsFeatureNotFound(LocIndView{ "badFeature" })).get()) == std::string::npos); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeatureOverrideRequired).get()) != std::string::npos); } TEST_CASE("InstallFlow_FailedToEnableWindowsFeature", "[windowsFeature]") @@ -184,6 +188,7 @@ 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(LocIndString{s_featureName})).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()); @@ -221,6 +226,42 @@ TEST_CASE("InstallFlow_RebootRequired", "[windowsFeature]") install.Execute(context); INFO(installOutput.str()); + REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL); + REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::RebootRequiredToEnableWindowsFeatureOverrideRequired).get()) != std::string::npos); +} + +TEST_CASE("InstallFlow_RebootRequired_Force", "[windowsFeature]") +{ + if (!AppInstaller::Runtime::IsRunningAsAdmin()) + { + WARN("Test requires admin privilege. Skipped."); + return; + } + + TestCommon::TempFile installResultPath("TestExeInstalled.txt"); + + TestCommon::TestUserSettings testSettings; + testSettings.Set(true); + + // Override with reboot required HRESULT. + TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(ERROR_SUCCESS_REBOOT_REQUIRED); + TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); + + std::ostringstream installOutput; + TestContext context{ installOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + OverrideForShellExecute(context); + context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string()); + context.Args.AddArg(Execution::Args::Type::Force); + + InstallCommand install({}); + install.Execute(context); + INFO(installOutput.str()); + + // Verify Installer is called and parameters are passed in. + REQUIRE(context.GetTerminationHR() == ERROR_SUCCESS); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::RebootRequiredToEnableWindowsFeatureOverridden).get()) != std::string::npos); REQUIRE(std::filesystem::exists(installResultPath.GetPath())); std::ifstream installResultFile(installResultPath.GetPath()); REQUIRE(installResultFile.is_open()); @@ -228,5 +269,4 @@ TEST_CASE("InstallFlow_RebootRequired", "[windowsFeature]") std::getline(installResultFile, installResultStr); REQUIRE(installResultStr.find("/custom") != std::string::npos); REQUIRE(installResultStr.find("/silentwithprogress") != std::string::npos); - REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::RebootRequiredForEnablingWindowsFeature).get()) != std::string::npos); -} +} \ No newline at end of file diff --git a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h index 7bf62d1454..38ff348e2a 100644 --- a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h +++ b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h @@ -6,7 +6,7 @@ namespace AppInstaller::WindowsFeature { /****************************************************************************\ - Declaration copied from DismApi.H to support enabling Windows Features. + Declaration copied from DismApi.h to support enabling Windows Features. Copyright (c) Microsoft Corporation. All rights reserved. @@ -114,22 +114,30 @@ namespace AppInstaller::WindowsFeature struct DismHelper { - - DismHelper(); - ~DismHelper() - { - Shutdown(); - }; - + /// + /// Struct representation of a single Windows Feature. + /// struct WindowsFeature { - bool DoesExist(); - bool IsEnabled(); - DismPackageFeatureState GetState() { return m_featureInfo->FeatureState; }; - std::wstring GetDisplayName() { return std::wstring { m_featureInfo->DisplayName }; } - DismRestartType GetRestartRequiredStatus() { return m_featureInfo->RestartRequired; } HRESULT Enable(); HRESULT Disable(); + bool DoesExist(); + bool IsEnabled(); + + std::wstring GetDisplayName() + { + return std::wstring{ m_featureInfo->DisplayName }; + } + + DismPackageFeatureState GetState() + { + return m_featureInfo->FeatureState; + } + + DismRestartType GetRestartRequiredStatus() + { + return m_featureInfo->RestartRequired; + } ~WindowsFeature() { @@ -142,17 +150,18 @@ namespace AppInstaller::WindowsFeature private: friend DismHelper; - WindowsFeature(const std::string& name, DismGetFeatureInfoPtr getFeatureInfoPtr, DismEnableFeaturePtr enableFeaturePtr, DismDisableFeaturePtr disableFeaturePtr, DismDeletePtr deletePtr, DismSession session); + WindowsFeature( + const std::string& name, + DismGetFeatureInfoPtr getFeatureInfoPtr, + DismEnableFeaturePtr enableFeaturePtr, + DismDisableFeaturePtr disableFeaturePtr, + DismDeletePtr deletePtr, + DismSession session); - WindowsFeature(const std::string& name) : m_featureName(name) - { - GetFeatureInfo(); - } + void GetFeatureInfo(); std::string m_featureName; DismFeatureInfo* m_featureInfo = nullptr; - void GetFeatureInfo(); - DismGetFeatureInfoPtr m_getFeatureInfoPtr = nullptr; DismEnableFeaturePtr m_enableFeature = nullptr; DismDisableFeaturePtr m_disableFeaturePtr = nullptr; @@ -160,16 +169,23 @@ namespace AppInstaller::WindowsFeature DismSession m_session; }; + DismHelper(); + + ~DismHelper() + { + Shutdown(); + }; + WindowsFeature CreateWindowsFeature(const std::string& name) { - return WindowsFeature(name, m_dismGetFeatureInfo, m_dismEnableFeature, m_dismDisableFeature, m_dismDelete, m_session); + return WindowsFeature(name, m_dismGetFeatureInfo, m_dismEnableFeature, m_dismDisableFeature, m_dismDelete, m_dismSession); } private: typedef UINT DismSession; wil::unique_hmodule m_module; - DismSession m_session = DISM_SESSION_DEFAULT; + DismSession m_dismSession = DISM_SESSION_DEFAULT; DismInitializePtr m_dismInitialize = nullptr; DismOpenSessionPtr m_dismOpenSession = nullptr; diff --git a/src/AppInstallerCommonCore/WindowsFeature.cpp b/src/AppInstallerCommonCore/WindowsFeature.cpp index d941814b94..6c773db9b0 100644 --- a/src/AppInstallerCommonCore/WindowsFeature.cpp +++ b/src/AppInstallerCommonCore/WindowsFeature.cpp @@ -88,7 +88,7 @@ namespace AppInstaller::WindowsFeature { if (m_dismOpenSession) { - LOG_IF_FAILED(m_dismOpenSession(DISM_ONLINE_IMAGE, NULL, NULL, &m_session)); + LOG_IF_FAILED(m_dismOpenSession(DISM_ONLINE_IMAGE, NULL, NULL, &m_dismSession)); } } @@ -100,6 +100,24 @@ namespace AppInstaller::WindowsFeature } } + DismHelper::WindowsFeature::WindowsFeature( + const std::string& name, + DismGetFeatureInfoPtr getFeatureInfoPtr, + DismEnableFeaturePtr enableFeaturePtr, + DismDisableFeaturePtr disableFeaturePtr, + DismDeletePtr deletePtr, + DismSession session) + { + m_featureName = name; + m_getFeatureInfoPtr = getFeatureInfoPtr; + m_enableFeature = enableFeaturePtr; + m_disableFeaturePtr = disableFeaturePtr; + m_deletePtr = deletePtr; + m_session = session; + + GetFeatureInfo(); + } + #ifndef AICLI_DISABLE_TEST_HOOKS static HRESULT* s_EnableWindowsFeatureResult_TestHook_Override = nullptr; @@ -165,7 +183,7 @@ namespace AppInstaller::WindowsFeature GetFeatureInfo(); DismPackageFeatureState featureState = GetState(); AICLI_LOG(Core, Verbose, << "Feature state of " << m_featureName << " is " << featureState); - return (featureState == DismStateInstalled || featureState == DismStateInstallPending); + return featureState == DismStateInstalled; } void DismHelper::WindowsFeature::GetFeatureInfo() @@ -175,16 +193,4 @@ namespace AppInstaller::WindowsFeature LOG_IF_FAILED(m_getFeatureInfoPtr(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, DismPackageNone, &m_featureInfo)); } } - - DismHelper::WindowsFeature::WindowsFeature(const std::string& name, DismGetFeatureInfoPtr getFeatureInfoPtr, DismEnableFeaturePtr enableFeaturePtr, DismDisableFeaturePtr disableFeaturePtr, DismDeletePtr deletePtr, DismSession session) - { - m_featureName = name; - m_getFeatureInfoPtr = getFeatureInfoPtr; - m_enableFeature = enableFeaturePtr; - m_disableFeaturePtr = disableFeaturePtr; - m_deletePtr = deletePtr; - m_session = session; - - GetFeatureInfo(); - } } \ No newline at end of file From d4d9430c27734f8e25051737695723d8e6e02f02 Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Fri, 3 Mar 2023 15:21:49 -0800 Subject: [PATCH 11/29] add close session --- src/AppInstallerCLITests/WindowsFeature.cpp | 21 ++++++++++--------- .../Public/winget/WindowsFeature.h | 6 +++++- src/AppInstallerCommonCore/WindowsFeature.cpp | 16 ++++++++++++++ 3 files changed, 32 insertions(+), 11 deletions(-) diff --git a/src/AppInstallerCLITests/WindowsFeature.cpp b/src/AppInstallerCLITests/WindowsFeature.cpp index 158b541400..91dc6c91e1 100644 --- a/src/AppInstallerCLITests/WindowsFeature.cpp +++ b/src/AppInstallerCLITests/WindowsFeature.cpp @@ -57,10 +57,15 @@ TEST_CASE("DisableEnableWindowsFeature", "[windowsFeature]") HRESULT enableResult = feature.Enable(); bool enableStatus = (enableResult == S_OK) || (enableResult == ERROR_SUCCESS_REBOOT_REQUIRED); REQUIRE(enableStatus); - REQUIRE(feature.IsEnabled()); + + if (!feature.IsEnabled()) + { + // If netfx3 is already enabled, this block will not be executed to avoid breaking this test. + REQUIRE((feature.GetRestartRequiredStatus() != DismRestartType::DismRestartNo)); + } } -TEST_CASE("InstallFlow_ValidWindowsFeature", "[windowsFeature]") +TEST_CASE("InstallFlow_ValidWindowsFeature_RebootRequired", "[windowsFeature]") { if (!AppInstaller::Runtime::IsRunningAsAdmin()) { @@ -83,14 +88,9 @@ TEST_CASE("InstallFlow_ValidWindowsFeature", "[windowsFeature]") install.Execute(context); INFO(installOutput.str()); - // Verify Installer is called and parameters are passed in. - REQUIRE(std::filesystem::exists(installResultPath.GetPath())); - std::ifstream installResultFile(installResultPath.GetPath()); - REQUIRE(installResultFile.is_open()); - std::string installResultStr; - std::getline(installResultFile, installResultStr); - REQUIRE(installResultStr.find("/custom") != std::string::npos); - REQUIRE(installResultStr.find("/silentwithprogress") != std::string::npos); + REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL); + REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::RebootRequiredToEnableWindowsFeatureOverrideRequired).get()) != std::string::npos); } TEST_CASE("InstallFlow_InvalidWindowsFeature", "[windowsFeature]") @@ -146,6 +146,7 @@ TEST_CASE("InstallFlow_FailedToEnableWindowsFeature", "[windowsFeature]") std::ostringstream installOutput; TestContext context{ installOutput, std::cin }; auto previousThreadGlobals = context.SetForCurrentThread(); + OverrideForShellExecute(context); context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); InstallCommand install({}); diff --git a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h index 38ff348e2a..c55dd2c1bc 100644 --- a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h +++ b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h @@ -105,7 +105,8 @@ namespace AppInstaller::WindowsFeature #endif // _DISMAPI_H_ using DismInitializePtr = HRESULT(WINAPI*)(int, PCWSTR, PCWSTR); - using DismOpenSessionPtr = HRESULT(WINAPI*)(PCWSTR, PCWSTR, PCWSTR, UINT*); + using DismOpenSessionPtr = HRESULT(WINAPI*)(PCWSTR, PCWSTR, PCWSTR, DismSession*); + using DismCloseSessionPtr = HRESULT(WINAPI*)(DismSession); using DismShutdownPtr = HRESULT(WINAPI*)(); using DismGetFeatureInfoPtr = HRESULT(WINAPI*)(UINT, PCWSTR, PCWSTR, DismPackageIdentifier, DismFeatureInfo**); using DismEnableFeaturePtr = HRESULT(WINAPI*)(UINT, PCWSTR, PCWSTR, DismPackageIdentifier, BOOL, PCWSTR*, UINT, BOOL, HANDLE, DISM_PROGRESS_CALLBACK, PVOID); @@ -173,6 +174,7 @@ namespace AppInstaller::WindowsFeature ~DismHelper() { + CloseSession(); Shutdown(); }; @@ -192,11 +194,13 @@ namespace AppInstaller::WindowsFeature DismGetFeatureInfoPtr m_dismGetFeatureInfo = nullptr; DismEnableFeaturePtr m_dismEnableFeature = nullptr; DismDisableFeaturePtr m_dismDisableFeature = nullptr; + DismCloseSessionPtr m_dismCloseSession = nullptr; DismShutdownPtr m_dismShutdown = nullptr; DismDeletePtr m_dismDelete = nullptr; void Initialize(); void OpenSession(); + void CloseSession(); void Shutdown(); }; } \ No newline at end of file diff --git a/src/AppInstallerCommonCore/WindowsFeature.cpp b/src/AppInstallerCommonCore/WindowsFeature.cpp index 6c773db9b0..25f8abee88 100644 --- a/src/AppInstallerCommonCore/WindowsFeature.cpp +++ b/src/AppInstallerCommonCore/WindowsFeature.cpp @@ -64,6 +64,14 @@ namespace AppInstaller::WindowsFeature return; } + m_dismCloseSession = + reinterpret_cast(GetProcAddress(m_module.get(), "DismCloseSession")); + if (!m_dismCloseSession) + { + AICLI_LOG(Core, Verbose, << "Could not get proc address of DismCloseSession"); + return; + } + m_dismShutdown = reinterpret_cast(GetProcAddress(m_module.get(), "DismShutdown")); if (!m_dismGetFeatureInfo) @@ -92,6 +100,14 @@ namespace AppInstaller::WindowsFeature } } + void DismHelper::CloseSession() + { + if (m_dismCloseSession) + { + LOG_IF_FAILED(m_dismCloseSession(m_dismSession)); + } + } + void DismHelper::Shutdown() { if (m_dismShutdown) From a4e0eee655319c3a4c3e2078ab6dc03b3fc02d0e Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Mon, 6 Mar 2023 12:14:21 -0800 Subject: [PATCH 12/29] set all dependencies to false --- .../TestData/InstallFlowTest_WindowsFeatures.yaml | 1 + src/AppInstallerCLITests/WindowsFeature.cpp | 11 ++++++++--- src/AppInstallerCommonCore/WindowsFeature.cpp | 2 +- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml b/src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml index 02edb938d1..76e070081f 100644 --- a/src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml +++ b/src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml @@ -19,5 +19,6 @@ Installers: Dependencies: WindowsFeatures: - netfx3 + - tftp ManifestType: singleton ManifestVersion: 1.4.0 \ No newline at end of file diff --git a/src/AppInstallerCLITests/WindowsFeature.cpp b/src/AppInstallerCLITests/WindowsFeature.cpp index 91dc6c91e1..b63f5a0e25 100644 --- a/src/AppInstallerCLITests/WindowsFeature.cpp +++ b/src/AppInstallerCLITests/WindowsFeature.cpp @@ -88,9 +88,14 @@ TEST_CASE("InstallFlow_ValidWindowsFeature_RebootRequired", "[windowsFeature]") install.Execute(context); INFO(installOutput.str()); - REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL); - REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); - REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::RebootRequiredToEnableWindowsFeatureOverrideRequired).get()) != std::string::npos); + REQUIRE(context.GetTerminationHR() == ERROR_SUCCESS); + REQUIRE(std::filesystem::exists(installResultPath.GetPath())); + std::ifstream installResultFile(installResultPath.GetPath()); + REQUIRE(installResultFile.is_open()); + std::string installResultStr; + std::getline(installResultFile, installResultStr); + REQUIRE(installResultStr.find("/custom") != std::string::npos); + REQUIRE(installResultStr.find("/silentwithprogress") != std::string::npos); } TEST_CASE("InstallFlow_InvalidWindowsFeature", "[windowsFeature]") diff --git a/src/AppInstallerCommonCore/WindowsFeature.cpp b/src/AppInstallerCommonCore/WindowsFeature.cpp index 25f8abee88..dffdb04172 100644 --- a/src/AppInstallerCommonCore/WindowsFeature.cpp +++ b/src/AppInstallerCommonCore/WindowsFeature.cpp @@ -154,7 +154,7 @@ namespace AppInstaller::WindowsFeature HRESULT hr = ERROR_PROC_NOT_FOUND; if (m_enableFeature) { - hr = m_enableFeature(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, DismPackageNone, FALSE, NULL, NULL, TRUE, NULL, NULL, NULL); + hr = m_enableFeature(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, DismPackageNone, FALSE, NULL, NULL, FALSE, NULL, NULL, NULL); LOG_IF_FAILED(hr); } From b5b2ae46aca6c8c6c00d298258d83b70bfd1dcf3 Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Mon, 6 Mar 2023 13:57:43 -0800 Subject: [PATCH 13/29] extend sleep time --- src/AppInstallerCLITests/Run-TestsInPackage.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/AppInstallerCLITests/Run-TestsInPackage.ps1 b/src/AppInstallerCLITests/Run-TestsInPackage.ps1 index 9234c9dab2..529ea818ba 100644 --- a/src/AppInstallerCLITests/Run-TestsInPackage.ps1 +++ b/src/AppInstallerCLITests/Run-TestsInPackage.ps1 @@ -46,7 +46,7 @@ function Wait-ForFileClose([string]$Path) { $Local:FileInfo = [System.IO.FileInfo]::new($Path) $Local:SleepCount = 0 - $Local:SleepCountMax = 600 + $Local:SleepCountMax = 1200 while ($Local:SleepCount -lt $Local:SleepCountMax) { From 61a41432cdfc15a2f97afc9f1d0f37f083279cb6 Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Mon, 6 Mar 2023 15:45:22 -0800 Subject: [PATCH 14/29] move to E2E tests --- .../FeaturesCommand.cs | 1 + src/AppInstallerCLIE2ETests/InstallCommand.cs | 36 +++++++++++ .../TestExeInstaller.WindowsFeature.yaml | 22 +++++++ .../WinGetSettingsHelper.cs | 1 + .../Run-TestsInPackage.ps1 | 2 +- .../InstallFlowTest_WindowsFeatures.yaml | 1 - src/AppInstallerCLITests/WindowsFeature.cpp | 61 ------------------- 7 files changed, 61 insertions(+), 63 deletions(-) create mode 100644 src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller.WindowsFeature.yaml diff --git a/src/AppInstallerCLIE2ETests/FeaturesCommand.cs b/src/AppInstallerCLIE2ETests/FeaturesCommand.cs index a729ce33d8..bb581138ac 100644 --- a/src/AppInstallerCLIE2ETests/FeaturesCommand.cs +++ b/src/AppInstallerCLIE2ETests/FeaturesCommand.cs @@ -53,6 +53,7 @@ public void EnableExperimentalFeatures() WinGetSettingsHelper.ConfigureFeature("experimentalCmd", true); WinGetSettingsHelper.ConfigureFeature("directMSI", true); WinGetSettingsHelper.ConfigureFeature("uninstallPreviousArgument", true); + WinGetSettingsHelper.ConfigureFeature("windowsFeature", true); var result = TestCommon.RunAICLICommand("features", string.Empty); Assert.True(result.StdOut.Contains("Enabled")); } diff --git a/src/AppInstallerCLIE2ETests/InstallCommand.cs b/src/AppInstallerCLIE2ETests/InstallCommand.cs index 3f9505ac2a..14b3e599ae 100644 --- a/src/AppInstallerCLIE2ETests/InstallCommand.cs +++ b/src/AppInstallerCLIE2ETests/InstallCommand.cs @@ -14,6 +14,15 @@ namespace AppInstallerCLIE2ETests /// public class InstallCommand : BaseCommand { + /// + /// One time setup. + /// + [OneTimeSetUp] + public void OneTimeSetup() + { + WinGetSettingsHelper.ConfigureFeature("windowsFeature", true); + } + /// /// Set up. /// @@ -613,5 +622,32 @@ public void InstallExeWithLatestInstalledWithForce() Assert.True(TestCommon.VerifyTestExeInstalledAndCleanup(baseDir)); Assert.True(TestCommon.VerifyTestExeInstalledAndCleanup(installDir, "/execustom")); } + + /// + /// Test install a package with a Windows Feature dependency that requires a reboot. + /// + [Test] + public void InstallWithWindowsFeatureDependency_RebootRequired() + { + var testDir = TestCommon.GetRandomTestDir(); + var installResult = TestCommon.RunAICLICommand("install", $"AppInstallerTest.WindowsFeature -l {testDir}"); + Assert.AreEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL, installResult.ExitCode); + Assert.True(installResult.StdOut.Contains("Reboot required to fully enable the Windows Feature(s); to override this check use --force")); + } + + /// + /// Test install a package with a Windows Feature dependency using the force argument. + /// + [Test] + public void InstallWithWindowsFeatureDependencyForce() + { + var testDir = TestCommon.GetRandomTestDir(); + var installResult = TestCommon.RunAICLICommand("install", $"AppInstallerTest.WindowsFeature --force -l {testDir}"); + + Assert.AreEqual(Constants.ErrorCode.S_OK, installResult.ExitCode); + Assert.True(installResult.StdOut.Contains("Reboot required to fully enable the Windows Feature(s); proceeding due to --force")); + Assert.True(installResult.StdOut.Contains("Successfully installed")); + Assert.True(TestCommon.VerifyTestExeInstalledAndCleanup(testDir)); + } } } \ No newline at end of file diff --git a/src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller.WindowsFeature.yaml b/src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller.WindowsFeature.yaml new file mode 100644 index 0000000000..226a78a714 --- /dev/null +++ b/src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller.WindowsFeature.yaml @@ -0,0 +1,22 @@ +PackageIdentifier: AppInstallerTest.WindowsFeature +PackageVersion: 1.0.0.0 +PackageLocale: en-US +PackageName: TestWindowsFeature +ShortDescription: Installs exe installer with valid Windows Features dependencies +Publisher: Microsoft Corporation +License: Test +Installers: + - Architecture: x86 + InstallerUrl: https://localhost:5001/TestKit/AppInstallerTestExeInstaller/AppInstallerTestExeInstaller.exe + InstallerType: exe + InstallerSha256: + InstallerSwitches: + Custom: /custom /scope=machine + SilentWithProgress: /silentwithprogress + Silent: /silence + Update: /update + Dependencies: + WindowsFeatures: + - netfx3 +ManifestType: singleton +ManifestVersion: 1.4.0 \ No newline at end of file diff --git a/src/AppInstallerCLIE2ETests/WinGetSettingsHelper.cs b/src/AppInstallerCLIE2ETests/WinGetSettingsHelper.cs index da7fea3b00..656617537c 100644 --- a/src/AppInstallerCLIE2ETests/WinGetSettingsHelper.cs +++ b/src/AppInstallerCLIE2ETests/WinGetSettingsHelper.cs @@ -189,6 +189,7 @@ public static void InitializeAllFeatures(bool status) ConfigureFeature("experimentalCmd", status); ConfigureFeature("dependencies", status); ConfigureFeature("directMSI", status); + ConfigureFeature("windowsFeature", status); } } } diff --git a/src/AppInstallerCLITests/Run-TestsInPackage.ps1 b/src/AppInstallerCLITests/Run-TestsInPackage.ps1 index 529ea818ba..9234c9dab2 100644 --- a/src/AppInstallerCLITests/Run-TestsInPackage.ps1 +++ b/src/AppInstallerCLITests/Run-TestsInPackage.ps1 @@ -46,7 +46,7 @@ function Wait-ForFileClose([string]$Path) { $Local:FileInfo = [System.IO.FileInfo]::new($Path) $Local:SleepCount = 0 - $Local:SleepCountMax = 1200 + $Local:SleepCountMax = 600 while ($Local:SleepCount -lt $Local:SleepCountMax) { diff --git a/src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml b/src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml index 76e070081f..02edb938d1 100644 --- a/src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml +++ b/src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml @@ -19,6 +19,5 @@ Installers: Dependencies: WindowsFeatures: - netfx3 - - tftp ManifestType: singleton ManifestVersion: 1.4.0 \ No newline at end of file diff --git a/src/AppInstallerCLITests/WindowsFeature.cpp b/src/AppInstallerCLITests/WindowsFeature.cpp index b63f5a0e25..15fd7d30a9 100644 --- a/src/AppInstallerCLITests/WindowsFeature.cpp +++ b/src/AppInstallerCLITests/WindowsFeature.cpp @@ -14,8 +14,6 @@ using namespace AppInstaller::Utility; using namespace AppInstaller::WindowsFeature; using namespace TestCommon; -// IMPORTANT: These tests require elevation and will modify your Windows Feature settings. - const std::string s_featureName = "netfx3"; TEST_CASE("GetWindowsFeature", "[windowsFeature]") @@ -39,65 +37,6 @@ TEST_CASE("GetWindowsFeature", "[windowsFeature]") REQUIRE_FALSE(invalidFeature.DoesExist()); } -TEST_CASE("DisableEnableWindowsFeature", "[windowsFeature]") -{ - if (!AppInstaller::Runtime::IsRunningAsAdmin()) - { - WARN("Test requires admin privilege. Skipped."); - return; - } - - DismHelper dismHelper = DismHelper(); - DismHelper::WindowsFeature feature = dismHelper.CreateWindowsFeature(s_featureName); - HRESULT disableResult = feature.Disable(); - bool disableStatus = (disableResult == S_OK || disableResult == ERROR_SUCCESS_REBOOT_REQUIRED); - REQUIRE(disableStatus); - REQUIRE_FALSE(feature.IsEnabled()); - - HRESULT enableResult = feature.Enable(); - bool enableStatus = (enableResult == S_OK) || (enableResult == ERROR_SUCCESS_REBOOT_REQUIRED); - REQUIRE(enableStatus); - - if (!feature.IsEnabled()) - { - // If netfx3 is already enabled, this block will not be executed to avoid breaking this test. - REQUIRE((feature.GetRestartRequiredStatus() != DismRestartType::DismRestartNo)); - } -} - -TEST_CASE("InstallFlow_ValidWindowsFeature_RebootRequired", "[windowsFeature]") -{ - if (!AppInstaller::Runtime::IsRunningAsAdmin()) - { - WARN("Test requires admin privilege. Skipped."); - return; - } - - TestCommon::TempFile installResultPath("TestExeInstalled.txt"); - - TestCommon::TestUserSettings testSettings; - testSettings.Set(true); - - std::ostringstream installOutput; - TestContext context{ installOutput, std::cin }; - auto previousThreadGlobals = context.SetForCurrentThread(); - OverrideForShellExecute(context); - context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string()); - - InstallCommand install({}); - install.Execute(context); - INFO(installOutput.str()); - - REQUIRE(context.GetTerminationHR() == ERROR_SUCCESS); - REQUIRE(std::filesystem::exists(installResultPath.GetPath())); - std::ifstream installResultFile(installResultPath.GetPath()); - REQUIRE(installResultFile.is_open()); - std::string installResultStr; - std::getline(installResultFile, installResultStr); - REQUIRE(installResultStr.find("/custom") != std::string::npos); - REQUIRE(installResultStr.find("/silentwithprogress") != std::string::npos); -} - TEST_CASE("InstallFlow_InvalidWindowsFeature", "[windowsFeature]") { if (!AppInstaller::Runtime::IsRunningAsAdmin()) From 3ba134a75df555cae04a166633a2b68e67aec27c Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Mon, 6 Mar 2023 19:35:06 -0800 Subject: [PATCH 15/29] fix tests --- .../Workflows/DependenciesFlow.cpp | 1 + src/AppInstallerCLIE2ETests/InstallCommand.cs | 12 +++++------ .../TestExeInstaller.WindowsFeature.yaml | 1 + src/AppInstallerCLITests/WindowsFeature.cpp | 21 ------------------- 4 files changed, 7 insertions(+), 28 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp index fcb164fab5..d64bc8b16b 100644 --- a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp @@ -197,6 +197,7 @@ namespace AppInstaller::CLI::Workflow } 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; warn << Resource::String::WindowsFeatureNotFound(Utility::LocIndView{ featureName }) << std::endl; diff --git a/src/AppInstallerCLIE2ETests/InstallCommand.cs b/src/AppInstallerCLIE2ETests/InstallCommand.cs index 14b3e599ae..6136cd924f 100644 --- a/src/AppInstallerCLIE2ETests/InstallCommand.cs +++ b/src/AppInstallerCLIE2ETests/InstallCommand.cs @@ -624,28 +624,26 @@ public void InstallExeWithLatestInstalledWithForce() } /// - /// Test install a package with a Windows Feature dependency that requires a reboot. + /// Test install a package with an invalid Windows Feature dependency. /// [Test] - public void InstallWithWindowsFeatureDependency_RebootRequired() + public void InstallWithWindowsFeatureDependency_FeatureNotFound() { var testDir = TestCommon.GetRandomTestDir(); var installResult = TestCommon.RunAICLICommand("install", $"AppInstallerTest.WindowsFeature -l {testDir}"); - Assert.AreEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL, installResult.ExitCode); - Assert.True(installResult.StdOut.Contains("Reboot required to fully enable the Windows Feature(s); to override this check use --force")); + Assert.AreEqual(Constants.ErrorCode.ERROR_INSTALL_MISSING_DEPENDENCY, installResult.ExitCode); + Assert.True(installResult.StdOut.Contains("The feature [invalidFeature] was not found.")); } /// /// Test install a package with a Windows Feature dependency using the force argument. /// [Test] - public void InstallWithWindowsFeatureDependencyForce() + public void InstallWithWindowsFeatureDependency_Force() { var testDir = TestCommon.GetRandomTestDir(); var installResult = TestCommon.RunAICLICommand("install", $"AppInstallerTest.WindowsFeature --force -l {testDir}"); - Assert.AreEqual(Constants.ErrorCode.S_OK, installResult.ExitCode); - Assert.True(installResult.StdOut.Contains("Reboot required to fully enable the Windows Feature(s); proceeding due to --force")); Assert.True(installResult.StdOut.Contains("Successfully installed")); Assert.True(TestCommon.VerifyTestExeInstalledAndCleanup(testDir)); } diff --git a/src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller.WindowsFeature.yaml b/src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller.WindowsFeature.yaml index 226a78a714..f67973f7d0 100644 --- a/src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller.WindowsFeature.yaml +++ b/src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller.WindowsFeature.yaml @@ -18,5 +18,6 @@ Installers: Dependencies: WindowsFeatures: - netfx3 + - invalidFeature ManifestType: singleton ManifestVersion: 1.4.0 \ No newline at end of file diff --git a/src/AppInstallerCLITests/WindowsFeature.cpp b/src/AppInstallerCLITests/WindowsFeature.cpp index 15fd7d30a9..7aba5b86f1 100644 --- a/src/AppInstallerCLITests/WindowsFeature.cpp +++ b/src/AppInstallerCLITests/WindowsFeature.cpp @@ -16,27 +16,6 @@ using namespace TestCommon; const std::string s_featureName = "netfx3"; -TEST_CASE("GetWindowsFeature", "[windowsFeature]") -{ - if (!AppInstaller::Runtime::IsRunningAsAdmin()) - { - WARN("Test requires admin privilege. Skipped."); - return; - } - - DismHelper dismHelper = DismHelper(); - - DismHelper::WindowsFeature validFeature = dismHelper.CreateWindowsFeature(s_featureName); - REQUIRE(validFeature.DoesExist()); - - DismHelper::WindowsFeature validFeatureWithCasing = dismHelper.CreateWindowsFeature("NeTFX3"); - REQUIRE(validFeatureWithCasing.DoesExist()); - - - DismHelper::WindowsFeature invalidFeature = dismHelper.CreateWindowsFeature("invalidFeature"); - REQUIRE_FALSE(invalidFeature.DoesExist()); -} - TEST_CASE("InstallFlow_InvalidWindowsFeature", "[windowsFeature]") { if (!AppInstaller::Runtime::IsRunningAsAdmin()) From f23ccb7cf9b61ecc71a4ae51d3cae31abdddd46f Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Tue, 7 Mar 2023 00:35:30 -0800 Subject: [PATCH 16/29] comment out windows feature tests --- .../Workflows/DependenciesFlow.cpp | 1 + src/AppInstallerCLIE2ETests/InstallCommand.cs | 2 +- .../TestExeInstaller.WindowsFeature.yaml | 28 +- ...nstallFlowTest_InvalidWindowsFeatures.yaml | 6 +- .../InstallFlowTest_WindowsFeatures.yaml | 5 +- src/AppInstallerCLITests/WindowsFeature.cpp | 356 +++++++++--------- src/AppInstallerCommonCore/WindowsFeature.cpp | 20 +- 7 files changed, 209 insertions(+), 209 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp index d64bc8b16b..7baedb5c8e 100644 --- a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp @@ -189,6 +189,7 @@ namespace AppInstaller::CLI::Workflow { warn << Resource::String::FailedToEnableWindowsFeature(locIndFeatureName) << std::endl; } + if (hr == ERROR_SUCCESS_REBOOT_REQUIRED || windowsFeature.GetRestartRequiredStatus() == DismRestartType::DismRestartRequired) { rebootRequired = true; diff --git a/src/AppInstallerCLIE2ETests/InstallCommand.cs b/src/AppInstallerCLIE2ETests/InstallCommand.cs index 6136cd924f..be8e46e710 100644 --- a/src/AppInstallerCLIE2ETests/InstallCommand.cs +++ b/src/AppInstallerCLIE2ETests/InstallCommand.cs @@ -642,7 +642,7 @@ public void InstallWithWindowsFeatureDependency_FeatureNotFound() public void InstallWithWindowsFeatureDependency_Force() { var testDir = TestCommon.GetRandomTestDir(); - var installResult = TestCommon.RunAICLICommand("install", $"AppInstallerTest.WindowsFeature --force -l {testDir}"); + var installResult = TestCommon.RunAICLICommand("install", $"AppInstallerTest.WindowsFeature --silent --force -l {testDir}"); Assert.AreEqual(Constants.ErrorCode.S_OK, installResult.ExitCode); Assert.True(installResult.StdOut.Contains("Successfully installed")); Assert.True(TestCommon.VerifyTestExeInstalledAndCleanup(testDir)); diff --git a/src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller.WindowsFeature.yaml b/src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller.WindowsFeature.yaml index f67973f7d0..e0245698b0 100644 --- a/src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller.WindowsFeature.yaml +++ b/src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller.WindowsFeature.yaml @@ -6,18 +6,20 @@ ShortDescription: Installs exe installer with valid Windows Features dependencie Publisher: Microsoft Corporation License: Test Installers: - - Architecture: x86 - InstallerUrl: https://localhost:5001/TestKit/AppInstallerTestExeInstaller/AppInstallerTestExeInstaller.exe - InstallerType: exe - InstallerSha256: - InstallerSwitches: - Custom: /custom /scope=machine - SilentWithProgress: /silentwithprogress - Silent: /silence - Update: /update - Dependencies: - WindowsFeatures: - - netfx3 - - invalidFeature + - Architecture: x64 + InstallerUrl: https://localhost:5001/TestKit/AppInstallerTestExeInstaller/AppInstallerTestExeInstaller.exe + InstallerType: exe + InstallerSha256: + InstallerSwitches: + SilentWithProgress: /exeswp + Silent: /exesilent + Interactive: /exeinteractive + Language: /exeenus + Log: /LogFile + InstallLocation: /InstallDir + Dependencies: + WindowsFeatures: + - netfx3 + - invalidFeature ManifestType: singleton ManifestVersion: 1.4.0 \ No newline at end of file diff --git a/src/AppInstallerCLITests/TestData/InstallFlowTest_InvalidWindowsFeatures.yaml b/src/AppInstallerCLITests/TestData/InstallFlowTest_InvalidWindowsFeatures.yaml index 7f29afc36e..655a44b211 100644 --- a/src/AppInstallerCLITests/TestData/InstallFlowTest_InvalidWindowsFeatures.yaml +++ b/src/AppInstallerCLITests/TestData/InstallFlowTest_InvalidWindowsFeatures.yaml @@ -1,10 +1,9 @@ -PackageIdentifier: AppInstallerCliTest.TestExeInstaller.InvalidWindowsFeatures +PackageIdentifier: AppInstallerCliTest.InvalidWindowsFeatures PackageVersion: 1.0.0.0 PackageLocale: en-US PackageName: AppInstaller Test Installer ShortDescription: Installs exe installer with invalid Windows Features dependencies Publisher: Microsoft Corporation -Moniker: AICLITestZip License: Test Installers: - Architecture: x86 @@ -18,8 +17,7 @@ Installers: Update: /update Dependencies: WindowsFeatures: - - netfx3 - invalidFeature - - badFeature + - netfx3 ManifestType: singleton ManifestVersion: 1.4.0 \ No newline at end of file diff --git a/src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml b/src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml index 02edb938d1..9ad0c0b434 100644 --- a/src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml +++ b/src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml @@ -1,10 +1,9 @@ -PackageIdentifier: AppInstallerCliTest.TestExeInstaller.WindowsFeatures +PackageIdentifier: AppInstallerCliTest.WindowsFeatures PackageVersion: 1.0.0.0 PackageLocale: en-US PackageName: AppInstaller Test Installer -ShortDescription: Installs exe installer with invalid Windows Features dependencies +ShortDescription: Installs exe installer with valid Windows Features dependencies Publisher: Microsoft Corporation -Moniker: AICLITestZip License: Test Installers: - Architecture: x86 diff --git a/src/AppInstallerCLITests/WindowsFeature.cpp b/src/AppInstallerCLITests/WindowsFeature.cpp index 7aba5b86f1..39e23f52f7 100644 --- a/src/AppInstallerCLITests/WindowsFeature.cpp +++ b/src/AppInstallerCLITests/WindowsFeature.cpp @@ -16,181 +16,181 @@ using namespace TestCommon; const std::string s_featureName = "netfx3"; -TEST_CASE("InstallFlow_InvalidWindowsFeature", "[windowsFeature]") -{ - if (!AppInstaller::Runtime::IsRunningAsAdmin()) - { - WARN("Test requires admin privilege. Skipped."); - return; - } - - TestCommon::TempFile installResultPath("TestExeInstalled.txt"); - - TestCommon::TestUserSettings testSettings; - testSettings.Set(true); - - std::ostringstream installOutput; - TestContext context{ installOutput, std::cin }; - auto previousThreadGlobals = context.SetForCurrentThread(); - OverrideForShellExecute(context); - context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); - - InstallCommand install({}); - install.Execute(context); - INFO(installOutput.str()); - - REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_MISSING_DEPENDENCY); - REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); - REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::WindowsFeatureNotFound(LocIndView{ "invalidFeature" })).get()) != std::string::npos); - - // "badFeature" should not be displayed as the flow should terminate after failing to find the first feature. - REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::WindowsFeatureNotFound(LocIndView{ "badFeature" })).get()) == std::string::npos); - REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeatureOverrideRequired).get()) != std::string::npos); -} - -TEST_CASE("InstallFlow_FailedToEnableWindowsFeature", "[windowsFeature]") -{ - if (!AppInstaller::Runtime::IsRunningAsAdmin()) - { - WARN("Test requires admin privilege. Skipped."); - return; - } - - TestCommon::TempFile installResultPath("TestExeInstalled.txt"); - - TestCommon::TestUserSettings testSettings; - testSettings.Set(true); - - // Override with arbitrary DISM api error (DISMAPI_E_DISMAPI_NOT_INITIALIZED) and make windows feature discoverable. - HRESULT dismErrorResult = 0xc0040001; - TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(dismErrorResult); - TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); - - std::ostringstream installOutput; - TestContext context{ installOutput, std::cin }; - auto previousThreadGlobals = context.SetForCurrentThread(); - OverrideForShellExecute(context); - context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); - - InstallCommand install({}); - install.Execute(context); - INFO(installOutput.str()); - - REQUIRE(context.GetTerminationHR() == dismErrorResult); - REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); - REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeatureOverrideRequired).get()) != std::string::npos); -} - -TEST_CASE("InstallFlow_FailedToEnableWindowsFeature_Force", "[windowsFeature]") -{ - if (!AppInstaller::Runtime::IsRunningAsAdmin()) - { - WARN("Test requires admin privilege. Skipped."); - return; - } - - TestCommon::TempFile installResultPath("TestExeInstalled.txt"); - - TestCommon::TestUserSettings testSettings; - testSettings.Set(true); - - // Override with arbitrary DISM api error (DISMAPI_E_DISMAPI_NOT_INITIALIZED) and make windows feature discoverable. - HRESULT dismErrorResult = 0xc0040001; - TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(dismErrorResult); - TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); - - std::ostringstream installOutput; - TestContext context{ installOutput, std::cin }; - auto previousThreadGlobals = context.SetForCurrentThread(); - OverrideForShellExecute(context); - context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); - context.Args.AddArg(Execution::Args::Type::Force); - - InstallCommand install({}); - install.Execute(context); - INFO(installOutput.str()); - - // Verify Installer is called and parameters are passed in. - REQUIRE(context.GetTerminationHR() == ERROR_SUCCESS); - REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeature(LocIndString{s_featureName})).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()); - REQUIRE(installResultFile.is_open()); - std::string installResultStr; - std::getline(installResultFile, installResultStr); - REQUIRE(installResultStr.find("/custom") != std::string::npos); - REQUIRE(installResultStr.find("/silentwithprogress") != std::string::npos); -} - -TEST_CASE("InstallFlow_RebootRequired", "[windowsFeature]") -{ - if (!AppInstaller::Runtime::IsRunningAsAdmin()) - { - WARN("Test requires admin privilege. Skipped."); - return; - } - - TestCommon::TempFile installResultPath("TestExeInstalled.txt"); - - TestCommon::TestUserSettings testSettings; - testSettings.Set(true); - - // Override with reboot required HRESULT. - TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(ERROR_SUCCESS_REBOOT_REQUIRED); - TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); - - std::ostringstream installOutput; - TestContext context{ installOutput, std::cin }; - auto previousThreadGlobals = context.SetForCurrentThread(); - OverrideForShellExecute(context); - context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string()); - - InstallCommand install({}); - install.Execute(context); - INFO(installOutput.str()); - - REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL); - REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); - REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::RebootRequiredToEnableWindowsFeatureOverrideRequired).get()) != std::string::npos); -} - -TEST_CASE("InstallFlow_RebootRequired_Force", "[windowsFeature]") -{ - if (!AppInstaller::Runtime::IsRunningAsAdmin()) - { - WARN("Test requires admin privilege. Skipped."); - return; - } - - TestCommon::TempFile installResultPath("TestExeInstalled.txt"); - - TestCommon::TestUserSettings testSettings; - testSettings.Set(true); - - // Override with reboot required HRESULT. - TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(ERROR_SUCCESS_REBOOT_REQUIRED); - TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); - - std::ostringstream installOutput; - TestContext context{ installOutput, std::cin }; - auto previousThreadGlobals = context.SetForCurrentThread(); - OverrideForShellExecute(context); - context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string()); - context.Args.AddArg(Execution::Args::Type::Force); - - InstallCommand install({}); - install.Execute(context); - INFO(installOutput.str()); - - // Verify Installer is called and parameters are passed in. - REQUIRE(context.GetTerminationHR() == ERROR_SUCCESS); - REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::RebootRequiredToEnableWindowsFeatureOverridden).get()) != std::string::npos); - REQUIRE(std::filesystem::exists(installResultPath.GetPath())); - std::ifstream installResultFile(installResultPath.GetPath()); - REQUIRE(installResultFile.is_open()); - std::string installResultStr; - std::getline(installResultFile, installResultStr); - REQUIRE(installResultStr.find("/custom") != std::string::npos); - REQUIRE(installResultStr.find("/silentwithprogress") != std::string::npos); -} \ No newline at end of file +//TEST_CASE("InstallFlow_InvalidWindowsFeature", "[windowsFeature]") +//{ +// if (!AppInstaller::Runtime::IsRunningAsAdmin()) +// { +// WARN("Test requires admin privilege. Skipped."); +// return; +// } +// +// TestCommon::TempFile installResultPath("TestExeInstalled.txt"); +// +// TestCommon::TestUserSettings testSettings; +// testSettings.Set(true); +// +// std::ostringstream installOutput; +// TestContext context{ installOutput, std::cin }; +// auto previousThreadGlobals = context.SetForCurrentThread(); +// OverrideForShellExecute(context); +// context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); +// +// InstallCommand install({}); +// install.Execute(context); +// INFO(installOutput.str()); +// +// REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_MISSING_DEPENDENCY); +// REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); +// REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::WindowsFeatureNotFound(LocIndView{ "invalidFeature" })).get()) != std::string::npos); +// +// // "badFeature" should not be displayed as the flow should terminate after failing to find the first feature. +// REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::WindowsFeatureNotFound(LocIndView{ "badFeature" })).get()) == std::string::npos); +// REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeatureOverrideRequired).get()) != std::string::npos); +//} +// +//TEST_CASE("InstallFlow_FailedToEnableWindowsFeature", "[windowsFeature]") +//{ +// if (!AppInstaller::Runtime::IsRunningAsAdmin()) +// { +// WARN("Test requires admin privilege. Skipped."); +// return; +// } +// +// TestCommon::TempFile installResultPath("TestExeInstalled.txt"); +// +// TestCommon::TestUserSettings testSettings; +// testSettings.Set(true); +// +// // Override with arbitrary DISM api error (DISMAPI_E_DISMAPI_NOT_INITIALIZED) and make windows feature discoverable. +// HRESULT dismErrorResult = 0xc0040001; +// TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(dismErrorResult); +// TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); +// +// std::ostringstream installOutput; +// TestContext context{ installOutput, std::cin }; +// auto previousThreadGlobals = context.SetForCurrentThread(); +// OverrideForShellExecute(context); +// context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); +// +// InstallCommand install({}); +// install.Execute(context); +// INFO(installOutput.str()); +// +// REQUIRE(context.GetTerminationHR() == dismErrorResult); +// REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); +// REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeatureOverrideRequired).get()) != std::string::npos); +//} +// +//TEST_CASE("InstallFlow_FailedToEnableWindowsFeature_Force", "[windowsFeature]") +//{ +// if (!AppInstaller::Runtime::IsRunningAsAdmin()) +// { +// WARN("Test requires admin privilege. Skipped."); +// return; +// } +// +// TestCommon::TempFile installResultPath("TestExeInstalled.txt"); +// +// TestCommon::TestUserSettings testSettings; +// testSettings.Set(true); +// +// // Override with arbitrary DISM api error (DISMAPI_E_DISMAPI_NOT_INITIALIZED) and make windows feature discoverable. +// HRESULT dismErrorResult = 0xc0040001; +// TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(dismErrorResult); +// TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); +// +// std::ostringstream installOutput; +// TestContext context{ installOutput, std::cin }; +// auto previousThreadGlobals = context.SetForCurrentThread(); +// OverrideForShellExecute(context); +// context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); +// context.Args.AddArg(Execution::Args::Type::Force); +// +// InstallCommand install({}); +// install.Execute(context); +// INFO(installOutput.str()); +// +// // Verify Installer is called and parameters are passed in. +// REQUIRE(context.GetTerminationHR() == ERROR_SUCCESS); +// REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeature(LocIndString{s_featureName})).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()); +// REQUIRE(installResultFile.is_open()); +// std::string installResultStr; +// std::getline(installResultFile, installResultStr); +// REQUIRE(installResultStr.find("/custom") != std::string::npos); +// REQUIRE(installResultStr.find("/silentwithprogress") != std::string::npos); +//} +// +//TEST_CASE("InstallFlow_RebootRequired", "[windowsFeature]") +//{ +// if (!AppInstaller::Runtime::IsRunningAsAdmin()) +// { +// WARN("Test requires admin privilege. Skipped."); +// return; +// } +// +// TestCommon::TempFile installResultPath("TestExeInstalled.txt"); +// +// TestCommon::TestUserSettings testSettings; +// testSettings.Set(true); +// +// // Override with reboot required HRESULT. +// TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(ERROR_SUCCESS_REBOOT_REQUIRED); +// TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); +// +// std::ostringstream installOutput; +// TestContext context{ installOutput, std::cin }; +// auto previousThreadGlobals = context.SetForCurrentThread(); +// OverrideForShellExecute(context); +// context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string()); +// +// InstallCommand install({}); +// install.Execute(context); +// INFO(installOutput.str()); +// +// REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL); +// REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); +// REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::RebootRequiredToEnableWindowsFeatureOverrideRequired).get()) != std::string::npos); +//} +// +//TEST_CASE("InstallFlow_RebootRequired_Force", "[windowsFeature]") +//{ +// if (!AppInstaller::Runtime::IsRunningAsAdmin()) +// { +// WARN("Test requires admin privilege. Skipped."); +// return; +// } +// +// TestCommon::TempFile installResultPath("TestExeInstalled.txt"); +// +// TestCommon::TestUserSettings testSettings; +// testSettings.Set(true); +// +// // Override with reboot required HRESULT. +// TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(ERROR_SUCCESS_REBOOT_REQUIRED); +// TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); +// +// std::ostringstream installOutput; +// TestContext context{ installOutput, std::cin }; +// auto previousThreadGlobals = context.SetForCurrentThread(); +// OverrideForShellExecute(context); +// context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string()); +// context.Args.AddArg(Execution::Args::Type::Force); +// +// InstallCommand install({}); +// install.Execute(context); +// INFO(installOutput.str()); +// +// // Verify Installer is called and parameters are passed in. +// REQUIRE(context.GetTerminationHR() == ERROR_SUCCESS); +// REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::RebootRequiredToEnableWindowsFeatureOverridden).get()) != std::string::npos); +// REQUIRE(std::filesystem::exists(installResultPath.GetPath())); +// std::ifstream installResultFile(installResultPath.GetPath()); +// REQUIRE(installResultFile.is_open()); +// std::string installResultStr; +// std::getline(installResultFile, installResultStr); +// REQUIRE(installResultStr.find("/custom") != std::string::npos); +// REQUIRE(installResultStr.find("/silentwithprogress") != std::string::npos); +//} \ No newline at end of file diff --git a/src/AppInstallerCommonCore/WindowsFeature.cpp b/src/AppInstallerCommonCore/WindowsFeature.cpp index dffdb04172..cab9e183cd 100644 --- a/src/AppInstallerCommonCore/WindowsFeature.cpp +++ b/src/AppInstallerCommonCore/WindowsFeature.cpp @@ -12,7 +12,7 @@ namespace AppInstaller::WindowsFeature m_module.reset(LoadLibraryEx(L"dismapi.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32)); if (!m_module) { - AICLI_LOG(Core, Verbose, << "Could not load dismapi.dll"); + AICLI_LOG(Core, Error, << "Could not load dismapi.dll"); return; } @@ -20,7 +20,7 @@ namespace AppInstaller::WindowsFeature reinterpret_cast(GetProcAddress(m_module.get(), "DismInitialize")); if (!m_dismInitialize) { - AICLI_LOG(Core, Verbose, << "Could not get proc address of DismInitialize"); + AICLI_LOG(Core, Error, << "Could not get proc address of DismInitialize"); return; } @@ -28,7 +28,7 @@ namespace AppInstaller::WindowsFeature reinterpret_cast(GetProcAddress(m_module.get(), "DismOpenSession")); if (!m_dismOpenSession) { - AICLI_LOG(Core, Verbose, << "Could not get proc address of DismOpenSession"); + AICLI_LOG(Core, Error, << "Could not get proc address of DismOpenSession"); return; } @@ -36,7 +36,7 @@ namespace AppInstaller::WindowsFeature reinterpret_cast(GetProcAddress(m_module.get(), "DismGetFeatureInfo")); if (!m_dismGetFeatureInfo) { - AICLI_LOG(Core, Verbose, << "Could not get proc address of DismGetFeatureInfo"); + AICLI_LOG(Core, Error, << "Could not get proc address of DismGetFeatureInfo"); return; } @@ -44,7 +44,7 @@ namespace AppInstaller::WindowsFeature reinterpret_cast(GetProcAddress(m_module.get(), "DismEnableFeature")); if (!m_dismEnableFeature) { - AICLI_LOG(Core, Verbose, << "Could not get proc address of DismEnableFeature"); + AICLI_LOG(Core, Error, << "Could not get proc address of DismEnableFeature"); return; } @@ -52,7 +52,7 @@ namespace AppInstaller::WindowsFeature reinterpret_cast(GetProcAddress(m_module.get(), "DismDisableFeature")); if (!m_dismDisableFeature) { - AICLI_LOG(Core, Verbose, << "Could not get proc address of DismDisableFeature"); + AICLI_LOG(Core, Error, << "Could not get proc address of DismDisableFeature"); return; } @@ -60,7 +60,7 @@ namespace AppInstaller::WindowsFeature reinterpret_cast(GetProcAddress(m_module.get(), "DismDelete")); if (!m_dismDelete) { - AICLI_LOG(Core, Verbose, << "Could not get proc address of DismDelete"); + AICLI_LOG(Core, Error, << "Could not get proc address of DismDelete"); return; } @@ -68,7 +68,7 @@ namespace AppInstaller::WindowsFeature reinterpret_cast(GetProcAddress(m_module.get(), "DismCloseSession")); if (!m_dismCloseSession) { - AICLI_LOG(Core, Verbose, << "Could not get proc address of DismCloseSession"); + AICLI_LOG(Core, Error, << "Could not get proc address of DismCloseSession"); return; } @@ -76,7 +76,7 @@ namespace AppInstaller::WindowsFeature reinterpret_cast(GetProcAddress(m_module.get(), "DismShutdown")); if (!m_dismGetFeatureInfo) { - AICLI_LOG(Core, Verbose, << "Could not get proc address of DismShutdown"); + AICLI_LOG(Core, Error, << "Could not get proc address of DismShutdown"); return; } @@ -198,7 +198,7 @@ namespace AppInstaller::WindowsFeature // Refresh feature info state prior to retrieving state info. GetFeatureInfo(); DismPackageFeatureState featureState = GetState(); - AICLI_LOG(Core, Verbose, << "Feature state of " << m_featureName << " is " << featureState); + AICLI_LOG(Core, Info, << "Feature state of " << m_featureName << " is " << featureState); return featureState == DismStateInstalled; } From 427cc0ad65b594e1395fd6bd9e82a7285b76f577 Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Thu, 9 Mar 2023 12:34:48 -0800 Subject: [PATCH 17/29] block file handle inheritance --- src/AppInstallerCLITests/WindowsFeature.cpp | 356 ++++++++++---------- src/AppInstallerCommonCore/FileLogger.cpp | 9 + 2 files changed, 187 insertions(+), 178 deletions(-) diff --git a/src/AppInstallerCLITests/WindowsFeature.cpp b/src/AppInstallerCLITests/WindowsFeature.cpp index 39e23f52f7..7aba5b86f1 100644 --- a/src/AppInstallerCLITests/WindowsFeature.cpp +++ b/src/AppInstallerCLITests/WindowsFeature.cpp @@ -16,181 +16,181 @@ using namespace TestCommon; const std::string s_featureName = "netfx3"; -//TEST_CASE("InstallFlow_InvalidWindowsFeature", "[windowsFeature]") -//{ -// if (!AppInstaller::Runtime::IsRunningAsAdmin()) -// { -// WARN("Test requires admin privilege. Skipped."); -// return; -// } -// -// TestCommon::TempFile installResultPath("TestExeInstalled.txt"); -// -// TestCommon::TestUserSettings testSettings; -// testSettings.Set(true); -// -// std::ostringstream installOutput; -// TestContext context{ installOutput, std::cin }; -// auto previousThreadGlobals = context.SetForCurrentThread(); -// OverrideForShellExecute(context); -// context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); -// -// InstallCommand install({}); -// install.Execute(context); -// INFO(installOutput.str()); -// -// REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_MISSING_DEPENDENCY); -// REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); -// REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::WindowsFeatureNotFound(LocIndView{ "invalidFeature" })).get()) != std::string::npos); -// -// // "badFeature" should not be displayed as the flow should terminate after failing to find the first feature. -// REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::WindowsFeatureNotFound(LocIndView{ "badFeature" })).get()) == std::string::npos); -// REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeatureOverrideRequired).get()) != std::string::npos); -//} -// -//TEST_CASE("InstallFlow_FailedToEnableWindowsFeature", "[windowsFeature]") -//{ -// if (!AppInstaller::Runtime::IsRunningAsAdmin()) -// { -// WARN("Test requires admin privilege. Skipped."); -// return; -// } -// -// TestCommon::TempFile installResultPath("TestExeInstalled.txt"); -// -// TestCommon::TestUserSettings testSettings; -// testSettings.Set(true); -// -// // Override with arbitrary DISM api error (DISMAPI_E_DISMAPI_NOT_INITIALIZED) and make windows feature discoverable. -// HRESULT dismErrorResult = 0xc0040001; -// TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(dismErrorResult); -// TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); -// -// std::ostringstream installOutput; -// TestContext context{ installOutput, std::cin }; -// auto previousThreadGlobals = context.SetForCurrentThread(); -// OverrideForShellExecute(context); -// context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); -// -// InstallCommand install({}); -// install.Execute(context); -// INFO(installOutput.str()); -// -// REQUIRE(context.GetTerminationHR() == dismErrorResult); -// REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); -// REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeatureOverrideRequired).get()) != std::string::npos); -//} -// -//TEST_CASE("InstallFlow_FailedToEnableWindowsFeature_Force", "[windowsFeature]") -//{ -// if (!AppInstaller::Runtime::IsRunningAsAdmin()) -// { -// WARN("Test requires admin privilege. Skipped."); -// return; -// } -// -// TestCommon::TempFile installResultPath("TestExeInstalled.txt"); -// -// TestCommon::TestUserSettings testSettings; -// testSettings.Set(true); -// -// // Override with arbitrary DISM api error (DISMAPI_E_DISMAPI_NOT_INITIALIZED) and make windows feature discoverable. -// HRESULT dismErrorResult = 0xc0040001; -// TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(dismErrorResult); -// TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); -// -// std::ostringstream installOutput; -// TestContext context{ installOutput, std::cin }; -// auto previousThreadGlobals = context.SetForCurrentThread(); -// OverrideForShellExecute(context); -// context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); -// context.Args.AddArg(Execution::Args::Type::Force); -// -// InstallCommand install({}); -// install.Execute(context); -// INFO(installOutput.str()); -// -// // Verify Installer is called and parameters are passed in. -// REQUIRE(context.GetTerminationHR() == ERROR_SUCCESS); -// REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeature(LocIndString{s_featureName})).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()); -// REQUIRE(installResultFile.is_open()); -// std::string installResultStr; -// std::getline(installResultFile, installResultStr); -// REQUIRE(installResultStr.find("/custom") != std::string::npos); -// REQUIRE(installResultStr.find("/silentwithprogress") != std::string::npos); -//} -// -//TEST_CASE("InstallFlow_RebootRequired", "[windowsFeature]") -//{ -// if (!AppInstaller::Runtime::IsRunningAsAdmin()) -// { -// WARN("Test requires admin privilege. Skipped."); -// return; -// } -// -// TestCommon::TempFile installResultPath("TestExeInstalled.txt"); -// -// TestCommon::TestUserSettings testSettings; -// testSettings.Set(true); -// -// // Override with reboot required HRESULT. -// TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(ERROR_SUCCESS_REBOOT_REQUIRED); -// TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); -// -// std::ostringstream installOutput; -// TestContext context{ installOutput, std::cin }; -// auto previousThreadGlobals = context.SetForCurrentThread(); -// OverrideForShellExecute(context); -// context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string()); -// -// InstallCommand install({}); -// install.Execute(context); -// INFO(installOutput.str()); -// -// REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL); -// REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); -// REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::RebootRequiredToEnableWindowsFeatureOverrideRequired).get()) != std::string::npos); -//} -// -//TEST_CASE("InstallFlow_RebootRequired_Force", "[windowsFeature]") -//{ -// if (!AppInstaller::Runtime::IsRunningAsAdmin()) -// { -// WARN("Test requires admin privilege. Skipped."); -// return; -// } -// -// TestCommon::TempFile installResultPath("TestExeInstalled.txt"); -// -// TestCommon::TestUserSettings testSettings; -// testSettings.Set(true); -// -// // Override with reboot required HRESULT. -// TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(ERROR_SUCCESS_REBOOT_REQUIRED); -// TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); -// -// std::ostringstream installOutput; -// TestContext context{ installOutput, std::cin }; -// auto previousThreadGlobals = context.SetForCurrentThread(); -// OverrideForShellExecute(context); -// context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string()); -// context.Args.AddArg(Execution::Args::Type::Force); -// -// InstallCommand install({}); -// install.Execute(context); -// INFO(installOutput.str()); -// -// // Verify Installer is called and parameters are passed in. -// REQUIRE(context.GetTerminationHR() == ERROR_SUCCESS); -// REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::RebootRequiredToEnableWindowsFeatureOverridden).get()) != std::string::npos); -// REQUIRE(std::filesystem::exists(installResultPath.GetPath())); -// std::ifstream installResultFile(installResultPath.GetPath()); -// REQUIRE(installResultFile.is_open()); -// std::string installResultStr; -// std::getline(installResultFile, installResultStr); -// REQUIRE(installResultStr.find("/custom") != std::string::npos); -// REQUIRE(installResultStr.find("/silentwithprogress") != std::string::npos); -//} \ No newline at end of file +TEST_CASE("InstallFlow_InvalidWindowsFeature", "[windowsFeature]") +{ + if (!AppInstaller::Runtime::IsRunningAsAdmin()) + { + WARN("Test requires admin privilege. Skipped."); + return; + } + + TestCommon::TempFile installResultPath("TestExeInstalled.txt"); + + TestCommon::TestUserSettings testSettings; + testSettings.Set(true); + + std::ostringstream installOutput; + TestContext context{ installOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + OverrideForShellExecute(context); + context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); + + InstallCommand install({}); + install.Execute(context); + INFO(installOutput.str()); + + REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_MISSING_DEPENDENCY); + REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::WindowsFeatureNotFound(LocIndView{ "invalidFeature" })).get()) != std::string::npos); + + // "badFeature" should not be displayed as the flow should terminate after failing to find the first feature. + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::WindowsFeatureNotFound(LocIndView{ "badFeature" })).get()) == std::string::npos); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeatureOverrideRequired).get()) != std::string::npos); +} + +TEST_CASE("InstallFlow_FailedToEnableWindowsFeature", "[windowsFeature]") +{ + if (!AppInstaller::Runtime::IsRunningAsAdmin()) + { + WARN("Test requires admin privilege. Skipped."); + return; + } + + TestCommon::TempFile installResultPath("TestExeInstalled.txt"); + + TestCommon::TestUserSettings testSettings; + testSettings.Set(true); + + // Override with arbitrary DISM api error (DISMAPI_E_DISMAPI_NOT_INITIALIZED) and make windows feature discoverable. + HRESULT dismErrorResult = 0xc0040001; + TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(dismErrorResult); + TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); + + std::ostringstream installOutput; + TestContext context{ installOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + OverrideForShellExecute(context); + context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); + + InstallCommand install({}); + install.Execute(context); + INFO(installOutput.str()); + + REQUIRE(context.GetTerminationHR() == dismErrorResult); + REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeatureOverrideRequired).get()) != std::string::npos); +} + +TEST_CASE("InstallFlow_FailedToEnableWindowsFeature_Force", "[windowsFeature]") +{ + if (!AppInstaller::Runtime::IsRunningAsAdmin()) + { + WARN("Test requires admin privilege. Skipped."); + return; + } + + TestCommon::TempFile installResultPath("TestExeInstalled.txt"); + + TestCommon::TestUserSettings testSettings; + testSettings.Set(true); + + // Override with arbitrary DISM api error (DISMAPI_E_DISMAPI_NOT_INITIALIZED) and make windows feature discoverable. + HRESULT dismErrorResult = 0xc0040001; + TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(dismErrorResult); + TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); + + std::ostringstream installOutput; + TestContext context{ installOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + OverrideForShellExecute(context); + context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); + context.Args.AddArg(Execution::Args::Type::Force); + + InstallCommand install({}); + install.Execute(context); + INFO(installOutput.str()); + + // Verify Installer is called and parameters are passed in. + REQUIRE(context.GetTerminationHR() == ERROR_SUCCESS); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeature(LocIndString{s_featureName})).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()); + REQUIRE(installResultFile.is_open()); + std::string installResultStr; + std::getline(installResultFile, installResultStr); + REQUIRE(installResultStr.find("/custom") != std::string::npos); + REQUIRE(installResultStr.find("/silentwithprogress") != std::string::npos); +} + +TEST_CASE("InstallFlow_RebootRequired", "[windowsFeature]") +{ + if (!AppInstaller::Runtime::IsRunningAsAdmin()) + { + WARN("Test requires admin privilege. Skipped."); + return; + } + + TestCommon::TempFile installResultPath("TestExeInstalled.txt"); + + TestCommon::TestUserSettings testSettings; + testSettings.Set(true); + + // Override with reboot required HRESULT. + TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(ERROR_SUCCESS_REBOOT_REQUIRED); + TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); + + std::ostringstream installOutput; + TestContext context{ installOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + OverrideForShellExecute(context); + context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string()); + + InstallCommand install({}); + install.Execute(context); + INFO(installOutput.str()); + + REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL); + REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::RebootRequiredToEnableWindowsFeatureOverrideRequired).get()) != std::string::npos); +} + +TEST_CASE("InstallFlow_RebootRequired_Force", "[windowsFeature]") +{ + if (!AppInstaller::Runtime::IsRunningAsAdmin()) + { + WARN("Test requires admin privilege. Skipped."); + return; + } + + TestCommon::TempFile installResultPath("TestExeInstalled.txt"); + + TestCommon::TestUserSettings testSettings; + testSettings.Set(true); + + // Override with reboot required HRESULT. + TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(ERROR_SUCCESS_REBOOT_REQUIRED); + TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); + + std::ostringstream installOutput; + TestContext context{ installOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + OverrideForShellExecute(context); + context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string()); + context.Args.AddArg(Execution::Args::Type::Force); + + InstallCommand install({}); + install.Execute(context); + INFO(installOutput.str()); + + // Verify Installer is called and parameters are passed in. + REQUIRE(context.GetTerminationHR() == ERROR_SUCCESS); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::RebootRequiredToEnableWindowsFeatureOverridden).get()) != std::string::npos); + REQUIRE(std::filesystem::exists(installResultPath.GetPath())); + std::ifstream installResultFile(installResultPath.GetPath()); + REQUIRE(installResultFile.is_open()); + std::string installResultStr; + std::getline(installResultFile, installResultStr); + REQUIRE(installResultStr.find("/custom") != std::string::npos); + REQUIRE(installResultStr.find("/silentwithprogress") != std::string::npos); +} \ No newline at end of file diff --git a/src/AppInstallerCommonCore/FileLogger.cpp b/src/AppInstallerCommonCore/FileLogger.cpp index 45311b6eb2..53c2cb8984 100644 --- a/src/AppInstallerCommonCore/FileLogger.cpp +++ b/src/AppInstallerCommonCore/FileLogger.cpp @@ -6,6 +6,7 @@ #include "Public/AppInstallerRuntime.h" #include "Public/AppInstallerStrings.h" #include "Public/AppInstallerDateTime.h" +#include namespace AppInstaller::Logging @@ -24,6 +25,14 @@ namespace AppInstaller::Logging m_name = GetNameForPath(filePath); m_filePath = filePath; + // Prevent inheritance to ensure log file handle is not opened by other processes. + FILE* filePtr; + if (!fopen_s(&filePtr, m_filePath.string().c_str(), "w") && filePtr != NULL) + { + SetHandleInformation((HANDLE)_get_osfhandle(_fileno(filePtr)), HANDLE_FLAG_INHERIT, 0); + std::ofstream outFile(filePtr); + } + m_stream.open(m_filePath, s_fileLoggerDefaultOpenMode); } From c28f5e415bdc8c67e7b630de41796b7e6d8a20ed Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Thu, 9 Mar 2023 16:59:58 -0800 Subject: [PATCH 18/29] fix logging stream and add error handling --- src/AppInstallerCommonCore/FileLogger.cpp | 32 ++++++++++++------- .../Public/AppInstallerFileLogger.h | 2 ++ 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/AppInstallerCommonCore/FileLogger.cpp b/src/AppInstallerCommonCore/FileLogger.cpp index 53c2cb8984..161e9e2133 100644 --- a/src/AppInstallerCommonCore/FileLogger.cpp +++ b/src/AppInstallerCommonCore/FileLogger.cpp @@ -24,16 +24,7 @@ namespace AppInstaller::Logging { m_name = GetNameForPath(filePath); m_filePath = filePath; - - // Prevent inheritance to ensure log file handle is not opened by other processes. - FILE* filePtr; - if (!fopen_s(&filePtr, m_filePath.string().c_str(), "w") && filePtr != NULL) - { - SetHandleInformation((HANDLE)_get_osfhandle(_fileno(filePtr)), HANDLE_FLAG_INHERIT, 0); - std::ofstream outFile(filePtr); - } - - m_stream.open(m_filePath, s_fileLoggerDefaultOpenMode); + OpenFileLoggerStream(); } FileLogger::FileLogger(const std::string_view fileNamePrefix) @@ -41,8 +32,7 @@ namespace AppInstaller::Logging m_name = "file"; m_filePath = Runtime::GetPathTo(Runtime::PathName::DefaultLogLocation); m_filePath /= fileNamePrefix.data() + ('-' + Utility::GetCurrentTimeForFilename() + s_fileLoggerDefaultFileExt.data()); - - m_stream.open(m_filePath, s_fileLoggerDefaultOpenMode); + OpenFileLoggerStream(); } FileLogger::~FileLogger() @@ -134,4 +124,22 @@ namespace AppInstaller::Logging catch (...) {} }).detach(); } + + void FileLogger::OpenFileLoggerStream() + { + // Prevent inheritance to ensure log file handle is not opened by other processes. + FILE* filePtr; + if (!fopen_s(&filePtr, m_filePath.string().c_str(), "w") && filePtr != NULL) + { + if (!SetHandleInformation((HANDLE)_get_osfhandle(_fileno(filePtr)), HANDLE_FLAG_INHERIT, 0)) + { + THROW_LAST_ERROR(); + } + m_stream = std::ofstream{ filePtr }; + } + else + { + throw std::system_error(errno, std::generic_category()); + } + } } diff --git a/src/AppInstallerCommonCore/Public/AppInstallerFileLogger.h b/src/AppInstallerCommonCore/Public/AppInstallerFileLogger.h index 843eb8beaf..c506eccab0 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerFileLogger.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerFileLogger.h @@ -50,5 +50,7 @@ namespace AppInstaller::Logging std::string m_name; std::filesystem::path m_filePath; std::ofstream m_stream; + + void OpenFileLoggerStream(); }; } From fa209453b3a23964268b939a229dc46cb17382bd Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Fri, 10 Mar 2023 13:14:00 -0800 Subject: [PATCH 19/29] mock everything --- .../AppInstallerCLITests.vcxproj | 3 - .../AppInstallerCLITests.vcxproj.filters | 3 - ...nstallFlowTest_InvalidWindowsFeatures.yaml | 23 ----- .../InstallFlowTest_WindowsFeatures.yaml | 3 +- src/AppInstallerCLITests/TestHooks.h | 52 +++++++++++ src/AppInstallerCLITests/WindowsFeature.cpp | 53 ++++++----- src/AppInstallerCommonCore/FileLogger.cpp | 16 ++-- .../Public/winget/WindowsFeature.h | 22 ++--- src/AppInstallerCommonCore/WindowsFeature.cpp | 88 +++++++++++++++++-- 9 files changed, 180 insertions(+), 83 deletions(-) delete mode 100644 src/AppInstallerCLITests/TestData/InstallFlowTest_InvalidWindowsFeatures.yaml diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj index bde2960451..f95bf0c608 100644 --- a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj +++ b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj @@ -333,9 +333,6 @@ true - - true - true diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters index 7a7c22d5ce..cf163e5dc5 100644 --- a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters +++ b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters @@ -618,9 +618,6 @@ TestData - - TestData - TestData diff --git a/src/AppInstallerCLITests/TestData/InstallFlowTest_InvalidWindowsFeatures.yaml b/src/AppInstallerCLITests/TestData/InstallFlowTest_InvalidWindowsFeatures.yaml deleted file mode 100644 index 655a44b211..0000000000 --- a/src/AppInstallerCLITests/TestData/InstallFlowTest_InvalidWindowsFeatures.yaml +++ /dev/null @@ -1,23 +0,0 @@ -PackageIdentifier: AppInstallerCliTest.InvalidWindowsFeatures -PackageVersion: 1.0.0.0 -PackageLocale: en-US -PackageName: AppInstaller Test Installer -ShortDescription: Installs exe installer with invalid Windows Features dependencies -Publisher: Microsoft Corporation -License: Test -Installers: - - Architecture: x86 - InstallerUrl: https://ThisIsNotUsed - InstallerType: exe - InstallerSha256: 65DB2F2AC2686C7F2FD69D4A4C6683B888DC55BFA20A0E32CA9F838B51689A3B - InstallerSwitches: - Custom: /custom /scope=machine - SilentWithProgress: /silentwithprogress - Silent: /silence - Update: /update - Dependencies: - WindowsFeatures: - - invalidFeature - - netfx3 -ManifestType: singleton -ManifestVersion: 1.4.0 \ No newline at end of file diff --git a/src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml b/src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml index 9ad0c0b434..e07b2da908 100644 --- a/src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml +++ b/src/AppInstallerCLITests/TestData/InstallFlowTest_WindowsFeatures.yaml @@ -17,6 +17,7 @@ Installers: Update: /update Dependencies: WindowsFeatures: - - netfx3 + - testFeature1 + - testFeature2 ManifestType: singleton ManifestVersion: 1.4.0 \ No newline at end of file diff --git a/src/AppInstallerCLITests/TestHooks.h b/src/AppInstallerCLITests/TestHooks.h index 5c21cb09f8..0d8c11be33 100644 --- a/src/AppInstallerCLITests/TestHooks.h +++ b/src/AppInstallerCLITests/TestHooks.h @@ -11,6 +11,7 @@ #include #include #include +#include #ifdef AICLI_DISABLE_TEST_HOOKS static_assert(false, "Test hooks have been disabled"); @@ -63,6 +64,9 @@ namespace AppInstaller { void TestHook_SetEnableWindowsFeatureResult_Override(HRESULT* result); void TestHook_SetIsWindowsFeatureEnabledResult_Override(bool* status); + void TestHook_SetDoesWindowsFeatureExistResult_Override(bool* status); + void TestHook_SetWindowsFeatureGetDisplayNameResult_Override(std::wstring* displayName); + void TestHook_SetWindowsFeatureGetRestartStatusResult_Override(AppInstaller::WindowsFeature::DismRestartType* restartType); } } @@ -144,4 +148,52 @@ namespace TestHook private: bool m_status; }; + + struct SetDoesWindowsFeatureExistResult_Override + { + SetDoesWindowsFeatureExistResult_Override(bool status) : m_status(status) + { + AppInstaller::WindowsFeature::TestHook_SetDoesWindowsFeatureExistResult_Override(&m_status); + } + + ~SetDoesWindowsFeatureExistResult_Override() + { + AppInstaller::WindowsFeature::TestHook_SetDoesWindowsFeatureExistResult_Override(nullptr); + } + + private: + bool m_status; + }; + + struct SetWindowsFeatureGetDisplayNameResult_Override + { + SetWindowsFeatureGetDisplayNameResult_Override(std::wstring displayName) : m_displayName(displayName) + { + AppInstaller::WindowsFeature::TestHook_SetWindowsFeatureGetDisplayNameResult_Override(&m_displayName); + } + + ~SetWindowsFeatureGetDisplayNameResult_Override() + { + AppInstaller::WindowsFeature::TestHook_SetWindowsFeatureGetDisplayNameResult_Override(nullptr); + } + + private: + std::wstring m_displayName; + }; + + struct SetWindowsFeatureGetRestartStatusResult_Override + { + SetWindowsFeatureGetRestartStatusResult_Override(AppInstaller::WindowsFeature::DismRestartType restartType) : m_restartType(restartType) + { + AppInstaller::WindowsFeature::TestHook_SetWindowsFeatureGetRestartStatusResult_Override(&m_restartType); + } + + ~SetWindowsFeatureGetRestartStatusResult_Override() + { + AppInstaller::WindowsFeature::TestHook_SetWindowsFeatureGetRestartStatusResult_Override(nullptr); + } + + private: + AppInstaller::WindowsFeature::DismRestartType m_restartType; + }; } \ No newline at end of file diff --git a/src/AppInstallerCLITests/WindowsFeature.cpp b/src/AppInstallerCLITests/WindowsFeature.cpp index 7aba5b86f1..599715bf63 100644 --- a/src/AppInstallerCLITests/WindowsFeature.cpp +++ b/src/AppInstallerCLITests/WindowsFeature.cpp @@ -14,9 +14,7 @@ using namespace AppInstaller::Utility; using namespace AppInstaller::WindowsFeature; using namespace TestCommon; -const std::string s_featureName = "netfx3"; - -TEST_CASE("InstallFlow_InvalidWindowsFeature", "[windowsFeature]") +TEST_CASE("InstallFlow_WindowsFeatureDoesNotExist", "[windowsFeature]") { if (!AppInstaller::Runtime::IsRunningAsAdmin()) { @@ -33,7 +31,9 @@ TEST_CASE("InstallFlow_InvalidWindowsFeature", "[windowsFeature]") TestContext context{ installOutput, std::cin }; auto previousThreadGlobals = context.SetForCurrentThread(); OverrideForShellExecute(context); - context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); + context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string()); + + auto doesFeatureExistOverride = TestHook::SetDoesWindowsFeatureExistResult_Override(false); InstallCommand install({}); install.Execute(context); @@ -41,10 +41,10 @@ TEST_CASE("InstallFlow_InvalidWindowsFeature", "[windowsFeature]") REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_MISSING_DEPENDENCY); REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); - REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::WindowsFeatureNotFound(LocIndView{ "invalidFeature" })).get()) != std::string::npos); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::WindowsFeatureNotFound(LocIndView{ "testFeature1" })).get()) != std::string::npos); // "badFeature" should not be displayed as the flow should terminate after failing to find the first feature. - REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::WindowsFeatureNotFound(LocIndView{ "badFeature" })).get()) == std::string::npos); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::WindowsFeatureNotFound(LocIndView{ "testFeature2" })).get()) == std::string::npos); REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeatureOverrideRequired).get()) != std::string::npos); } @@ -61,16 +61,19 @@ TEST_CASE("InstallFlow_FailedToEnableWindowsFeature", "[windowsFeature]") TestCommon::TestUserSettings testSettings; testSettings.Set(true); - // Override with arbitrary DISM api error (DISMAPI_E_DISMAPI_NOT_INITIALIZED) and make windows feature discoverable. - HRESULT dismErrorResult = 0xc0040001; - TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(dismErrorResult); - TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); - std::ostringstream installOutput; TestContext context{ installOutput, std::cin }; auto previousThreadGlobals = context.SetForCurrentThread(); OverrideForShellExecute(context); - context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); + 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 setEnableFeatureOverride = TestHook::SetEnableWindowsFeatureResult_Override(dismErrorResult); + auto doesFeatureExistOverride = TestHook::SetDoesWindowsFeatureExistResult_Override(true); + auto setIsFeatureEnabledOverride = TestHook::SetIsWindowsFeatureEnabledResult_Override(false); + auto getDisplayNameOverride = TestHook::SetWindowsFeatureGetDisplayNameResult_Override(L"Test Windows Feature"); + auto getRestartStatusOverride = TestHook::SetWindowsFeatureGetRestartStatusResult_Override(DismRestartNo); InstallCommand install({}); install.Execute(context); @@ -96,14 +99,17 @@ TEST_CASE("InstallFlow_FailedToEnableWindowsFeature_Force", "[windowsFeature]") // Override with arbitrary DISM api error (DISMAPI_E_DISMAPI_NOT_INITIALIZED) and make windows feature discoverable. HRESULT dismErrorResult = 0xc0040001; - TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(dismErrorResult); - TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); + auto setEnableFeatureOverride = TestHook::SetEnableWindowsFeatureResult_Override(dismErrorResult); + auto doesFeatureExistOverride = TestHook::SetDoesWindowsFeatureExistResult_Override(true); + auto setIsFeatureEnabledOverride = TestHook::SetIsWindowsFeatureEnabledResult_Override(false); + auto getDisplayNameOverride = TestHook::SetWindowsFeatureGetDisplayNameResult_Override(L"Test Windows Feature"); + auto getRestartStatusOverride = TestHook::SetWindowsFeatureGetRestartStatusResult_Override(DismRestartNo); std::ostringstream installOutput; TestContext context{ installOutput, std::cin }; auto previousThreadGlobals = context.SetForCurrentThread(); OverrideForShellExecute(context); - context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_InvalidWindowsFeatures.yaml").GetPath().u8string()); + context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string()); context.Args.AddArg(Execution::Args::Type::Force); InstallCommand install({}); @@ -112,7 +118,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(LocIndString{s_featureName})).get()) != std::string::npos); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeature(LocIndView{ "testFeature1" })).get()) != std::string::npos); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeature(LocIndView{ "testFeature2" })).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()); @@ -137,8 +144,11 @@ TEST_CASE("InstallFlow_RebootRequired", "[windowsFeature]") testSettings.Set(true); // Override with reboot required HRESULT. - TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(ERROR_SUCCESS_REBOOT_REQUIRED); - TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); + 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(L"Test Windows Feature"); + auto getRestartStatusOverride = TestHook::SetWindowsFeatureGetRestartStatusResult_Override(DismRestartRequired); std::ostringstream installOutput; TestContext context{ installOutput, std::cin }; @@ -169,8 +179,11 @@ TEST_CASE("InstallFlow_RebootRequired_Force", "[windowsFeature]") testSettings.Set(true); // Override with reboot required HRESULT. - TestHook::SetEnableWindowsFeatureResult_Override enableWindowsFeatureResultOverride(ERROR_SUCCESS_REBOOT_REQUIRED); - TestHook::SetIsWindowsFeatureEnabledResult_Override isWindowsFeatureEnabledResultOverride(false); + 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(L"Test Windows Feature"); + auto getRestartStatusOverride = TestHook::SetWindowsFeatureGetRestartStatusResult_Override(DismRestartRequired); std::ostringstream installOutput; TestContext context{ installOutput, std::cin }; diff --git a/src/AppInstallerCommonCore/FileLogger.cpp b/src/AppInstallerCommonCore/FileLogger.cpp index 161e9e2133..1f3e756823 100644 --- a/src/AppInstallerCommonCore/FileLogger.cpp +++ b/src/AppInstallerCommonCore/FileLogger.cpp @@ -16,7 +16,6 @@ namespace AppInstaller::Logging static constexpr std::string_view s_fileLoggerDefaultFilePrefix = "WinGet"sv; static constexpr std::string_view s_fileLoggerDefaultFileExt = ".log"sv; - static constexpr std::ios_base::openmode s_fileLoggerDefaultOpenMode = std::ios_base::out | std::ios_base::app; FileLogger::FileLogger() : FileLogger(s_fileLoggerDefaultFilePrefix) {} @@ -129,17 +128,16 @@ namespace AppInstaller::Logging { // Prevent inheritance to ensure log file handle is not opened by other processes. FILE* filePtr; - if (!fopen_s(&filePtr, m_filePath.string().c_str(), "w") && filePtr != NULL) + errno_t error = fopen_s(&filePtr, m_filePath.string().c_str(), "w"); + if (error || filePtr == NULL) { - if (!SetHandleInformation((HANDLE)_get_osfhandle(_fileno(filePtr)), HANDLE_FLAG_INHERIT, 0)) - { - THROW_LAST_ERROR(); - } - m_stream = std::ofstream{ filePtr }; + throw std::system_error(errno, std::generic_category()); } - else + + if (!SetHandleInformation((HANDLE)_get_osfhandle(_fileno(filePtr)), HANDLE_FLAG_INHERIT, 0)) { - throw std::system_error(errno, std::generic_category()); + THROW_LAST_ERROR(); } + m_stream = std::ofstream{ filePtr }; } } diff --git a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h index c55dd2c1bc..b90415c76a 100644 --- a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h +++ b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h @@ -124,22 +124,14 @@ namespace AppInstaller::WindowsFeature HRESULT Disable(); bool DoesExist(); bool IsEnabled(); - - std::wstring GetDisplayName() - { - return std::wstring{ m_featureInfo->DisplayName }; - } + std::wstring GetDisplayName(); + DismRestartType GetRestartRequiredStatus(); DismPackageFeatureState GetState() { return m_featureInfo->FeatureState; } - DismRestartType GetRestartRequiredStatus() - { - return m_featureInfo->RestartRequired; - } - ~WindowsFeature() { if (m_featureInfo) @@ -151,6 +143,9 @@ namespace AppInstaller::WindowsFeature private: friend DismHelper; + // This default constructor is only used for mocking unit tests. + WindowsFeature(){}; + WindowsFeature( const std::string& name, DismGetFeatureInfoPtr getFeatureInfoPtr, @@ -167,7 +162,7 @@ namespace AppInstaller::WindowsFeature DismEnableFeaturePtr m_enableFeature = nullptr; DismDisableFeaturePtr m_disableFeaturePtr = nullptr; DismDeletePtr m_deletePtr = nullptr; - DismSession m_session; + DismSession m_session = DISM_SESSION_DEFAULT; }; DismHelper(); @@ -178,10 +173,7 @@ namespace AppInstaller::WindowsFeature Shutdown(); }; - WindowsFeature CreateWindowsFeature(const std::string& name) - { - return WindowsFeature(name, m_dismGetFeatureInfo, m_dismEnableFeature, m_dismDisableFeature, m_dismDelete, m_dismSession); - } + WindowsFeature CreateWindowsFeature(const std::string& name); private: typedef UINT DismSession; diff --git a/src/AppInstallerCommonCore/WindowsFeature.cpp b/src/AppInstallerCommonCore/WindowsFeature.cpp index cab9e183cd..d8ce3ee495 100644 --- a/src/AppInstallerCommonCore/WindowsFeature.cpp +++ b/src/AppInstallerCommonCore/WindowsFeature.cpp @@ -7,8 +7,21 @@ namespace AppInstaller::WindowsFeature { +#ifndef AICLI_DISABLE_TEST_HOOKS + static bool s_Mock_DismHelper_Override = true; +#endif + DismHelper::DismHelper() { +#ifndef AICLI_DISABLE_TEST_HOOKS + // The entire DismHelper class needs to be mocked since DismHost.exe inherits log file handles. + // Without this, the unit tests will fail to complete waiting for DismHost.exe to release the log file handles. + if (s_Mock_DismHelper_Override) + { + return; + } +#endif + m_module.reset(LoadLibraryEx(L"dismapi.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32)); if (!m_module) { @@ -84,6 +97,16 @@ namespace AppInstaller::WindowsFeature OpenSession(); } + DismHelper::WindowsFeature DismHelper::CreateWindowsFeature(const std::string& name) + { + if (s_Mock_DismHelper_Override) + { + return WindowsFeature(); + } + + return WindowsFeature(name, m_dismGetFeatureInfo, m_dismEnableFeature, m_dismDisableFeature, m_dismDelete, m_dismSession); + } + void DismHelper::Initialize() { if (m_dismInitialize) @@ -141,6 +164,34 @@ namespace AppInstaller::WindowsFeature { s_EnableWindowsFeatureResult_TestHook_Override = result; } + + static bool* s_DoesWindowsFeatureExistResult_TestHook_Override = nullptr; + + void TestHook_SetDoesWindowsFeatureExistResult_Override(bool* result) + { + s_DoesWindowsFeatureExistResult_TestHook_Override = result; + } + + static bool* s_IsWindowsFeatureEnabledResult_TestHook_Override = nullptr; + + void TestHook_SetIsWindowsFeatureEnabledResult_Override(bool* status) + { + s_IsWindowsFeatureEnabledResult_TestHook_Override = status; + } + + static std::wstring* s_WindowsFeatureGetDisplayNameResult_TestHook_Override = nullptr; + + void TestHook_SetWindowsFeatureGetDisplayNameResult_Override(std::wstring* displayName) + { + s_WindowsFeatureGetDisplayNameResult_TestHook_Override = displayName; + } + + static DismRestartType* s_WindowsFeatureGetRestartRequiredStatusResult_TestHook_Override = nullptr; + + void TestHook_SetWindowsFeatureGetRestartStatusResult_Override(DismRestartType* restartType) + { + s_WindowsFeatureGetRestartRequiredStatusResult_TestHook_Override = restartType; + } #endif HRESULT DismHelper::WindowsFeature::Enable() @@ -175,17 +226,14 @@ namespace AppInstaller::WindowsFeature bool DismHelper::WindowsFeature::DoesExist() { - return m_featureInfo; - } - #ifndef AICLI_DISABLE_TEST_HOOKS - static bool* s_IsWindowsFeatureEnabledResult_TestHook_Override = nullptr; - - void TestHook_SetIsWindowsFeatureEnabledResult_Override(bool* status) - { - s_IsWindowsFeatureEnabledResult_TestHook_Override = status; - } + if (s_DoesWindowsFeatureExistResult_TestHook_Override) + { + return *s_DoesWindowsFeatureExistResult_TestHook_Override; + } #endif + return m_featureInfo; + } bool DismHelper::WindowsFeature::IsEnabled() { @@ -202,6 +250,28 @@ namespace AppInstaller::WindowsFeature return featureState == DismStateInstalled; } + std::wstring DismHelper::WindowsFeature::GetDisplayName() + { +#ifndef AICLI_DISABLE_TEST_HOOKS + if (s_WindowsFeatureGetDisplayNameResult_TestHook_Override) + { + return *s_WindowsFeatureGetDisplayNameResult_TestHook_Override; + } +#endif + return std::wstring{ m_featureInfo->DisplayName }; + } + + DismRestartType DismHelper::WindowsFeature::GetRestartRequiredStatus() + { +#ifndef AICLI_DISABLE_TEST_HOOKS + if (s_WindowsFeatureGetRestartRequiredStatusResult_TestHook_Override) + { + return *s_WindowsFeatureGetRestartRequiredStatusResult_TestHook_Override; + } +#endif + return m_featureInfo->RestartRequired; + } + void DismHelper::WindowsFeature::GetFeatureInfo() { if (m_getFeatureInfoPtr) From fb1a0345cfcc1d845c41f7604402f0a1022a7fae Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Fri, 10 Mar 2023 14:36:05 -0800 Subject: [PATCH 20/29] address comments --- .github/actions/spelling/expect.txt | 2 ++ src/AppInstallerCommonCore/FileLogger.cpp | 14 +++++++------- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt index b9e9030fc7..09b382dc4a 100644 --- a/.github/actions/spelling/expect.txt +++ b/.github/actions/spelling/expect.txt @@ -71,6 +71,7 @@ contactsupport contentfiles contoso contractversion +corecrt count'th countof countryregion @@ -250,6 +251,7 @@ nuffing objbase objidl ofile +osfhandle Outptr packageinuse packageinusebyapplication diff --git a/src/AppInstallerCommonCore/FileLogger.cpp b/src/AppInstallerCommonCore/FileLogger.cpp index 1f3e756823..e47444f185 100644 --- a/src/AppInstallerCommonCore/FileLogger.cpp +++ b/src/AppInstallerCommonCore/FileLogger.cpp @@ -128,16 +128,16 @@ namespace AppInstaller::Logging { // Prevent inheritance to ensure log file handle is not opened by other processes. FILE* filePtr; - errno_t error = fopen_s(&filePtr, m_filePath.string().c_str(), "w"); - if (error || filePtr == NULL) + errno_t fopenError = _wfopen_s(&filePtr, m_filePath.wstring().c_str(), L"w"); + if (!fopenError) { - throw std::system_error(errno, std::generic_category()); + THROW_HR_IF(E_UNEXPECTED, filePtr == nullptr); + THROW_IF_WIN32_BOOL_FALSE(SetHandleInformation(reinterpret_cast(_get_osfhandle(_fileno(filePtr))), HANDLE_FLAG_INHERIT, 0)); + m_stream = std::ofstream{ filePtr }; } - - if (!SetHandleInformation((HANDLE)_get_osfhandle(_fileno(filePtr)), HANDLE_FLAG_INHERIT, 0)) + else { - THROW_LAST_ERROR(); + throw std::system_error(fopenError, std::generic_category()); } - m_stream = std::ofstream{ filePtr }; } } From 0e027e85d49274f4fb66d68d2019291a0643eafe Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Fri, 10 Mar 2023 14:39:21 -0800 Subject: [PATCH 21/29] fix spelling --- .github/actions/spelling/expect.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt index 09b382dc4a..3e44460d34 100644 --- a/.github/actions/spelling/expect.txt +++ b/.github/actions/spelling/expect.txt @@ -402,6 +402,7 @@ Webserver websites WERSJA wesome +wfopen Whatif winapifamily windir From 09b1a0f8eb38160a6d359db9b903aa2725c311f0 Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Fri, 10 Mar 2023 16:07:23 -0800 Subject: [PATCH 22/29] fix test hooks --- src/AppInstallerCLITests/TestHooks.h | 14 ++++++++++++++ src/AppInstallerCLITests/WindowsFeature.cpp | 5 +++++ src/AppInstallerCommonCore/WindowsFeature.cpp | 14 ++++++++++---- 3 files changed, 29 insertions(+), 4 deletions(-) diff --git a/src/AppInstallerCLITests/TestHooks.h b/src/AppInstallerCLITests/TestHooks.h index 0d8c11be33..05d796df30 100644 --- a/src/AppInstallerCLITests/TestHooks.h +++ b/src/AppInstallerCLITests/TestHooks.h @@ -62,6 +62,7 @@ namespace AppInstaller namespace WindowsFeature { + void TestHook_MockDismHelper_Override(bool status); void TestHook_SetEnableWindowsFeatureResult_Override(HRESULT* result); void TestHook_SetIsWindowsFeatureEnabledResult_Override(bool* status); void TestHook_SetDoesWindowsFeatureExistResult_Override(bool* status); @@ -117,6 +118,19 @@ namespace TestHook } }; + struct MockDismHelper_Override + { + MockDismHelper_Override() + { + AppInstaller::WindowsFeature::TestHook_MockDismHelper_Override(true); + } + + ~MockDismHelper_Override() + { + AppInstaller::WindowsFeature::TestHook_MockDismHelper_Override(false); + } + }; + struct SetEnableWindowsFeatureResult_Override { SetEnableWindowsFeatureResult_Override(HRESULT result) : m_result(result) diff --git a/src/AppInstallerCLITests/WindowsFeature.cpp b/src/AppInstallerCLITests/WindowsFeature.cpp index 599715bf63..8c879589b0 100644 --- a/src/AppInstallerCLITests/WindowsFeature.cpp +++ b/src/AppInstallerCLITests/WindowsFeature.cpp @@ -33,6 +33,7 @@ TEST_CASE("InstallFlow_WindowsFeatureDoesNotExist", "[windowsFeature]") OverrideForShellExecute(context); context.Args.AddArg(Execution::Args::Type::Manifest, TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string()); + auto mockDismHelperOverride = TestHook::MockDismHelper_Override(); auto doesFeatureExistOverride = TestHook::SetDoesWindowsFeatureExistResult_Override(false); InstallCommand install({}); @@ -69,6 +70,7 @@ TEST_CASE("InstallFlow_FailedToEnableWindowsFeature", "[windowsFeature]") // 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 setEnableFeatureOverride = TestHook::SetEnableWindowsFeatureResult_Override(dismErrorResult); auto doesFeatureExistOverride = TestHook::SetDoesWindowsFeatureExistResult_Override(true); auto setIsFeatureEnabledOverride = TestHook::SetIsWindowsFeatureEnabledResult_Override(false); @@ -99,6 +101,7 @@ TEST_CASE("InstallFlow_FailedToEnableWindowsFeature_Force", "[windowsFeature]") // 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 setEnableFeatureOverride = TestHook::SetEnableWindowsFeatureResult_Override(dismErrorResult); auto doesFeatureExistOverride = TestHook::SetDoesWindowsFeatureExistResult_Override(true); auto setIsFeatureEnabledOverride = TestHook::SetIsWindowsFeatureEnabledResult_Override(false); @@ -144,6 +147,7 @@ TEST_CASE("InstallFlow_RebootRequired", "[windowsFeature]") testSettings.Set(true); // Override with reboot required HRESULT. + auto mockDismHelperOverride = TestHook::MockDismHelper_Override(); auto setEnableFeatureOverride = TestHook::SetEnableWindowsFeatureResult_Override(ERROR_SUCCESS_REBOOT_REQUIRED); auto setIsFeatureEnabledOverride = TestHook::SetIsWindowsFeatureEnabledResult_Override (false); auto doesFeatureExistOverride = TestHook::SetDoesWindowsFeatureExistResult_Override(true); @@ -179,6 +183,7 @@ TEST_CASE("InstallFlow_RebootRequired_Force", "[windowsFeature]") testSettings.Set(true); // Override with reboot required HRESULT. + auto mockDismHelperOverride = TestHook::MockDismHelper_Override(); auto setEnableFeatureOverride = TestHook::SetEnableWindowsFeatureResult_Override(ERROR_SUCCESS_REBOOT_REQUIRED); auto setIsFeatureEnabledOverride = TestHook::SetIsWindowsFeatureEnabledResult_Override(false); auto doesFeatureExistOverride = TestHook::SetDoesWindowsFeatureExistResult_Override(true); diff --git a/src/AppInstallerCommonCore/WindowsFeature.cpp b/src/AppInstallerCommonCore/WindowsFeature.cpp index d8ce3ee495..a217e2337a 100644 --- a/src/AppInstallerCommonCore/WindowsFeature.cpp +++ b/src/AppInstallerCommonCore/WindowsFeature.cpp @@ -8,7 +8,12 @@ namespace AppInstaller::WindowsFeature { #ifndef AICLI_DISABLE_TEST_HOOKS - static bool s_Mock_DismHelper_Override = true; + static bool s_MockDismHelper_Override = false; + + void TestHook_MockDismHelper_Override(bool status) + { + s_MockDismHelper_Override = status; + } #endif DismHelper::DismHelper() @@ -16,7 +21,7 @@ namespace AppInstaller::WindowsFeature #ifndef AICLI_DISABLE_TEST_HOOKS // The entire DismHelper class needs to be mocked since DismHost.exe inherits log file handles. // Without this, the unit tests will fail to complete waiting for DismHost.exe to release the log file handles. - if (s_Mock_DismHelper_Override) + if (s_MockDismHelper_Override) { return; } @@ -99,11 +104,12 @@ namespace AppInstaller::WindowsFeature DismHelper::WindowsFeature DismHelper::CreateWindowsFeature(const std::string& name) { - if (s_Mock_DismHelper_Override) +#ifndef AICLI_DISABLE_TEST_HOOKS + if (s_MockDismHelper_Override) { return WindowsFeature(); } - +#endif return WindowsFeature(name, m_dismGetFeatureInfo, m_dismEnableFeature, m_dismDisableFeature, m_dismDelete, m_dismSession); } From 12c4cf57219d584b633bcfeabd6bfdc704a26664 Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Thu, 23 Mar 2023 10:32:31 -0700 Subject: [PATCH 23/29] address PR comments --- .../Workflows/DependenciesFlow.cpp | 22 +-- .../Workflows/DependenciesFlow.h | 4 + .../Workflows/InstallFlow.cpp | 2 +- .../Shared/Strings/en-us/winget.resw | 9 +- src/AppInstallerCLITests/TestHooks.h | 6 +- src/AppInstallerCLITests/WindowsFeature.cpp | 17 +- src/AppInstallerCommonCore/FileLogger.cpp | 1 + .../Public/winget/WindowsFeature.h | 105 +++++------ src/AppInstallerCommonCore/WindowsFeature.cpp | 174 +++++------------- 9 files changed, 128 insertions(+), 212 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp index 7baedb5c8e..4835ec1dd9 100644 --- a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp @@ -158,36 +158,34 @@ namespace AppInstaller::CLI::Workflow } HRESULT hr = S_OK; - DismHelper dismHelper = DismHelper(); + std::shared_ptr dismHelper = std::make_shared(); bool force = context.Args.Contains(Execution::Args::Type::Force); bool rebootRequired = false; - auto info = context.Reporter.Info(); - auto warn = context.Reporter.Warn(); - - rootDependencies.ApplyToType(DependencyType::WindowsFeature, [&hr, &dismHelper, &info, &warn, &force, &rebootRequired](Dependency dependency) + rootDependencies.ApplyToType(DependencyType::WindowsFeature, [&context, &hr, &dismHelper, &force, &rebootRequired](Dependency dependency) { if (SUCCEEDED(hr) || force) { auto featureName = dependency.Id(); - DismHelper::WindowsFeature windowsFeature = dismHelper.CreateWindowsFeature(featureName); + WindowsFeature::WindowsFeature windowsFeature{ dismHelper, featureName }; if (windowsFeature.DoesExist()) { if (!windowsFeature.IsEnabled()) { - std::string featureDisplayName = AppInstaller::Utility::ConvertToUTF8(windowsFeature.GetDisplayName()); + Utility::LocIndString featureDisplayName = windowsFeature.GetDisplayName(); Utility::LocIndView locIndFeatureName{ featureName }; - info << Resource::String::EnablingWindowsFeature(Utility::LocIndView{ featureDisplayName }, locIndFeatureName) << std::endl; + context.Reporter.Info() << Resource::String::EnablingWindowsFeature(featureDisplayName, locIndFeatureName) << std::endl; - hr = windowsFeature.Enable(); 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)) { - warn << Resource::String::FailedToEnableWindowsFeature(locIndFeatureName) << std::endl; + context.Reporter.Warn() << Resource::String::FailedToEnableWindowsFeature(locIndFeatureName, HRESULT_FROM_WIN32(hr)) << std::endl; } if (hr == ERROR_SUCCESS_REBOOT_REQUIRED || windowsFeature.GetRestartRequiredStatus() == DismRestartType::DismRestartRequired) @@ -201,12 +199,12 @@ namespace AppInstaller::CLI::Workflow // 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; - warn << Resource::String::WindowsFeatureNotFound(Utility::LocIndView{ featureName }) << std::endl; + context.Reporter.Warn() << Resource::String::WindowsFeatureNotFound(Utility::LocIndView{ featureName }) << std::endl; } } }); - if (FAILED(hr) || hr == APPINSTALLER_CLI_ERROR_INSTALL_MISSING_DEPENDENCY) + if (FAILED(hr)) { if (force) { diff --git a/src/AppInstallerCLICore/Workflows/DependenciesFlow.h b/src/AppInstallerCLICore/Workflows/DependenciesFlow.h index a55df0c6cc..7e99d4a6f8 100644 --- a/src/AppInstallerCLICore/Workflows/DependenciesFlow.h +++ b/src/AppInstallerCLICore/Workflows/DependenciesFlow.h @@ -60,5 +60,9 @@ namespace AppInstaller::CLI::Workflow // Outputs: DependencySource void OpenDependencySource(Execution::Context& context); + // Enables the Windows Feature dependencies. + // Required Args: None + // Inputs: Manifest, Installer + // Outputs: None void EnableWindowsFeaturesDependencies(Execution::Context& context); } \ No newline at end of file diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index baebf72eef..6d0af03e4c 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -494,6 +494,7 @@ namespace AppInstaller::CLI::Workflow Workflow::ShowPromptsForSinglePackage(/* ensureAcceptance */ true) << Workflow::GetDependenciesFromInstaller << Workflow::ReportDependencies(Resource::String::InstallAndUpgradeCommandsReportDependencies) << + Workflow::EnableWindowsFeaturesDependencies << Workflow::ManagePackageDependencies(Resource::String::InstallAndUpgradeCommandsReportDependencies) << Workflow::DownloadInstaller; } @@ -503,7 +504,6 @@ namespace AppInstaller::CLI::Workflow context << Workflow::CheckForUnsupportedArgs << Workflow::DownloadSinglePackage << - Workflow::EnableWindowsFeaturesDependencies << Workflow::InstallPackageInstaller; } diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index da917c64b3..ae0fe040cd 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -1787,11 +1787,11 @@ Please specify one of them using the --source option to proceed. {Locked="{0}"} Error message displayed when a Windows feature was not found on the system. - Failed to enable Windows Feature dependencies. To proceed with installation, use --force + Failed to enable Windows Feature dependencies. To proceed with installation, use '--force' {Locked="--force"} - Reboot required to fully enable the Windows Feature(s); to override this check use --force + Reboot required to fully enable the Windows Feature(s); to override this check use '--force' "Windows Feature(s)" is the name of the options Windows features setting. @@ -1804,8 +1804,9 @@ Please specify one of them using the --source option to proceed. {Locked="{0}","{1}"} Message displayed to the user regarding which Windows Feature is being enabled. - Failed to enable [{0}] feature. - {Locked="{0}"} Error message displayed when a failure was encountered when enabling a Windows Feature. + Failed to enable [{0}] feature with exit code: {1} + {Locked="{0}"} Error message displayed when a failure was encountered when enabling a Windows Feature. +{Locked="{1}} Failed exit code Reboot required to fully enable the Windows Feature(s); proceeding due to --force diff --git a/src/AppInstallerCLITests/TestHooks.h b/src/AppInstallerCLITests/TestHooks.h index 05d796df30..9f733137e5 100644 --- a/src/AppInstallerCLITests/TestHooks.h +++ b/src/AppInstallerCLITests/TestHooks.h @@ -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(std::wstring* displayName); + 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(std::wstring displayName) : m_displayName(displayName) + SetWindowsFeatureGetDisplayNameResult_Override(AppInstaller::Utility::LocIndString displayName) : m_displayName(displayName) { AppInstaller::WindowsFeature::TestHook_SetWindowsFeatureGetDisplayNameResult_Override(&m_displayName); } @@ -192,7 +192,7 @@ namespace TestHook } private: - std::wstring m_displayName; + AppInstaller::Utility::LocIndString m_displayName; }; struct SetWindowsFeatureGetRestartStatusResult_Override diff --git a/src/AppInstallerCLITests/WindowsFeature.cpp b/src/AppInstallerCLITests/WindowsFeature.cpp index 8c879589b0..8d8d8a6613 100644 --- a/src/AppInstallerCLITests/WindowsFeature.cpp +++ b/src/AppInstallerCLITests/WindowsFeature.cpp @@ -30,7 +30,6 @@ TEST_CASE("InstallFlow_WindowsFeatureDoesNotExist", "[windowsFeature]") std::ostringstream installOutput; TestContext context{ installOutput, std::cin }; auto previousThreadGlobals = context.SetForCurrentThread(); - OverrideForShellExecute(context); 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(); - OverrideForShellExecute(context); 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 setEnableFeatureOverride = TestHook::SetEnableWindowsFeatureResult_Override(dismErrorResult); auto doesFeatureExistOverride = TestHook::SetDoesWindowsFeatureExistResult_Override(true); auto setIsFeatureEnabledOverride = TestHook::SetIsWindowsFeatureEnabledResult_Override(false); - auto getDisplayNameOverride = TestHook::SetWindowsFeatureGetDisplayNameResult_Override(L"Test Windows Feature"); + 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(L"Test Windows Feature"); + 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" })).get()) != std::string::npos); - REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeature(LocIndView{ "testFeature2" })).get()) != std::string::npos); + 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(L"Test Windows Feature"); + 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(); - OverrideForShellExecute(context); 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(L"Test Windows Feature"); + auto getDisplayNameOverride = TestHook::SetWindowsFeatureGetDisplayNameResult_Override(LocIndString{ "Test Windows Feature"_liv }); auto getRestartStatusOverride = TestHook::SetWindowsFeatureGetRestartStatusResult_Override(DismRestartRequired); std::ostringstream installOutput; diff --git a/src/AppInstallerCommonCore/FileLogger.cpp b/src/AppInstallerCommonCore/FileLogger.cpp index e47444f185..2671281ee9 100644 --- a/src/AppInstallerCommonCore/FileLogger.cpp +++ b/src/AppInstallerCommonCore/FileLogger.cpp @@ -137,6 +137,7 @@ namespace AppInstaller::Logging } else { + AICLI_LOG(Core, Error, << "Failed to open log file"); throw std::system_error(fopenError, std::generic_category()); } } diff --git a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h index b90415c76a..5f6fa37012 100644 --- a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h +++ b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h @@ -1,6 +1,8 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. #pragma once +#include +#include 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 { - /// - /// Struct representation of a single Windows Feature. - /// - struct WindowsFeature - { - HRESULT Enable(); - HRESULT Disable(); - bool DoesExist(); - bool IsEnabled(); - std::wstring GetDisplayName(); - DismRestartType GetRestartRequiredStatus(); - - DismPackageFeatureState GetState() - { - return m_featureInfo->FeatureState; - } - - ~WindowsFeature() - { - if (m_featureInfo) - { - m_deletePtr(m_featureInfo); - } - } - - private: - friend DismHelper; - - // This default constructor is only used for mocking unit tests. - WindowsFeature(){}; - - WindowsFeature( - const std::string& name, - DismGetFeatureInfoPtr getFeatureInfoPtr, - DismEnableFeaturePtr enableFeaturePtr, - DismDisableFeaturePtr disableFeaturePtr, - DismDeletePtr deletePtr, - DismSession session); - - void GetFeatureInfo(); - - std::string m_featureName; - DismFeatureInfo* m_featureInfo = nullptr; - DismGetFeatureInfoPtr m_getFeatureInfoPtr = nullptr; - DismEnableFeaturePtr m_enableFeature = nullptr; - DismDisableFeaturePtr m_disableFeaturePtr = nullptr; - DismDeletePtr m_deletePtr = nullptr; - DismSession m_session = DISM_SESSION_DEFAULT; - }; - DismHelper(); - ~DismHelper() - { - CloseSession(); - Shutdown(); - }; - - WindowsFeature CreateWindowsFeature(const std::string& name); + ~DismHelper(); private: + friend WindowsFeature; typedef UINT DismSession; wil::unique_hmodule m_module; @@ -194,5 +144,46 @@ namespace AppInstaller::WindowsFeature void OpenSession(); void CloseSession(); void Shutdown(); + + template + FuncType GetProcAddressHelper(HMODULE module, LPCSTR functionName); + }; + + /// + /// Struct representation of a single Windows Feature. + /// + struct WindowsFeature + { + // This default constructor is only used for mocking unit tests. + WindowsFeature() = default; + WindowsFeature(std::shared_ptr dismHelper, const std::string& name); + + HRESULT Enable(IProgressCallback& progress); + 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 m_dismHelper; + DismFeatureInfo* m_featureInfo = nullptr; + DismSession m_session = DISM_SESSION_DEFAULT; }; } \ No newline at end of file diff --git a/src/AppInstallerCommonCore/WindowsFeature.cpp b/src/AppInstallerCommonCore/WindowsFeature.cpp index a217e2337a..6170004966 100644 --- a/src/AppInstallerCommonCore/WindowsFeature.cpp +++ b/src/AppInstallerCommonCore/WindowsFeature.cpp @@ -28,139 +28,74 @@ namespace AppInstaller::WindowsFeature #endif m_module.reset(LoadLibraryEx(L"dismapi.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32)); + if (!m_module) { AICLI_LOG(Core, Error, << "Could not load dismapi.dll"); - return; - } - - m_dismInitialize = - reinterpret_cast(GetProcAddress(m_module.get(), "DismInitialize")); - if (!m_dismInitialize) - { - AICLI_LOG(Core, Error, << "Could not get proc address of DismInitialize"); - return; - } - - m_dismOpenSession = - reinterpret_cast(GetProcAddress(m_module.get(), "DismOpenSession")); - if (!m_dismOpenSession) - { - AICLI_LOG(Core, Error, << "Could not get proc address of DismOpenSession"); - return; - } - - m_dismGetFeatureInfo = - reinterpret_cast(GetProcAddress(m_module.get(), "DismGetFeatureInfo")); - if (!m_dismGetFeatureInfo) - { - AICLI_LOG(Core, Error, << "Could not get proc address of DismGetFeatureInfo"); - return; - } - - m_dismEnableFeature = - reinterpret_cast(GetProcAddress(m_module.get(), "DismEnableFeature")); - if (!m_dismEnableFeature) - { - AICLI_LOG(Core, Error, << "Could not get proc address of DismEnableFeature"); - return; - } - - m_dismDisableFeature = - reinterpret_cast(GetProcAddress(m_module.get(), "DismDisableFeature")); - if (!m_dismDisableFeature) - { - AICLI_LOG(Core, Error, << "Could not get proc address of DismDisableFeature"); - return; - } - - m_dismDelete = - reinterpret_cast(GetProcAddress(m_module.get(), "DismDelete")); - if (!m_dismDelete) - { - AICLI_LOG(Core, Error, << "Could not get proc address of DismDelete"); - return; - } - - m_dismCloseSession = - reinterpret_cast(GetProcAddress(m_module.get(), "DismCloseSession")); - if (!m_dismCloseSession) - { - AICLI_LOG(Core, Error, << "Could not get proc address of DismCloseSession"); - return; + THROW_LAST_ERROR(); } - m_dismShutdown = - reinterpret_cast(GetProcAddress(m_module.get(), "DismShutdown")); - if (!m_dismGetFeatureInfo) - { - AICLI_LOG(Core, Error, << "Could not get proc address of DismShutdown"); - return; - } + m_dismInitialize = GetProcAddressHelper(m_module.get(), "DismInitialize"); + m_dismOpenSession = GetProcAddressHelper(m_module.get(), "DismOpenSession"); + m_dismGetFeatureInfo = GetProcAddressHelper(m_module.get(), "DismGetFeatureInfo"); + m_dismEnableFeature = GetProcAddressHelper(m_module.get(), "DismEnableFeature"); + m_dismDisableFeature = GetProcAddressHelper(m_module.get(), "DismDisableFeature"); + m_dismDelete = GetProcAddressHelper(m_module.get(), "DismDelete"); + m_dismCloseSession = GetProcAddressHelper(m_module.get(), "DismCloseSession"); + m_dismShutdown = GetProcAddressHelper(m_module.get(), "DismShutdown"); Initialize(); OpenSession(); } - DismHelper::WindowsFeature DismHelper::CreateWindowsFeature(const std::string& name) + DismHelper::~DismHelper() { #ifndef AICLI_DISABLE_TEST_HOOKS if (s_MockDismHelper_Override) { - return WindowsFeature(); + return; } #endif - return WindowsFeature(name, m_dismGetFeatureInfo, m_dismEnableFeature, m_dismDisableFeature, m_dismDelete, m_dismSession); - } + CloseSession(); + Shutdown(); + }; - void DismHelper::Initialize() + template + FuncType DismHelper::GetProcAddressHelper(HMODULE module, LPCSTR functionName) { - if (m_dismInitialize) + FuncType result = reinterpret_cast(GetProcAddress(module, functionName)); + if (!result) { - LOG_IF_FAILED(m_dismInitialize(2, NULL, NULL)); + AICLI_LOG(Core, Error, << "Could not get proc address of " << functionName); + THROW_LAST_ERROR(); } + return result; + } + + void DismHelper::Initialize() + { + LOG_IF_FAILED(m_dismInitialize(DismLogErrorsWarningsInfo, NULL, NULL)); } void DismHelper::OpenSession() { - if (m_dismOpenSession) - { - LOG_IF_FAILED(m_dismOpenSession(DISM_ONLINE_IMAGE, NULL, NULL, &m_dismSession)); - } + LOG_IF_FAILED(m_dismOpenSession(DISM_ONLINE_IMAGE, NULL, NULL, &m_dismSession)); } void DismHelper::CloseSession() { - if (m_dismCloseSession) - { - LOG_IF_FAILED(m_dismCloseSession(m_dismSession)); - } + LOG_IF_FAILED(m_dismCloseSession(m_dismSession)); } void DismHelper::Shutdown() { - if (m_dismShutdown) - { - LOG_IF_FAILED(m_dismShutdown()); - } + LOG_IF_FAILED(m_dismShutdown()); } - DismHelper::WindowsFeature::WindowsFeature( - const std::string& name, - DismGetFeatureInfoPtr getFeatureInfoPtr, - DismEnableFeaturePtr enableFeaturePtr, - DismDisableFeaturePtr disableFeaturePtr, - DismDeletePtr deletePtr, - DismSession session) + WindowsFeature::WindowsFeature(std::shared_ptr dismHelper, const std::string& name) { + m_dismHelper = std::move(dismHelper); m_featureName = name; - m_getFeatureInfoPtr = getFeatureInfoPtr; - m_enableFeature = enableFeaturePtr; - m_disableFeaturePtr = disableFeaturePtr; - m_deletePtr = deletePtr; - m_session = session; - - GetFeatureInfo(); } #ifndef AICLI_DISABLE_TEST_HOOKS @@ -185,9 +120,9 @@ namespace AppInstaller::WindowsFeature s_IsWindowsFeatureEnabledResult_TestHook_Override = status; } - static std::wstring* s_WindowsFeatureGetDisplayNameResult_TestHook_Override = nullptr; + static Utility::LocIndString* s_WindowsFeatureGetDisplayNameResult_TestHook_Override = nullptr; - void TestHook_SetWindowsFeatureGetDisplayNameResult_Override(std::wstring* displayName) + void TestHook_SetWindowsFeatureGetDisplayNameResult_Override(Utility::LocIndString* displayName) { s_WindowsFeatureGetDisplayNameResult_TestHook_Override = displayName; } @@ -200,37 +135,29 @@ namespace AppInstaller::WindowsFeature } #endif - HRESULT DismHelper::WindowsFeature::Enable() + HRESULT WindowsFeature::Enable(AppInstaller::IProgressCallback& progress) { + UNREFERENCED_PARAMETER(progress); + #ifndef AICLI_DISABLE_TEST_HOOKS if (s_EnableWindowsFeatureResult_TestHook_Override) { return *s_EnableWindowsFeatureResult_TestHook_Override; } #endif - HRESULT hr = ERROR_PROC_NOT_FOUND; - if (m_enableFeature) - { - hr = m_enableFeature(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, DismPackageNone, FALSE, NULL, NULL, FALSE, NULL, NULL, NULL); - LOG_IF_FAILED(hr); - } - + HRESULT hr = m_dismHelper->m_dismEnableFeature(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, DismPackageNone, FALSE, NULL, NULL, FALSE, NULL, NULL, NULL); + LOG_IF_FAILED(hr); return hr; } - HRESULT DismHelper::WindowsFeature::Disable() + HRESULT WindowsFeature::Disable() { - HRESULT hr = ERROR_PROC_NOT_FOUND; - if (m_disableFeaturePtr) - { - hr = m_disableFeaturePtr(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, FALSE, NULL, NULL, NULL); - LOG_IF_FAILED(hr); - } - + HRESULT hr = m_dismHelper->m_dismDisableFeature(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, FALSE, NULL, NULL, NULL); + LOG_IF_FAILED(hr); return hr; } - bool DismHelper::WindowsFeature::DoesExist() + bool WindowsFeature::DoesExist() { #ifndef AICLI_DISABLE_TEST_HOOKS if (s_DoesWindowsFeatureExistResult_TestHook_Override) @@ -241,7 +168,7 @@ namespace AppInstaller::WindowsFeature return m_featureInfo; } - bool DismHelper::WindowsFeature::IsEnabled() + bool WindowsFeature::IsEnabled() { #ifndef AICLI_DISABLE_TEST_HOOKS if (s_IsWindowsFeatureEnabledResult_TestHook_Override) @@ -256,7 +183,7 @@ namespace AppInstaller::WindowsFeature return featureState == DismStateInstalled; } - std::wstring DismHelper::WindowsFeature::GetDisplayName() + Utility::LocIndString WindowsFeature::GetDisplayName() { #ifndef AICLI_DISABLE_TEST_HOOKS if (s_WindowsFeatureGetDisplayNameResult_TestHook_Override) @@ -264,10 +191,10 @@ namespace AppInstaller::WindowsFeature return *s_WindowsFeatureGetDisplayNameResult_TestHook_Override; } #endif - return std::wstring{ m_featureInfo->DisplayName }; + return Utility::LocIndString{ Utility::ConvertToUTF8(std::wstring{ m_featureInfo->DisplayName }) }; } - DismRestartType DismHelper::WindowsFeature::GetRestartRequiredStatus() + DismRestartType WindowsFeature::GetRestartRequiredStatus() { #ifndef AICLI_DISABLE_TEST_HOOKS if (s_WindowsFeatureGetRestartRequiredStatusResult_TestHook_Override) @@ -278,11 +205,8 @@ namespace AppInstaller::WindowsFeature return m_featureInfo->RestartRequired; } - void DismHelper::WindowsFeature::GetFeatureInfo() + void WindowsFeature::GetFeatureInfo() { - if (m_getFeatureInfoPtr) - { - LOG_IF_FAILED(m_getFeatureInfoPtr(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, DismPackageNone, &m_featureInfo)); - } + LOG_IF_FAILED(m_dismHelper->m_dismGetFeatureInfo(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, DismPackageNone, &m_featureInfo)); } } \ No newline at end of file From e1dd56415892f6397a3ef8b7e45954e834aaf9f4 Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Thu, 23 Mar 2023 10:41:21 -0700 Subject: [PATCH 24/29] add period to strings --- src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index ae0fe040cd..5232517306 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -1787,11 +1787,11 @@ Please specify one of them using the --source option to proceed. {Locked="{0}"} Error message displayed when a Windows feature was not found on the system. - Failed to enable Windows Feature dependencies. To proceed with installation, use '--force' + Failed to enable Windows Feature dependencies. To proceed with installation, use '--force'. {Locked="--force"} - Reboot required to fully enable the Windows Feature(s); to override this check use '--force' + Reboot required to fully enable the Windows Feature(s); to override this check use '--force'. "Windows Feature(s)" is the name of the options Windows features setting. From fdc86bb2b8d2ac96c6f292f565b5afeb0b94bd59 Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Thu, 23 Mar 2023 10:51:52 -0700 Subject: [PATCH 25/29] add todo comment --- src/AppInstallerCLICore/Workflows/InstallFlow.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index 6d0af03e4c..9dd3778e6b 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -489,6 +489,7 @@ namespace AppInstaller::CLI::Workflow void DownloadSinglePackage(Execution::Context& context) { + // TODO: Split dependencies from download flow to prevent multiple installations. context << Workflow::ReportIdentityAndInstallationDisclaimer << Workflow::ShowPromptsForSinglePackage(/* ensureAcceptance */ true) << From 918875f35e22f84b4f93056477719c45f3d6da9a Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Thu, 23 Mar 2023 13:55:06 -0700 Subject: [PATCH 26/29] fix e2e test --- src/AppInstallerCommonCore/WindowsFeature.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/AppInstallerCommonCore/WindowsFeature.cpp b/src/AppInstallerCommonCore/WindowsFeature.cpp index 6170004966..5ffda244f0 100644 --- a/src/AppInstallerCommonCore/WindowsFeature.cpp +++ b/src/AppInstallerCommonCore/WindowsFeature.cpp @@ -96,6 +96,7 @@ namespace AppInstaller::WindowsFeature { m_dismHelper = std::move(dismHelper); m_featureName = name; + GetFeatureInfo(); } #ifndef AICLI_DISABLE_TEST_HOOKS From 0f36ef9ec667b3d139aa3bc36d9942d152836f69 Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Thu, 23 Mar 2023 17:57:20 -0700 Subject: [PATCH 27/29] save work --- .../Workflows/DependenciesFlow.cpp | 4 +- .../Shared/Strings/en-us/winget.resw | 6 +-- src/AppInstallerCLITests/WindowsFeature.cpp | 7 +-- src/AppInstallerCommonCore/FileLogger.cpp | 2 +- .../Public/winget/WindowsFeature.h | 41 ++++++++++----- src/AppInstallerCommonCore/WindowsFeature.cpp | 51 +++++++++++++++++-- 6 files changed, 87 insertions(+), 24 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp index 4835ec1dd9..75f6ecc769 100644 --- a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp @@ -185,7 +185,9 @@ namespace AppInstaller::CLI::Workflow if (FAILED(hr)) { - context.Reporter.Warn() << Resource::String::FailedToEnableWindowsFeature(locIndFeatureName, HRESULT_FROM_WIN32(hr)) << std::endl; + 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) diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index 5232517306..b3a72833aa 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -1804,9 +1804,9 @@ Please specify one of them using the --source option to proceed. {Locked="{0}","{1}"} Message displayed to the user regarding which Windows Feature is being enabled. - Failed to enable [{0}] feature with exit code: {1} - {Locked="{0}"} Error message displayed when a failure was encountered when enabling a Windows Feature. -{Locked="{1}} Failed exit code + Failed to enable {0} [{1}] feature. + {Locked="{0}"} Windows Feature display name +{Locked="{1}"} Windows Feature name Reboot required to fully enable the Windows Feature(s); proceeding due to --force diff --git a/src/AppInstallerCLITests/WindowsFeature.cpp b/src/AppInstallerCLITests/WindowsFeature.cpp index 8d8d8a6613..0f00b55658 100644 --- a/src/AppInstallerCLITests/WindowsFeature.cpp +++ b/src/AppInstallerCLITests/WindowsFeature.cpp @@ -99,11 +99,12 @@ TEST_CASE("InstallFlow_FailedToEnableWindowsFeature_Force", "[windowsFeature]") // Override with arbitrary DISM api error (DISMAPI_E_DISMAPI_NOT_INITIALIZED) and make windows feature discoverable. HRESULT dismErrorResult = 0xc0040001; + LocIndString testFeatureDisplayName = LocIndString{ "Test Windows Feature"_liv }; auto mockDismHelperOverride = TestHook::MockDismHelper_Override(); 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 getDisplayNameOverride = TestHook::SetWindowsFeatureGetDisplayNameResult_Override(testFeatureDisplayName); auto getRestartStatusOverride = TestHook::SetWindowsFeatureGetRestartStatusResult_Override(DismRestartNo); std::ostringstream installOutput; @@ -119,8 +120,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::FailedToEnableWindowsFeature(testFeatureDisplayName, LocIndView{ "testFeature1" })).get()) != std::string::npos); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToEnableWindowsFeature(testFeatureDisplayName, LocIndView{ "testFeature2" })).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()); diff --git a/src/AppInstallerCommonCore/FileLogger.cpp b/src/AppInstallerCommonCore/FileLogger.cpp index 2671281ee9..dc3265b239 100644 --- a/src/AppInstallerCommonCore/FileLogger.cpp +++ b/src/AppInstallerCommonCore/FileLogger.cpp @@ -137,7 +137,7 @@ namespace AppInstaller::Logging } else { - AICLI_LOG(Core, Error, << "Failed to open log file"); + AICLI_LOG(Core, Error, << "Failed to open log file " << m_filePath.u8string()); throw std::system_error(fopenError, std::generic_category()); } } diff --git a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h index 5f6fa37012..06dccd40c3 100644 --- a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h +++ b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h @@ -115,17 +115,37 @@ 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 + struct DismHelper : public std::enable_shared_from_this { + std::shared_ptr CreateDismHelper() + { + return shared_from_this(); + } + DismHelper(); ~DismHelper(); + HRESULT EnableFeature( + UINT session, + PCWSTR featureName, + PCWSTR identifier, + DismPackageIdentifier packageIdentifier, + BOOL limitAccess, + PCWSTR* sourcePaths, + UINT sourcePathCount, + BOOL enableAll, + HANDLE cancelEvent, + DISM_PROGRESS_CALLBACK progress, + PVOID userData); + + HRESULT DisableFeature(UINT session, PCWSTR featureName, PCWSTR packageName, BOOL removePayload, HANDLE cancelEvent, DISM_PROGRESS_CALLBACK progress, PVOID userData); + + HRESULT GetFeatureInfo(UINT session, PCWSTR featureName, PCWSTR identifier, DismPackageIdentifier packageIdentifier, DismFeatureInfo** featureInfo); + + HRESULT Delete(VOID* dismStructure); + private: - friend WindowsFeature; typedef UINT DismSession; wil::unique_hmodule m_module; @@ -158,6 +178,9 @@ namespace AppInstaller::WindowsFeature WindowsFeature() = default; WindowsFeature(std::shared_ptr dismHelper, const std::string& name); + ~WindowsFeature(); + + // TODO: Implement progress via DismProgressFunction HRESULT Enable(IProgressCallback& progress); HRESULT Disable(); bool DoesExist(); @@ -170,14 +193,6 @@ namespace AppInstaller::WindowsFeature return m_featureInfo->FeatureState; } - ~WindowsFeature() - { - if (m_featureInfo) - { - m_dismHelper->m_dismDelete(m_featureInfo); - } - } - private: void GetFeatureInfo(); diff --git a/src/AppInstallerCommonCore/WindowsFeature.cpp b/src/AppInstallerCommonCore/WindowsFeature.cpp index 5ffda244f0..7bd4b80148 100644 --- a/src/AppInstallerCommonCore/WindowsFeature.cpp +++ b/src/AppInstallerCommonCore/WindowsFeature.cpp @@ -87,6 +87,37 @@ namespace AppInstaller::WindowsFeature LOG_IF_FAILED(m_dismCloseSession(m_dismSession)); } + HRESULT DismHelper::EnableFeature( + UINT session, + PCWSTR featureName, + PCWSTR identifier, + DismPackageIdentifier packageIdentifier, + BOOL limitAccess, + PCWSTR* sourcePaths, + UINT sourcePathCount, + BOOL enableAll, + HANDLE cancelEvent, + DISM_PROGRESS_CALLBACK progress, + PVOID userData) + { + return m_dismEnableFeature(session, featureName, identifier, packageIdentifier, limitAccess, sourcePaths, sourcePathCount, enableAll, cancelEvent, progress, userData); + } + + HRESULT DismHelper::DisableFeature(UINT session, PCWSTR featureName, PCWSTR packageName, BOOL removePayload, HANDLE cancelEvent, DISM_PROGRESS_CALLBACK progress, PVOID userData) + { + return m_dismDisableFeature(session, featureName, packageName, removePayload, cancelEvent, progress, userData); + } + + HRESULT DismHelper::GetFeatureInfo(UINT session, PCWSTR featureName, PCWSTR identifier, DismPackageIdentifier packageIdentifier, DismFeatureInfo** featureInfo) + { + return m_dismGetFeatureInfo(session, featureName, identifier, packageIdentifier, featureInfo); + } + + HRESULT DismHelper::Delete(VOID* dismStructure) + { + return m_dismDelete(dismStructure); + } + void DismHelper::Shutdown() { LOG_IF_FAILED(m_dismShutdown()); @@ -94,11 +125,25 @@ namespace AppInstaller::WindowsFeature WindowsFeature::WindowsFeature(std::shared_ptr dismHelper, const std::string& name) { +#ifndef AICLI_DISABLE_TEST_HOOKS + if (s_MockDismHelper_Override) + { + return; + } +#endif m_dismHelper = std::move(dismHelper); m_featureName = name; GetFeatureInfo(); } + WindowsFeature::~WindowsFeature() + { + if (m_featureInfo) + { + LOG_IF_FAILED(m_dismHelper->Delete(m_featureInfo)); + } + } + #ifndef AICLI_DISABLE_TEST_HOOKS static HRESULT* s_EnableWindowsFeatureResult_TestHook_Override = nullptr; @@ -146,14 +191,14 @@ namespace AppInstaller::WindowsFeature return *s_EnableWindowsFeatureResult_TestHook_Override; } #endif - HRESULT hr = m_dismHelper->m_dismEnableFeature(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, DismPackageNone, FALSE, NULL, NULL, FALSE, NULL, NULL, NULL); + HRESULT hr = m_dismHelper->EnableFeature(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, DismPackageNone, FALSE, NULL, NULL, FALSE, NULL, NULL, NULL); LOG_IF_FAILED(hr); return hr; } HRESULT WindowsFeature::Disable() { - HRESULT hr = m_dismHelper->m_dismDisableFeature(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, FALSE, NULL, NULL, NULL); + HRESULT hr = m_dismHelper->DisableFeature(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, FALSE, NULL, NULL, NULL); LOG_IF_FAILED(hr); return hr; } @@ -208,6 +253,6 @@ namespace AppInstaller::WindowsFeature void WindowsFeature::GetFeatureInfo() { - LOG_IF_FAILED(m_dismHelper->m_dismGetFeatureInfo(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, DismPackageNone, &m_featureInfo)); + LOG_IF_FAILED(m_dismHelper->GetFeatureInfo(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, DismPackageNone, &m_featureInfo)); } } \ No newline at end of file From 13200c303dcc768e3c3d2e67358b7b82d84cf818 Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Fri, 24 Mar 2023 09:52:22 -0700 Subject: [PATCH 28/29] fix issues --- .../Workflows/DependenciesFlow.cpp | 2 +- .../Public/winget/WindowsFeature.h | 8 +++---- src/AppInstallerCommonCore/WindowsFeature.cpp | 22 +++++++++---------- 3 files changed, 14 insertions(+), 18 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp index 75f6ecc769..94a8535c9f 100644 --- a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp @@ -168,7 +168,7 @@ namespace AppInstaller::CLI::Workflow if (SUCCEEDED(hr) || force) { auto featureName = dependency.Id(); - WindowsFeature::WindowsFeature windowsFeature{ dismHelper, featureName }; + WindowsFeature::WindowsFeature windowsFeature{ std::move(dismHelper->GetPtr()), featureName}; if (windowsFeature.DoesExist()) { diff --git a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h index 06dccd40c3..fa0271cb3a 100644 --- a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h +++ b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h @@ -117,7 +117,7 @@ namespace AppInstaller::WindowsFeature struct DismHelper : public std::enable_shared_from_this { - std::shared_ptr CreateDismHelper() + std::shared_ptr GetPtr() { return shared_from_this(); } @@ -127,7 +127,6 @@ namespace AppInstaller::WindowsFeature ~DismHelper(); HRESULT EnableFeature( - UINT session, PCWSTR featureName, PCWSTR identifier, DismPackageIdentifier packageIdentifier, @@ -139,9 +138,9 @@ namespace AppInstaller::WindowsFeature DISM_PROGRESS_CALLBACK progress, PVOID userData); - HRESULT DisableFeature(UINT session, PCWSTR featureName, PCWSTR packageName, BOOL removePayload, HANDLE cancelEvent, DISM_PROGRESS_CALLBACK progress, PVOID userData); + HRESULT DisableFeature(PCWSTR featureName, PCWSTR packageName, BOOL removePayload, HANDLE cancelEvent, DISM_PROGRESS_CALLBACK progress, PVOID userData); - HRESULT GetFeatureInfo(UINT session, PCWSTR featureName, PCWSTR identifier, DismPackageIdentifier packageIdentifier, DismFeatureInfo** featureInfo); + HRESULT GetFeatureInfo(PCWSTR featureName, PCWSTR identifier, DismPackageIdentifier packageIdentifier, DismFeatureInfo** featureInfo); HRESULT Delete(VOID* dismStructure); @@ -199,6 +198,5 @@ namespace AppInstaller::WindowsFeature std::string m_featureName; std::shared_ptr m_dismHelper; DismFeatureInfo* m_featureInfo = nullptr; - DismSession m_session = DISM_SESSION_DEFAULT; }; } \ No newline at end of file diff --git a/src/AppInstallerCommonCore/WindowsFeature.cpp b/src/AppInstallerCommonCore/WindowsFeature.cpp index 7bd4b80148..d21875c60f 100644 --- a/src/AppInstallerCommonCore/WindowsFeature.cpp +++ b/src/AppInstallerCommonCore/WindowsFeature.cpp @@ -19,7 +19,7 @@ namespace AppInstaller::WindowsFeature DismHelper::DismHelper() { #ifndef AICLI_DISABLE_TEST_HOOKS - // The entire DismHelper class needs to be mocked since DismHost.exe inherits log file handles. + // The entire DismHelper class and its functions needs to be mocked since DismHost.exe inherits log file handles. // Without this, the unit tests will fail to complete waiting for DismHost.exe to release the log file handles. if (s_MockDismHelper_Override) { @@ -88,7 +88,6 @@ namespace AppInstaller::WindowsFeature } HRESULT DismHelper::EnableFeature( - UINT session, PCWSTR featureName, PCWSTR identifier, DismPackageIdentifier packageIdentifier, @@ -100,17 +99,17 @@ namespace AppInstaller::WindowsFeature DISM_PROGRESS_CALLBACK progress, PVOID userData) { - return m_dismEnableFeature(session, featureName, identifier, packageIdentifier, limitAccess, sourcePaths, sourcePathCount, enableAll, cancelEvent, progress, userData); + return m_dismEnableFeature(m_dismSession, featureName, identifier, packageIdentifier, limitAccess, sourcePaths, sourcePathCount, enableAll, cancelEvent, progress, userData); } - HRESULT DismHelper::DisableFeature(UINT session, PCWSTR featureName, PCWSTR packageName, BOOL removePayload, HANDLE cancelEvent, DISM_PROGRESS_CALLBACK progress, PVOID userData) + HRESULT DismHelper::DisableFeature(PCWSTR featureName, PCWSTR packageName, BOOL removePayload, HANDLE cancelEvent, DISM_PROGRESS_CALLBACK progress, PVOID userData) { - return m_dismDisableFeature(session, featureName, packageName, removePayload, cancelEvent, progress, userData); + return m_dismDisableFeature(m_dismSession, featureName, packageName, removePayload, cancelEvent, progress, userData); } - HRESULT DismHelper::GetFeatureInfo(UINT session, PCWSTR featureName, PCWSTR identifier, DismPackageIdentifier packageIdentifier, DismFeatureInfo** featureInfo) + HRESULT DismHelper::GetFeatureInfo(PCWSTR featureName, PCWSTR identifier, DismPackageIdentifier packageIdentifier, DismFeatureInfo** featureInfo) { - return m_dismGetFeatureInfo(session, featureName, identifier, packageIdentifier, featureInfo); + return m_dismGetFeatureInfo(m_dismSession, featureName, identifier, packageIdentifier, featureInfo); } HRESULT DismHelper::Delete(VOID* dismStructure) @@ -124,6 +123,7 @@ namespace AppInstaller::WindowsFeature } WindowsFeature::WindowsFeature(std::shared_ptr dismHelper, const std::string& name) + : m_dismHelper(dismHelper), m_featureName(name) { #ifndef AICLI_DISABLE_TEST_HOOKS if (s_MockDismHelper_Override) @@ -131,8 +131,6 @@ namespace AppInstaller::WindowsFeature return; } #endif - m_dismHelper = std::move(dismHelper); - m_featureName = name; GetFeatureInfo(); } @@ -191,14 +189,14 @@ namespace AppInstaller::WindowsFeature return *s_EnableWindowsFeatureResult_TestHook_Override; } #endif - HRESULT hr = m_dismHelper->EnableFeature(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, DismPackageNone, FALSE, NULL, NULL, FALSE, NULL, NULL, NULL); + HRESULT hr = m_dismHelper->EnableFeature(Utility::ConvertToUTF16(m_featureName).c_str(), NULL, DismPackageNone, FALSE, NULL, NULL, FALSE, NULL, NULL, NULL); LOG_IF_FAILED(hr); return hr; } HRESULT WindowsFeature::Disable() { - HRESULT hr = m_dismHelper->DisableFeature(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, FALSE, NULL, NULL, NULL); + HRESULT hr = m_dismHelper->DisableFeature(Utility::ConvertToUTF16(m_featureName).c_str(), NULL, FALSE, NULL, NULL, NULL); LOG_IF_FAILED(hr); return hr; } @@ -253,6 +251,6 @@ namespace AppInstaller::WindowsFeature void WindowsFeature::GetFeatureInfo() { - LOG_IF_FAILED(m_dismHelper->GetFeatureInfo(m_session, Utility::ConvertToUTF16(m_featureName).c_str(), NULL, DismPackageNone, &m_featureInfo)); + LOG_IF_FAILED(m_dismHelper->GetFeatureInfo(Utility::ConvertToUTF16(m_featureName).c_str(), NULL, DismPackageNone, &m_featureInfo)); } } \ No newline at end of file From db3c8ac7195b19dfbfbf19c9f0b7a6f4b0374003 Mon Sep 17 00:00:00 2001 From: Ryan Fu Date: Fri, 24 Mar 2023 11:12:43 -0700 Subject: [PATCH 29/29] fix call site for getwindowsfeature --- .../Workflows/DependenciesFlow.cpp | 2 +- .../Public/winget/WindowsFeature.h | 80 ++++++++++--------- 2 files changed, 45 insertions(+), 37 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp index 94a8535c9f..e86788d156 100644 --- a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp @@ -168,7 +168,7 @@ namespace AppInstaller::CLI::Workflow if (SUCCEEDED(hr) || force) { auto featureName = dependency.Id(); - WindowsFeature::WindowsFeature windowsFeature{ std::move(dismHelper->GetPtr()), featureName}; + WindowsFeature::WindowsFeature windowsFeature = dismHelper->GetWindowsFeature(featureName); if (windowsFeature.DoesExist()) { diff --git a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h index fa0271cb3a..a2ec6c09dc 100644 --- a/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h +++ b/src/AppInstallerCommonCore/Public/winget/WindowsFeature.h @@ -115,17 +115,52 @@ namespace AppInstaller::WindowsFeature using DismDisableFeaturePtr = HRESULT(WINAPI*)(UINT, PCWSTR, PCWSTR, BOOL, HANDLE, DISM_PROGRESS_CALLBACK, PVOID); using DismDeletePtr = HRESULT(WINAPI*)(VOID*); - struct DismHelper : public std::enable_shared_from_this + // Forward declaration + struct DismHelper; + + /// + /// Struct representation of a single Windows Feature. + /// + struct WindowsFeature { - std::shared_ptr GetPtr() + friend DismHelper; + + ~WindowsFeature(); + + // TODO: Implement progress via DismProgressFunction + HRESULT Enable(IProgressCallback& progress); + HRESULT Disable(); + bool DoesExist(); + bool IsEnabled(); + Utility::LocIndString GetDisplayName(); + DismRestartType GetRestartRequiredStatus(); + + DismPackageFeatureState GetState() { - return shared_from_this(); + return m_featureInfo->FeatureState; } - DismHelper(); + protected: + WindowsFeature(std::shared_ptr dismHelper, const std::string& name); + + private: + void GetFeatureInfo(); + + std::string m_featureName; + std::shared_ptr m_dismHelper; + DismFeatureInfo* m_featureInfo = nullptr; + }; + struct DismHelper : public std::enable_shared_from_this + { + DismHelper(); ~DismHelper(); + WindowsFeature GetWindowsFeature(const std::string& featureName) + { + return WindowsFeature(std::move(GetPtr()), featureName); + } + HRESULT EnableFeature( PCWSTR featureName, PCWSTR identifier, @@ -159,6 +194,11 @@ namespace AppInstaller::WindowsFeature DismShutdownPtr m_dismShutdown = nullptr; DismDeletePtr m_dismDelete = nullptr; + std::shared_ptr GetPtr() + { + return shared_from_this(); + } + void Initialize(); void OpenSession(); void CloseSession(); @@ -167,36 +207,4 @@ namespace AppInstaller::WindowsFeature template FuncType GetProcAddressHelper(HMODULE module, LPCSTR functionName); }; - - /// - /// Struct representation of a single Windows Feature. - /// - struct WindowsFeature - { - // This default constructor is only used for mocking unit tests. - WindowsFeature() = default; - WindowsFeature(std::shared_ptr dismHelper, const std::string& name); - - ~WindowsFeature(); - - // TODO: Implement progress via DismProgressFunction - HRESULT Enable(IProgressCallback& progress); - HRESULT Disable(); - bool DoesExist(); - bool IsEnabled(); - Utility::LocIndString GetDisplayName(); - DismRestartType GetRestartRequiredStatus(); - - DismPackageFeatureState GetState() - { - return m_featureInfo->FeatureState; - } - - private: - void GetFeatureInfo(); - - std::string m_featureName; - std::shared_ptr m_dismHelper; - DismFeatureInfo* m_featureInfo = nullptr; - }; } \ No newline at end of file