From 16fd465870a911a5683903abf0af7e277d32a631 Mon Sep 17 00:00:00 2001 From: JohnMcPMS Date: Mon, 17 Apr 2023 14:38:57 -0700 Subject: [PATCH] Improve configure user experience (#3158) This change improves the `configure` user experience by: 1. Ensuring that the configuration file exists before doing any other initialization. This speeds up the time to getting the error dramatically if the file does not exist. 2. Creating a better mechanism to report progress of async activities and using it to give the user information on what is happening during the beginning of the `configure` command. 3. Adding specific error strings for every known configuration error and error source, as well as generic fallbacks. 4. Outputting the result description in the event that it is supplied, and informing the user that there is additional information in the log file in case it is truncated or the `Details` property is not empty. It also logs the entire configuration file input stream to the log in verbose, and improves the progress reporting for the pre-check of the apply flow. The initialization phases are reported as: - "Initializing configuration system" while creating the server process and factory objects. - "Reading configuration file" while parsing the configuration file (this is usually fast enough that you won't see it). - "Retrieving configuration details" while we wait for the detailed information to come in about each configuration unit. This is reported repeatedly any time we are still waiting on more details. --- src/AppInstallerCLICore/ChannelStreams.cpp | 1 - src/AppInstallerCLICore/Command.cpp | 2 + .../Commands/ConfigureCommand.cpp | 1 + .../Commands/ConfigureShowCommand.cpp | 1 + .../Commands/RootCommand.cpp | 1 + .../Commands/ValidateCommand.cpp | 1 + src/AppInstallerCLICore/CompletionData.cpp | 3 +- ...nfigurationSetProcessorFactoryRemoting.cpp | 2 + src/AppInstallerCLICore/ExecutionProgress.cpp | 10 +- src/AppInstallerCLICore/ExecutionProgress.h | 4 +- src/AppInstallerCLICore/ExecutionReporter.cpp | 42 +++++ src/AppInstallerCLICore/ExecutionReporter.h | 41 +++-- src/AppInstallerCLICore/PackageCollection.cpp | 6 +- src/AppInstallerCLICore/PortableInstaller.cpp | 1 + src/AppInstallerCLICore/Resources.h | 24 +++ src/AppInstallerCLICore/VTSupport.cpp | 1 + .../Workflows/ConfigurationFlow.cpp | 145 ++++++++++++++++-- .../Workflows/DownloadFlow.cpp | 5 +- .../Workflows/ImportExportFlow.cpp | 1 + .../Workflows/InstallFlow.cpp | 1 + .../Workflows/MSStoreInstallerHandler.cpp | 3 +- .../Workflows/ManifestComparator.cpp | 6 +- .../Workflows/ManifestComparator.h | 1 + .../Workflows/MsiInstallFlow.cpp | 3 +- .../ShellExecuteInstallerHandler.cpp | 3 +- .../Workflows/UninstallFlow.cpp | 1 + .../Workflows/WorkflowBase.cpp | 1 + .../Workflows/WorkflowBase.h | 1 + src/AppInstallerCLICore/pch.h | 16 +- .../Shared/Strings/en-us/winget.resw | 97 ++++++++++-- .../AppInstallerStrings.cpp | 63 ++++++++ .../Public/AppInstallerErrors.h | 1 + .../Public/AppInstallerStrings.h | 8 + .../Helpers/Errors.cs | 22 +-- .../Tests/ConfigurationProcessorApplyTests.cs | 2 +- .../ConfigurationSetApplyProcessor.cpp | 35 ++++- .../ConfigurationSetApplyProcessor.h | 1 + .../ConfigurationSetParser.cpp | 2 + 38 files changed, 478 insertions(+), 81 deletions(-) diff --git a/src/AppInstallerCLICore/ChannelStreams.cpp b/src/AppInstallerCLICore/ChannelStreams.cpp index bf7ba6d62f..4ef60deb5f 100644 --- a/src/AppInstallerCLICore/ChannelStreams.cpp +++ b/src/AppInstallerCLICore/ChannelStreams.cpp @@ -6,7 +6,6 @@ namespace AppInstaller::CLI::Execution { - using namespace Settings; using namespace VirtualTerminal; size_t GetConsoleWidth() diff --git a/src/AppInstallerCLICore/Command.cpp b/src/AppInstallerCLICore/Command.cpp index 46b108df5d..b4153d913b 100644 --- a/src/AppInstallerCLICore/Command.cpp +++ b/src/AppInstallerCLICore/Command.cpp @@ -4,6 +4,8 @@ #include "Command.h" #include "Resources.h" #include +#include +#include using namespace std::string_view_literals; using namespace AppInstaller::Utility::literals; diff --git a/src/AppInstallerCLICore/Commands/ConfigureCommand.cpp b/src/AppInstallerCLICore/Commands/ConfigureCommand.cpp index fefe754237..110d4722f4 100644 --- a/src/AppInstallerCLICore/Commands/ConfigureCommand.cpp +++ b/src/AppInstallerCLICore/Commands/ConfigureCommand.cpp @@ -54,6 +54,7 @@ namespace AppInstaller::CLI void ConfigureCommand::ExecuteInternal(Execution::Context& context) const { context << + VerifyFile(Execution::Args::Type::ConfigurationFile) << CreateConfigurationProcessor << OpenConfigurationSet << ShowConfigurationSet << diff --git a/src/AppInstallerCLICore/Commands/ConfigureShowCommand.cpp b/src/AppInstallerCLICore/Commands/ConfigureShowCommand.cpp index a460710737..82d5308104 100644 --- a/src/AppInstallerCLICore/Commands/ConfigureShowCommand.cpp +++ b/src/AppInstallerCLICore/Commands/ConfigureShowCommand.cpp @@ -35,6 +35,7 @@ namespace AppInstaller::CLI void ConfigureShowCommand::ExecuteInternal(Execution::Context& context) const { context << + VerifyFile(Execution::Args::Type::ConfigurationFile) << CreateConfigurationProcessor << OpenConfigurationSet << ShowConfigurationSet; diff --git a/src/AppInstallerCLICore/Commands/RootCommand.cpp b/src/AppInstallerCLICore/Commands/RootCommand.cpp index 8a8c51cb16..b3d532e9a3 100644 --- a/src/AppInstallerCLICore/Commands/RootCommand.cpp +++ b/src/AppInstallerCLICore/Commands/RootCommand.cpp @@ -2,6 +2,7 @@ // Licensed under the MIT License. #include "pch.h" #include "RootCommand.h" +#include #include "InstallCommand.h" #include "ShowCommand.h" diff --git a/src/AppInstallerCLICore/Commands/ValidateCommand.cpp b/src/AppInstallerCLICore/Commands/ValidateCommand.cpp index f11d8647b7..fdf3e3e817 100644 --- a/src/AppInstallerCLICore/Commands/ValidateCommand.cpp +++ b/src/AppInstallerCLICore/Commands/ValidateCommand.cpp @@ -5,6 +5,7 @@ #include "Workflows/WorkflowBase.h" #include "Workflows/DependenciesFlow.h" #include "Resources.h" +#include namespace AppInstaller::CLI { diff --git a/src/AppInstallerCLICore/CompletionData.cpp b/src/AppInstallerCLICore/CompletionData.cpp index b2730352b9..508172b36c 100644 --- a/src/AppInstallerCLICore/CompletionData.cpp +++ b/src/AppInstallerCLICore/CompletionData.cpp @@ -3,12 +3,13 @@ #include "pch.h" #include "CompletionData.h" #include "Resources.h" +#include +#include namespace AppInstaller::CLI { using namespace std::string_view_literals; using namespace Utility::literals; - using namespace Settings; // Completion takes in the following values: // Word :: The token from the command line that is being targeted for completion. diff --git a/src/AppInstallerCLICore/ConfigurationSetProcessorFactoryRemoting.cpp b/src/AppInstallerCLICore/ConfigurationSetProcessorFactoryRemoting.cpp index f8cf93ec34..bae92d0baf 100644 --- a/src/AppInstallerCLICore/ConfigurationSetProcessorFactoryRemoting.cpp +++ b/src/AppInstallerCLICore/ConfigurationSetProcessorFactoryRemoting.cpp @@ -2,6 +2,8 @@ // Licensed under the MIT License. #include "pch.h" #include "ConfigurationSetProcessorFactoryRemoting.h" +#include +#include using namespace winrt::Windows::Foundation; using namespace winrt::Microsoft::Management::Configuration; diff --git a/src/AppInstallerCLICore/ExecutionProgress.cpp b/src/AppInstallerCLICore/ExecutionProgress.cpp index 9b0e6f5f16..768bbd827f 100644 --- a/src/AppInstallerCLICore/ExecutionProgress.cpp +++ b/src/AppInstallerCLICore/ExecutionProgress.cpp @@ -166,7 +166,12 @@ namespace AppInstaller::CLI::Execution void ProgressVisualizerBase::SetMessage(std::string_view message) { - m_message = message; + std::atomic_store(&m_message, std::make_shared(message)); + } + + std::shared_ptr ProgressVisualizerBase::GetMessage() + { + return std::atomic_load(&m_message); } } @@ -213,7 +218,8 @@ namespace AppInstaller::CLI::Execution ApplyStyle(i % repetitionCount, repetitionCount, true); m_out << '\r' << indent << spinnerChars[i % ARRAYSIZE(spinnerChars)]; m_out.RestoreDefault(); - m_out << ' ' << m_message << std::flush; + std::shared_ptr message = this->GetMessage(); + m_out << ' ' << (message ? *message : std::string{}) << std::flush; Sleep(250); } diff --git a/src/AppInstallerCLICore/ExecutionProgress.h b/src/AppInstallerCLICore/ExecutionProgress.h index 43f8b34524..60b8e34c5f 100644 --- a/src/AppInstallerCLICore/ExecutionProgress.h +++ b/src/AppInstallerCLICore/ExecutionProgress.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -28,11 +29,11 @@ namespace AppInstaller::CLI::Execution void SetStyle(AppInstaller::Settings::VisualStyle style) { m_style = style; } void SetMessage(std::string_view message); + std::shared_ptr GetMessage(); 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; } @@ -43,6 +44,7 @@ namespace AppInstaller::CLI::Execution private: bool m_enableVT = false; + std::shared_ptr m_message; }; } diff --git a/src/AppInstallerCLICore/ExecutionReporter.cpp b/src/AppInstallerCLICore/ExecutionReporter.cpp index fe3ed62f6b..68d63b5bfe 100644 --- a/src/AppInstallerCLICore/ExecutionReporter.cpp +++ b/src/AppInstallerCLICore/ExecutionReporter.cpp @@ -2,6 +2,7 @@ // Licensed under the MIT License. #include "pch.h" #include "ExecutionReporter.h" +#include namespace AppInstaller::CLI::Execution @@ -246,9 +247,50 @@ namespace AppInstaller::CLI::Execution GetBasicOutputStream() << VirtualTerminal::Cursor::Visibility::EnableShow; }; + Reporter::AsyncProgressScope::AsyncProgressScope(Reporter& reporter, IProgressSink* sink, bool hideProgressWhenDone) : + m_reporter(reporter), m_callback(sink) + { + reporter.SetProgressCallback(&m_callback); + sink->BeginProgress(); + m_hideProgressWhenDone = hideProgressWhenDone; + } + + Reporter::AsyncProgressScope::~AsyncProgressScope() + { + m_reporter.get().SetProgressCallback(nullptr); + m_callback.GetSink()->EndProgress(m_hideProgressWhenDone); + } + + ProgressCallback& Reporter::AsyncProgressScope::Callback() + { + return m_callback; + } + + IProgressCallback* Reporter::AsyncProgressScope::operator->() + { + return &m_callback; + } + + bool Reporter::AsyncProgressScope::HideProgressWhenDone() const + { + return m_hideProgressWhenDone; + } + + void Reporter::AsyncProgressScope::HideProgressWhenDone(bool value) + { + m_hideProgressWhenDone.store(value); + } + + std::unique_ptr Reporter::BeginAsyncProgress(bool hideProgressWhenDone) + { + return std::make_unique(*this, m_progressSink.load(), hideProgressWhenDone); + } + void Reporter::SetProgressCallback(ProgressCallback* callback) { auto lock = m_progressCallbackLock.lock_exclusive(); + // Attempting two progress operations at the same time; not supported. + THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_STATE), m_progressCallback != nullptr && callback != nullptr); m_progressCallback = callback; } diff --git a/src/AppInstallerCLICore/ExecutionReporter.h b/src/AppInstallerCLICore/ExecutionReporter.h index a0ffeb837c..2dca92cc93 100644 --- a/src/AppInstallerCLICore/ExecutionReporter.h +++ b/src/AppInstallerCLICore/ExecutionReporter.h @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -116,23 +117,41 @@ namespace AppInstaller::CLI::Execution void SetProgressMessage(std::string_view message) override; void EndProgress(bool hideProgressWhenDone) override; + // Contains the objects used for async progress and the lifetime of said progress. + struct AsyncProgressScope + { + AsyncProgressScope(Reporter& reporter, IProgressSink* sink, bool hideProgressWhenDone); + ~AsyncProgressScope(); + + AsyncProgressScope(const AsyncProgressScope&) = delete; + AsyncProgressScope& operator=(const AsyncProgressScope&) = delete; + + AsyncProgressScope(AsyncProgressScope&&) = delete; + AsyncProgressScope& operator=(AsyncProgressScope&&) = delete; + + ProgressCallback& Callback(); + IProgressCallback* operator->(); + + bool HideProgressWhenDone() const; + void HideProgressWhenDone(bool value); + + private: + std::reference_wrapper m_reporter; + ProgressCallback m_callback; + std::atomic_bool m_hideProgressWhenDone; + }; + // Runs the given callable of type: auto(IProgressCallback&) template auto ExecuteWithProgress(F&& f, bool hideProgressWhenDone = false) { - IProgressSink* sink = m_progressSink.load(); - ProgressCallback callback(sink); - SetProgressCallback(&callback); - sink->BeginProgress(); - - auto hideProgress = wil::scope_exit([this, hideProgressWhenDone]() - { - SetProgressCallback(nullptr); - m_progressSink.load()->EndProgress(hideProgressWhenDone); - }); - return f(callback); + auto progressScope = BeginAsyncProgress(hideProgressWhenDone); + return f(progressScope->Callback()); } + // Begins an asynchronous progress operation. + std::unique_ptr BeginAsyncProgress(bool hideProgressWhenDone = false); + // Sets the in progress callback. void SetProgressCallback(ProgressCallback* callback); diff --git a/src/AppInstallerCLICore/PackageCollection.cpp b/src/AppInstallerCLICore/PackageCollection.cpp index 8a1cb15df9..567a01b5f0 100644 --- a/src/AppInstallerCLICore/PackageCollection.cpp +++ b/src/AppInstallerCLICore/PackageCollection.cpp @@ -1,11 +1,11 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. #include "pch.h" - #include "PackageCollection.h" -#include "AppInstallerRuntime.h" -#include "winget/JsonSchemaValidation.h" +#include +#include +#include #include "PackagesSchema.h" diff --git a/src/AppInstallerCLICore/PortableInstaller.cpp b/src/AppInstallerCLICore/PortableInstaller.cpp index 19d7a0a396..97862b83d8 100644 --- a/src/AppInstallerCLICore/PortableInstaller.cpp +++ b/src/AppInstallerCLICore/PortableInstaller.cpp @@ -10,6 +10,7 @@ #include "Microsoft/PortableIndex.h" #include "Microsoft/Schema/IPortableIndex.h" #include +#include using namespace AppInstaller::Utility; using namespace AppInstaller::Registry; diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index 536dd0bd37..771e42568e 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -51,6 +51,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationApply); WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationAssert); WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationDependencies); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationDescriptionWasTruncated); WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationFailedToApply); WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationFailedToGetDetails); WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationFieldInvalid); @@ -58,13 +59,36 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationFileEmpty); WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationFileInvalid); WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationFileVersionUnknown); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationGettingDetails); WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationInform); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationInitializing); WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationLocal); WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationModuleNameOnly); WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationModuleWithDetails); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationReadingConfigFile); WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationSettings); WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationSuccessfullyApplied); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitAssertHadNegativeResult); WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitFailed); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitFailedConfigSet); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitFailedDuringGet); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitFailedDuringSet); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitFailedDuringTest); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitFailedInternal); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitFailedPrecondition); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitFailedSystemState); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitFailedUnitProcessing); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitHasDuplicateIdentifier); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitHasMissingDependency); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitManuallySkipped); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitModuleConflict); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitModuleImportFailed); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitMultipleMatches); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitNotFound); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitNotFoundInModule); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitNotRunDueToDependency); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitNotRunDueToFailedAssert); + WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitReturnedInvalidResult); WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationUnitSkipped); WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationWaitingOnAnother); WINGET_DEFINE_RESOURCE_STRINGID(ConfigurationWarning); diff --git a/src/AppInstallerCLICore/VTSupport.cpp b/src/AppInstallerCLICore/VTSupport.cpp index 821b62ecc8..7dc4c5111a 100644 --- a/src/AppInstallerCLICore/VTSupport.cpp +++ b/src/AppInstallerCLICore/VTSupport.cpp @@ -2,6 +2,7 @@ // Licensed under the MIT License. #include "pch.h" #include "VTSupport.h" +#include namespace AppInstaller::CLI::VirtualTerminal diff --git a/src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp b/src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp index 6ce8ba8cea..5d31726d4c 100644 --- a/src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp @@ -4,6 +4,7 @@ #include "ConfigurationFlow.h" #include "PromptFlow.h" #include "ConfigurationSetProcessorFactoryRemoting.h" +#include #include using namespace AppInstaller::CLI::Execution; @@ -361,6 +362,62 @@ namespace AppInstaller::CLI::Workflow } } + struct UnitFailedMessageData + { + Utility::LocIndString Message; + bool ShowDescription = true; + }; + + // TODO: We may need a detailed result code to enable the internal error to be exposed. + // Additionally, some of the processor exceptions that generate these errors should be enlightened to produce better, localized descriptions. + UnitFailedMessageData GetUnitFailedData(const ConfigurationUnit& unit, const ConfigurationUnitResultInformation& resultInformation) + { + int32_t resultCode = resultInformation.ResultCode(); + + switch (resultCode) + { + case WINGET_CONFIG_ERROR_DUPLICATE_IDENTIFIER: return { Resource::String::ConfigurationUnitHasDuplicateIdentifier(Utility::LocIndString{ Utility::ConvertToUTF8(unit.Identifier()) }), false }; + case WINGET_CONFIG_ERROR_MISSING_DEPENDENCY: return { Resource::String::ConfigurationUnitHasMissingDependency(Utility::LocIndString{ Utility::ConvertToUTF8(resultInformation.Details()) }), false }; + case WINGET_CONFIG_ERROR_ASSERTION_FAILED: return { Resource::String::ConfigurationUnitAssertHadNegativeResult(), false }; + case WINGET_CONFIG_ERROR_UNIT_NOT_INSTALLED: return { Resource::String::ConfigurationUnitNotFoundInModule(), false }; + case WINGET_CONFIG_ERROR_UNIT_NOT_FOUND_REPOSITORY: return { Resource::String::ConfigurationUnitNotFound(), false }; + case WINGET_CONFIG_ERROR_UNIT_MULTIPLE_MATCHES: return { Resource::String::ConfigurationUnitMultipleMatches(), false }; + case WINGET_CONFIG_ERROR_UNIT_INVOKE_GET: return { Resource::String::ConfigurationUnitFailedDuringGet(), true }; + case WINGET_CONFIG_ERROR_UNIT_INVOKE_TEST: return { Resource::String::ConfigurationUnitFailedDuringTest(), true }; + case WINGET_CONFIG_ERROR_UNIT_INVOKE_SET: return { Resource::String::ConfigurationUnitFailedDuringSet(), true }; + case WINGET_CONFIG_ERROR_UNIT_MODULE_CONFLICT: return { Resource::String::ConfigurationUnitModuleConflict(), false }; + case WINGET_CONFIG_ERROR_UNIT_IMPORT_MODULE: return { Resource::String::ConfigurationUnitModuleImportFailed(), true }; + case WINGET_CONFIG_ERROR_UNIT_INVOKE_INVALID_RESULT: return { Resource::String::ConfigurationUnitReturnedInvalidResult(), false }; + } + + switch (resultInformation.ResultSource()) + { + case ConfigurationUnitResultSource::ConfigurationSet: return { Resource::String::ConfigurationUnitFailedConfigSet(resultCode), true }; + case ConfigurationUnitResultSource::Internal: return { Resource::String::ConfigurationUnitFailedInternal(resultCode), true }; + case ConfigurationUnitResultSource::Precondition: return { Resource::String::ConfigurationUnitFailedPrecondition(resultCode), true }; + case ConfigurationUnitResultSource::SystemState: return { Resource::String::ConfigurationUnitFailedSystemState(resultCode), true }; + case ConfigurationUnitResultSource::UnitProcessing: return { Resource::String::ConfigurationUnitFailedUnitProcessing(resultCode), true }; + } + + // All other errors use a generic message + return { Resource::String::ConfigurationUnitFailed(resultCode), true }; + } + + Utility::LocIndString GetUnitSkippedMessage(const ConfigurationUnitResultInformation& resultInformation) + { + int32_t resultCode = resultInformation.ResultCode(); + + switch (resultInformation.ResultCode()) + { + case WINGET_CONFIG_ERROR_MANUALLY_SKIPPED: return Resource::String::ConfigurationUnitManuallySkipped(); + case WINGET_CONFIG_ERROR_DEPENDENCY_UNSATISFIED: return Resource::String::ConfigurationUnitNotRunDueToDependency(); + case WINGET_CONFIG_ERROR_ASSERTION_FAILED: return Resource::String::ConfigurationUnitNotRunDueToFailedAssert(); + } + + // If new cases arise and are not handled here, at least have a generic backstop message. + return Resource::String::ConfigurationUnitSkipped(resultCode); + } + // Helper to handle progress callbacks from ApplyConfigurationSetAsync struct ApplyConfigurationSetProgressOutput { @@ -417,6 +474,11 @@ namespace AppInstaller::CLI::Workflow private: void HandleUnitProgress(const ConfigurationUnit& unit, ConfigurationUnitState state, const ConfigurationUnitResultInformation& resultInformation) { + if (UnitHasPreviouslyCompleted(unit)) + { + return; + } + switch (state) { case ConfigurationUnitState::Pending: @@ -435,20 +497,41 @@ namespace AppInstaller::CLI::Workflow } else { - AICLI_LOG(Config, Error, << "Configuration unit " << Utility::ConvertToUTF8(unit.UnitName()) << "[" << Utility::ConvertToUTF8(unit.Identifier()) << "] failed with code 0x" - << Logging::SetHRFormat << resultInformation.ResultCode() << " and error message:\n" << Utility::ConvertToUTF8(resultInformation.Description()) << '\n' - << Utility::ConvertToUTF8(resultInformation.Details())); - // TODO: Improve error reporting for failures: use message, known HRs, getting HR system string, etc. - m_context.Reporter.Error() << " "_liv << Resource::String::ConfigurationUnitFailed << " 0x"_liv << Logging::SetHRFormat << resultInformation.ResultCode() << std::endl; + std::string description = Utility::Trim(Utility::ConvertToUTF8(resultInformation.Description())); + + AICLI_LOG_LARGE_STRING(Config, Error, << "Configuration unit " << Utility::ConvertToUTF8(unit.UnitName()) << "[" << Utility::ConvertToUTF8(unit.Identifier()) << "] failed with code 0x" + << Logging::SetHRFormat << resultInformation.ResultCode() << " and error message:\n" << description, Utility::ConvertToUTF8(resultInformation.Details())); + + UnitFailedMessageData messageData = GetUnitFailedData(unit, resultInformation); + auto error = m_context.Reporter.Error(); + error << " "_liv << messageData.Message << std::endl; + + if (messageData.ShowDescription && !description.empty()) + { + constexpr size_t maximumDescriptionLines = 3; + size_t consoleWidth = GetConsoleWidth(); + std::vector lines = Utility::SplitIntoLines(description, maximumDescriptionLines + 1); + bool wasLimited = Utility::LimitOutputLines(lines, consoleWidth, maximumDescriptionLines); + + for (const auto & line : lines) + { + error << line << std::endl; + } + + if (wasLimited || !resultInformation.Details().empty()) + { + error << Resource::String::ConfigurationDescriptionWasTruncated << std::endl; + } + } } OutputUnitCompletionProgress(); + MarkCompleted(unit); break; case ConfigurationUnitState::Skipped: OutputUnitInProgressIfNeeded(unit); - AICLI_LOG(Config, Error, << "Configuration unit " << Utility::ConvertToUTF8(unit.UnitName()) << "[" << Utility::ConvertToUTF8(unit.Identifier()) << "] was skipped with code 0x" + AICLI_LOG(Config, Warning, << "Configuration unit " << Utility::ConvertToUTF8(unit.UnitName()) << "[" << Utility::ConvertToUTF8(unit.Identifier()) << "] was skipped with code 0x" << Logging::SetHRFormat << resultInformation.ResultCode()); - // TODO: Unique message per skip reason? - m_context.Reporter.Warn() << " "_liv << Resource::String::ConfigurationUnitSkipped << " 0x"_liv << Logging::SetHRFormat << resultInformation.ResultCode() << std::endl; + m_context.Reporter.Warn() << " "_liv << GetUnitSkippedMessage(resultInformation) << std::endl; OutputUnitCompletionProgress(); break; } @@ -466,6 +549,18 @@ namespace AppInstaller::CLI::Workflow } } + void MarkCompleted(const ConfigurationUnit& unit) + { + winrt::guid unitInstance = unit.InstanceIdentifier(); + m_unitsCompleted.insert(unitInstance); + } + + bool UnitHasPreviouslyCompleted(const ConfigurationUnit& unit) + { + winrt::guid unitInstance = unit.InstanceIdentifier(); + return m_unitsCompleted.count(unitInstance) != 0; + } + // Sends VT progress to the console void OutputUnitCompletionProgress() { @@ -477,12 +572,22 @@ namespace AppInstaller::CLI::Workflow Context& m_context; std::set m_unitsSeen; + std::set m_unitsCompleted; bool m_isFirstProgress = true; }; + + std::filesystem::path GetConfigurationFilePath(Execution::Context& context) + { + std::filesystem::path argPath = Utility::ConvertToUTF16(context.Args.GetArg(Args::Type::ConfigurationFile)); + return std::filesystem::weakly_canonical(argPath); + } } void CreateConfigurationProcessor(Context& context) { + auto progressScope = context.Reporter.BeginAsyncProgress(true); + progressScope->Callback().SetProgressMessage(Resource::String::ConfigurationInitializing()); + ConfigurationProcessor processor{ CreateConfigurationSetProcessorFactory() }; // Set the processor to the current level of the logging. @@ -507,13 +612,10 @@ namespace AppInstaller::CLI::Workflow void OpenConfigurationSet(Context& context) { - std::filesystem::path argPath = Utility::ConvertToUTF16(context.Args.GetArg(Args::Type::ConfigurationFile)); - std::filesystem::path absolutePath = std::filesystem::weakly_canonical(argPath); - if (!std::filesystem::exists(absolutePath)) - { - context.Reporter.Error() << Resource::String::FileNotFound << std::endl; - AICLI_TERMINATE_CONTEXT(HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND)); - } + auto progressScope = context.Reporter.BeginAsyncProgress(true); + progressScope->Callback().SetProgressMessage(Resource::String::ConfigurationReadingConfigFile()); + + std::filesystem::path absolutePath = GetConfigurationFilePath(context); Streams::IInputStream inputStream = nullptr; inputStream = Streams::FileRandomAccessStream::OpenAsync(absolutePath.wstring(), FileAccessMode::Read).get(); @@ -561,6 +663,10 @@ namespace AppInstaller::CLI::Workflow AICLI_TERMINATE_CONTEXT(S_FALSE); } + auto gettingDetailString = Resource::String::ConfigurationGettingDetails(); + auto progressScope = context.Reporter.BeginAsyncProgress(true); + progressScope->Callback().SetProgressMessage(gettingDetailString); + auto getDetailsOperation = configContext.Processor().GetSetDetailsAsync(configContext.Set(), ConfigurationUnitDetailLevel::Catalog); OutputStream out = context.Reporter.Info(); @@ -570,6 +676,8 @@ namespace AppInstaller::CLI::Workflow { auto threadContext = context.SetForCurrentThread(); + progressScope.reset(); + auto unitResults = operation.GetResults().UnitResults(); for (unitsShown; unitsShown < unitResults.Size(); ++unitsShown) { @@ -577,12 +685,17 @@ namespace AppInstaller::CLI::Workflow LogFailedGetConfigurationUnitDetails(unitResult.Unit(), unitResult.ResultInformation()); OutputConfigurationUnitInformation(out, unitResult.Unit()); } + + progressScope = context.Reporter.BeginAsyncProgress(true); + progressScope->Callback().SetProgressMessage(gettingDetailString); }); try { GetConfigurationSetDetailsResult result = getDetailsOperation.get(); + progressScope.reset(); + // Handle any missing progress callbacks auto unitResults = result.UnitResults(); for (unitsShown; unitsShown < unitResults.Size(); ++unitsShown) @@ -594,6 +707,8 @@ namespace AppInstaller::CLI::Workflow } CATCH_LOG(); + progressScope.reset(); + // In the event of an exception from GetSetDetailsAsync, show the data we do have if (!unitsShown) { diff --git a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp index f5abc050cc..f1f7579add 100644 --- a/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DownloadFlow.cpp @@ -2,10 +2,11 @@ // Licensed under the MIT License. #include "pch.h" #include "DownloadFlow.h" -#include "winget/Filesystem.h" - +#include +#include #include #include +#include namespace AppInstaller::CLI::Workflow { diff --git a/src/AppInstallerCLICore/Workflows/ImportExportFlow.cpp b/src/AppInstallerCLICore/Workflows/ImportExportFlow.cpp index 42aef1428b..8e22b998ec 100644 --- a/src/AppInstallerCLICore/Workflows/ImportExportFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/ImportExportFlow.cpp @@ -7,6 +7,7 @@ #include "PackageCollection.h" #include "WorkflowBase.h" #include +#include namespace AppInstaller::CLI::Workflow { diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index 4d0fe11601..54b67fa6eb 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -22,6 +22,7 @@ #include #include #include +#include using namespace winrt::Windows::ApplicationModel::Store::Preview::InstallControl; using namespace winrt::Windows::Foundation; diff --git a/src/AppInstallerCLICore/Workflows/MSStoreInstallerHandler.cpp b/src/AppInstallerCLICore/Workflows/MSStoreInstallerHandler.cpp index a4df6a873a..965c207721 100644 --- a/src/AppInstallerCLICore/Workflows/MSStoreInstallerHandler.cpp +++ b/src/AppInstallerCLICore/Workflows/MSStoreInstallerHandler.cpp @@ -1,8 +1,9 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. #include "pch.h" -#include #include "MSStoreInstallerHandler.h" +#include +#include namespace AppInstaller::CLI::Workflow { diff --git a/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp b/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp index d09a63d2e3..5d8331838b 100644 --- a/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp +++ b/src/AppInstallerCLICore/Workflows/ManifestComparator.cpp @@ -1,10 +1,12 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. #include "pch.h" -#include "WorkflowBase.h" -#include "ExecutionContext.h" #include "ManifestComparator.h" +#include "WorkflowBase.h" +#include #include +#include +#include using namespace AppInstaller::CLI; using namespace AppInstaller::Manifest; diff --git a/src/AppInstallerCLICore/Workflows/ManifestComparator.h b/src/AppInstallerCLICore/Workflows/ManifestComparator.h index ff83105441..1131b4b4be 100644 --- a/src/AppInstallerCLICore/Workflows/ManifestComparator.h +++ b/src/AppInstallerCLICore/Workflows/ManifestComparator.h @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. #pragma once +#include "ExecutionContext.h" #include "ExecutionArgs.h" #include #include diff --git a/src/AppInstallerCLICore/Workflows/MsiInstallFlow.cpp b/src/AppInstallerCLICore/Workflows/MsiInstallFlow.cpp index 00f350320c..a0f0553b96 100644 --- a/src/AppInstallerCLICore/Workflows/MsiInstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/MsiInstallFlow.cpp @@ -2,7 +2,8 @@ // Licensed under the MIT License. #include "pch.h" #include "MsiInstallFlow.h" -#include "winget/MsiExecArguments.h" +#include +#include namespace AppInstaller::CLI::Workflow { diff --git a/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp b/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp index cdd09cebc0..31b7eb64d9 100644 --- a/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp +++ b/src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp @@ -2,7 +2,8 @@ // Licensed under the MIT License. #include "pch.h" #include "ShellExecuteInstallerHandler.h" -#include "AppInstallerFileLogger.h" +#include +#include using namespace AppInstaller::CLI; using namespace AppInstaller::Utility; diff --git a/src/AppInstallerCLICore/Workflows/UninstallFlow.cpp b/src/AppInstallerCLICore/Workflows/UninstallFlow.cpp index 1712e876bf..2331b89be2 100644 --- a/src/AppInstallerCLICore/Workflows/UninstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/UninstallFlow.cpp @@ -10,6 +10,7 @@ #include "PortableFlow.h" #include #include +#include using namespace AppInstaller::CLI::Execution; using namespace AppInstaller::Manifest; diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index 4544bf7930..2f4f693dde 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -9,6 +9,7 @@ #include #include #include +#include using namespace std::string_literals; using namespace AppInstaller::Utility::literals; diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.h b/src/AppInstallerCLICore/Workflows/WorkflowBase.h index bea6a958b9..1d0dc1d433 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.h +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.h @@ -5,6 +5,7 @@ #include "ExecutionReporter.h" #include #include +#include #include #include diff --git a/src/AppInstallerCLICore/pch.h b/src/AppInstallerCLICore/pch.h index 6148122edd..50a37a2cc0 100644 --- a/src/AppInstallerCLICore/pch.h +++ b/src/AppInstallerCLICore/pch.h @@ -17,6 +17,7 @@ #pragma warning( pop ) #include +#include #include #include #include @@ -50,18 +51,3 @@ #include #include - -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index e567fa12a4..853c4340eb 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -1787,12 +1787,6 @@ Please specify one of them using the --source option to proceed. Configuration successfully applied. - - Failed: - - - Skipped: - Another configuration is being applied to the system. This configuration will continue as soon as is possible... @@ -1807,9 +1801,6 @@ Please specify one of them using the --source option to proceed. The configuration is empty. - - File not found. - The feature [{0}] was not found. {Locked="{0}"} Error message displayed when a Windows feature was not found on the system. @@ -1843,4 +1834,92 @@ Please specify one of them using the --source option to proceed. Waiting for another install/uninstall to complete... + + <See the log file for additional details> + The brackets are intended to make the value stand out from other text which it will follow. Any locale appropriate mechanism that achieves this is acceptable. + + + Retrieving configuration details + + + Initializing configuration system + + + Reading configuration file + + + The system is not in the desired state asserted by the configuration. + + + This configuration unit failed for an unknown reason: {0} + {Locked="{0}"} {0} is a placeholder for the unrecognized error code. + + + The configuration unit failed due to the configuration: {0} + {Locked="{0}"} {0} is a placeholder for the unrecognized error code. + + + The configuration unit failed while attempting to get the current system state. + + + The configuration unit failed while attempting to apply the desired state. + + + The configuration unit failed while attempting to test the current system state. + + + The configuration unit failed due to an internal error: {0} + {Locked="{0}"} {0} is a placeholder for the unrecognized error code. + + + The configuration unit failed due to a precondition not being valid: {0} + {Locked="{0}"} {0} is a placeholder for the unrecognized error code. + + + The configuration unit failed due to the system state: {0} + {Locked="{0}"} {0} is a placeholder for the unrecognized error code. + + + The configuration unit failed while attempting to run: {0} + {Locked="{0}"} {0} is a placeholder for the unrecognized error code. + + + The configuration contains the identifier `{0}` multiple times. + {Locked="{0}"} {0} is a placeholder that is replaced by the identifier string from the user input file. + + + The dependency `{0}` was not found within the configuration. + {Locked="{0}"} {0} is a placeholder that is replaced by the identifier string from the user input file. + + + This configuration unit was manually skipped. + + + The module for the configuration unit is available in multiple locations with the same version. + + + Loading the module for the configuration unit failed. + + + Multiple matches were found for the configuration unit; specify the module to select the correct one. + + + The configuration unit could not be found. + + + The configuration unit was not in the module as expected. + + + This configuration unit was not run because a dependency failed or was not run. + + + This configuration unit was not run because an assert failed or was false. + + + The configuration unit returned an unexpected result during execution. + + + This configuration unit was not run for an unknown reason: {0} + {Locked="{0}"} {0} is a placeholder for the unrecognized error code. + \ No newline at end of file diff --git a/src/AppInstallerSharedLib/AppInstallerStrings.cpp b/src/AppInstallerSharedLib/AppInstallerStrings.cpp index bbca0701cc..ec29b1173b 100644 --- a/src/AppInstallerSharedLib/AppInstallerStrings.cpp +++ b/src/AppInstallerSharedLib/AppInstallerStrings.cpp @@ -703,6 +703,69 @@ namespace AppInstaller::Utility return result; } + std::vector SplitIntoLines(std::string_view input, size_t maximum) + { + std::size_t currentOffset = 0; + std::vector result; + + while (currentOffset < input.size() && (!maximum || result.size() < maximum)) + { + std::size_t nextOffset = input.find_first_of("\r\n", currentOffset); + if (nextOffset == std::string_view::npos) + { + nextOffset = input.size(); + } + + if (nextOffset - currentOffset > 1) + { + result.emplace_back(input.substr(currentOffset, nextOffset - currentOffset)); + } + + currentOffset = nextOffset; + } + + return result; + } + + bool LimitOutputLines(std::vector& lines, size_t lineWidth, size_t maximum) + { + size_t totalLines = 0; + size_t currentLine = 0; + bool result = false; + + for (; currentLine < lines.size() && totalLines < maximum; ++currentLine) + { + size_t currentLineWidth = UTF8ColumnWidth(lines[currentLine]); + // If current line is empty, the cost is 1 line (0 + 1). + // If not, round up to the next line count (by rounding down through integer division after subtracting 1 + 1). + size_t currentLineActualLineCount = (currentLineWidth ? (currentLineWidth - 1) / lineWidth : 0) + 1; + + // The current line may be too big to be the last line. + size_t availableLines = maximum - totalLines; + if (currentLineActualLineCount > availableLines) + { + size_t actualWidth = 0; + std::string trimmedLine = UTF8TrimRightToColumnWidth(lines[currentLine], (availableLines * lineWidth) - 1, actualWidth); + trimmedLine += "\xE2\x80\xA6"; // UTF8 encoding of ellipsis (…) character + lines[currentLine] = trimmedLine; + + currentLineActualLineCount = availableLines; + result = true; + } + + totalLines += currentLineActualLineCount; + } + + // Drop any unprocessed lines + if (currentLine != lines.size()) + { + lines.resize(currentLine); + result = true; + } + + return result; + } + std::string ConvertToHexString(const std::vector& buffer, size_t byteCount) { if (byteCount && buffer.size() != byteCount) diff --git a/src/AppInstallerSharedLib/Public/AppInstallerErrors.h b/src/AppInstallerSharedLib/Public/AppInstallerErrors.h index 71c9c09cfc..da69880423 100644 --- a/src/AppInstallerSharedLib/Public/AppInstallerErrors.h +++ b/src/AppInstallerSharedLib/Public/AppInstallerErrors.h @@ -164,6 +164,7 @@ #define WINGET_CONFIG_ERROR_ASSERTION_FAILED ((HRESULT)0x8A15C009) #define WINGET_CONFIG_ERROR_MANUALLY_SKIPPED ((HRESULT)0x8A15C00A) #define WINGET_CONFIG_ERROR_WARNING_NOT_ACCEPTED ((HRESULT)0x8A15C00B) +#define WINGET_CONFIG_ERROR_SET_DEPENDENCY_CYCLE ((HRESULT)0x8A15C00C) // Configuration Processor Errors #define WINGET_CONFIG_ERROR_UNIT_NOT_INSTALLED ((HRESULT)0x8A15C101) diff --git a/src/AppInstallerSharedLib/Public/AppInstallerStrings.h b/src/AppInstallerSharedLib/Public/AppInstallerStrings.h index b7bd0a4303..d7bcfb8413 100644 --- a/src/AppInstallerSharedLib/Public/AppInstallerStrings.h +++ b/src/AppInstallerSharedLib/Public/AppInstallerStrings.h @@ -185,6 +185,14 @@ namespace AppInstaller::Utility // Splits the string into words. std::vector SplitIntoWords(std::string_view input); + // Splits the string into lines. + // Drops empty lines. + std::vector SplitIntoLines(std::string_view input, size_t maximum = 0); + + // Removes lines from the vector (and/or characters from the last line) so that it contains the maximum number of lines. + // Returns true if changes were made, false if not. + bool LimitOutputLines(std::vector& lines, size_t lineWidth, size_t maximum); + // Converts a container to a string representation of it. template std::string ConvertContainerToString(const T& container, U toString) diff --git a/src/Microsoft.Management.Configuration.UnitTests/Helpers/Errors.cs b/src/Microsoft.Management.Configuration.UnitTests/Helpers/Errors.cs index d4d7cd7746..7e2c806a92 100644 --- a/src/Microsoft.Management.Configuration.UnitTests/Helpers/Errors.cs +++ b/src/Microsoft.Management.Configuration.UnitTests/Helpers/Errors.cs @@ -15,16 +15,18 @@ internal static class Errors #pragma warning disable SA1600 // Elements should be documented #pragma warning disable SA1025 // Code should not contain multiple whitespace in a row - public static readonly int WINGET_CONFIG_ERROR_INVALID_CONFIGURATION_FILE = unchecked((int)0x8A15C001); - public static readonly int WINGET_CONFIG_ERROR_INVALID_YAML = unchecked((int)0x8A15C002); - public static readonly int WINGET_CONFIG_ERROR_INVALID_FIELD = unchecked((int)0x8A15C003); - public static readonly int WINGET_CONFIG_ERROR_UNKNOWN_CONFIGURATION_FILE_VERSION = unchecked((int)0x8A15C004); - public static readonly int WINGET_CONFIG_ERROR_SET_APPLY_FAILED = unchecked((int)0x8A15C005); - public static readonly int WINGET_CONFIG_ERROR_DUPLICATE_IDENTIFIER = unchecked((int)0x8A15C006); - public static readonly int WINGET_CONFIG_ERROR_MISSING_DEPENDENCY = unchecked((int)0x8A15C007); - public static readonly int WINGET_CONFIG_ERROR_DEPENDENCY_UNSATISFIED = unchecked((int)0x8A15C008); - public static readonly int WINGET_CONFIG_ERROR_ASSERTION_FAILED = unchecked((int)0x8A15C009); - public static readonly int WINGET_CONFIG_ERROR_MANUALLY_SKIPPED = unchecked((int)0x8A15C00A); + public static readonly int WINGET_CONFIG_ERROR_INVALID_CONFIGURATION_FILE = unchecked((int)0x8A15C001); + public static readonly int WINGET_CONFIG_ERROR_INVALID_YAML = unchecked((int)0x8A15C002); + public static readonly int WINGET_CONFIG_ERROR_INVALID_FIELD = unchecked((int)0x8A15C003); + public static readonly int WINGET_CONFIG_ERROR_UNKNOWN_CONFIGURATION_FILE_VERSION = unchecked((int)0x8A15C004); + public static readonly int WINGET_CONFIG_ERROR_SET_APPLY_FAILED = unchecked((int)0x8A15C005); + public static readonly int WINGET_CONFIG_ERROR_DUPLICATE_IDENTIFIER = unchecked((int)0x8A15C006); + public static readonly int WINGET_CONFIG_ERROR_MISSING_DEPENDENCY = unchecked((int)0x8A15C007); + public static readonly int WINGET_CONFIG_ERROR_DEPENDENCY_UNSATISFIED = unchecked((int)0x8A15C008); + public static readonly int WINGET_CONFIG_ERROR_ASSERTION_FAILED = unchecked((int)0x8A15C009); + public static readonly int WINGET_CONFIG_ERROR_MANUALLY_SKIPPED = unchecked((int)0x8A15C00A); + public static readonly int WINGET_CONFIG_ERROR_WARNING_NOT_ACCEPTED = unchecked((int)0x8A15C00B); + public static readonly int WINGET_CONFIG_ERROR_SET_DEPENDENCY_CYCLE = unchecked((int)0x8A15C00C); #pragma warning restore SA1025 // Code should not contain multiple whitespace in a row #pragma warning restore SA1600 // Elements should be documented diff --git a/src/Microsoft.Management.Configuration.UnitTests/Tests/ConfigurationProcessorApplyTests.cs b/src/Microsoft.Management.Configuration.UnitTests/Tests/ConfigurationProcessorApplyTests.cs index 40c9fc2ce6..a7d8ea7c40 100644 --- a/src/Microsoft.Management.Configuration.UnitTests/Tests/ConfigurationProcessorApplyTests.cs +++ b/src/Microsoft.Management.Configuration.UnitTests/Tests/ConfigurationProcessorApplyTests.cs @@ -171,7 +171,7 @@ public void ApplySet_DependencyCycle() ApplyConfigurationSetResult result = processor.ApplySet(configurationSet, ApplyConfigurationSetFlags.None); Assert.NotNull(result); Assert.NotNull(result.ResultCode); - Assert.Equal(Errors.WINGET_CONFIG_ERROR_DEPENDENCY_UNSATISFIED, result.ResultCode.HResult); + Assert.Equal(Errors.WINGET_CONFIG_ERROR_SET_DEPENDENCY_CYCLE, result.ResultCode.HResult); Assert.Equal(3, result.UnitResults.Count); foreach (var unitResult in result.UnitResults) diff --git a/src/Microsoft.Management.Configuration/ConfigurationSetApplyProcessor.cpp b/src/Microsoft.Management.Configuration/ConfigurationSetApplyProcessor.cpp index abee587eb5..11211117c7 100644 --- a/src/Microsoft.Management.Configuration/ConfigurationSetApplyProcessor.cpp +++ b/src/Microsoft.Management.Configuration/ConfigurationSetApplyProcessor.cpp @@ -45,14 +45,16 @@ namespace winrt::Microsoft::Management::Configuration::implementation { if (PreProcess()) { - // TODO: When cross process is implemented, send Pending until we actually start + // TODO: Send pending when blocked by another configuration run + //SendProgress(ConfigurationSetState::Pending); + SendProgress(ConfigurationSetState::InProgress); ProcessInternal(HasProcessedSuccessfully, &ConfigurationSetApplyProcessor::ProcessUnit, true); - - SendProgress(ConfigurationSetState::Completed); } + SendProgress(ConfigurationSetState::Completed); + m_telemetry.LogConfigProcessingSummaryForApply(*winrt::get_self(m_configurationSet), *m_result); } @@ -99,7 +101,11 @@ namespace winrt::Microsoft::Management::Configuration::implementation { AICLI_LOG(Config, Error, << "Found missing dependency: " << dependency); unitInfo.ResultInformation->Initialize(WINGET_CONFIG_ERROR_MISSING_DEPENDENCY, ConfigurationUnitResultSource::ConfigurationSet); + unitInfo.ResultInformation->Details(dependencyHstring); + SendProgress(ConfigurationUnitState::Completed, unitInfo); result = false; + // TODO: Consider collecting all missing dependencies, for now just the first + break; } else { @@ -115,7 +121,16 @@ namespace winrt::Microsoft::Management::Configuration::implementation return false; } - return ProcessInternal(HasPreprocessed, &ConfigurationSetApplyProcessor::MarkPreprocessed); + if (!ProcessInternal(HasPreprocessed, &ConfigurationSetApplyProcessor::MarkPreprocessed)) + { + // The preprocessing simulates processing as if every unit run was successful. + // If it fails, this means that there are unit definitions whose dependencies cannot be satisfied. + // The only reason for that is a cycle in the dependency graph somewhere. + m_result->ResultCode(WINGET_CONFIG_ERROR_SET_DEPENDENCY_CYCLE); + return false; + } + + return true; } bool ConfigurationSetApplyProcessor::AddUnitToMap(UnitInfo& unitInfo, size_t unitInfoIndex) @@ -133,8 +148,10 @@ namespace winrt::Microsoft::Management::Configuration::implementation { AICLI_LOG(Config, Error, << "Found duplicate identifier: " << identifier); // Found a duplicate identifier, mark both as such - unitInfo.ResultInformation->Initialize(WINGET_CONFIG_ERROR_DUPLICATE_IDENTIFIER, ConfigurationUnitResultSource::ConfigurationSet); m_unitInfo[itr->second].ResultInformation->Initialize(WINGET_CONFIG_ERROR_DUPLICATE_IDENTIFIER, ConfigurationUnitResultSource::ConfigurationSet); + SendProgressIfNotComplete(ConfigurationUnitState::Completed, m_unitInfo[itr->second]); + unitInfo.ResultInformation->Initialize(WINGET_CONFIG_ERROR_DUPLICATE_IDENTIFIER, ConfigurationUnitResultSource::ConfigurationSet); + SendProgress(ConfigurationUnitState::Completed, unitInfo); return false; } else @@ -457,4 +474,12 @@ namespace winrt::Microsoft::Management::Configuration::implementation CATCH_LOG(); } } + + void ConfigurationSetApplyProcessor::SendProgressIfNotComplete(ConfigurationUnitState state, const UnitInfo& unitInfo) + { + if (unitInfo.Result->State() != ConfigurationUnitState::Completed) + { + SendProgress(state, unitInfo); + } + } } diff --git a/src/Microsoft.Management.Configuration/ConfigurationSetApplyProcessor.h b/src/Microsoft.Management.Configuration/ConfigurationSetApplyProcessor.h index ba625c01a7..c922bce3f9 100644 --- a/src/Microsoft.Management.Configuration/ConfigurationSetApplyProcessor.h +++ b/src/Microsoft.Management.Configuration/ConfigurationSetApplyProcessor.h @@ -90,6 +90,7 @@ namespace winrt::Microsoft::Management::Configuration::implementation // TODO: Eventually these functions/call sites will be used for history void SendProgress(ConfigurationSetState state); void SendProgress(ConfigurationUnitState state, const UnitInfo& unitInfo); + void SendProgressIfNotComplete(ConfigurationUnitState state, const UnitInfo& unitInfo); ConfigurationSet m_configurationSet; IConfigurationSetProcessor m_setProcessor; diff --git a/src/Microsoft.Management.Configuration/ConfigurationSetParser.cpp b/src/Microsoft.Management.Configuration/ConfigurationSetParser.cpp index 3a32523828..ab3e25ae1f 100644 --- a/src/Microsoft.Management.Configuration/ConfigurationSetParser.cpp +++ b/src/Microsoft.Management.Configuration/ConfigurationSetParser.cpp @@ -50,6 +50,8 @@ namespace winrt::Microsoft::Management::Configuration::implementation std::unique_ptr ConfigurationSetParser::Create(std::string_view input) { + AICLI_LOG_LARGE_STRING(Config, Verbose, << "Parsing configuration set:", input); + AppInstaller::YAML::Node document; try