From 741244e0363578bccc7d5e0052ee108d00c7d153 Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Thu, 30 Mar 2023 17:16:19 -0700 Subject: [PATCH] Enforce single install across winget processes (#3118) This change uses a named mutex to enforce a single install/uninstall operation is occurring for the entire session. The current behavior is that every process was able to install a single thing at a time, but as we have more usage this could lead to a potential issue. The mutex is acquired and held for the workflow task that routes out to the installer type specific tasks. Some installer types are explicitly excluded, as they are known to contain their own synchronization internally. In addition to the mutex, the ability to set a message to show with the indefinite progress (spinner) was added. This allows for the message to be removed when the wait is over. The message was not implemented into the progress bar, nor is it being sent to COM progress. The COM scheduling was not made aware of the mutex, and so it will execute as it has in the past, blocking on the mutex when appropriate. This could be improved in the future to enable the parallel capable installer types to operate. One potential option would be a second installer processing thread that only operated on the excluded types, and thus would never block. --- .github/actions/spelling/expect.txt | 1 + src/AppInstallerCLICore/COMContext.cpp | 5 + src/AppInstallerCLICore/COMContext.h | 1 + src/AppInstallerCLICore/ChannelStreams.cpp | 13 + src/AppInstallerCLICore/ChannelStreams.h | 3 + src/AppInstallerCLICore/ExecutionProgress.cpp | 39 +- src/AppInstallerCLICore/ExecutionProgress.h | 9 +- src/AppInstallerCLICore/ExecutionReporter.cpp | 14 + src/AppInstallerCLICore/ExecutionReporter.h | 1 + src/AppInstallerCLICore/Resources.h | 1 + src/AppInstallerCLICore/TableOutput.h | 19 +- .../Workflows/InstallFlow.cpp | 248 +++++---- .../Workflows/InstallFlow.h | 70 ++- .../Workflows/UninstallFlow.cpp | 527 +++++++++--------- .../Shared/Strings/en-us/winget.resw | 3 + src/AppInstallerCLITests/InstallFlow.cpp | 54 +- src/AppInstallerCLITests/Synchronization.cpp | 74 ++- src/AppInstallerCLITests/TestCommon.cpp | 4 + src/AppInstallerCLITests/TestCommon.h | 4 +- src/AppInstallerCLITests/WorkflowCommon.cpp | 4 +- src/AppInstallerCommonCore/Progress.cpp | 14 + .../Public/AppInstallerProgress.h | 8 + .../Public/AppInstallerSynchronization.h | 24 + .../Synchronization.cpp | 47 ++ 24 files changed, 753 insertions(+), 434 deletions(-) diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt index 2643093ebf..25f7fb00b1 100644 --- a/.github/actions/spelling/expect.txt +++ b/.github/actions/spelling/expect.txt @@ -75,6 +75,7 @@ corecrt count'th countof countryregion +CPIL cplusplus createmanifestmetadata cswinrt diff --git a/src/AppInstallerCLICore/COMContext.cpp b/src/AppInstallerCLICore/COMContext.cpp index e914e617cf..1ce09b60b8 100644 --- a/src/AppInstallerCLICore/COMContext.cpp +++ b/src/AppInstallerCLICore/COMContext.cpp @@ -41,6 +41,11 @@ namespace AppInstaller::CLI::Execution FireCallbacks(ReportType::Progressing, current, maximum, progressType, m_executionStage); } + void COMContext::SetProgressMessage(std::string_view) + { + // TODO: Consider sending message to COM progress + } + void COMContext::EndProgress(bool) { FireCallbacks(ReportType::EndProgress, 0, 0, ProgressType::None, m_executionStage); diff --git a/src/AppInstallerCLICore/COMContext.h b/src/AppInstallerCLICore/COMContext.h index c458f4570c..472907d76e 100644 --- a/src/AppInstallerCLICore/COMContext.h +++ b/src/AppInstallerCLICore/COMContext.h @@ -54,6 +54,7 @@ namespace AppInstaller::CLI::Execution // IProgressSink void BeginProgress() override; void OnProgress(uint64_t current, uint64_t maximum, ProgressType type) override; + void SetProgressMessage(std::string_view message) override; void EndProgress(bool) override; //Execution::Context diff --git a/src/AppInstallerCLICore/ChannelStreams.cpp b/src/AppInstallerCLICore/ChannelStreams.cpp index 289d6b7763..bf7ba6d62f 100644 --- a/src/AppInstallerCLICore/ChannelStreams.cpp +++ b/src/AppInstallerCLICore/ChannelStreams.cpp @@ -9,6 +9,19 @@ namespace AppInstaller::CLI::Execution using namespace Settings; using namespace VirtualTerminal; + size_t GetConsoleWidth() + { + CONSOLE_SCREEN_BUFFER_INFO consoleInfo{}; + if (GetConsoleScreenBufferInfo(GetStdHandle(STD_OUTPUT_HANDLE), &consoleInfo)) + { + return static_cast(consoleInfo.dwSize.X); + } + else + { + return 120; + } + } + BaseStream::BaseStream(std::ostream& out, bool enabled, bool VTEnabled) : m_out(out), m_enabled(enabled), m_VTEnabled(VTEnabled) {} diff --git a/src/AppInstallerCLICore/ChannelStreams.h b/src/AppInstallerCLICore/ChannelStreams.h index 64816786de..5fa7d77651 100644 --- a/src/AppInstallerCLICore/ChannelStreams.h +++ b/src/AppInstallerCLICore/ChannelStreams.h @@ -11,6 +11,9 @@ namespace AppInstaller::CLI::Execution { + // Gets the current console width. + size_t GetConsoleWidth(); + // The base stream for all channels. struct BaseStream { diff --git a/src/AppInstallerCLICore/ExecutionProgress.cpp b/src/AppInstallerCLICore/ExecutionProgress.cpp index 27defe8b8c..9b0e6f5f16 100644 --- a/src/AppInstallerCLICore/ExecutionProgress.cpp +++ b/src/AppInstallerCLICore/ExecutionProgress.cpp @@ -151,6 +151,23 @@ namespace AppInstaller::CLI::Execution LOG_HR(E_UNEXPECTED); } } + + void ProgressVisualizerBase::ClearLine() + { + if (UseVT()) + { + m_out << TextModification::EraseLineEntirely << '\r'; + } + else + { + m_out << '\r' << std::string(GetConsoleWidth(), ' ') << '\r'; + } + } + + void ProgressVisualizerBase::SetMessage(std::string_view message) + { + m_message = message; + } } void IndefiniteSpinner::ShowSpinner() @@ -188,17 +205,19 @@ namespace AppInstaller::CLI::Execution } // Indent two spaces for the spinner, but three here so that we can overwrite it in the loop. - m_out << " "; + std::string_view indent = " "; for (size_t i = 0; !m_canceled; ++i) { constexpr size_t repetitionCount = 20; ApplyStyle(i % repetitionCount, repetitionCount, true); - m_out << '\b' << spinnerChars[i % ARRAYSIZE(spinnerChars)] << std::flush; + m_out << '\r' << indent << spinnerChars[i % ARRAYSIZE(spinnerChars)]; + m_out.RestoreDefault(); + m_out << ' ' << m_message << std::flush; Sleep(250); } - m_out << "\b \r"; + ClearLine(); if (UseVT()) { @@ -217,6 +236,7 @@ namespace AppInstaller::CLI::Execution ClearLine(); } + // TODO: Progress bar does not currently use message if (UseVT()) { ShowProgressWithVT(current, maximum, type); @@ -255,19 +275,6 @@ namespace AppInstaller::CLI::Execution } } - void ProgressBar::ClearLine() - { - if (UseVT()) - { - m_out << TextModification::EraseLineEntirely << '\r'; - } - else - { - // Best effort when no VT (arbitrary number of spaces that seems to work) - m_out << "\r \r"; - } - } - void ProgressBar::ShowProgressNoVT(uint64_t current, uint64_t maximum, ProgressType type) { m_out << "\r "; diff --git a/src/AppInstallerCLICore/ExecutionProgress.h b/src/AppInstallerCLICore/ExecutionProgress.h index ba959c358c..43f8b34524 100644 --- a/src/AppInstallerCLICore/ExecutionProgress.h +++ b/src/AppInstallerCLICore/ExecutionProgress.h @@ -27,15 +27,20 @@ namespace AppInstaller::CLI::Execution void SetStyle(AppInstaller::Settings::VisualStyle style) { m_style = style; } + void SetMessage(std::string_view message); + protected: BaseStream& m_out; Settings::VisualStyle m_style = AppInstaller::Settings::VisualStyle::Accent; + std::string m_message; bool UseVT() const { return m_enableVT && m_style != AppInstaller::Settings::VisualStyle::NoVT; } // Applies the selected visual style. void ApplyStyle(size_t i, size_t max, bool foregroundOnly); + void ClearLine(); + private: bool m_enableVT = false; }; @@ -70,14 +75,10 @@ namespace AppInstaller::CLI::Execution void EndProgress(bool hideProgressWhenDone); - void SetStyle(AppInstaller::Settings::VisualStyle style) { m_style = style; } - private: std::atomic m_isVisible = false; uint64_t m_lastCurrent = 0; - void ClearLine(); - void ShowProgressNoVT(uint64_t current, uint64_t maximum, ProgressType type); void ShowProgressWithVT(uint64_t current, uint64_t maximum, ProgressType type); diff --git a/src/AppInstallerCLICore/ExecutionReporter.cpp b/src/AppInstallerCLICore/ExecutionReporter.cpp index 177fbeaeaa..fe3ed62f6b 100644 --- a/src/AppInstallerCLICore/ExecutionReporter.cpp +++ b/src/AppInstallerCLICore/ExecutionReporter.cpp @@ -216,6 +216,19 @@ namespace AppInstaller::CLI::Execution } } + void Reporter::SetProgressMessage(std::string_view message) + { + if (m_spinner) + { + m_spinner->SetMessage(message); + } + + if (m_progressBar) + { + m_progressBar->SetMessage(message); + } + } + void Reporter::BeginProgress() { GetBasicOutputStream() << VirtualTerminal::Cursor::Visibility::DisableShow; @@ -229,6 +242,7 @@ namespace AppInstaller::CLI::Execution { m_progressBar->EndProgress(hideProgressWhenDone); } + SetProgressMessage({}); GetBasicOutputStream() << VirtualTerminal::Cursor::Visibility::EnableShow; }; diff --git a/src/AppInstallerCLICore/ExecutionReporter.h b/src/AppInstallerCLICore/ExecutionReporter.h index 6c02c87161..a0ffeb837c 100644 --- a/src/AppInstallerCLICore/ExecutionReporter.h +++ b/src/AppInstallerCLICore/ExecutionReporter.h @@ -113,6 +113,7 @@ namespace AppInstaller::CLI::Execution // IProgressSink void BeginProgress() override; void OnProgress(uint64_t current, uint64_t maximum, ProgressType type) override; + void SetProgressMessage(std::string_view message) override; void EndProgress(bool hideProgressWhenDone) override; // Runs the given callable of type: auto(IProgressCallback&) diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index 021c50398c..bff7ff6f76 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -209,6 +209,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(InstallFlowStartingPackageInstall); WINGET_DEFINE_RESOURCE_STRINGID(InstallLocationNotProvided); WINGET_DEFINE_RESOURCE_STRINGID(InstallScopeDescription); + WINGET_DEFINE_RESOURCE_STRINGID(InstallWaitingOnAnother); WINGET_DEFINE_RESOURCE_STRINGID(InteractiveArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(InvalidAliasError); WINGET_DEFINE_RESOURCE_STRINGID(InvalidArgumentSpecifierError); diff --git a/src/AppInstallerCLICore/TableOutput.h b/src/AppInstallerCLICore/TableOutput.h index c9c63260a0..6916602e40 100644 --- a/src/AppInstallerCLICore/TableOutput.h +++ b/src/AppInstallerCLICore/TableOutput.h @@ -12,23 +12,6 @@ namespace AppInstaller::CLI::Execution { - namespace details - { - // Gets the column width of the console. - inline size_t GetConsoleWidth() - { - CONSOLE_SCREEN_BUFFER_INFO consoleInfo{}; - if (GetConsoleScreenBufferInfo(GetStdHandle(STD_OUTPUT_HANDLE), &consoleInfo)) - { - return static_cast(consoleInfo.dwSize.X); - } - else - { - return 120; - } - } - } - // Enables output data in a table format. // TODO: Improve for use with sparse data. template @@ -144,7 +127,7 @@ namespace AppInstaller::CLI::Execution totalRequired += m_columns[i].MaxLength + (m_columns[i].SpaceAfter ? 1 : 0); } - size_t consoleWidth = details::GetConsoleWidth(); + size_t consoleWidth = GetConsoleWidth(); // If the total space would be too big, shrink them. // We don't want to use the last column, lest we auto-wrap diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index 9dd3778e6b..4d0fe11601 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -21,6 +21,7 @@ #include #include #include +#include using namespace winrt::Windows::ApplicationModel::Store::Preview::InstallControl; using namespace winrt::Windows::Foundation; @@ -151,6 +152,135 @@ namespace AppInstaller::CLI::Workflow }; } + namespace details + { + // Runs the installer via ShellExecute. + // Required Args: None + // Inputs: Installer, InstallerPath + // Outputs: None + void ShellExecuteInstall(Execution::Context& context) + { + context << + GetInstallerArgs << + ShellExecuteInstallImpl << + ReportInstallerResult("ShellExecute"sv, APPINSTALLER_CLI_ERROR_SHELLEXEC_INSTALL_FAILED); + } + + // Runs an MSI installer directly via MSI APIs. + // Required Args: None + // Inputs: Installer, InstallerPath + // Outputs: None + void DirectMSIInstall(Execution::Context& context) + { + context << + GetInstallerArgs << + DirectMSIInstallImpl << + ReportInstallerResult("MsiInstallProduct"sv, APPINSTALLER_CLI_ERROR_MSI_INSTALL_FAILED); + } + + // Deploys the MSIX. + // Required Args: None + // Inputs: Manifest?, Installer || InstallerPath + // Outputs: None + void MsixInstall(Execution::Context& context) + { + std::string uri; + if (context.Contains(Execution::Data::InstallerPath)) + { + uri = context.Get().u8string(); + } + else + { + uri = context.Get()->Url; + } + + bool isMachineScope = Manifest::ConvertToScopeEnum(context.Args.GetArg(Execution::Args::Type::InstallScope)) == Manifest::ScopeEnum::Machine; + + // TODO: There was a bug in deployment api if provision api was called in packaged context. + // Remove this check when the OS bug is fixed and back ported. + if (isMachineScope && Runtime::IsRunningInPackagedContext()) + { + context.Reporter.Error() << Resource::String::InstallFlowReturnCodeSystemNotSupported << std::endl; + context.Add(static_cast(APPINSTALLER_CLI_ERROR_INSTALL_SYSTEM_NOT_SUPPORTED)); + AICLI_LOG(CLI, Error, << "Device wide install for msix type is not supported in packaged context."); + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALL_SYSTEM_NOT_SUPPORTED); + } + + context.Reporter.Info() << Resource::String::InstallFlowStartingPackageInstall << std::endl; + + bool registrationDeferred = false; + + try + { + registrationDeferred = context.Reporter.ExecuteWithProgress([&](IProgressCallback& callback) + { + if (isMachineScope) + { + return Deployment::AddPackageMachineScope(uri, callback); + } + else + { + return Deployment::AddPackageWithDeferredFallback(uri, WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::InstallerTrusted), callback); + } + }); + } + catch (const wil::ResultException& re) + { + context.Add(re.GetErrorCode()); + context << ReportInstallerResult("MSIX"sv, re.GetErrorCode(), /* isHResult */ true); + return; + } + + if (registrationDeferred) + { + context.Reporter.Warn() << Resource::String::InstallFlowRegistrationDeferred << std::endl; + } + else + { + context.Reporter.Info() << Resource::String::InstallFlowInstallSuccess << std::endl; + } + } + + // Runs the flow for installing a Portable package. + // Required Args: None + // Inputs: Installer, InstallerPath + // Outputs: None + void PortableInstall(Execution::Context& context) + { + context << + InitializePortableInstaller << + VerifyPackageAndSourceMatch << + PortableInstallImpl << + ReportInstallerResult("Portable"sv, APPINSTALLER_CLI_ERROR_PORTABLE_INSTALL_FAILED, true); + } + + // Runs the flow for installing a package from an archive. + // Required Args: None + // Inputs: Installer, InstallerPath, Manifest + // Outputs: None + void ArchiveInstall(Execution::Context& context) + { + context << + ScanArchiveFromLocalManifest << + ExtractFilesFromArchive << + VerifyAndSetNestedInstaller << + ExecuteInstallerForType(context.Get().value().NestedInstallerType); + } + } + + bool ExemptFromSingleInstallLocking(InstallerTypeEnum type) + { + switch (type) + { + // MSStore installs are always MSIX based; MSIX installs are safe to run in parallel. + case InstallerTypeEnum::Msix: + case InstallerTypeEnum::MSStore: + return true; + default: + return false; + } + } + void EnsureApplicableInstaller(Execution::Context& context) { const auto& installer = context.Get(); @@ -249,6 +379,21 @@ namespace AppInstaller::CLI::Workflow UpdateBehaviorEnum updateBehavior = context.Get().value().UpdateBehavior; bool doUninstallPrevious = isUpdate && (updateBehavior == UpdateBehaviorEnum::UninstallPrevious || context.Args.Contains(Execution::Args::Type::UninstallPrevious)); + Synchronization::CrossProcessInstallLock lock; + if (!ExemptFromSingleInstallLocking(m_installerType)) + { + // Acquire install lock; if the operation is cancelled it will return false so we will also return. + if (!context.Reporter.ExecuteWithProgress([&](IProgressCallback& callback) + { + callback.SetProgressMessage(Resource::String::InstallWaitingOnAnother()); + return lock.Acquire(callback); + })) + { + AICLI_LOG(CLI, Info, << "Abandoning attempt to acquire install lock due to cancellation"); + return; + } + } + switch (m_installerType) { case InstallerTypeEnum::Exe: @@ -266,15 +411,15 @@ namespace AppInstaller::CLI::Workflow } if (ShouldUseDirectMSIInstall(m_installerType, context.Args.Contains(Execution::Args::Type::Silent))) { - context << DirectMSIInstall; + context << details::DirectMSIInstall; } else { - context << ShellExecuteInstall; + context << details::ShellExecuteInstall; } break; case InstallerTypeEnum::Msix: - context << MsixInstall; + context << details::MsixInstall; break; case InstallerTypeEnum::MSStore: context << @@ -289,10 +434,10 @@ namespace AppInstaller::CLI::Workflow ExecuteUninstaller; context.ClearFlags(Execution::ContextFlag::InstallerExecutionUseUpdate); } - context << PortableInstall; + context << details::PortableInstall; break; case InstallerTypeEnum::Zip: - context << ArchiveInstall; + context << details::ArchiveInstall; break; default: THROW_HR(HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED)); @@ -319,99 +464,6 @@ namespace AppInstaller::CLI::Workflow context << Workflow::ExecuteInstallerForType(context.Get().value().BaseInstallerType); } - void ArchiveInstall(Execution::Context& context) - { - context << - ScanArchiveFromLocalManifest << - ExtractFilesFromArchive << - VerifyAndSetNestedInstaller << - ExecuteInstallerForType(context.Get().value().NestedInstallerType); - } - - void ShellExecuteInstall(Execution::Context& context) - { - context << - GetInstallerArgs << - ShellExecuteInstallImpl << - ReportInstallerResult("ShellExecute"sv, APPINSTALLER_CLI_ERROR_SHELLEXEC_INSTALL_FAILED); - } - - void DirectMSIInstall(Execution::Context& context) - { - context << - GetInstallerArgs << - DirectMSIInstallImpl << - ReportInstallerResult("MsiInstallProduct"sv, APPINSTALLER_CLI_ERROR_MSI_INSTALL_FAILED); - } - - void PortableInstall(Execution::Context& context) - { - context << - InitializePortableInstaller << - VerifyPackageAndSourceMatch << - PortableInstallImpl << - ReportInstallerResult("Portable"sv, APPINSTALLER_CLI_ERROR_PORTABLE_INSTALL_FAILED, true); - } - - void MsixInstall(Execution::Context& context) - { - std::string uri; - if (context.Contains(Execution::Data::InstallerPath)) - { - uri = context.Get().u8string(); - } - else - { - uri = context.Get()->Url; - } - - bool isMachineScope = Manifest::ConvertToScopeEnum(context.Args.GetArg(Execution::Args::Type::InstallScope)) == Manifest::ScopeEnum::Machine; - - // TODO: There was a bug in deployment api if provision api was called in packaged context. - // Remove this check when the OS bug is fixed and back ported. - if (isMachineScope && Runtime::IsRunningInPackagedContext()) - { - context.Reporter.Error() << Resource::String::InstallFlowReturnCodeSystemNotSupported << std::endl; - context.Add(static_cast(APPINSTALLER_CLI_ERROR_INSTALL_SYSTEM_NOT_SUPPORTED)); - AICLI_LOG(CLI, Error, << "Device wide install for msix type is not supported in packaged context."); - AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALL_SYSTEM_NOT_SUPPORTED); - } - - context.Reporter.Info() << Resource::String::InstallFlowStartingPackageInstall << std::endl; - - bool registrationDeferred = false; - - try - { - registrationDeferred = context.Reporter.ExecuteWithProgress([&](IProgressCallback& callback) - { - if (isMachineScope) - { - return Deployment::AddPackageMachineScope(uri, callback); - } - else - { - return Deployment::AddPackageWithDeferredFallback(uri, WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::InstallerTrusted), callback); - } - }); - } - catch (const wil::ResultException& re) - { - context.Add(re.GetErrorCode()); - context << ReportInstallerResult("MSIX"sv, re.GetErrorCode(), /* isHResult */ true); - return; - } - - if (registrationDeferred) - { - context.Reporter.Warn() << Resource::String::InstallFlowRegistrationDeferred << std::endl; - } - else - { - context.Reporter.Info() << Resource::String::InstallFlowInstallSuccess << std::endl; - } - } - void ReportInstallerResult::operator()(Execution::Context& context) const { DWORD installResult = context.Get(); diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.h b/src/AppInstallerCLICore/Workflows/InstallFlow.h index 947eac5bf4..22f7ac6116 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.h +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.h @@ -2,6 +2,7 @@ // Licensed under the MIT License. #pragma once #include "ExecutionContext.h" +#include namespace AppInstaller::CLI::Workflow { @@ -11,6 +12,45 @@ namespace AppInstaller::CLI::Workflow static constexpr std::string_view ARG_TOKEN_LOGPATH = ""sv; static constexpr std::string_view ARG_TOKEN_INSTALLPATH = ""sv; + // Determines if an installer type is allowed to install/uninstall in parallel. + bool ExemptFromSingleInstallLocking(AppInstaller::Manifest::InstallerTypeEnum type); + + namespace details + { + // These single type install flows should remain "internal" and only ExecuteInstallerForType should be used externally + // so that all installs can properly handle single install locking. + + // Runs the installer via ShellExecute. + // Required Args: None + // Inputs: Installer, InstallerPath + // Outputs: None + void ShellExecuteInstall(Execution::Context& context); + + // Runs an MSI installer directly via MSI APIs. + // Required Args: None + // Inputs: Installer, InstallerPath + // Outputs: None + void DirectMSIInstall(Execution::Context& context); + + // Deploys the MSIX. + // Required Args: None + // Inputs: Manifest?, Installer || InstallerPath + // Outputs: None + void MsixInstall(Execution::Context& context); + + // Runs the flow for installing a Portable package. + // Required Args: None + // Inputs: Installer, InstallerPath + // Outputs: None + void PortableInstall(Execution::Context& context); + + // Runs the flow for installing a package from an archive. + // Required Args: None + // Inputs: Installer, InstallerPath, Manifest + // Outputs: None + void ArchiveInstall(Execution::Context& context); + } + // Ensures that there is an applicable installer. // Required Args: None // Inputs: Installer @@ -61,36 +101,6 @@ namespace AppInstaller::CLI::Workflow Manifest::InstallerTypeEnum m_installerType; }; - // Runs the installer via ShellExecute. - // Required Args: None - // Inputs: Installer, InstallerPath - // Outputs: None - void ShellExecuteInstall(Execution::Context& context); - - // Runs an MSI installer directly via MSI APIs. - // Required Args: None - // Inputs: Installer, InstallerPath - // Outputs: None - void DirectMSIInstall(Execution::Context& context); - - // Deploys the MSIX. - // Required Args: None - // Inputs: Manifest?, Installer || InstallerPath - // Outputs: None - void MsixInstall(Execution::Context& context); - - // Runs the flow for installing a Portable package. - // Required Args: None - // Inputs: Installer, InstallerPath - // Outputs: None - void PortableInstall(Execution::Context& context); - - // Runs the flow for installing a package from an archive. - // Required Args: None - // Inputs: Installer, InstallerPath, Manifest - // Outputs: None - void ArchiveInstall(Execution::Context& context); - // Verifies parameters for install to ensure success. // Required Args: None // Inputs: diff --git a/src/AppInstallerCLICore/Workflows/UninstallFlow.cpp b/src/AppInstallerCLICore/Workflows/UninstallFlow.cpp index 9372f7f69e..1712e876bf 100644 --- a/src/AppInstallerCLICore/Workflows/UninstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/UninstallFlow.cpp @@ -1,71 +1,73 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. -#include "pch.h" -#include "UninstallFlow.h" -#include "WorkflowBase.h" -#include "DependenciesFlow.h" -#include "ShellExecuteInstallerHandler.h" -#include "AppInstallerMsixInfo.h" +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +#include "pch.h" +#include "UninstallFlow.h" +#include "InstallFlow.h" +#include "WorkflowBase.h" +#include "DependenciesFlow.h" +#include "ShellExecuteInstallerHandler.h" +#include "AppInstallerMsixInfo.h" #include "PortableFlow.h" -#include - -using namespace AppInstaller::CLI::Execution; -using namespace AppInstaller::Manifest; -using namespace AppInstaller::Msix; -using namespace AppInstaller::Repository; -using namespace AppInstaller::Registry; -using namespace AppInstaller::CLI::Portable; -using namespace AppInstaller::Utility::literals; - -namespace AppInstaller::CLI::Workflow -{ - namespace - { - // Helper for RecordUninstall - struct UninstallCorrelatedSources - { - struct Item - { - Utility::LocIndString Identifier; - Source FromSource; - std::string SourceIdentifier; - }; - - void AddIfRemoteAndNotPresent(const std::shared_ptr& packageVersion) - { - auto source = packageVersion->GetSource(); - const auto details = source.GetDetails(); - if (!source.ContainsAvailablePackages()) - { - return; - } - - for (const auto& item : Items) - { - if (item.SourceIdentifier == details.Identifier) - { - return; - } - } - - Items.emplace_back(Item{ packageVersion->GetProperty(PackageVersionProperty::Id), std::move(source), details.Identifier }); - } - - std::vector Items; - }; - } - +#include +#include + +using namespace AppInstaller::CLI::Execution; +using namespace AppInstaller::Manifest; +using namespace AppInstaller::Msix; +using namespace AppInstaller::Repository; +using namespace AppInstaller::Registry; +using namespace AppInstaller::CLI::Portable; +using namespace AppInstaller::Utility::literals; + +namespace AppInstaller::CLI::Workflow +{ + namespace + { + // Helper for RecordUninstall + struct UninstallCorrelatedSources + { + struct Item + { + Utility::LocIndString Identifier; + Source FromSource; + std::string SourceIdentifier; + }; + + void AddIfRemoteAndNotPresent(const std::shared_ptr& packageVersion) + { + auto source = packageVersion->GetSource(); + const auto details = source.GetDetails(); + if (!source.ContainsAvailablePackages()) + { + return; + } + + for (const auto& item : Items) + { + if (item.SourceIdentifier == details.Identifier) + { + return; + } + } + + Items.emplace_back(Item{ packageVersion->GetProperty(PackageVersionProperty::Id), std::move(source), details.Identifier }); + } + + std::vector Items; + }; + } + void UninstallSinglePackage(Execution::Context& context) { - context << - Workflow::GetInstalledPackageVersion << - Workflow::EnsureSupportForUninstall << - Workflow::GetUninstallInfo << - Workflow::GetDependenciesInfoForUninstall << - Workflow::ReportDependencies(Resource::String::UninstallCommandReportDependencies) << - Workflow::ReportExecutionStage(ExecutionStage::Execution) << - Workflow::ExecuteUninstaller << - Workflow::ReportExecutionStage(ExecutionStage::PostExecution) << + context << + Workflow::GetInstalledPackageVersion << + Workflow::EnsureSupportForUninstall << + Workflow::GetUninstallInfo << + Workflow::GetDependenciesInfoForUninstall << + Workflow::ReportDependencies(Resource::String::UninstallCommandReportDependencies) << + Workflow::ReportExecutionStage(ExecutionStage::Execution) << + Workflow::ExecuteUninstaller << + Workflow::ReportExecutionStage(ExecutionStage::PostExecution) << Workflow::RecordUninstall; } @@ -117,128 +119,145 @@ namespace AppInstaller::CLI::Workflow } } - void GetUninstallInfo(Execution::Context& context) - { - auto installedPackageVersion = context.Get(); - const std::string installedTypeString = installedPackageVersion->GetMetadata()[PackageVersionMetadata::InstalledType]; - switch (ConvertToInstallerTypeEnum(installedTypeString)) - { - case InstallerTypeEnum::Exe: - case InstallerTypeEnum::Burn: - case InstallerTypeEnum::Inno: - case InstallerTypeEnum::Nullsoft: - { - IPackageVersion::Metadata packageMetadata = installedPackageVersion->GetMetadata(); - - // Default to silent unless it is not present or interactivity is requested - auto uninstallCommandItr = packageMetadata.find(PackageVersionMetadata::SilentUninstallCommand); - if (uninstallCommandItr == packageMetadata.end() || context.Args.Contains(Execution::Args::Type::Interactive)) - { - auto interactiveItr = packageMetadata.find(PackageVersionMetadata::StandardUninstallCommand); - if (interactiveItr != packageMetadata.end()) - { - uninstallCommandItr = interactiveItr; - } - } - - if (uninstallCommandItr == packageMetadata.end()) - { - context.Reporter.Error() << Resource::String::NoUninstallInfoFound << std::endl; - AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_NO_UNINSTALL_INFO_FOUND); - } - - context.Add(uninstallCommandItr->second); - break; - } - case InstallerTypeEnum::Msi: - case InstallerTypeEnum::Wix: - { - // Uninstall strings for MSI don't include UI level (/q) needed to avoid interactivity, - // so we handle them differently. - auto productCodes = installedPackageVersion->GetMultiProperty(PackageVersionMultiProperty::ProductCode); - if (productCodes.empty()) - { - context.Reporter.Error() << Resource::String::NoUninstallInfoFound << std::endl; - AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_NO_UNINSTALL_INFO_FOUND); - } - - context.Add(std::move(productCodes)); - break; - } - case InstallerTypeEnum::Msix: - case InstallerTypeEnum::MSStore: - { - auto packageFamilyNames = installedPackageVersion->GetMultiProperty(PackageVersionMultiProperty::PackageFamilyName); - if (packageFamilyNames.empty()) - { - context.Reporter.Error() << Resource::String::NoUninstallInfoFound << std::endl; - AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_NO_UNINSTALL_INFO_FOUND); - } - - context.Add(packageFamilyNames); - break; - } - case InstallerTypeEnum::Portable: - { + void GetUninstallInfo(Execution::Context& context) + { + auto installedPackageVersion = context.Get(); + const std::string installedTypeString = installedPackageVersion->GetMetadata()[PackageVersionMetadata::InstalledType]; + switch (ConvertToInstallerTypeEnum(installedTypeString)) + { + case InstallerTypeEnum::Exe: + case InstallerTypeEnum::Burn: + case InstallerTypeEnum::Inno: + case InstallerTypeEnum::Nullsoft: + { + IPackageVersion::Metadata packageMetadata = installedPackageVersion->GetMetadata(); + + // Default to silent unless it is not present or interactivity is requested + auto uninstallCommandItr = packageMetadata.find(PackageVersionMetadata::SilentUninstallCommand); + if (uninstallCommandItr == packageMetadata.end() || context.Args.Contains(Execution::Args::Type::Interactive)) + { + auto interactiveItr = packageMetadata.find(PackageVersionMetadata::StandardUninstallCommand); + if (interactiveItr != packageMetadata.end()) + { + uninstallCommandItr = interactiveItr; + } + } + + if (uninstallCommandItr == packageMetadata.end()) + { + context.Reporter.Error() << Resource::String::NoUninstallInfoFound << std::endl; + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_NO_UNINSTALL_INFO_FOUND); + } + + context.Add(uninstallCommandItr->second); + break; + } + case InstallerTypeEnum::Msi: + case InstallerTypeEnum::Wix: + { + // Uninstall strings for MSI don't include UI level (/q) needed to avoid interactivity, + // so we handle them differently. auto productCodes = installedPackageVersion->GetMultiProperty(PackageVersionMultiProperty::ProductCode); if (productCodes.empty()) { context.Reporter.Error() << Resource::String::NoUninstallInfoFound << std::endl; AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_NO_UNINSTALL_INFO_FOUND); - } - + } + + context.Add(std::move(productCodes)); + break; + } + case InstallerTypeEnum::Msix: + case InstallerTypeEnum::MSStore: + { + auto packageFamilyNames = installedPackageVersion->GetMultiProperty(PackageVersionMultiProperty::PackageFamilyName); + if (packageFamilyNames.empty()) + { + context.Reporter.Error() << Resource::String::NoUninstallInfoFound << std::endl; + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_NO_UNINSTALL_INFO_FOUND); + } + + context.Add(packageFamilyNames); + break; + } + case InstallerTypeEnum::Portable: + { + auto productCodes = installedPackageVersion->GetMultiProperty(PackageVersionMultiProperty::ProductCode); + if (productCodes.empty()) + { + context.Reporter.Error() << Resource::String::NoUninstallInfoFound << std::endl; + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_NO_UNINSTALL_INFO_FOUND); + } + const std::string installedScope = context.Get()->GetMetadata()[Repository::PackageVersionMetadata::InstalledScope]; - const std::string installedArch = context.Get()->GetMetadata()[Repository::PackageVersionMetadata::InstalledArchitecture]; - + const std::string installedArch = context.Get()->GetMetadata()[Repository::PackageVersionMetadata::InstalledArchitecture]; + PortableInstaller portableInstaller = PortableInstaller( Manifest::ConvertToScopeEnum(installedScope), Utility::ConvertToArchitectureEnum(installedArch), - productCodes[0]); - context.Add(std::move(portableInstaller)); - break; - } - default: - THROW_HR(HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED)); - } - } - - void ExecuteUninstaller(Execution::Context& context) - { - const std::string installedTypeString = context.Get()->GetMetadata()[PackageVersionMetadata::InstalledType]; - switch (ConvertToInstallerTypeEnum(installedTypeString)) - { - case InstallerTypeEnum::Exe: - case InstallerTypeEnum::Burn: - case InstallerTypeEnum::Inno: - case InstallerTypeEnum::Nullsoft: - context << - Workflow::ShellExecuteUninstallImpl << - ReportUninstallerResult("UninstallString", APPINSTALLER_CLI_ERROR_EXEC_UNINSTALL_COMMAND_FAILED); - break; - case InstallerTypeEnum::Msi: - case InstallerTypeEnum::Wix: - context << - Workflow::ShellExecuteMsiExecUninstall << - ReportUninstallerResult("MsiExec", APPINSTALLER_CLI_ERROR_EXEC_UNINSTALL_COMMAND_FAILED); - break; - case InstallerTypeEnum::Msix: - case InstallerTypeEnum::MSStore: - context << Workflow::MsixUninstall; - break; - case InstallerTypeEnum::Portable: - context << - Workflow::PortableUninstallImpl << - ReportUninstallerResult("PortableUninstall"sv, APPINSTALLER_CLI_ERROR_PORTABLE_UNINSTALL_FAILED, true); - break; - default: - THROW_HR(HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED)); - } - } - - void MsixUninstall(Execution::Context& context) - { - bool isMachineScope = Manifest::ConvertToScopeEnum(context.Args.GetArg(Execution::Args::Type::InstallScope)) == Manifest::ScopeEnum::Machine; - + productCodes[0]); + context.Add(std::move(portableInstaller)); + break; + } + default: + THROW_HR(HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED)); + } + } + + void ExecuteUninstaller(Execution::Context& context) + { + const std::string installedTypeString = context.Get()->GetMetadata()[PackageVersionMetadata::InstalledType]; + InstallerTypeEnum installerType = ConvertToInstallerTypeEnum(installedTypeString); + + Synchronization::CrossProcessInstallLock lock; + if (!ExemptFromSingleInstallLocking(installerType)) + { + // Acquire install lock; if the operation is cancelled it will return false so we will also return. + if (!context.Reporter.ExecuteWithProgress([&](IProgressCallback& callback) + { + callback.SetProgressMessage(Resource::String::InstallWaitingOnAnother()); + return lock.Acquire(callback); + })) + { + AICLI_LOG(CLI, Info, << "Abandoning attempt to acquire install lock due to cancellation"); + return; + } + } + + switch (installerType) + { + case InstallerTypeEnum::Exe: + case InstallerTypeEnum::Burn: + case InstallerTypeEnum::Inno: + case InstallerTypeEnum::Nullsoft: + context << + Workflow::ShellExecuteUninstallImpl << + ReportUninstallerResult("UninstallString", APPINSTALLER_CLI_ERROR_EXEC_UNINSTALL_COMMAND_FAILED); + break; + case InstallerTypeEnum::Msi: + case InstallerTypeEnum::Wix: + context << + Workflow::ShellExecuteMsiExecUninstall << + ReportUninstallerResult("MsiExec", APPINSTALLER_CLI_ERROR_EXEC_UNINSTALL_COMMAND_FAILED); + break; + case InstallerTypeEnum::Msix: + case InstallerTypeEnum::MSStore: + context << Workflow::MsixUninstall; + break; + case InstallerTypeEnum::Portable: + context << + Workflow::PortableUninstallImpl << + ReportUninstallerResult("PortableUninstall"sv, APPINSTALLER_CLI_ERROR_PORTABLE_UNINSTALL_FAILED, true); + break; + default: + THROW_HR(HRESULT_FROM_WIN32(ERROR_NOT_SUPPORTED)); + } + } + + void MsixUninstall(Execution::Context& context) + { + bool isMachineScope = Manifest::ConvertToScopeEnum(context.Args.GetArg(Execution::Args::Type::InstallScope)) == Manifest::ScopeEnum::Machine; + // TODO: There was a bug in deployment api if deprovision api was called in packaged context. // Remove this check when the OS bug is fixed and back ported. if (isMachineScope && Runtime::IsRunningInPackagedContext()) @@ -247,66 +266,66 @@ namespace AppInstaller::CLI::Workflow context.Add(static_cast(APPINSTALLER_CLI_ERROR_INSTALL_SYSTEM_NOT_SUPPORTED)); AICLI_LOG(CLI, Error, << "Device wide uninstall for msix type is not supported in packaged context."); AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALL_SYSTEM_NOT_SUPPORTED); - } - - const auto& packageFamilyNames = context.Get(); - context.Reporter.Info() << Resource::String::UninstallFlowStartingPackageUninstall << std::endl; - - for (const auto& packageFamilyName : packageFamilyNames) - { - auto packageFullName = Msix::GetPackageFullNameFromFamilyName(packageFamilyName); - if (!packageFullName.has_value()) - { - AICLI_LOG(CLI, Warning, << "No package found with family name: " << packageFamilyName); - continue; - } - - AICLI_LOG(CLI, Info, << "Removing MSIX package: " << packageFullName.value()); - try - { - if (isMachineScope) - { - context.Reporter.ExecuteWithProgress(std::bind(Deployment::RemovePackageMachineScope, packageFamilyName, packageFullName.value(), std::placeholders::_1)); - } - else - { - context.Reporter.ExecuteWithProgress(std::bind(Deployment::RemovePackage, packageFullName.value(), winrt::Windows::Management::Deployment::RemovalOptions::None, std::placeholders::_1)); - } - } + } + + const auto& packageFamilyNames = context.Get(); + context.Reporter.Info() << Resource::String::UninstallFlowStartingPackageUninstall << std::endl; + + for (const auto& packageFamilyName : packageFamilyNames) + { + auto packageFullName = Msix::GetPackageFullNameFromFamilyName(packageFamilyName); + if (!packageFullName.has_value()) + { + AICLI_LOG(CLI, Warning, << "No package found with family name: " << packageFamilyName); + continue; + } + + AICLI_LOG(CLI, Info, << "Removing MSIX package: " << packageFullName.value()); + try + { + if (isMachineScope) + { + context.Reporter.ExecuteWithProgress(std::bind(Deployment::RemovePackageMachineScope, packageFamilyName, packageFullName.value(), std::placeholders::_1)); + } + else + { + context.Reporter.ExecuteWithProgress(std::bind(Deployment::RemovePackage, packageFullName.value(), winrt::Windows::Management::Deployment::RemovalOptions::None, std::placeholders::_1)); + } + } catch (const wil::ResultException& re) { context.Add(re.GetErrorCode()); context << ReportUninstallerResult("MSIXUninstall"sv, re.GetErrorCode(), /* isHResult */ true); return; - } - } - - context.Reporter.Info() << Resource::String::UninstallFlowUninstallSuccess << std::endl; - } - - void RecordUninstall(Context& context) - { - // In order to report an uninstall to every correlated tracking catalog, we first need to find them all. - auto package = context.Get(); - UninstallCorrelatedSources correlatedSources; - - // Start with the installed version - correlatedSources.AddIfRemoteAndNotPresent(package->GetInstalledVersion()); - - // Then look through all available versions - for (const auto& versionKey : package->GetAvailableVersionKeys()) - { - correlatedSources.AddIfRemoteAndNotPresent(package->GetAvailableVersion(versionKey)); - } - - // Finally record the uninstall for each found value - for (const auto& item : correlatedSources.Items) - { - auto trackingCatalog = item.FromSource.GetTrackingCatalog(); - trackingCatalog.RecordUninstall(item.Identifier); - } - } - + } + } + + context.Reporter.Info() << Resource::String::UninstallFlowUninstallSuccess << std::endl; + } + + void RecordUninstall(Context& context) + { + // In order to report an uninstall to every correlated tracking catalog, we first need to find them all. + auto package = context.Get(); + UninstallCorrelatedSources correlatedSources; + + // Start with the installed version + correlatedSources.AddIfRemoteAndNotPresent(package->GetInstalledVersion()); + + // Then look through all available versions + for (const auto& versionKey : package->GetAvailableVersionKeys()) + { + correlatedSources.AddIfRemoteAndNotPresent(package->GetAvailableVersion(versionKey)); + } + + // Finally record the uninstall for each found value + for (const auto& item : correlatedSources.Items) + { + auto trackingCatalog = item.FromSource.GetTrackingCatalog(); + trackingCatalog.RecordUninstall(item.Identifier); + } + } + void ReportUninstallerResult::operator()(Execution::Context& context) const { DWORD uninstallResult = context.Get(); @@ -347,25 +366,25 @@ namespace AppInstaller::CLI::Workflow } } - void EnsureSupportForUninstall(Execution::Context& context) - { - auto installedPackageVersion = context.Get(); - const std::string installedTypeString = installedPackageVersion->GetMetadata()[PackageVersionMetadata::InstalledType]; - auto installedType = ConvertToInstallerTypeEnum(installedTypeString); - if (installedType == InstallerTypeEnum::Portable) - { - const std::string installedScope = installedPackageVersion->GetMetadata()[Repository::PackageVersionMetadata::InstalledScope]; - if (Manifest::ConvertToScopeEnum(installedScope) == Manifest::ScopeEnum::Machine) - { - context << EnsureRunningAsAdmin; - } - } - else if (installedType == InstallerTypeEnum::Msix) - { - if (Manifest::ConvertToScopeEnum(context.Args.GetArg(Execution::Args::Type::InstallScope)) == Manifest::ScopeEnum::Machine) - { - context << EnsureRunningAsAdmin; - } - } + void EnsureSupportForUninstall(Execution::Context& context) + { + auto installedPackageVersion = context.Get(); + const std::string installedTypeString = installedPackageVersion->GetMetadata()[PackageVersionMetadata::InstalledType]; + auto installedType = ConvertToInstallerTypeEnum(installedTypeString); + if (installedType == InstallerTypeEnum::Portable) + { + const std::string installedScope = installedPackageVersion->GetMetadata()[Repository::PackageVersionMetadata::InstalledScope]; + if (Manifest::ConvertToScopeEnum(installedScope) == Manifest::ScopeEnum::Machine) + { + context << EnsureRunningAsAdmin; + } + } + else if (installedType == InstallerTypeEnum::Msix) + { + if (Manifest::ConvertToScopeEnum(context.Args.GetArg(Execution::Args::Type::InstallScope)) == Manifest::ScopeEnum::Machine) + { + context << EnsureRunningAsAdmin; + } + } } -} +} diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index 30b7486d38..24775cb6b1 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -1821,4 +1821,7 @@ Please specify one of them using the --source option to proceed. 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. + + Waiting for another install/uninstall to complete... + \ No newline at end of file diff --git a/src/AppInstallerCLITests/InstallFlow.cpp b/src/AppInstallerCLITests/InstallFlow.cpp index e66e2682ef..7e684b5f78 100644 --- a/src/AppInstallerCLITests/InstallFlow.cpp +++ b/src/AppInstallerCLITests/InstallFlow.cpp @@ -5,6 +5,7 @@ #include "TestHooks.h" #include #include +#include #include #include #include @@ -1151,4 +1152,55 @@ TEST_CASE("InstallFlow_InstallMultiple_SearchFailed", "[InstallFlow][workflow][M INFO(installOutput.str()); REQUIRE_TERMINATED_WITH(context, APPINSTALLER_CLI_ERROR_NOT_ALL_QUERIES_FOUND_SINGLE); -} \ No newline at end of file +} + +TEST_CASE("InstallFlow_InstallAcquiresLock", "[InstallFlow][workflow]") +{ + TestCommon::TempFile installResultPath("TestExeInstalled.txt"); + + std::ostringstream installOutput; + TestContext context{ installOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + OverrideForOpenSource(context, CreateTestSource({ TSR::TestQuery_ReturnOne }), true); + OverrideForShellExecute(context); + context.Args.AddArg(Execution::Args::Type::Query, TSR::TestQuery_ReturnOne.Query); + + wil::unique_event enteredShellExecute; + enteredShellExecute.create(); + wil::unique_event canLeaveShellExecute; + canLeaveShellExecute.create(); + AppInstaller::ProgressCallback progress; + + context.Override({ ShellExecuteInstallImpl, [&](TestContext& context) + { + enteredShellExecute.SetEvent(); + canLeaveShellExecute.wait(500); + ShellExecuteInstallImpl(context); + }}); + + { + std::thread otherThread([&]() { + InstallCommand install({}); + install.Execute(context); + }); + + REQUIRE(enteredShellExecute.wait(5000)); + + AppInstaller::Synchronization::CrossProcessInstallLock mainThreadLock; + REQUIRE(!mainThreadLock.TryAcquireNoWait()); + + canLeaveShellExecute.SetEvent(); + otherThread.join(); + } + + 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); +} diff --git a/src/AppInstallerCLITests/Synchronization.cpp b/src/AppInstallerCLITests/Synchronization.cpp index c54ff7e074..e144803aa4 100644 --- a/src/AppInstallerCLITests/Synchronization.cpp +++ b/src/AppInstallerCLITests/Synchronization.cpp @@ -23,8 +23,7 @@ TEST_CASE("CPRWL_MultipleReaders", "[CrossProcessReaderWriteLock]") // In the event of bugs, we don't want to block the test waiting forever otherThread.detach(); - // Wait up to a second for the other thread to do one thing... - REQUIRE(signal.wait(1000)); + REQUIRE(signal.wait(500)); } TEST_CASE("CPRWL_WriterBlocksReader", "[CrossProcessReaderWriteLock]") @@ -44,11 +43,11 @@ TEST_CASE("CPRWL_WriterBlocksReader", "[CrossProcessReaderWriteLock]") // In the event of bugs, we don't want to block the test waiting forever otherThread.detach(); - REQUIRE(!signal.wait(1000)); + REQUIRE(!signal.wait(500)); } // Upon release of the writer, the other thread should signal - REQUIRE(signal.wait(1000)); + REQUIRE(signal.wait(500)); } TEST_CASE("CPRWL_ReaderBlocksWriter", "[CrossProcessReaderWriteLock]") @@ -68,11 +67,11 @@ TEST_CASE("CPRWL_ReaderBlocksWriter", "[CrossProcessReaderWriteLock]") // In the event of bugs, we don't want to block the test waiting forever otherThread.detach(); - REQUIRE(!signal.wait(1000)); + REQUIRE(!signal.wait(500)); } // Upon release of the writer, the other thread should signal - REQUIRE(signal.wait(1000)); + REQUIRE(signal.wait(500)); } TEST_CASE("CPRWL_WriterBlocksWriter", "[CrossProcessReaderWriteLock]") @@ -92,11 +91,11 @@ TEST_CASE("CPRWL_WriterBlocksWriter", "[CrossProcessReaderWriteLock]") // In the event of bugs, we don't want to block the test waiting forever otherThread.detach(); - REQUIRE(!signal.wait(1000)); + REQUIRE(!signal.wait(500)); } // Upon release of the writer, the other thread should signal - REQUIRE(signal.wait(1000)); + REQUIRE(signal.wait(500)); } TEST_CASE("CPRWL_CancelEndsWait", "[CrossProcessReaderWriteLock]") @@ -116,10 +115,65 @@ TEST_CASE("CPRWL_CancelEndsWait", "[CrossProcessReaderWriteLock]") // In the event of bugs, we don't want to block the test waiting forever otherThread.detach(); - REQUIRE(!signal.wait(1000)); + REQUIRE(!signal.wait(500)); progress.Cancel(); // Upon release of the writer, the other thread should signal - REQUIRE(signal.wait(1000)); + REQUIRE(signal.wait(500)); +} + +TEST_CASE("CPIL_BlocksOthers", "[CrossProcessInstallLock]") +{ + wil::unique_event signal; + signal.create(); + AppInstaller::ProgressCallback progress; + + { + CrossProcessInstallLock mainThreadLock; + mainThreadLock.Acquire(progress); + + std::thread otherThread([&signal, &progress]() { + CrossProcessInstallLock otherThreadLock; + otherThreadLock.Acquire(progress); + signal.SetEvent(); + }); + // In the event of bugs, we don't want to block the test waiting forever + otherThread.detach(); + + REQUIRE(!signal.wait(500)); + } + + // Upon release of the writer, the other thread should signal + REQUIRE(signal.wait(500)); +} + +TEST_CASE("CPIL_CancelEndsWait", "[CrossProcessInstallLock]") +{ + wil::unique_event signal; + signal.create(); + AppInstaller::ProgressCallback progress; + + CrossProcessInstallLock mainThreadLock; + mainThreadLock.Acquire(progress); + + std::optional otherThreadAcquireResult = std::nullopt; + + std::thread otherThread([&]() { + CrossProcessInstallLock otherThreadLock; + otherThreadAcquireResult = otherThreadLock.Acquire(progress); + signal.SetEvent(); + }); + // In the event of bugs, we don't want to block the test waiting forever + otherThread.detach(); + + REQUIRE(!signal.wait(500)); + + progress.Cancel(); + + // Upon release of the writer, the other thread should signal + REQUIRE(signal.wait(500)); + + REQUIRE(otherThreadAcquireResult.has_value()); + REQUIRE(!otherThreadAcquireResult.value()); } diff --git a/src/AppInstallerCLITests/TestCommon.cpp b/src/AppInstallerCLITests/TestCommon.cpp index f0728d4de6..9abea67321 100644 --- a/src/AppInstallerCLITests/TestCommon.cpp +++ b/src/AppInstallerCLITests/TestCommon.cpp @@ -162,6 +162,10 @@ namespace TestCommon } } + void TestProgress::SetProgressMessage(std::string_view) + { + } + void TestProgress::BeginProgress() { } diff --git a/src/AppInstallerCLITests/TestCommon.h b/src/AppInstallerCLITests/TestCommon.h index 77358d4dd3..c6e719362b 100644 --- a/src/AppInstallerCLITests/TestCommon.h +++ b/src/AppInstallerCLITests/TestCommon.h @@ -100,7 +100,9 @@ namespace TestCommon void BeginProgress() override; void OnProgress(uint64_t current, uint64_t maximum, AppInstaller::ProgressType type) override; - + + void SetProgressMessage(std::string_view message) override; + void EndProgress(bool) override; bool IsCancelled() override; diff --git a/src/AppInstallerCLITests/WorkflowCommon.cpp b/src/AppInstallerCLITests/WorkflowCommon.cpp index de8cc4144e..8405bc7600 100644 --- a/src/AppInstallerCLITests/WorkflowCommon.cpp +++ b/src/AppInstallerCLITests/WorkflowCommon.cpp @@ -565,7 +565,7 @@ namespace TestCommon void OverrideForPortableInstall(TestContext& context) { - context.Override({ PortableInstall, [](TestContext&) + context.Override({ Workflow::details::PortableInstall, [](TestContext&) { std::filesystem::path temp = std::filesystem::temp_directory_path(); temp /= "TestPortableInstalled.txt"; @@ -628,7 +628,7 @@ namespace TestCommon void OverrideForMSIX(TestContext& context) { - context.Override({ MsixInstall, [](TestContext& context) + context.Override({ Workflow::details::MsixInstall, [](TestContext& context) { std::filesystem::path temp = std::filesystem::temp_directory_path(); temp /= "TestMsixInstalled.txt"; diff --git a/src/AppInstallerCommonCore/Progress.cpp b/src/AppInstallerCommonCore/Progress.cpp index f5084a495f..bb5c410fd5 100644 --- a/src/AppInstallerCommonCore/Progress.cpp +++ b/src/AppInstallerCommonCore/Progress.cpp @@ -27,6 +27,15 @@ namespace AppInstaller } } + void ProgressCallback::SetProgressMessage(std::string_view message) + { + IProgressSink* sink = GetSink(); + if (sink) + { + sink->SetProgressMessage(message); + } + } + void ProgressCallback::EndProgress(bool hideProgressWhenDone) { IProgressSink* sink = GetSink(); @@ -85,6 +94,11 @@ namespace AppInstaller m_baseCallback.OnProgress(m_rangeMin + (m_rangeMax - m_rangeMin) * current / maximum, m_globalMax, type); } + void PartialPercentProgressCallback::SetProgressMessage(std::string_view message) + { + m_baseCallback.SetProgressMessage(message); + } + void PartialPercentProgressCallback::EndProgress(bool) { THROW_HR(E_NOTIMPL); diff --git a/src/AppInstallerCommonCore/Public/AppInstallerProgress.h b/src/AppInstallerCommonCore/Public/AppInstallerProgress.h index a80f716138..316b374ec8 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerProgress.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerProgress.h @@ -4,6 +4,7 @@ #include #include #include +#include namespace AppInstaller { @@ -35,6 +36,9 @@ namespace AppInstaller // If maximum is 0, the maximum is unknown. virtual void OnProgress(uint64_t current, uint64_t maximum, ProgressType type) = 0; + // Sets a message for the current progress state. + virtual void SetProgressMessage(std::string_view message) = 0; + // Called as progress begins. virtual void BeginProgress() = 0; @@ -65,6 +69,8 @@ namespace AppInstaller void OnProgress(uint64_t current, uint64_t maximum, ProgressType type) override; + void SetProgressMessage(std::string_view message) override; + void EndProgress(bool hideProgressWhenDone) override; bool IsCancelled() override; @@ -90,6 +96,8 @@ namespace AppInstaller void OnProgress(uint64_t current, uint64_t maximum, ProgressType type) override; + void SetProgressMessage(std::string_view message) override; + void EndProgress(bool hideProgressWhenDone) override; [[nodiscard]] IProgressCallback::CancelFunctionRemoval SetCancellationFunction(std::function&& f) override; diff --git a/src/AppInstallerCommonCore/Public/AppInstallerSynchronization.h b/src/AppInstallerCommonCore/Public/AppInstallerSynchronization.h index 4e65a09117..44794ccde0 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerSynchronization.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerSynchronization.h @@ -55,4 +55,28 @@ namespace AppInstaller::Synchronization std::vector m_mutexesHeld; }; + + // This lock is used to prevent multiple winget related processes from attempting to install (or uninstall) at the same time. + struct CrossProcessInstallLock + { + CrossProcessInstallLock(); + + // Acquires the lock; cancellation is enabled via the progress object. + // Returns true when the lock is acquired and false if the wait is cancelled. + bool Acquire(IProgressCallback& progress); + + // Optionally release the lock before destroying the object. + void Release(); + + // Attempts to acquire the mutex without a wait. + // Returns true if it was able, false if not. + bool TryAcquireNoWait(); + + // Indicates whether the lock is held. + operator bool() const; + + private: + wil::unique_mutex m_mutex; + wil::mutex_release_scope_exit m_lock; + }; } diff --git a/src/AppInstallerCommonCore/Synchronization.cpp b/src/AppInstallerCommonCore/Synchronization.cpp index 3d62cf2ea4..59f0225e4f 100644 --- a/src/AppInstallerCommonCore/Synchronization.cpp +++ b/src/AppInstallerCommonCore/Synchronization.cpp @@ -20,6 +20,9 @@ namespace AppInstaller::Synchronization // Arbitrary limit that should not ever cause a problem (theoretically 1 per process) constexpr size_t s_CrossProcessReaderWriteLock_MaxReaders = 8; + // The amount of time that we wait in between checking for cancellation + constexpr std::chrono::milliseconds s_CrossProcessInstallLock_WaitLoopTime = 250ms; + namespace { wil::unique_mutex OpenControlMutex(const std::wstring& name) @@ -259,4 +262,48 @@ namespace AppInstaller::Synchronization return result; } + + CrossProcessInstallLock::CrossProcessInstallLock() + { + m_mutex.create(L"WinGetCrossProcessInstallLock", 0, SYNCHRONIZE); + } + + bool CrossProcessInstallLock::Acquire(IProgressCallback& progress) + { + while (!progress.IsCancelled()) + { + auto lock = m_mutex.acquire(nullptr, static_cast(std::chrono::duration_cast(s_CrossProcessInstallLock_WaitLoopTime).count())); + + if (lock) + { + m_lock = std::move(lock); + return true; + } + } + + return false; + } + + void CrossProcessInstallLock::Release() + { + m_lock.reset(); + } + + bool CrossProcessInstallLock::TryAcquireNoWait() + { + auto lock = m_mutex.acquire(nullptr, 0); + + if (lock) + { + m_lock = std::move(lock); + return true; + } + + return false; + } + + CrossProcessInstallLock::operator bool() const + { + return static_cast(m_lock); + } }