From c44120d43dbed1f856b92650b39e126e3aff37e8 Mon Sep 17 00:00:00 2001 From: ryfu-msft Date: Mon, 23 Oct 2023 15:24:23 -0700 Subject: [PATCH 01/23] save work --- src/AppInstallerCLICore/CheckpointManager.h | 3 +++ .../Commands/InstallCommand.cpp | 2 -- src/AppInstallerCLICore/ExecutionContext.cpp | 5 ++++ src/AppInstallerCLICore/ExecutionContext.h | 3 +++ .../Workflows/InstallFlow.cpp | 1 + .../Workflows/ResumeFlow.cpp | 24 +++++++++++++++++++ .../Workflows/ResumeFlow.h | 2 ++ 7 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/AppInstallerCLICore/CheckpointManager.h b/src/AppInstallerCLICore/CheckpointManager.h index 2268e84ff2..f7212ae63e 100644 --- a/src/AppInstallerCLICore/CheckpointManager.h +++ b/src/AppInstallerCLICore/CheckpointManager.h @@ -30,6 +30,9 @@ namespace AppInstaller::Checkpoints // Gets the file path of the checkpoint database. static std::filesystem::path GetCheckpointDatabasePath(const std::string_view& resumeId, bool createCheckpointDirectory = false); + // Gets the resume id. + std::string GetResumeId() { return m_resumeId; }; + // Gets the automatic checkpoint. std::optional> GetAutomaticCheckpoint(); diff --git a/src/AppInstallerCLICore/Commands/InstallCommand.cpp b/src/AppInstallerCLICore/Commands/InstallCommand.cpp index c336988e79..9b3316a967 100644 --- a/src/AppInstallerCLICore/Commands/InstallCommand.cpp +++ b/src/AppInstallerCLICore/Commands/InstallCommand.cpp @@ -123,7 +123,6 @@ namespace AppInstaller::CLI Workflow::GetManifestFromArg << Workflow::SelectInstaller << Workflow::EnsureApplicableInstaller << - Workflow::Checkpoint("exampleCheckpoint", {}) << // TODO: Checkpoint example Workflow::InstallSinglePackage; } else @@ -153,7 +152,6 @@ namespace AppInstaller::CLI else { context << - Workflow::Checkpoint("exampleCheckpoint", {}) << // TODO: Checkpoint example Workflow::InstallOrUpgradeSinglePackage(OperationType::Install); } } diff --git a/src/AppInstallerCLICore/ExecutionContext.cpp b/src/AppInstallerCLICore/ExecutionContext.cpp index 9918a77c24..b5c03b7e59 100644 --- a/src/AppInstallerCLICore/ExecutionContext.cpp +++ b/src/AppInstallerCLICore/ExecutionContext.cpp @@ -427,6 +427,11 @@ namespace AppInstaller::CLI::Execution } #endif + std::string Context::GetResumeId() + { + return m_checkpointManager->GetResumeId(); + } + std::optional> Context::LoadCheckpoint(const std::string& resumeId) { m_checkpointManager = std::make_unique(resumeId); diff --git a/src/AppInstallerCLICore/ExecutionContext.h b/src/AppInstallerCLICore/ExecutionContext.h index f815955c10..5bc40f55d0 100644 --- a/src/AppInstallerCLICore/ExecutionContext.h +++ b/src/AppInstallerCLICore/ExecutionContext.h @@ -169,6 +169,9 @@ namespace AppInstaller::CLI::Execution bool ShouldExecuteWorkflowTask(const Workflow::WorkflowTask& task); #endif + // Returns the resume id. + std::string GetResumeId(); + // Called by the resume command. Loads the checkpoint manager with the resume id and returns the automatic checkpoint. std::optional> LoadCheckpoint(const std::string& resumeId); diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index 9060ede725..4c3dd9f726 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -597,6 +597,7 @@ namespace AppInstaller::CLI::Workflow Workflow::InstallDependencies << Workflow::DownloadInstaller << Workflow::InstallPackageInstaller << + Workflow::RegisterForRestart << Workflow::InitiateRebootIfApplicable(); } diff --git a/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp b/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp index 1cbcf36c6f..c8849bfb17 100644 --- a/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp @@ -43,4 +43,28 @@ namespace AppInstaller::CLI::Workflow } } } + + void RegisterForRestart(Execution::Context& context) + { + if (!Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::Resume) && + !Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::Reboot)) + { + return; + } + + if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::RegisterResume)) + { + const auto& commandLineArg = L"resume -g " + Utility::ConvertToUTF16(context.GetResumeId()); + + //AICLI_LOG(CLI, Info, << "Registering for restart with command line args: " << commandLineArg); + + HRESULT result = RegisterApplicationRestart(commandLineArg.c_str(), 0); + + if (FAILED(result)) + { + AICLI_LOG(CLI, Error, << "RegisterApplicationRestart error: " << result); + context.Reporter.Error() << "Failed to register for restart." << std::endl; + } + } + } } diff --git a/src/AppInstallerCLICore/Workflows/ResumeFlow.h b/src/AppInstallerCLICore/Workflows/ResumeFlow.h index 1c86f719cc..0ac73eaa39 100644 --- a/src/AppInstallerCLICore/Workflows/ResumeFlow.h +++ b/src/AppInstallerCLICore/Workflows/ResumeFlow.h @@ -33,4 +33,6 @@ namespace AppInstaller::CLI::Workflow void operator()(Execution::Context& context) const override; }; + + void RegisterForRestart(Execution::Context& context); } From 9e29c64e2441b4564d4bedd2c9d05dc16b1172c1 Mon Sep 17 00:00:00 2001 From: ryfu-msft Date: Tue, 24 Oct 2023 15:18:58 -0700 Subject: [PATCH 02/23] add workflows --- .../Commands/InstallCommand.cpp | 2 + src/AppInstallerCLICore/ExecutionContext.cpp | 4 +- src/AppInstallerCLICore/Resources.h | 1 + .../Workflows/DependenciesFlow.cpp | 2 + .../Workflows/InstallFlow.cpp | 2 +- .../Workflows/ResumeFlow.cpp | 18 +++---- .../Workflows/ResumeFlow.h | 13 ++++- .../Shared/Strings/en-us/winget.resw | 3 ++ src/AppInstallerCLITests/ResumeFlow.cpp | 50 ++++++++++++++++--- src/AppInstallerCLITests/TestHooks.h | 17 +++++++ .../Public/winget/Reboot.h | 2 + src/AppInstallerCommonCore/Reboot.cpp | 31 ++++++++++++ 12 files changed, 123 insertions(+), 22 deletions(-) diff --git a/src/AppInstallerCLICore/Commands/InstallCommand.cpp b/src/AppInstallerCLICore/Commands/InstallCommand.cpp index 9b3316a967..c336988e79 100644 --- a/src/AppInstallerCLICore/Commands/InstallCommand.cpp +++ b/src/AppInstallerCLICore/Commands/InstallCommand.cpp @@ -123,6 +123,7 @@ namespace AppInstaller::CLI Workflow::GetManifestFromArg << Workflow::SelectInstaller << Workflow::EnsureApplicableInstaller << + Workflow::Checkpoint("exampleCheckpoint", {}) << // TODO: Checkpoint example Workflow::InstallSinglePackage; } else @@ -152,6 +153,7 @@ namespace AppInstaller::CLI else { context << + Workflow::Checkpoint("exampleCheckpoint", {}) << // TODO: Checkpoint example Workflow::InstallOrUpgradeSinglePackage(OperationType::Install); } } diff --git a/src/AppInstallerCLICore/ExecutionContext.cpp b/src/AppInstallerCLICore/ExecutionContext.cpp index b5c03b7e59..26fbb93a08 100644 --- a/src/AppInstallerCLICore/ExecutionContext.cpp +++ b/src/AppInstallerCLICore/ExecutionContext.cpp @@ -255,9 +255,9 @@ namespace AppInstaller::CLI::Execution Context::~Context() { - if (Settings::ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::Resume) && !IsTerminated()) + if (Settings::ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::Resume)) { - if (m_checkpointManager) + if (m_checkpointManager && IsTerminated() && GetTerminationHR() != APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL) { m_checkpointManager->CleanUpDatabase(); } diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index e00683ec22..6df09aacc1 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -185,6 +185,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(FailedToEnableWindowsFeatureOverrideRequired); WINGET_DEFINE_RESOURCE_STRINGID(FailedToInitiateReboot); WINGET_DEFINE_RESOURCE_STRINGID(FailedToRefreshPathWarning); + WINGET_DEFINE_RESOURCE_STRINGID(FailedToRegisterReboot); WINGET_DEFINE_RESOURCE_STRINGID(FeatureDisabledByAdminSettingMessage); WINGET_DEFINE_RESOURCE_STRINGID(FeatureDisabledMessage); WINGET_DEFINE_RESOURCE_STRINGID(FeaturesCommandLongDescription); diff --git a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp index 87237e02c1..81ecff2fb9 100644 --- a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp @@ -228,6 +228,8 @@ namespace AppInstaller::CLI::Workflow else { context.Reporter.Error() << Resource::String::RebootRequiredToEnableWindowsFeatureOverrideRequired << std::endl; + context.SetFlags(Execution::ContextFlag::RegisterResume); + context.SetFlags(Execution::ContextFlag::RebootRequired); AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL); } } diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index 4c3dd9f726..a5a5eb20a4 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -597,7 +597,7 @@ namespace AppInstaller::CLI::Workflow Workflow::InstallDependencies << Workflow::DownloadInstaller << Workflow::InstallPackageInstaller << - Workflow::RegisterForRestart << + Workflow::RegisterForReboot() << Workflow::InitiateRebootIfApplicable(); } diff --git a/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp b/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp index c8849bfb17..406ba1c951 100644 --- a/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp @@ -44,7 +44,7 @@ namespace AppInstaller::CLI::Workflow } } - void RegisterForRestart(Execution::Context& context) + void RegisterForReboot::operator()(Execution::Context& context) const { if (!Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::Resume) && !Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::Reboot)) @@ -54,16 +54,16 @@ namespace AppInstaller::CLI::Workflow if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::RegisterResume)) { - const auto& commandLineArg = L"resume -g " + Utility::ConvertToUTF16(context.GetResumeId()); + std::string commandLineArg = "resume -g " + context.GetResumeId(); - //AICLI_LOG(CLI, Info, << "Registering for restart with command line args: " << commandLineArg); - - HRESULT result = RegisterApplicationRestart(commandLineArg.c_str(), 0); - - if (FAILED(result)) + if (!Reboot::RegisterApplicationForReboot(commandLineArg)) { - AICLI_LOG(CLI, Error, << "RegisterApplicationRestart error: " << result); - context.Reporter.Error() << "Failed to register for restart." << std::endl; + context.Reporter.Error() << Resource::String::FailedToRegisterReboot << std::endl; + + if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::RebootRequired)) + { + context.ClearFlags(Execution::ContextFlag::RebootRequired); + } } } } diff --git a/src/AppInstallerCLICore/Workflows/ResumeFlow.h b/src/AppInstallerCLICore/Workflows/ResumeFlow.h index 0ac73eaa39..29113414da 100644 --- a/src/AppInstallerCLICore/Workflows/ResumeFlow.h +++ b/src/AppInstallerCLICore/Workflows/ResumeFlow.h @@ -29,10 +29,19 @@ namespace AppInstaller::CLI::Workflow // Outputs: None struct InitiateRebootIfApplicable : public WorkflowTask { - InitiateRebootIfApplicable() : WorkflowTask("InitiateRebootIfApplicable", /* executeAlways */true) {} + InitiateRebootIfApplicable() : WorkflowTask("InitiateRebootIfApplicable", /* executeAlways */ true) {} void operator()(Execution::Context& context) const override; }; - void RegisterForRestart(Execution::Context& context); + // Registers the resume command to execute upon reboot if applicable. This task always executes even if context terminates. + // Required Args: None + // Inputs: None + // Outputs: None + struct RegisterForReboot : public WorkflowTask + { + RegisterForReboot() : WorkflowTask("RegisterForReboot", /* executeAlways*/ true) {} + + void operator()(Execution::Context & context) const override; + }; } diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index cef16113e7..6f319168b4 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -2619,4 +2619,7 @@ Please specify one of them using the --source option to proceed. Failed to initiate a reboot. + + Failed to register winget for reboot. + \ No newline at end of file diff --git a/src/AppInstallerCLITests/ResumeFlow.cpp b/src/AppInstallerCLITests/ResumeFlow.cpp index a9cf8f314f..7fd370b500 100644 --- a/src/AppInstallerCLITests/ResumeFlow.cpp +++ b/src/AppInstallerCLITests/ResumeFlow.cpp @@ -154,7 +154,6 @@ TEST_CASE("ResumeFlow_InstallSuccess", "[Resume]") REQUIRE(checkpointFiles.size() == 0); } -// TODO: This test will need to be updated once saving the resume state is restricted to certain HRs. TEST_CASE("ResumeFlow_InstallFailure", "[Resume]") { TestCommon::TempDirectory tempCheckpointRecordDirectory("TempCheckpointRecordDirectory", false); @@ -183,15 +182,50 @@ TEST_CASE("ResumeFlow_InstallFailure", "[Resume]") REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_UNSUPPORTED_ARGUMENT); } - // Only one checkpoint file should be created. - std::vector checkpointFiles; - for (const auto& entry : std::filesystem::directory_iterator(tempCheckpointRecordDirectoryPath)) + // Checkpoint file should be cleaned up if the hr is not reboot related. + REQUIRE(std::filesystem::is_empty(tempCheckpointRecordDirectoryPath)); +} + +TEST_CASE("ResumeFlow_WindowsFeature_RebootRequired", "[Resume]") +{ + if (!AppInstaller::Runtime::IsRunningAsAdmin()) { - checkpointFiles.emplace_back(entry.path()); + WARN("Test requires admin privilege. Skipped."); + return; } - REQUIRE(checkpointFiles.size() == 1); + TestCommon::TempDirectory tempCheckpointRecordDirectory("TempCheckpointRecordDirectory", false); + + const auto& tempCheckpointRecordDirectoryPath = tempCheckpointRecordDirectory.GetPath(); + TestHook_SetPathOverride(PathName::CheckpointsLocation, tempCheckpointRecordDirectoryPath); + + TestCommon::TestUserSettings testSettings; + testSettings.Set(true); + testSettings.Set(true); + testSettings.Set(true); + + // Override with reboot required HRESULT. + auto doesFeatureExistOverride = TestHook::SetDoesWindowsFeatureExistResult_Override(ERROR_SUCCESS); + auto setEnableFeatureOverride = TestHook::SetEnableWindowsFeatureResult_Override(ERROR_SUCCESS_REBOOT_REQUIRED); + TestHook::SetRegisterForRestartResult_Override registerForRestartResultOverride(false); + TestHook::SetInitiateRebootResult_Override initiateRebootResultOverride(true); - std::filesystem::path checkpointRecordPath = checkpointFiles[0]; - REQUIRE(std::filesystem::exists(checkpointRecordPath)); + { + std::ostringstream installOutput; + TestContext context{ installOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + OverrideOpenDependencySource(context); + + const auto& testManifestPath = TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string(); + context.Args.AddArg(Execution::Args::Type::Manifest, testManifestPath); + context.Args.AddArg(Execution::Args::Type::AllowReboot); + + InstallCommand install({}); + install.Execute(context); + INFO(installOutput.str()); + + // Verify unsupported arguments error message is shown + REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToRegisterReboot).get()) != std::string::npos); + } } diff --git a/src/AppInstallerCLITests/TestHooks.h b/src/AppInstallerCLITests/TestHooks.h index c0085bcb4f..d814592b6b 100644 --- a/src/AppInstallerCLITests/TestHooks.h +++ b/src/AppInstallerCLITests/TestHooks.h @@ -70,6 +70,7 @@ namespace AppInstaller namespace Reboot { void TestHook_SetInitiateRebootResult_Override(bool* status); + void TestHook_SetRegisterForRestartResult_Override(bool* status); } } @@ -177,4 +178,20 @@ namespace TestHook private: bool m_status; }; + + struct SetRegisterForRestartResult_Override + { + SetRegisterForRestartResult_Override(bool status) : m_status(status) + { + AppInstaller::Reboot::TestHook_SetRegisterForRestartResult_Override(&m_status); + } + + ~SetRegisterForRestartResult_Override() + { + AppInstaller::Reboot::TestHook_SetRegisterForRestartResult_Override(nullptr); + } + + private: + bool m_status; + }; } \ No newline at end of file diff --git a/src/AppInstallerCommonCore/Public/winget/Reboot.h b/src/AppInstallerCommonCore/Public/winget/Reboot.h index 2f1a28c5a2..23e77cca37 100644 --- a/src/AppInstallerCommonCore/Public/winget/Reboot.h +++ b/src/AppInstallerCommonCore/Public/winget/Reboot.h @@ -5,4 +5,6 @@ namespace AppInstaller::Reboot { bool InitiateReboot(); + + bool RegisterApplicationForReboot(const std::string& commandLineArgs); } \ No newline at end of file diff --git a/src/AppInstallerCommonCore/Reboot.cpp b/src/AppInstallerCommonCore/Reboot.cpp index 57fa335000..27ee2338f5 100644 --- a/src/AppInstallerCommonCore/Reboot.cpp +++ b/src/AppInstallerCommonCore/Reboot.cpp @@ -2,6 +2,7 @@ // Licensed under the MIT License. #include "pch.h" #include "AppInstallerLogging.h" +#include "AppInstallerStrings.h" #include "Public/winget/Reboot.h" #include @@ -14,6 +15,13 @@ namespace AppInstaller::Reboot { s_InitiateRebootResult_TestHook_Override = status; } + + static bool* s_RegisterForRestartResult_TestHook_Override = nullptr; + + void TestHook_SetRegisterForRestartResult_Override(bool* status) + { + s_RegisterForRestartResult_TestHook_Override = status; + } #endif bool InitiateReboot() @@ -54,4 +62,27 @@ namespace AppInstaller::Reboot AICLI_LOG(Core, Info, << "Initiating reboot."); return ExitWindowsEx(EWX_RESTARTAPPS, SHTDN_REASON_MINOR_INSTALLATION); } + + bool RegisterApplicationForReboot(const std::string& commandLineArgs) + { +#ifndef AICLI_DISABLE_TEST_HOOKS + if (s_RegisterForRestartResult_TestHook_Override) + { + return *s_RegisterForRestartResult_TestHook_Override; + } +#endif + + HRESULT result = RegisterApplicationRestart(AppInstaller::Utility::ConvertToUTF16(commandLineArgs).c_str(), 0 /* Always restart process */); + AICLI_LOG(CLI, Info, << "Register for restart with command line args: " << commandLineArgs); + + if (FAILED(result)) + { + AICLI_LOG(Core, Error, << "RegisterApplicationRestart failed with hr: " << result); + return false; + } + else + { + return true; + } + } } From 8064a582c7074f233ac6508219edde85b4dd03f8 Mon Sep 17 00:00:00 2001 From: ryfu-msft Date: Fri, 27 Oct 2023 16:18:54 -0700 Subject: [PATCH 03/23] add user settings --- schemas/JSON/settings/settings.schema.0.2.json | 6 ++++++ src/AppInstallerCLICore/Commands/ResumeCommand.cpp | 11 +++++++++++ .../Public/winget/UserSettings.h | 2 ++ src/AppInstallerCommonCore/UserSettings.cpp | 1 + .../Public/winget/Checkpoint.h | 3 ++- src/AppInstallerSharedLib/Public/AppInstallerErrors.h | 1 + 6 files changed, 23 insertions(+), 1 deletion(-) diff --git a/schemas/JSON/settings/settings.schema.0.2.json b/schemas/JSON/settings/settings.schema.0.2.json index fb007b76bd..7ca7da0709 100644 --- a/schemas/JSON/settings/settings.schema.0.2.json +++ b/schemas/JSON/settings/settings.schema.0.2.json @@ -148,6 +148,12 @@ "description": "Default install location to use for packages that require it when not specified", "type": "string", "maxLength": "32767" + }, + "maxReboots": { + "description": "The maximum number of reboots allowed when performing a command. This is to prevent continuous reboots if a install that requires a reboot is not properly detected.", + "type": "integer", + "default": 3, + "minimum": 1 } } }, diff --git a/src/AppInstallerCLICore/Commands/ResumeCommand.cpp b/src/AppInstallerCLICore/Commands/ResumeCommand.cpp index e620e6278e..17cba3002f 100644 --- a/src/AppInstallerCLICore/Commands/ResumeCommand.cpp +++ b/src/AppInstallerCLICore/Commands/ResumeCommand.cpp @@ -99,6 +99,17 @@ namespace AppInstaller::CLI AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_CLIENT_VERSION_MISMATCH); } + int currentRebootCount = std::stoi(automaticCheckpoint.Get(AutomaticCheckpointData::ResumeCount, {})); + if (currentRebootCount > Settings::User().Get()) + { + context.Reporter.Error() << "Maximum number of reboot encountered. Terminating installation." << std::endl; + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALLER_HASH_MISMATCH); + } + else + { + automaticCheckpoint.Set(AutomaticCheckpointData::ResumeCount, {}, std::to_string(currentRebootCount + 1)); + } + const auto& checkpointCommand = automaticCheckpoint.Get(AutomaticCheckpointData::Command, {}); AICLI_LOG(CLI, Info, << "Resuming command: " << checkpointCommand); std::unique_ptr commandToResume = FindCommandToResume(checkpointCommand); diff --git a/src/AppInstallerCommonCore/Public/winget/UserSettings.h b/src/AppInstallerCommonCore/Public/winget/UserSettings.h index 6869b9b5b8..b0d3c698e9 100644 --- a/src/AppInstallerCommonCore/Public/winget/UserSettings.h +++ b/src/AppInstallerCommonCore/Public/winget/UserSettings.h @@ -89,6 +89,7 @@ namespace AppInstaller::Settings DisableInstallNotes, PortablePackageUserRoot, PortablePackageMachineRoot, + MaxReboots, // Network NetworkDownloader, NetworkDOProgressTimeoutInSeconds, @@ -167,6 +168,7 @@ namespace AppInstaller::Settings SETTINGMAPPING_SPECIALIZATION(Setting::PortablePackageUserRoot, std::string, std::filesystem::path, {}, ".installBehavior.portablePackageUserRoot"sv); SETTINGMAPPING_SPECIALIZATION(Setting::PortablePackageMachineRoot, std::string, std::filesystem::path, {}, ".installBehavior.portablePackageMachineRoot"sv); SETTINGMAPPING_SPECIALIZATION(Setting::InstallDefaultRoot, std::string, std::filesystem::path, {}, ".installBehavior.defaultInstallRoot"sv); + SETTINGMAPPING_SPECIALIZATION(Setting::MaxReboots, uint32_t, int, {}, ".installBehavior.maxReboots"sv); // Uninstall behavior SETTINGMAPPING_SPECIALIZATION(Setting::UninstallPurgePortablePackage, bool, bool, false, ".uninstallBehavior.purgePortablePackage"sv); // Download behavior diff --git a/src/AppInstallerCommonCore/UserSettings.cpp b/src/AppInstallerCommonCore/UserSettings.cpp index 81d8d4b2a2..33b41cbb5f 100644 --- a/src/AppInstallerCommonCore/UserSettings.cpp +++ b/src/AppInstallerCommonCore/UserSettings.cpp @@ -269,6 +269,7 @@ namespace AppInstaller::Settings WINGET_VALIDATE_PASS_THROUGH(DisableInstallNotes) WINGET_VALIDATE_PASS_THROUGH(UninstallPurgePortablePackage) WINGET_VALIDATE_PASS_THROUGH(NetworkWingetAlternateSourceURL) + WINGET_VALIDATE_PASS_THROUGH(MaxReboots) #ifndef AICLI_DISABLE_TEST_HOOKS WINGET_VALIDATE_PASS_THROUGH(EnableSelfInitiatedMinidump) diff --git a/src/AppInstallerRepositoryCore/Public/winget/Checkpoint.h b/src/AppInstallerRepositoryCore/Public/winget/Checkpoint.h index 924c30145e..36a81df148 100644 --- a/src/AppInstallerRepositoryCore/Public/winget/Checkpoint.h +++ b/src/AppInstallerRepositoryCore/Public/winget/Checkpoint.h @@ -12,7 +12,8 @@ namespace AppInstaller::Checkpoints { ClientVersion, Command, - Arguments + Arguments, + ResumeCount }; struct CheckpointManager; diff --git a/src/AppInstallerSharedLib/Public/AppInstallerErrors.h b/src/AppInstallerSharedLib/Public/AppInstallerErrors.h index 6702ab5227..f8a6f434bb 100644 --- a/src/AppInstallerSharedLib/Public/AppInstallerErrors.h +++ b/src/AppInstallerSharedLib/Public/AppInstallerErrors.h @@ -130,6 +130,7 @@ #define APPINSTALLER_CLI_ERROR_CLIENT_VERSION_MISMATCH ((HRESULT)0x8A15006F) #define APPINSTALLER_CLI_ERROR_INVALID_RESUME_STATE ((HRESULT)0x8A150070) #define APPINSTALLER_CLI_ERROR_CANNOT_OPEN_CHECKPOINT_INDEX ((HRESULT)0x8A150071) +#define APPINSTALLER_CLI_ERROR_RESUME_EXCEEDS_LIMIT ((HRESULT)0x8A150072) // Install errors. #define APPINSTALLER_CLI_ERROR_INSTALL_PACKAGE_IN_USE ((HRESULT)0x8A150101) #define APPINSTALLER_CLI_ERROR_INSTALL_INSTALL_IN_PROGRESS ((HRESULT)0x8A150102) From 76e6a7cc3536e5eaf5ccea18fa98a54ddcb7da76 Mon Sep 17 00:00:00 2001 From: ryfu-msft Date: Thu, 2 Nov 2023 12:09:33 -0700 Subject: [PATCH 04/23] add tests and check for resume limit --- .../JSON/settings/settings.schema.0.2.json | 6 +- src/AppInstallerCLICore/CheckpointManager.cpp | 2 + .../Commands/ResumeCommand.cpp | 11 +- src/AppInstallerCLICore/Resources.h | 1 + .../Shared/Strings/en-us/winget.resw | 4 + .../CheckpointDatabase.cpp | 14 ++- src/AppInstallerCLITests/ResumeFlow.cpp | 107 ++++++++++++++++-- .../Public/winget/UserSettings.h | 4 +- src/AppInstallerCommonCore/UserSettings.cpp | 2 +- .../Microsoft/CheckpointDatabase.cpp | 13 +++ .../Microsoft/CheckpointDatabase.h | 3 + .../Checkpoint_1_0/CheckpointDataTable.cpp | 2 +- .../Checkpoint_1_0/CheckpointDataTable.h | 2 +- .../CheckpointDatabaseInterface.h | 1 + .../CheckpointDatabaseInterface_1_0.cpp | 5 + .../Microsoft/Schema/ICheckpointDatabase.h | 3 + .../Public/winget/Checkpoint.h | 7 ++ .../Public/AppInstallerErrors.h | 2 +- 18 files changed, 166 insertions(+), 23 deletions(-) diff --git a/schemas/JSON/settings/settings.schema.0.2.json b/schemas/JSON/settings/settings.schema.0.2.json index 7ca7da0709..ce80020097 100644 --- a/schemas/JSON/settings/settings.schema.0.2.json +++ b/schemas/JSON/settings/settings.schema.0.2.json @@ -149,10 +149,10 @@ "type": "string", "maxLength": "32767" }, - "maxReboots": { - "description": "The maximum number of reboots allowed when performing a command. This is to prevent continuous reboots if a install that requires a reboot is not properly detected.", + "maxResumes": { + "description": "The maximum number of resumes allowed for a single resume id. This is to prevent continuous reboots if an install that requires a reboot is not properly detected.", "type": "integer", - "default": 3, + "default": 4, "minimum": 1 } } diff --git a/src/AppInstallerCLICore/CheckpointManager.cpp b/src/AppInstallerCLICore/CheckpointManager.cpp index 4bc762c810..dfe92c6275 100644 --- a/src/AppInstallerCLICore/CheckpointManager.cpp +++ b/src/AppInstallerCLICore/CheckpointManager.cpp @@ -82,6 +82,8 @@ namespace AppInstaller::Checkpoints automaticCheckpoint.SetMany(AutomaticCheckpointData::Arguments, argument, values); } } + + automaticCheckpoint.Set(AutomaticCheckpointData::ResumeCount, {}, std::to_string(0)); } void LoadCommandArgsFromAutomaticCheckpoint(CLI::Execution::Context& context, Checkpoint& automaticCheckpoint) diff --git a/src/AppInstallerCLICore/Commands/ResumeCommand.cpp b/src/AppInstallerCLICore/Commands/ResumeCommand.cpp index 17cba3002f..87fc36ea07 100644 --- a/src/AppInstallerCLICore/Commands/ResumeCommand.cpp +++ b/src/AppInstallerCLICore/Commands/ResumeCommand.cpp @@ -99,15 +99,16 @@ namespace AppInstaller::CLI AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_CLIENT_VERSION_MISMATCH); } - int currentRebootCount = std::stoi(automaticCheckpoint.Get(AutomaticCheckpointData::ResumeCount, {})); - if (currentRebootCount > Settings::User().Get()) + const auto& resumeCountString = automaticCheckpoint.Get(AutomaticCheckpointData::ResumeCount, {}); + int resumeCount = std::stoi(resumeCountString); + if (resumeCount >= Settings::User().Get()) { - context.Reporter.Error() << "Maximum number of reboot encountered. Terminating installation." << std::endl; - AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALLER_HASH_MISMATCH); + context.Reporter.Error() << Resource::String::ResumeLimitExceeded(Utility::LocIndView{ resumeCountString }) << std::endl; + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_RESUME_LIMIT_EXCEEDED); } else { - automaticCheckpoint.Set(AutomaticCheckpointData::ResumeCount, {}, std::to_string(currentRebootCount + 1)); + automaticCheckpoint.Update(AutomaticCheckpointData::ResumeCount, {}, std::to_string(resumeCount + 1)); } const auto& checkpointCommand = automaticCheckpoint.Get(AutomaticCheckpointData::Command, {}); diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index 6df09aacc1..296ed7ee60 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -424,6 +424,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(ResumeCommandShortDescription); WINGET_DEFINE_RESOURCE_STRINGID(ResumeIdArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(ResumeIdNotFoundError); + WINGET_DEFINE_RESOURCE_STRINGID(ResumeLimitExceeded); WINGET_DEFINE_RESOURCE_STRINGID(ResumeStateDataNotFoundError); WINGET_DEFINE_RESOURCE_STRINGID(RetroArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(SearchCommandLongDescription); diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index 6f319168b4..d867876473 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -2622,4 +2622,8 @@ Please specify one of them using the --source option to proceed. Failed to register winget for reboot. + + Resume operation exceeds the allowable limit of {0} resume(s). + {Locked="{0}"} {0} is a placeholder that is replaced by an integer number of the number of allowed resumes. + \ No newline at end of file diff --git a/src/AppInstallerCLITests/CheckpointDatabase.cpp b/src/AppInstallerCLITests/CheckpointDatabase.cpp index 3353394cae..c6f82ba22a 100644 --- a/src/AppInstallerCLITests/CheckpointDatabase.cpp +++ b/src/AppInstallerCLITests/CheckpointDatabase.cpp @@ -34,7 +34,7 @@ TEST_CASE("CheckpointDatabaseCreateLatestAndReopen", "[checkpointDatabase]") } } -TEST_CASE("CheckpointDatabase_WriteMetadata", "[checkpointDatabase]") +TEST_CASE("CheckpointDatabase_WriteAndRemoveMetadata", "[checkpointDatabase]") { TempFile tempFile{ "repolibtest_tempdb"s, ".db"s }; INFO("Using temporary file named: " << tempFile.GetPath()); @@ -58,6 +58,18 @@ TEST_CASE("CheckpointDatabase_WriteMetadata", "[checkpointDatabase]") auto checkpointId = checkpointIds[0]; REQUIRE(testCommand == database->GetDataFieldSingleValue(checkpointId, AutomaticCheckpointData::Command, {})); REQUIRE(testClientVersion == database->GetDataFieldSingleValue(checkpointId, AutomaticCheckpointData::ClientVersion, {})); + + database->RemoveDataType(checkpointId, AutomaticCheckpointData::Command); + database->RemoveDataType(checkpointId, AutomaticCheckpointData::ClientVersion); + } + + { + std::shared_ptr database = CheckpointDatabase::Open(tempFile); + const auto& checkpointIds = database->GetCheckpointIds(); + REQUIRE_FALSE(checkpointIds.empty()); + + auto checkpointId = checkpointIds[0]; + REQUIRE(database->GetDataTypes(checkpointId).empty()); } } diff --git a/src/AppInstallerCLITests/ResumeFlow.cpp b/src/AppInstallerCLITests/ResumeFlow.cpp index 7fd370b500..8fbaca5030 100644 --- a/src/AppInstallerCLITests/ResumeFlow.cpp +++ b/src/AppInstallerCLITests/ResumeFlow.cpp @@ -9,6 +9,7 @@ #include #include #include +#include using namespace std::string_literals; using namespace AppInstaller::CLI; @@ -186,7 +187,7 @@ TEST_CASE("ResumeFlow_InstallFailure", "[Resume]") REQUIRE(std::filesystem::is_empty(tempCheckpointRecordDirectoryPath)); } -TEST_CASE("ResumeFlow_WindowsFeature_RebootRequired", "[Resume]") +TEST_CASE("ResumeFlow_WindowsFeature_RebootFailures", "[Resume][windowsFeature]") { if (!AppInstaller::Runtime::IsRunningAsAdmin()) { @@ -204,17 +205,19 @@ TEST_CASE("ResumeFlow_WindowsFeature_RebootRequired", "[Resume]") testSettings.Set(true); testSettings.Set(true); + std::ostringstream installOutput; + TestContext context{ installOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + OverrideOpenDependencySource(context); + // Override with reboot required HRESULT. auto doesFeatureExistOverride = TestHook::SetDoesWindowsFeatureExistResult_Override(ERROR_SUCCESS); auto setEnableFeatureOverride = TestHook::SetEnableWindowsFeatureResult_Override(ERROR_SUCCESS_REBOOT_REQUIRED); - TestHook::SetRegisterForRestartResult_Override registerForRestartResultOverride(false); - TestHook::SetInitiateRebootResult_Override initiateRebootResultOverride(true); + SECTION("Register for reboot fails") { - std::ostringstream installOutput; - TestContext context{ installOutput, std::cin }; - auto previousThreadGlobals = context.SetForCurrentThread(); - OverrideOpenDependencySource(context); + TestHook::SetRegisterForRestartResult_Override registerForRestartResultOverride(false); + TestHook::SetInitiateRebootResult_Override initiateRebootResultOverride(true); const auto& testManifestPath = TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string(); context.Args.AddArg(Execution::Args::Type::Manifest, testManifestPath); @@ -224,8 +227,96 @@ TEST_CASE("ResumeFlow_WindowsFeature_RebootRequired", "[Resume]") install.Execute(context); INFO(installOutput.str()); - // Verify unsupported arguments error message is shown REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL); REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToRegisterReboot).get()) != std::string::npos); } + SECTION("Initiate reboot fails") + { + TestHook::SetRegisterForRestartResult_Override registerForRestartResultOverride(true); + TestHook::SetInitiateRebootResult_Override initiateRebootResultOverride(false); + + const auto& testManifestPath = TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string(); + context.Args.AddArg(Execution::Args::Type::Manifest, testManifestPath); + context.Args.AddArg(Execution::Args::Type::AllowReboot); + + InstallCommand install({}); + install.Execute(context); + INFO(installOutput.str()); + + REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToInitiateReboot).get()) != std::string::npos); + } +} + +TEST_CASE("ResumeFlow_ResumeLimitExceeded", "[Resume]") +{ + if (!AppInstaller::Runtime::IsRunningAsAdmin()) + { + WARN("Test requires admin privilege. Skipped."); + return; + } + + TestCommon::TempDirectory tempCheckpointRecordDirectory("TempCheckpointRecordDirectory", false); + + const auto& tempCheckpointRecordDirectoryPath = tempCheckpointRecordDirectory.GetPath(); + TestHook_SetPathOverride(PathName::CheckpointsLocation, tempCheckpointRecordDirectoryPath); + + TestCommon::TestUserSettings testSettings; + testSettings.Set(true); + testSettings.Set(true); + testSettings.Set(true); + testSettings.Set(1); + + std::ostringstream installOutput; + TestContext context{ installOutput, std::cin }; + auto previousThreadGlobals = context.SetForCurrentThread(); + + // Override with reboot required HRESULT. + auto doesFeatureExistOverride = TestHook::SetDoesWindowsFeatureExistResult_Override(ERROR_SUCCESS); + auto setEnableFeatureOverride = TestHook::SetEnableWindowsFeatureResult_Override(ERROR_SUCCESS_REBOOT_REQUIRED); + + TestHook::SetRegisterForRestartResult_Override registerForRestartResultOverride(true); + TestHook::SetInitiateRebootResult_Override initiateRebootResultOverride(true); + + const auto& testManifestPath = TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string(); + context.Args.AddArg(Execution::Args::Type::Manifest, testManifestPath); + context.Args.AddArg(Execution::Args::Type::AllowReboot); + context.Args.AddArg(Execution::Args::Type::AcceptSourceAgreements); + + InstallCommand install({}); + context.SetExecutingCommand(&install); + install.Execute(context); + INFO(installOutput.str()); + + std::string resumeId; + for (const auto& entry : std::filesystem::directory_iterator(tempCheckpointRecordDirectoryPath)) + { + // There should only be one checkpoint folder created. + resumeId = entry.path().filename().u8string(); + } + + { + // Execute resume once. + std::ostringstream resumeOutput; + TestContext resumeContext{ resumeOutput, std::cin }; + previousThreadGlobals = resumeContext.SetForCurrentThread(); + resumeContext.Args.AddArg(Execution::Args::Type::ResumeId, resumeId); + + ResumeCommand resume({}); + resume.Execute(resumeContext); + } + + { + // Resume should fail as the resume limit has been exceeded. + std::ostringstream resumeOutput; + TestContext resumeContext{ resumeOutput, std::cin }; + previousThreadGlobals = resumeContext.SetForCurrentThread(); + resumeContext.Args.AddArg(Execution::Args::Type::ResumeId, resumeId); + + ResumeCommand resume({}); + resume.Execute(resumeContext); + INFO(resumeOutput.str()); + REQUIRE(resumeContext.IsTerminated()); + REQUIRE(resumeContext.GetTerminationHR() == APPINSTALLER_CLI_ERROR_RESUME_LIMIT_EXCEEDED); + } } diff --git a/src/AppInstallerCommonCore/Public/winget/UserSettings.h b/src/AppInstallerCommonCore/Public/winget/UserSettings.h index b0d3c698e9..ab86050f5f 100644 --- a/src/AppInstallerCommonCore/Public/winget/UserSettings.h +++ b/src/AppInstallerCommonCore/Public/winget/UserSettings.h @@ -89,7 +89,7 @@ namespace AppInstaller::Settings DisableInstallNotes, PortablePackageUserRoot, PortablePackageMachineRoot, - MaxReboots, + MaxResumes, // Network NetworkDownloader, NetworkDOProgressTimeoutInSeconds, @@ -168,7 +168,7 @@ namespace AppInstaller::Settings SETTINGMAPPING_SPECIALIZATION(Setting::PortablePackageUserRoot, std::string, std::filesystem::path, {}, ".installBehavior.portablePackageUserRoot"sv); SETTINGMAPPING_SPECIALIZATION(Setting::PortablePackageMachineRoot, std::string, std::filesystem::path, {}, ".installBehavior.portablePackageMachineRoot"sv); SETTINGMAPPING_SPECIALIZATION(Setting::InstallDefaultRoot, std::string, std::filesystem::path, {}, ".installBehavior.defaultInstallRoot"sv); - SETTINGMAPPING_SPECIALIZATION(Setting::MaxReboots, uint32_t, int, {}, ".installBehavior.maxReboots"sv); + SETTINGMAPPING_SPECIALIZATION(Setting::MaxResumes, uint32_t, int, {}, ".installBehavior.maxResumes"sv); // Uninstall behavior SETTINGMAPPING_SPECIALIZATION(Setting::UninstallPurgePortablePackage, bool, bool, false, ".uninstallBehavior.purgePortablePackage"sv); // Download behavior diff --git a/src/AppInstallerCommonCore/UserSettings.cpp b/src/AppInstallerCommonCore/UserSettings.cpp index 33b41cbb5f..899464454d 100644 --- a/src/AppInstallerCommonCore/UserSettings.cpp +++ b/src/AppInstallerCommonCore/UserSettings.cpp @@ -269,7 +269,7 @@ namespace AppInstaller::Settings WINGET_VALIDATE_PASS_THROUGH(DisableInstallNotes) WINGET_VALIDATE_PASS_THROUGH(UninstallPurgePortablePackage) WINGET_VALIDATE_PASS_THROUGH(NetworkWingetAlternateSourceURL) - WINGET_VALIDATE_PASS_THROUGH(MaxReboots) + WINGET_VALIDATE_PASS_THROUGH(MaxResumes) #ifndef AICLI_DISABLE_TEST_HOOKS WINGET_VALIDATE_PASS_THROUGH(EnableSelfInitiatedMinidump) diff --git a/src/AppInstallerRepositoryCore/Microsoft/CheckpointDatabase.cpp b/src/AppInstallerRepositoryCore/Microsoft/CheckpointDatabase.cpp index a8867c6a2e..744fe6f490 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/CheckpointDatabase.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/CheckpointDatabase.cpp @@ -77,6 +77,19 @@ namespace AppInstaller::Repository::Microsoft savepoint.Commit(); } + void CheckpointDatabase::RemoveDataType(IdType checkpointId, int dataType) + { + std::lock_guard lockInterface{ *m_interfaceLock }; + AICLI_LOG(Repo, Verbose, << "Removing checkpoint data [" << dataType << "]"); + + SQLite::Savepoint savepoint = SQLite::Savepoint::Create(m_dbconn, "CheckpointDatabase_removeDataValue"); + + m_interface->RemoveCheckpointDataType(m_dbconn, checkpointId, dataType); + + SetLastWriteTime(); + savepoint.Commit(); + } + std::string CheckpointDatabase::GetDataFieldSingleValue(IdType checkpointId, int dataType, const std::string& field) { const auto& values = m_interface->GetCheckpointDataFieldValues(m_dbconn, checkpointId, dataType, field); diff --git a/src/AppInstallerRepositoryCore/Microsoft/CheckpointDatabase.h b/src/AppInstallerRepositoryCore/Microsoft/CheckpointDatabase.h index 665622e2af..5ec9ef6ff9 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/CheckpointDatabase.h +++ b/src/AppInstallerRepositoryCore/Microsoft/CheckpointDatabase.h @@ -56,6 +56,9 @@ namespace AppInstaller::Repository::Microsoft // Gets multiple values for a data type field. std::vector GetDataFieldMultiValue(IdType checkpointId, int dataType, const std::string& field); + // Removes the value(s) for a data type. + void RemoveDataType(IdType checkpointId, int dataType); + private: // Constructor used to open an existing index. CheckpointDatabase(const std::string& target, SQLiteStorageBase::OpenDisposition disposition, Utility::ManagedFile&& indexFile); diff --git a/src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDataTable.cpp b/src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDataTable.cpp index 268121b129..cbbe1af4a8 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDataTable.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDataTable.cpp @@ -199,7 +199,7 @@ namespace AppInstaller::Repository::Microsoft::Schema::Checkpoint_V1_0 } } - void CheckpointDataTable::RemoveData(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int contextData) + void CheckpointDataTable::RemoveDataType(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int contextData) { SQLite::Builder::StatementBuilder builder; builder.DeleteFrom(s_CheckpointDataTable_Table_Name).Where(s_CheckpointDataTable_CheckpointId_Column).Equals(checkpointId) diff --git a/src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDataTable.h b/src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDataTable.h index 584bdf952d..de9f5e6807 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDataTable.h +++ b/src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDataTable.h @@ -34,7 +34,7 @@ namespace AppInstaller::Repository::Microsoft::Schema::Checkpoint_V1_0 static bool HasDataField(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int type, std::string_view name); // Removes the context data by checkpoint id. - static void RemoveData(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int contextData); + static void RemoveDataType(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int contextData); // Gets a single data value for a context data. static std::string GetDataValue(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int type); diff --git a/src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDatabaseInterface.h b/src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDatabaseInterface.h index ef655be011..3de505c6ce 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDatabaseInterface.h +++ b/src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDatabaseInterface.h @@ -19,5 +19,6 @@ namespace AppInstaller::Repository::Microsoft::Schema::Checkpoint_V1_0 std::vector GetCheckpointDataFields(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int dataType) override; void SetCheckpointDataValues(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int dataType, std::string_view name, const std::vector& values) override; std::optional> GetCheckpointDataFieldValues(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int dataType, std::string_view name) override; + void RemoveCheckpointDataType(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int dataType) override; }; } \ No newline at end of file diff --git a/src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDatabaseInterface_1_0.cpp b/src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDatabaseInterface_1_0.cpp index d3250ef33c..39dea1bbc0 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDatabaseInterface_1_0.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/Schema/Checkpoint_1_0/CheckpointDatabaseInterface_1_0.cpp @@ -74,4 +74,9 @@ namespace AppInstaller::Repository::Microsoft::Schema::Checkpoint_V1_0 { return CheckpointDataTable::GetDataValuesByFieldName(connection, checkpointId, dataType, name); } + + void CheckpointDatabaseInterface::RemoveCheckpointDataType(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int dataType) + { + CheckpointDataTable::RemoveDataType(connection, checkpointId, dataType); + } } \ No newline at end of file diff --git a/src/AppInstallerRepositoryCore/Microsoft/Schema/ICheckpointDatabase.h b/src/AppInstallerRepositoryCore/Microsoft/Schema/ICheckpointDatabase.h index 8516b2e119..c2de853d17 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/Schema/ICheckpointDatabase.h +++ b/src/AppInstallerRepositoryCore/Microsoft/Schema/ICheckpointDatabase.h @@ -37,5 +37,8 @@ namespace AppInstaller::Repository::Microsoft::Schema // Gets all field values for a checkpoint data type. virtual std::optional> GetCheckpointDataFieldValues(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int dataType, std::string_view name) = 0; + + // Removes all values for a checkpoint data type. + virtual void RemoveCheckpointDataType(SQLite::Connection& connection, SQLite::rowid_t checkpointId, int dataType) = 0; }; } \ No newline at end of file diff --git a/src/AppInstallerRepositoryCore/Public/winget/Checkpoint.h b/src/AppInstallerRepositoryCore/Public/winget/Checkpoint.h index 36a81df148..cb83f8d672 100644 --- a/src/AppInstallerRepositoryCore/Public/winget/Checkpoint.h +++ b/src/AppInstallerRepositoryCore/Public/winget/Checkpoint.h @@ -54,6 +54,13 @@ namespace AppInstaller::Checkpoints m_checkpointDatabase->SetDataValue(m_checkpointId, dataType, fieldName, values); } + // Update a single existing field value for a data type. + void Update(T dataType, const std::string& fieldName, const std::string& value) + { + m_checkpointDatabase->RemoveDataType(m_checkpointId, dataType); + m_checkpointDatabase->SetDataValue(m_checkpointId, dataType, fieldName, { value }); + } + // Gets a single field value for a data type. std::string Get(T dataType, const std::string& fieldName) { diff --git a/src/AppInstallerSharedLib/Public/AppInstallerErrors.h b/src/AppInstallerSharedLib/Public/AppInstallerErrors.h index f8a6f434bb..d37fb20507 100644 --- a/src/AppInstallerSharedLib/Public/AppInstallerErrors.h +++ b/src/AppInstallerSharedLib/Public/AppInstallerErrors.h @@ -130,7 +130,7 @@ #define APPINSTALLER_CLI_ERROR_CLIENT_VERSION_MISMATCH ((HRESULT)0x8A15006F) #define APPINSTALLER_CLI_ERROR_INVALID_RESUME_STATE ((HRESULT)0x8A150070) #define APPINSTALLER_CLI_ERROR_CANNOT_OPEN_CHECKPOINT_INDEX ((HRESULT)0x8A150071) -#define APPINSTALLER_CLI_ERROR_RESUME_EXCEEDS_LIMIT ((HRESULT)0x8A150072) +#define APPINSTALLER_CLI_ERROR_RESUME_LIMIT_EXCEEDED ((HRESULT)0x8A150072) // Install errors. #define APPINSTALLER_CLI_ERROR_INSTALL_PACKAGE_IN_USE ((HRESULT)0x8A150101) #define APPINSTALLER_CLI_ERROR_INSTALL_INSTALL_IN_PROGRESS ((HRESULT)0x8A150102) From d58ee565e15501ee875a86e6c28144d2f2266131 Mon Sep 17 00:00:00 2001 From: ryfu-msft Date: Mon, 6 Nov 2023 10:54:33 -0800 Subject: [PATCH 05/23] verify restart and resume behavior --- .../JSON/settings/settings.schema.0.2.json | 2 +- src/AppInstallerCLICore/Command.cpp | 46 +++++++++++++++---- .../Workflows/InstallFlow.cpp | 3 +- .../Workflows/ResumeFlow.cpp | 28 ----------- .../Workflows/ResumeFlow.h | 11 ----- src/AppInstallerCLITests/UserSettings.cpp | 14 ++++++ .../Public/winget/UserSettings.h | 2 +- 7 files changed, 55 insertions(+), 51 deletions(-) diff --git a/schemas/JSON/settings/settings.schema.0.2.json b/schemas/JSON/settings/settings.schema.0.2.json index ce80020097..561422f295 100644 --- a/schemas/JSON/settings/settings.schema.0.2.json +++ b/schemas/JSON/settings/settings.schema.0.2.json @@ -152,7 +152,7 @@ "maxResumes": { "description": "The maximum number of resumes allowed for a single resume id. This is to prevent continuous reboots if an install that requires a reboot is not properly detected.", "type": "integer", - "default": 4, + "default": 3, "minimum": 1 } } diff --git a/src/AppInstallerCLICore/Command.cpp b/src/AppInstallerCLICore/Command.cpp index e7eb7943ad..3aea7e147f 100644 --- a/src/AppInstallerCLICore/Command.cpp +++ b/src/AppInstallerCLICore/Command.cpp @@ -6,6 +6,7 @@ #include #include #include +#include using namespace std::string_view_literals; using namespace AppInstaller::Utility::literals; @@ -870,16 +871,45 @@ namespace AppInstaller::CLI ExecuteInternal(context); } - if (context.Args.Contains(Execution::Args::Type::OpenLogs)) - { - // TODO: Consider possibly adding functionality that if the context contains 'Execution::Args::Type::Log' to open the path provided for the log - // The above was omitted initially as a security precaution to ensure that user input to '--log' wouldn't be passed directly to ShellExecute - ShellExecute(NULL, NULL, Runtime::GetPathTo(Runtime::PathName::DefaultLogLocation).wstring().c_str(), NULL, NULL, SW_SHOWNORMAL); - } + if (Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::Reboot) && + context.Args.Contains(Execution::Args::Type::AllowReboot) && + WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::RebootRequired)) + { + context.Reporter.Warn() << Resource::String::InitiatingReboot << std::endl; + + if (context.Args.Contains(Execution::Args::Type::Wait)) + { + context.Reporter.PromptForEnter(); + } + + context.ClearFlags(Execution::ContextFlag::RebootRequired); + + if (!Reboot::InitiateReboot()) + { + context.Reporter.Error() << Resource::String::FailedToInitiateReboot << std::endl; + } + else if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::RegisterResume)) + { + context.ClearFlags(Execution::ContextFlag::RegisterResume); - if (context.Args.Contains(Execution::Args::Type::Wait)) + // For Windows Error Reporting to restart this process, the process must be rebooted while it is still running. + // Workaround is to have the program sleep while the reboot process is kicked off. Only applies to resume. + Sleep(5000); + } + } + else { - context.Reporter.PromptForEnter(); + if (context.Args.Contains(Execution::Args::Type::OpenLogs)) + { + // TODO: Consider possibly adding functionality that if the context contains 'Execution::Args::Type::Log' to open the path provided for the log + // The above was omitted initially as a security precaution to ensure that user input to '--log' wouldn't be passed directly to ShellExecute + ShellExecute(NULL, NULL, Runtime::GetPathTo(Runtime::PathName::DefaultLogLocation).wstring().c_str(), NULL, NULL, SW_SHOWNORMAL); + } + + if (context.Args.Contains(Execution::Args::Type::Wait)) + { + context.Reporter.PromptForEnter(); + } } } diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index a5a5eb20a4..2249837842 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -597,8 +597,7 @@ namespace AppInstaller::CLI::Workflow Workflow::InstallDependencies << Workflow::DownloadInstaller << Workflow::InstallPackageInstaller << - Workflow::RegisterForReboot() << - Workflow::InitiateRebootIfApplicable(); + Workflow::RegisterForReboot(); } void EnsureSupportForInstall(Execution::Context& context) diff --git a/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp b/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp index 406ba1c951..5df369b354 100644 --- a/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp @@ -16,34 +16,6 @@ namespace AppInstaller::CLI::Workflow context.Checkpoint(m_checkpointName, m_contextData); } - void InitiateRebootIfApplicable::operator()(Execution::Context& context) const - { - if (!Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::Reboot)) - { - return; - } - - if (!context.Args.Contains(Execution::Args::Type::AllowReboot)) - { - AICLI_LOG(CLI, Info, << "No reboot flag found; skipping reboot flow."); - return; - } - - if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::RebootRequired)) - { - context.ClearFlags(Execution::ContextFlag::RebootRequired); - - if (Reboot::InitiateReboot()) - { - context.Reporter.Warn() << Resource::String::InitiatingReboot << std::endl; - } - else - { - context.Reporter.Error() << Resource::String::FailedToInitiateReboot << std::endl; - } - } - } - void RegisterForReboot::operator()(Execution::Context& context) const { if (!Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::Resume) && diff --git a/src/AppInstallerCLICore/Workflows/ResumeFlow.h b/src/AppInstallerCLICore/Workflows/ResumeFlow.h index 29113414da..1c9b8290f6 100644 --- a/src/AppInstallerCLICore/Workflows/ResumeFlow.h +++ b/src/AppInstallerCLICore/Workflows/ResumeFlow.h @@ -23,17 +23,6 @@ namespace AppInstaller::CLI::Workflow std::vector m_contextData; }; - // Initiates a reboot if applicable. This task always executes even if context terminates. - // Required Args: None - // Inputs: None - // Outputs: None - struct InitiateRebootIfApplicable : public WorkflowTask - { - InitiateRebootIfApplicable() : WorkflowTask("InitiateRebootIfApplicable", /* executeAlways */ true) {} - - void operator()(Execution::Context& context) const override; - }; - // Registers the resume command to execute upon reboot if applicable. This task always executes even if context terminates. // Required Args: None // Inputs: None diff --git a/src/AppInstallerCLITests/UserSettings.cpp b/src/AppInstallerCLITests/UserSettings.cpp index 01b93204c0..adc3bfdda4 100644 --- a/src/AppInstallerCLITests/UserSettings.cpp +++ b/src/AppInstallerCLITests/UserSettings.cpp @@ -562,3 +562,17 @@ TEST_CASE("SettingsInstallScope", "[settings]") REQUIRE(userSettingTest.Get() == AppInstaller::Manifest::ScopeEnum::Machine); } } + +TEST_CASE("SettingsMaxResumes", "[settings]") +{ + auto again = DeleteUserSettingsFiles(); + + SECTION("Modify max number of resumes") + { + std::string_view json = R"({ "installBehavior": { "maxResumes": 5 } })"; + SetSetting(Stream::PrimaryUserSettings, json); + UserSettingsTest userSettingTest; + + REQUIRE(userSettingTest.Get() == 5); + } +} diff --git a/src/AppInstallerCommonCore/Public/winget/UserSettings.h b/src/AppInstallerCommonCore/Public/winget/UserSettings.h index ab86050f5f..eed3c77ca1 100644 --- a/src/AppInstallerCommonCore/Public/winget/UserSettings.h +++ b/src/AppInstallerCommonCore/Public/winget/UserSettings.h @@ -168,7 +168,7 @@ namespace AppInstaller::Settings SETTINGMAPPING_SPECIALIZATION(Setting::PortablePackageUserRoot, std::string, std::filesystem::path, {}, ".installBehavior.portablePackageUserRoot"sv); SETTINGMAPPING_SPECIALIZATION(Setting::PortablePackageMachineRoot, std::string, std::filesystem::path, {}, ".installBehavior.portablePackageMachineRoot"sv); SETTINGMAPPING_SPECIALIZATION(Setting::InstallDefaultRoot, std::string, std::filesystem::path, {}, ".installBehavior.defaultInstallRoot"sv); - SETTINGMAPPING_SPECIALIZATION(Setting::MaxResumes, uint32_t, int, {}, ".installBehavior.maxResumes"sv); + SETTINGMAPPING_SPECIALIZATION(Setting::MaxResumes, uint32_t, int, 3, ".installBehavior.maxResumes"sv); // Uninstall behavior SETTINGMAPPING_SPECIALIZATION(Setting::UninstallPurgePortablePackage, bool, bool, false, ".uninstallBehavior.purgePortablePackage"sv); // Download behavior From 71cfa080a3f87acb0a75948e67f0c7f4939065ea Mon Sep 17 00:00:00 2001 From: ryfu-msft Date: Mon, 6 Nov 2023 12:14:46 -0800 Subject: [PATCH 06/23] fix reboot tests --- src/AppInstallerCLICore/Workflows/ResumeFlow.cpp | 2 +- src/AppInstallerCLITests/InstallFlow.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp b/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp index 5df369b354..6bd8ee98b1 100644 --- a/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp @@ -18,7 +18,7 @@ namespace AppInstaller::CLI::Workflow void RegisterForReboot::operator()(Execution::Context& context) const { - if (!Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::Resume) && + if (!Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::Resume) || !Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::Reboot)) { return; diff --git a/src/AppInstallerCLITests/InstallFlow.cpp b/src/AppInstallerCLITests/InstallFlow.cpp index b6fd32ee10..1e9fcf3f2c 100644 --- a/src/AppInstallerCLITests/InstallFlow.cpp +++ b/src/AppInstallerCLITests/InstallFlow.cpp @@ -1252,7 +1252,7 @@ TEST_CASE("InstallFlow_InstallWithReboot", "[InstallFlow][workflow][reboot]") REQUIRE(context.IsTerminated()); REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); - REQUIRE_FALSE(installOutput.str().find(Resource::LocString(Resource::String::InitiatingReboot).get()) != std::string::npos); + REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::InitiatingReboot).get()) != std::string::npos); REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToInitiateReboot).get()) != std::string::npos); } } From 5a16c00c6d32091982a5ef131b3aefdac6ca8acd Mon Sep 17 00:00:00 2001 From: ryfu-msft Date: Fri, 10 Nov 2023 12:31:18 -0800 Subject: [PATCH 07/23] fix tests --- src/AppInstallerCLICore/ExecutionContext.cpp | 2 +- src/AppInstallerCLITests/ResumeFlow.cpp | 57 ++++++-------------- 2 files changed, 16 insertions(+), 43 deletions(-) diff --git a/src/AppInstallerCLICore/ExecutionContext.cpp b/src/AppInstallerCLICore/ExecutionContext.cpp index 26fbb93a08..11f0f5fcb8 100644 --- a/src/AppInstallerCLICore/ExecutionContext.cpp +++ b/src/AppInstallerCLICore/ExecutionContext.cpp @@ -257,7 +257,7 @@ namespace AppInstaller::CLI::Execution { if (Settings::ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::Resume)) { - if (m_checkpointManager && IsTerminated() && GetTerminationHR() != APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL) + if (m_checkpointManager && (!IsTerminated() || GetTerminationHR() != APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL)) { m_checkpointManager->CleanUpDatabase(); } diff --git a/src/AppInstallerCLITests/ResumeFlow.cpp b/src/AppInstallerCLITests/ResumeFlow.cpp index 8fbaca5030..b2d5f3f3a8 100644 --- a/src/AppInstallerCLITests/ResumeFlow.cpp +++ b/src/AppInstallerCLITests/ResumeFlow.cpp @@ -256,62 +256,35 @@ TEST_CASE("ResumeFlow_ResumeLimitExceeded", "[Resume]") return; } - TestCommon::TempDirectory tempCheckpointRecordDirectory("TempCheckpointRecordDirectory", false); + TestCommon::TempDirectory tempCheckpointsLocationDir("TempCheckpointsLocationDir", true); - const auto& tempCheckpointRecordDirectoryPath = tempCheckpointRecordDirectory.GetPath(); + const auto& tempCheckpointRecordDirectoryPath = tempCheckpointsLocationDir.GetPath(); TestHook_SetPathOverride(PathName::CheckpointsLocation, tempCheckpointRecordDirectoryPath); TestCommon::TestUserSettings testSettings; testSettings.Set(true); - testSettings.Set(true); - testSettings.Set(true); testSettings.Set(1); - std::ostringstream installOutput; - TestContext context{ installOutput, std::cin }; - auto previousThreadGlobals = context.SetForCurrentThread(); - - // Override with reboot required HRESULT. - auto doesFeatureExistOverride = TestHook::SetDoesWindowsFeatureExistResult_Override(ERROR_SUCCESS); - auto setEnableFeatureOverride = TestHook::SetEnableWindowsFeatureResult_Override(ERROR_SUCCESS_REBOOT_REQUIRED); - - TestHook::SetRegisterForRestartResult_Override registerForRestartResultOverride(true); - TestHook::SetInitiateRebootResult_Override initiateRebootResultOverride(true); - - const auto& testManifestPath = TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string(); - context.Args.AddArg(Execution::Args::Type::Manifest, testManifestPath); - context.Args.AddArg(Execution::Args::Type::AllowReboot); - context.Args.AddArg(Execution::Args::Type::AcceptSourceAgreements); - - InstallCommand install({}); - context.SetExecutingCommand(&install); - install.Execute(context); - INFO(installOutput.str()); - - std::string resumeId; - for (const auto& entry : std::filesystem::directory_iterator(tempCheckpointRecordDirectoryPath)) - { - // There should only be one checkpoint folder created. - resumeId = entry.path().filename().u8string(); - } + std::filesystem::path testResumeId = L"testResumeId"; + std::filesystem::path tempResumeDir = tempCheckpointRecordDirectoryPath / testResumeId; + std::filesystem::path tempCheckpointDatabasePath = tempResumeDir / L"checkpoints.db"; { - // Execute resume once. - std::ostringstream resumeOutput; - TestContext resumeContext{ resumeOutput, std::cin }; - previousThreadGlobals = resumeContext.SetForCurrentThread(); - resumeContext.Args.AddArg(Execution::Args::Type::ResumeId, resumeId); - - ResumeCommand resume({}); - resume.Execute(resumeContext); + std::filesystem::create_directory(tempResumeDir); + std::shared_ptr database = CheckpointDatabase::CreateNew(tempCheckpointDatabasePath.u8string(), {1, 0}); + std::string_view testCheckpointName = "testCheckpoint"sv; + + CheckpointDatabase::IdType checkpointId = database->AddCheckpoint(testCheckpointName); + database->SetDataValue(checkpointId, AutomaticCheckpointData::Command, {}, { "install" }); + database->SetDataValue(checkpointId, AutomaticCheckpointData::ClientVersion, {}, { GetClientVersion()}); + database->SetDataValue(checkpointId, AutomaticCheckpointData::ResumeCount, {}, {"1"}); } { - // Resume should fail as the resume limit has been exceeded. std::ostringstream resumeOutput; TestContext resumeContext{ resumeOutput, std::cin }; - previousThreadGlobals = resumeContext.SetForCurrentThread(); - resumeContext.Args.AddArg(Execution::Args::Type::ResumeId, resumeId); + auto previousThreadGlobals = resumeContext.SetForCurrentThread(); + resumeContext.Args.AddArg(Execution::Args::Type::ResumeId, testResumeId.u8string()); ResumeCommand resume({}); resume.Execute(resumeContext); From 9a4e140df36ddb161eb338cd5fbc285fb2de903c Mon Sep 17 00:00:00 2001 From: ryfu-msft Date: Fri, 10 Nov 2023 13:30:32 -0800 Subject: [PATCH 08/23] fix test --- src/AppInstallerCLITests/UpdateFlow.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/AppInstallerCLITests/UpdateFlow.cpp b/src/AppInstallerCLITests/UpdateFlow.cpp index 19df1ede5e..7dc0873630 100644 --- a/src/AppInstallerCLITests/UpdateFlow.cpp +++ b/src/AppInstallerCLITests/UpdateFlow.cpp @@ -1052,7 +1052,7 @@ TEST_CASE("UpdateFlow_UpdateWithReboot", "[UpdateFlow][workflow][reboot]") INFO(updateOutput.str()); REQUIRE_FALSE(context.IsTerminated()); - REQUIRE_FALSE(updateOutput.str().find(Resource::LocString(Resource::String::InitiatingReboot).get()) != std::string::npos); + REQUIRE(updateOutput.str().find(Resource::LocString(Resource::String::InitiatingReboot).get()) != std::string::npos); REQUIRE(updateOutput.str().find(Resource::LocString(Resource::String::FailedToInitiateReboot).get()) != std::string::npos); } } From 97dba493ccb79438d2f4ceccefe574e8e1197cb1 Mon Sep 17 00:00:00 2001 From: ryfu-msft Date: Fri, 10 Nov 2023 17:37:37 -0800 Subject: [PATCH 09/23] fix e2e test --- src/AppInstallerCLIE2ETests/ResumeCommand.cs | 41 +++++++++++++------ .../TestExeInstaller_RebootRequired.yaml | 28 +++++++++++++ 2 files changed, 56 insertions(+), 13 deletions(-) create mode 100644 src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller_RebootRequired.yaml diff --git a/src/AppInstallerCLIE2ETests/ResumeCommand.cs b/src/AppInstallerCLIE2ETests/ResumeCommand.cs index ab6b84303f..ea32eb6127 100644 --- a/src/AppInstallerCLIE2ETests/ResumeCommand.cs +++ b/src/AppInstallerCLIE2ETests/ResumeCommand.cs @@ -67,24 +67,39 @@ public void ResumeIdNotFound() Assert.AreEqual(Constants.ErrorCode.ERROR_RESUME_ID_NOT_FOUND, resumeResult.ExitCode); } - /// - /// Verifies that a checkpoint record persists after a failed install. + /// + /// Test install a package that returns REBOOT_REQUIRED_TO_FINISH and verify that the checkpoint database is properly cleaned up. + /// + [Test] + public void InstallRequiresRebootToFinish() + { + var checkpointsDir = TestCommon.GetCheckpointsDirectory(); + int initialCheckpointsCount = Directory.Exists(checkpointsDir) ? Directory.GetDirectories(checkpointsDir).Length : 0; + + var result = TestCommon.RunAICLICommand("install", $"AppInstallerTest.RebootRequired --custom '/ExitCode 9'"); + Assert.AreNotEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_TO_FINISH, result.ExitCode); + Assert.True(result.StdOut.Contains("Restart your PC to finish installation.")); + + int actualCheckpointsCount = Directory.GetDirectories(checkpointsDir).Length; + + // Checkpoint database should be cleaned up since resume is not needed to complete installation. + Assert.AreEqual(initialCheckpointsCount, actualCheckpointsCount); + } + + /// + /// Test install a package that returns REBOOT_REQUIRED_TO_INSTALL and verify that resume command can be called successfully. /// - [Test] - public void ResumeRecordPreserved() - { + public void InstallRequiresRebootToInstall() + { var checkpointsDir = TestCommon.GetCheckpointsDirectory(); int initialCheckpointsCount = Directory.Exists(checkpointsDir) ? Directory.GetDirectories(checkpointsDir).Length : 0; - // TODO: Refine test case to more accurately reflect usage once resume is fully implemented. - var result = TestCommon.RunAICLICommand("install", $"AppInstallerTest.TestZipInvalidRelativePath"); - Assert.AreNotEqual(Constants.ErrorCode.S_OK, result.ExitCode); - Assert.True(result.StdOut.Contains("Invalid relative file path to the nested installer; path points to a location outside of the install directory")); + var result = TestCommon.RunAICLICommand("install", $"AppInstallerTest.RebootRequired --custom '/ExitCode 10'"); + Assert.AreNotEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL, result.ExitCode); + Assert.True(result.StdOut.Contains("Installation failed. Restart your PC then try again.")); int actualCheckpointsCount = Directory.GetDirectories(checkpointsDir).Length; - - // One new checkpoint record should be created after running the install command. Assert.AreEqual(initialCheckpointsCount + 1, actualCheckpointsCount); var checkpointsDirectoryInfo = new DirectoryInfo(checkpointsDir); @@ -95,8 +110,8 @@ public void ResumeRecordPreserved() // Resume output should be the same as the install result. var resumeResult = TestCommon.RunAICLICommand("resume", $"-g {checkpoint.Name}"); - Assert.AreNotEqual(Constants.ErrorCode.S_OK, resumeResult.ExitCode); - Assert.True(resumeResult.StdOut.Contains("Invalid relative file path to the nested installer; path points to a location outside of the install directory")); + Assert.AreNotEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL, resumeResult.ExitCode); + Assert.True(resumeResult.StdOut.Contains("Installation failed. Restart your PC then try again.")); } } } \ No newline at end of file diff --git a/src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller_RebootRequired.yaml b/src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller_RebootRequired.yaml new file mode 100644 index 0000000000..09ab71c0e1 --- /dev/null +++ b/src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller_RebootRequired.yaml @@ -0,0 +1,28 @@ +PackageIdentifier: AppInstallerTest.RebootRequired +PackageVersion: 1.0.0.0 +PackageLocale: en-US +PackageName: TestRebootRequired +ShortDescription: Emulates an installer that requires a reboot to finish. +Publisher: Microsoft Corporation +License: Test +Installers: + - Architecture: x64 + InstallerUrl: https://localhost:5001/TestKit/AppInstallerTestExeInstaller/AppInstallerTestExeInstaller.exe + InstallerType: exe + InstallerSha256: + InstallerSwitches: + SilentWithProgress: /exeswp + Silent: /exesilent + Interactive: /exeinteractive + Language: /exeenus + Log: /LogFile + InstallLocation: /InstallDir + ExpectedReturnCodes: + - InstallerReturnCode: 9 + ReturnResponse: rebootRequiredToFinish + ReturnResponseUrl: https://DefaultReturnResponseUrl.com + - InstallerReturnCode: 10 + ReturnResponse: rebootRequiredToInstall + ReturnResponseUrl: https://DefaultReturnResponseUrl.com +ManifestType: singleton +ManifestVersion: 1.4.0 \ No newline at end of file From 01ccda45bd7e9c6b67caac7309d9953c8233627b Mon Sep 17 00:00:00 2001 From: ryfu-msft Date: Mon, 13 Nov 2023 10:14:36 -0800 Subject: [PATCH 10/23] fix test manifest --- src/AppInstallerCLICore/ExecutionContext.cpp | 2 +- src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp | 2 +- src/AppInstallerCLICore/Workflows/InstallFlow.cpp | 4 ++-- src/AppInstallerCLIE2ETests/Constants.cs | 2 +- src/AppInstallerCLIE2ETests/ResumeCommand.cs | 9 +++++---- .../Manifests/TestExeInstaller_RebootRequired.yaml | 4 ++-- src/AppInstallerCLITests/ResumeFlow.cpp | 4 ++-- src/AppInstallerCLITests/WindowsFeature.cpp | 2 +- src/AppInstallerSharedLib/Errors.cpp | 2 +- src/AppInstallerSharedLib/Public/AppInstallerErrors.h | 2 +- 10 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/AppInstallerCLICore/ExecutionContext.cpp b/src/AppInstallerCLICore/ExecutionContext.cpp index 11f0f5fcb8..732796d182 100644 --- a/src/AppInstallerCLICore/ExecutionContext.cpp +++ b/src/AppInstallerCLICore/ExecutionContext.cpp @@ -257,7 +257,7 @@ namespace AppInstaller::CLI::Execution { if (Settings::ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::Resume)) { - if (m_checkpointManager && (!IsTerminated() || GetTerminationHR() != APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL)) + if (m_checkpointManager && (!IsTerminated() || GetTerminationHR() != APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL)) { m_checkpointManager->CleanUpDatabase(); } diff --git a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp index 81ecff2fb9..62ac64eccf 100644 --- a/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp @@ -230,7 +230,7 @@ namespace AppInstaller::CLI::Workflow context.Reporter.Error() << Resource::String::RebootRequiredToEnableWindowsFeatureOverrideRequired << std::endl; context.SetFlags(Execution::ContextFlag::RegisterResume); context.SetFlags(Execution::ContextFlag::RebootRequired); - AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL); + AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL); } } else diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index 129c79893f..c3cecd3802 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -132,7 +132,7 @@ namespace AppInstaller::CLI::Workflow case ExpectedReturnCodeEnum::RebootRequiredToFinish: return ExpectedReturnCode(returnCode, APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_FINISH, Resource::String::InstallFlowReturnCodeRebootRequiredToFinish); case ExpectedReturnCodeEnum::RebootRequiredForInstall: - return ExpectedReturnCode(returnCode, APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL, Resource::String::InstallFlowReturnCodeRebootRequiredForInstall); + return ExpectedReturnCode(returnCode, APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL, Resource::String::InstallFlowReturnCodeRebootRequiredForInstall); case ExpectedReturnCodeEnum::RebootInitiated: return ExpectedReturnCode(returnCode, APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_INITIATED, Resource::String::InstallFlowReturnCodeRebootInitiated); case ExpectedReturnCodeEnum::CancelledByUser: @@ -484,7 +484,7 @@ namespace AppInstaller::CLI::Workflow context.Reporter.Warn() << returnCode.Message << std::endl; terminationHR = S_OK; break; - case APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL: + case APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL: // REBOOT_REQUIRED_TO_INSTALL is treated as an error since installation has not yet completed. context.SetFlags(ContextFlag::RebootRequired); // TODO: Add separate workflow to handle restart registration for resume. diff --git a/src/AppInstallerCLIE2ETests/Constants.cs b/src/AppInstallerCLIE2ETests/Constants.cs index a578331548..87eace8626 100644 --- a/src/AppInstallerCLIE2ETests/Constants.cs +++ b/src/AppInstallerCLIE2ETests/Constants.cs @@ -271,7 +271,7 @@ public class ErrorCode public const int ERROR_INSTALL_NO_NETWORK = unchecked((int)0x8A150107); public const int ERROR_INSTALL_CONTACT_SUPPORT = unchecked((int)0x8A150108); public const int ERROR_INSTALL_REBOOT_REQUIRED_TO_FINISH = unchecked((int)0x8A150109); - public const int ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL = unchecked((int)0x8A15010A); + public const int ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL = unchecked((int)0x8A15010A); public const int ERROR_INSTALL_REBOOT_INITIATED = unchecked((int)0x8A15010B); public const int ERROR_INSTALL_CANCELLED_BY_USER = unchecked((int)0x8A15010C); public const int ERROR_INSTALL_ALREADY_INSTALLED = unchecked((int)0x8A15010D); diff --git a/src/AppInstallerCLIE2ETests/ResumeCommand.cs b/src/AppInstallerCLIE2ETests/ResumeCommand.cs index ea32eb6127..579f9ccbef 100644 --- a/src/AppInstallerCLIE2ETests/ResumeCommand.cs +++ b/src/AppInstallerCLIE2ETests/ResumeCommand.cs @@ -76,7 +76,7 @@ public void InstallRequiresRebootToFinish() var checkpointsDir = TestCommon.GetCheckpointsDirectory(); int initialCheckpointsCount = Directory.Exists(checkpointsDir) ? Directory.GetDirectories(checkpointsDir).Length : 0; - var result = TestCommon.RunAICLICommand("install", $"AppInstallerTest.RebootRequired --custom '/ExitCode 9'"); + var result = TestCommon.RunAICLICommand("install", $"--id AppInstallerTest.RebootRequired --custom '/ExitCode 9'"); Assert.AreNotEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_TO_FINISH, result.ExitCode); Assert.True(result.StdOut.Contains("Restart your PC to finish installation.")); @@ -89,14 +89,15 @@ public void InstallRequiresRebootToFinish() /// /// Test install a package that returns REBOOT_REQUIRED_TO_INSTALL and verify that resume command can be called successfully. /// + [Test] public void InstallRequiresRebootToInstall() { var checkpointsDir = TestCommon.GetCheckpointsDirectory(); int initialCheckpointsCount = Directory.Exists(checkpointsDir) ? Directory.GetDirectories(checkpointsDir).Length : 0; - var result = TestCommon.RunAICLICommand("install", $"AppInstallerTest.RebootRequired --custom '/ExitCode 10'"); - Assert.AreNotEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL, result.ExitCode); + var result = TestCommon.RunAICLICommand("install", $"--id AppInstallerTest.RebootRequired --custom '/ExitCode 10'"); + Assert.AreNotEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL, result.ExitCode); Assert.True(result.StdOut.Contains("Installation failed. Restart your PC then try again.")); int actualCheckpointsCount = Directory.GetDirectories(checkpointsDir).Length; @@ -110,7 +111,7 @@ public void InstallRequiresRebootToInstall() // Resume output should be the same as the install result. var resumeResult = TestCommon.RunAICLICommand("resume", $"-g {checkpoint.Name}"); - Assert.AreNotEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL, resumeResult.ExitCode); + Assert.AreNotEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL, resumeResult.ExitCode); Assert.True(resumeResult.StdOut.Contains("Installation failed. Restart your PC then try again.")); } } diff --git a/src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller_RebootRequired.yaml b/src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller_RebootRequired.yaml index 09ab71c0e1..cab8141fad 100644 --- a/src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller_RebootRequired.yaml +++ b/src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller_RebootRequired.yaml @@ -22,7 +22,7 @@ Installers: ReturnResponse: rebootRequiredToFinish ReturnResponseUrl: https://DefaultReturnResponseUrl.com - InstallerReturnCode: 10 - ReturnResponse: rebootRequiredToInstall + ReturnResponse: rebootRequiredForInstall ReturnResponseUrl: https://DefaultReturnResponseUrl.com ManifestType: singleton -ManifestVersion: 1.4.0 \ No newline at end of file +ManifestVersion: 1.6.0 \ No newline at end of file diff --git a/src/AppInstallerCLITests/ResumeFlow.cpp b/src/AppInstallerCLITests/ResumeFlow.cpp index b2d5f3f3a8..0dad1a16d0 100644 --- a/src/AppInstallerCLITests/ResumeFlow.cpp +++ b/src/AppInstallerCLITests/ResumeFlow.cpp @@ -227,7 +227,7 @@ TEST_CASE("ResumeFlow_WindowsFeature_RebootFailures", "[Resume][windowsFeature]" install.Execute(context); INFO(installOutput.str()); - REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL); + REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL); REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToRegisterReboot).get()) != std::string::npos); } SECTION("Initiate reboot fails") @@ -243,7 +243,7 @@ TEST_CASE("ResumeFlow_WindowsFeature_RebootFailures", "[Resume][windowsFeature]" install.Execute(context); INFO(installOutput.str()); - REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL); + REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL); REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToInitiateReboot).get()) != std::string::npos); } } diff --git a/src/AppInstallerCLITests/WindowsFeature.cpp b/src/AppInstallerCLITests/WindowsFeature.cpp index 4307985d7d..93c8b3e97f 100644 --- a/src/AppInstallerCLITests/WindowsFeature.cpp +++ b/src/AppInstallerCLITests/WindowsFeature.cpp @@ -149,7 +149,7 @@ TEST_CASE("InstallFlow_RebootRequired", "[windowsFeature]") install.Execute(context); INFO(installOutput.str()); - REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL); + REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL); REQUIRE(!std::filesystem::exists(installResultPath.GetPath())); REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::RebootRequiredToEnableWindowsFeatureOverrideRequired).get()) != std::string::npos); } diff --git a/src/AppInstallerSharedLib/Errors.cpp b/src/AppInstallerSharedLib/Errors.cpp index 7f7c4acf2c..15bb868061 100644 --- a/src/AppInstallerSharedLib/Errors.cpp +++ b/src/AppInstallerSharedLib/Errors.cpp @@ -212,7 +212,7 @@ namespace AppInstaller WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_INSTALL_NO_NETWORK, "This application requires internet connectivity. Connect to a network then try again."), WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_INSTALL_CONTACT_SUPPORT, "This application encountered an error during installation. Contact support."), WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_FINISH, "Restart your PC to finish installation."), - WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL, "Installation failed. Restart your PC then try again."), + WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL, "Installation failed. Restart your PC then try again."), WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_INITIATED, "Your PC will restart to finish installation."), WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_INSTALL_CANCELLED_BY_USER, "You cancelled the installation."), WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_INSTALL_ALREADY_INSTALLED, "Another version of this application is already installed."), diff --git a/src/AppInstallerSharedLib/Public/AppInstallerErrors.h b/src/AppInstallerSharedLib/Public/AppInstallerErrors.h index eb1a12f876..5305d22220 100644 --- a/src/AppInstallerSharedLib/Public/AppInstallerErrors.h +++ b/src/AppInstallerSharedLib/Public/AppInstallerErrors.h @@ -141,7 +141,7 @@ #define APPINSTALLER_CLI_ERROR_INSTALL_NO_NETWORK ((HRESULT)0x8A150107) #define APPINSTALLER_CLI_ERROR_INSTALL_CONTACT_SUPPORT ((HRESULT)0x8A150108) #define APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_FINISH ((HRESULT)0x8A150109) -#define APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL ((HRESULT)0x8A15010A) +#define APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL ((HRESULT)0x8A15010A) #define APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_INITIATED ((HRESULT)0x8A15010B) #define APPINSTALLER_CLI_ERROR_INSTALL_CANCELLED_BY_USER ((HRESULT)0x8A15010C) #define APPINSTALLER_CLI_ERROR_INSTALL_ALREADY_INSTALLED ((HRESULT)0x8A15010D) From 0b6da996e019e5cf616c3a590aa45cbd05d3a247 Mon Sep 17 00:00:00 2001 From: ryfu-msft Date: Mon, 13 Nov 2023 17:26:09 -0800 Subject: [PATCH 11/23] try rename --- .../Helpers/WinGetSettingsHelper.cs | 2 ++ src/AppInstallerCLIE2ETests/ResumeCommand.cs | 8 ++++++-- ...Required.yaml => TestExeInstaller.RebootRequired.yaml} | 0 3 files changed, 8 insertions(+), 2 deletions(-) rename src/AppInstallerCLIE2ETests/TestData/Manifests/{TestExeInstaller_RebootRequired.yaml => TestExeInstaller.RebootRequired.yaml} (100%) diff --git a/src/AppInstallerCLIE2ETests/Helpers/WinGetSettingsHelper.cs b/src/AppInstallerCLIE2ETests/Helpers/WinGetSettingsHelper.cs index b3f4be3f63..b89f21ba4e 100644 --- a/src/AppInstallerCLIE2ETests/Helpers/WinGetSettingsHelper.cs +++ b/src/AppInstallerCLIE2ETests/Helpers/WinGetSettingsHelper.cs @@ -44,6 +44,7 @@ public static void InitializeWingetSettings() { "directMSI", false }, { "windowsFeature", false }, { "resume", false }, + { "reboot", false }, } }, { @@ -210,6 +211,7 @@ public static void InitializeAllFeatures(bool status) ConfigureFeature("directMSI", status); ConfigureFeature("windowsFeature", status); ConfigureFeature("resume", status); + ConfigureFeature("reboot", status); } private static JObject GetJsonSettingsObject(string objectName) diff --git a/src/AppInstallerCLIE2ETests/ResumeCommand.cs b/src/AppInstallerCLIE2ETests/ResumeCommand.cs index 579f9ccbef..c612799580 100644 --- a/src/AppInstallerCLIE2ETests/ResumeCommand.cs +++ b/src/AppInstallerCLIE2ETests/ResumeCommand.cs @@ -76,9 +76,11 @@ public void InstallRequiresRebootToFinish() var checkpointsDir = TestCommon.GetCheckpointsDirectory(); int initialCheckpointsCount = Directory.Exists(checkpointsDir) ? Directory.GetDirectories(checkpointsDir).Length : 0; - var result = TestCommon.RunAICLICommand("install", $"--id AppInstallerTest.RebootRequired --custom '/ExitCode 9'"); + var installDir = TestCommon.GetRandomTestDir(); + var result = TestCommon.RunAICLICommand("install", $"TestRebootRequired --custom '/ExitCode 9' -l {installDir}"); Assert.AreNotEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_TO_FINISH, result.ExitCode); Assert.True(result.StdOut.Contains("Restart your PC to finish installation.")); + Assert.True(TestCommon.VerifyTestExeInstalledAndCleanup(installDir)); int actualCheckpointsCount = Directory.GetDirectories(checkpointsDir).Length; @@ -96,9 +98,11 @@ public void InstallRequiresRebootToInstall() int initialCheckpointsCount = Directory.Exists(checkpointsDir) ? Directory.GetDirectories(checkpointsDir).Length : 0; - var result = TestCommon.RunAICLICommand("install", $"--id AppInstallerTest.RebootRequired --custom '/ExitCode 10'"); + var installDir = TestCommon.GetRandomTestDir(); + var result = TestCommon.RunAICLICommand("install", $"TestRebootRequired --custom '/ExitCode 10' -l {installDir}"); Assert.AreNotEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL, result.ExitCode); Assert.True(result.StdOut.Contains("Installation failed. Restart your PC then try again.")); + Assert.True(TestCommon.VerifyTestExeInstalledAndCleanup(installDir)); int actualCheckpointsCount = Directory.GetDirectories(checkpointsDir).Length; Assert.AreEqual(initialCheckpointsCount + 1, actualCheckpointsCount); diff --git a/src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller_RebootRequired.yaml b/src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller.RebootRequired.yaml similarity index 100% rename from src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller_RebootRequired.yaml rename to src/AppInstallerCLIE2ETests/TestData/Manifests/TestExeInstaller.RebootRequired.yaml From a5dfeb6533ac88352b45dd74a6f285b4b1d69ea1 Mon Sep 17 00:00:00 2001 From: ryfu-msft Date: Mon, 13 Nov 2023 19:57:34 -0800 Subject: [PATCH 12/23] try quotes --- src/AppInstallerCLIE2ETests/ResumeCommand.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/AppInstallerCLIE2ETests/ResumeCommand.cs b/src/AppInstallerCLIE2ETests/ResumeCommand.cs index c612799580..b122f6aba4 100644 --- a/src/AppInstallerCLIE2ETests/ResumeCommand.cs +++ b/src/AppInstallerCLIE2ETests/ResumeCommand.cs @@ -77,7 +77,7 @@ public void InstallRequiresRebootToFinish() int initialCheckpointsCount = Directory.Exists(checkpointsDir) ? Directory.GetDirectories(checkpointsDir).Length : 0; var installDir = TestCommon.GetRandomTestDir(); - var result = TestCommon.RunAICLICommand("install", $"TestRebootRequired --custom '/ExitCode 9' -l {installDir}"); + var result = TestCommon.RunAICLICommand("install", $"TestRebootRequired --custom \"/ExitCode 9\" -l {installDir}"); Assert.AreNotEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_TO_FINISH, result.ExitCode); Assert.True(result.StdOut.Contains("Restart your PC to finish installation.")); Assert.True(TestCommon.VerifyTestExeInstalledAndCleanup(installDir)); @@ -99,7 +99,7 @@ public void InstallRequiresRebootToInstall() int initialCheckpointsCount = Directory.Exists(checkpointsDir) ? Directory.GetDirectories(checkpointsDir).Length : 0; var installDir = TestCommon.GetRandomTestDir(); - var result = TestCommon.RunAICLICommand("install", $"TestRebootRequired --custom '/ExitCode 10' -l {installDir}"); + var result = TestCommon.RunAICLICommand("install", $"TestRebootRequired --custom \"ExitCode 10\" -l {installDir}"); Assert.AreNotEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL, result.ExitCode); Assert.True(result.StdOut.Contains("Installation failed. Restart your PC then try again.")); Assert.True(TestCommon.VerifyTestExeInstalledAndCleanup(installDir)); From 1750404fbd9e8280ffb7fec190d62d3bb13211af Mon Sep 17 00:00:00 2001 From: ryfu-msft Date: Tue, 14 Nov 2023 00:40:41 -0800 Subject: [PATCH 13/23] fix E2E tests part 2 --- src/AppInstallerCLICore/Workflows/InstallFlow.cpp | 2 +- src/AppInstallerCLIE2ETests/ResumeCommand.cs | 13 ++++++++----- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index c3cecd3802..06dbb1586c 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -485,7 +485,7 @@ namespace AppInstaller::CLI::Workflow terminationHR = S_OK; break; case APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL: - // REBOOT_REQUIRED_TO_INSTALL is treated as an error since installation has not yet completed. + // REBOOT_REQUIRED_FOR_INSTALL is treated as an error since installation has not yet completed. context.SetFlags(ContextFlag::RebootRequired); // TODO: Add separate workflow to handle restart registration for resume. context.SetFlags(ContextFlag::RegisterResume); diff --git a/src/AppInstallerCLIE2ETests/ResumeCommand.cs b/src/AppInstallerCLIE2ETests/ResumeCommand.cs index b122f6aba4..fdf7e3bc6b 100644 --- a/src/AppInstallerCLIE2ETests/ResumeCommand.cs +++ b/src/AppInstallerCLIE2ETests/ResumeCommand.cs @@ -78,7 +78,9 @@ public void InstallRequiresRebootToFinish() var installDir = TestCommon.GetRandomTestDir(); var result = TestCommon.RunAICLICommand("install", $"TestRebootRequired --custom \"/ExitCode 9\" -l {installDir}"); - Assert.AreNotEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_TO_FINISH, result.ExitCode); + + // REBOOT_REQUIRED_TO_FINISH is treated as a success. + Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode); Assert.True(result.StdOut.Contains("Restart your PC to finish installation.")); Assert.True(TestCommon.VerifyTestExeInstalledAndCleanup(installDir)); @@ -89,18 +91,18 @@ public void InstallRequiresRebootToFinish() } /// - /// Test install a package that returns REBOOT_REQUIRED_TO_INSTALL and verify that resume command can be called successfully. + /// Test install a package that returns REBOOT_REQUIRED_FOR_INSTALL and verify that resume command can be called successfully. /// [Test] - public void InstallRequiresRebootToInstall() + public void InstallRequiresRebootForInstall() { var checkpointsDir = TestCommon.GetCheckpointsDirectory(); int initialCheckpointsCount = Directory.Exists(checkpointsDir) ? Directory.GetDirectories(checkpointsDir).Length : 0; var installDir = TestCommon.GetRandomTestDir(); - var result = TestCommon.RunAICLICommand("install", $"TestRebootRequired --custom \"ExitCode 10\" -l {installDir}"); - Assert.AreNotEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL, result.ExitCode); + var result = TestCommon.RunAICLICommand("install", $"TestRebootRequired --custom \"/ExitCode 10\" -l {installDir}"); + Assert.AreEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL, result.ExitCode); Assert.True(result.StdOut.Contains("Installation failed. Restart your PC then try again.")); Assert.True(TestCommon.VerifyTestExeInstalledAndCleanup(installDir)); @@ -117,6 +119,7 @@ public void InstallRequiresRebootToInstall() var resumeResult = TestCommon.RunAICLICommand("resume", $"-g {checkpoint.Name}"); Assert.AreNotEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL, resumeResult.ExitCode); Assert.True(resumeResult.StdOut.Contains("Installation failed. Restart your PC then try again.")); + Assert.True(TestCommon.VerifyTestExeInstalledAndCleanup(installDir)); } } } \ No newline at end of file From f7f42aba8c8708ae72fd4397b06e9dcba46efa51 Mon Sep 17 00:00:00 2001 From: ryfu-msft Date: Tue, 14 Nov 2023 09:47:13 -0800 Subject: [PATCH 14/23] fix tests again --- src/AppInstallerCLIE2ETests/ResumeCommand.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/AppInstallerCLIE2ETests/ResumeCommand.cs b/src/AppInstallerCLIE2ETests/ResumeCommand.cs index fdf7e3bc6b..4d75b02a68 100644 --- a/src/AppInstallerCLIE2ETests/ResumeCommand.cs +++ b/src/AppInstallerCLIE2ETests/ResumeCommand.cs @@ -103,7 +103,7 @@ public void InstallRequiresRebootForInstall() var installDir = TestCommon.GetRandomTestDir(); var result = TestCommon.RunAICLICommand("install", $"TestRebootRequired --custom \"/ExitCode 10\" -l {installDir}"); Assert.AreEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL, result.ExitCode); - Assert.True(result.StdOut.Contains("Installation failed. Restart your PC then try again.")); + Assert.True(result.StdOut.Contains("Your PC will restart to finish installation.")); Assert.True(TestCommon.VerifyTestExeInstalledAndCleanup(installDir)); int actualCheckpointsCount = Directory.GetDirectories(checkpointsDir).Length; @@ -117,8 +117,8 @@ public void InstallRequiresRebootForInstall() // Resume output should be the same as the install result. var resumeResult = TestCommon.RunAICLICommand("resume", $"-g {checkpoint.Name}"); - Assert.AreNotEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL, resumeResult.ExitCode); - Assert.True(resumeResult.StdOut.Contains("Installation failed. Restart your PC then try again.")); + Assert.AreEqual(Constants.ErrorCode.ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL, resumeResult.ExitCode); + Assert.True(resumeResult.StdOut.Contains("Your PC will restart to finish installation.")); Assert.True(TestCommon.VerifyTestExeInstalledAndCleanup(installDir)); } } From 5b20e4f5a63c3bdaa28890738464ea6ac0f8bb8a Mon Sep 17 00:00:00 2001 From: ryfu-msft Date: Wed, 15 Nov 2023 14:32:15 -0800 Subject: [PATCH 15/23] call RunOnce --- src/AppInstallerCLICore/Command.cpp | 17 ++++---- src/AppInstallerCLICore/ExecutionContext.cpp | 14 ++++-- src/AppInstallerCLICore/Resources.h | 1 - .../Workflows/InstallFlow.cpp | 2 +- .../Workflows/ResumeFlow.cpp | 15 ++----- .../Workflows/ResumeFlow.h | 4 +- .../Shared/Strings/en-us/winget.resw | 3 -- src/AppInstallerCLITests/ResumeFlow.cpp | 43 ++++++------------- src/AppInstallerCLITests/WorkflowCommon.cpp | 7 +++ src/AppInstallerCLITests/WorkflowCommon.h | 2 + .../Public/winget/Reboot.h | 7 +++ src/AppInstallerCommonCore/Reboot.cpp | 41 ++++++++++++++++++ 12 files changed, 94 insertions(+), 62 deletions(-) diff --git a/src/AppInstallerCLICore/Command.cpp b/src/AppInstallerCLICore/Command.cpp index 3aea7e147f..5a324dd1b5 100644 --- a/src/AppInstallerCLICore/Command.cpp +++ b/src/AppInstallerCLICore/Command.cpp @@ -882,20 +882,21 @@ namespace AppInstaller::CLI context.Reporter.PromptForEnter(); } + if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::RegisterResume)) + { + // RegisterResume context flag assumes we already wrote to the RunOnce registry. + // Since we are about to initiate a restart, this is no longer needed as a safety net. + Reboot::UnregisterApplicationForReboot(); + + context.ClearFlags(Execution::ContextFlag::RegisterResume); + } + context.ClearFlags(Execution::ContextFlag::RebootRequired); if (!Reboot::InitiateReboot()) { context.Reporter.Error() << Resource::String::FailedToInitiateReboot << std::endl; } - else if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::RegisterResume)) - { - context.ClearFlags(Execution::ContextFlag::RegisterResume); - - // For Windows Error Reporting to restart this process, the process must be rebooted while it is still running. - // Workaround is to have the program sleep while the reboot process is kicked off. Only applies to resume. - Sleep(5000); - } } else { diff --git a/src/AppInstallerCLICore/ExecutionContext.cpp b/src/AppInstallerCLICore/ExecutionContext.cpp index 732796d182..2be6be2c7e 100644 --- a/src/AppInstallerCLICore/ExecutionContext.cpp +++ b/src/AppInstallerCLICore/ExecutionContext.cpp @@ -1,13 +1,14 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. #include "pch.h" -#include "ExecutionContext.h" -#include "COMContext.h" -#include "Argument.h" -#include "winget/UserSettings.h" #include "AppInstallerRuntime.h" +#include "Argument.h" +#include "COMContext.h" #include "Command.h" +#include "ExecutionContext.h" #include "Public/winget/Checkpoint.h" +#include "winget/Reboot.h" +#include "winget/UserSettings.h" using namespace AppInstaller::Checkpoints; @@ -260,6 +261,7 @@ namespace AppInstaller::CLI::Execution if (m_checkpointManager && (!IsTerminated() || GetTerminationHR() != APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL)) { m_checkpointManager->CleanUpDatabase(); + AppInstaller::Reboot::UnregisterApplicationForReboot(); } } @@ -452,6 +454,10 @@ namespace AppInstaller::CLI::Execution { m_checkpointManager = std::make_unique(); m_checkpointManager->CreateAutomaticCheckpoint(*this); + + // Register for restart only when we first call checkpoint to support restarting from an unexpected shutdown. + std::string commandLineArg = "resume -g " + GetResumeId(); + AppInstaller::Reboot::RegisterApplicationForReboot(commandLineArg); } // TODO: Capture context data for checkpoint. diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index 10655d8132..4511a75fa7 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -187,7 +187,6 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(FailedToEnableWindowsFeatureOverrideRequired); WINGET_DEFINE_RESOURCE_STRINGID(FailedToInitiateReboot); WINGET_DEFINE_RESOURCE_STRINGID(FailedToRefreshPathWarning); - WINGET_DEFINE_RESOURCE_STRINGID(FailedToRegisterReboot); WINGET_DEFINE_RESOURCE_STRINGID(FeatureDisabledByAdminSettingMessage); WINGET_DEFINE_RESOURCE_STRINGID(FeatureDisabledMessage); WINGET_DEFINE_RESOURCE_STRINGID(FeaturesCommandLongDescription); diff --git a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp index 06dbb1586c..6ce9512c5e 100644 --- a/src/AppInstallerCLICore/Workflows/InstallFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/InstallFlow.cpp @@ -599,7 +599,7 @@ namespace AppInstaller::CLI::Workflow Workflow::InstallDependencies << Workflow::DownloadInstaller << Workflow::InstallPackageInstaller << - Workflow::RegisterForReboot(); + Workflow::RegisterStartupAfterReboot(); } void EnsureSupportForInstall(Execution::Context& context) diff --git a/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp b/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp index 6bd8ee98b1..bc0523a963 100644 --- a/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp @@ -16,7 +16,7 @@ namespace AppInstaller::CLI::Workflow context.Checkpoint(m_checkpointName, m_contextData); } - void RegisterForReboot::operator()(Execution::Context& context) const + void RegisterStartupAfterReboot::operator()(Execution::Context& context) const { if (!Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::Resume) || !Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::Reboot)) @@ -26,17 +26,8 @@ namespace AppInstaller::CLI::Workflow if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::RegisterResume)) { - std::string commandLineArg = "resume -g " + context.GetResumeId(); - - if (!Reboot::RegisterApplicationForReboot(commandLineArg)) - { - context.Reporter.Error() << Resource::String::FailedToRegisterReboot << std::endl; - - if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::RebootRequired)) - { - context.ClearFlags(Execution::ContextFlag::RebootRequired); - } - } + std::string commandLine = "winget resume -g " + context.GetResumeId(); + Reboot::WriteToRunOnceRegistry(commandLine); } } } diff --git a/src/AppInstallerCLICore/Workflows/ResumeFlow.h b/src/AppInstallerCLICore/Workflows/ResumeFlow.h index 1c9b8290f6..6f8dfadcb0 100644 --- a/src/AppInstallerCLICore/Workflows/ResumeFlow.h +++ b/src/AppInstallerCLICore/Workflows/ResumeFlow.h @@ -27,9 +27,9 @@ namespace AppInstaller::CLI::Workflow // Required Args: None // Inputs: None // Outputs: None - struct RegisterForReboot : public WorkflowTask + struct RegisterStartupAfterReboot : public WorkflowTask { - RegisterForReboot() : WorkflowTask("RegisterForReboot", /* executeAlways*/ true) {} + RegisterStartupAfterReboot() : WorkflowTask("RegisterStartupAfterReboot", /* executeAlways*/ true) {} void operator()(Execution::Context & context) const override; }; diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index 5490a4ae8b..bdfb2bdffd 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -2631,9 +2631,6 @@ Please specify one of them using the --source option to proceed. Failed to initiate a reboot. - - Failed to register winget for reboot. - Resume operation exceeds the allowable limit of {0} resume(s). {Locked="{0}"} {0} is a placeholder that is replaced by an integer number of the number of allowed resumes. diff --git a/src/AppInstallerCLITests/ResumeFlow.cpp b/src/AppInstallerCLITests/ResumeFlow.cpp index 0dad1a16d0..d0435bc8fd 100644 --- a/src/AppInstallerCLITests/ResumeFlow.cpp +++ b/src/AppInstallerCLITests/ResumeFlow.cpp @@ -187,7 +187,8 @@ TEST_CASE("ResumeFlow_InstallFailure", "[Resume]") REQUIRE(std::filesystem::is_empty(tempCheckpointRecordDirectoryPath)); } -TEST_CASE("ResumeFlow_WindowsFeature_RebootFailures", "[Resume][windowsFeature]") + +TEST_CASE("ResumeFlow_WriteToRunOnceRegistry", "[Reboot][Resume][windowsFeature]") { if (!AppInstaller::Runtime::IsRunningAsAdmin()) { @@ -209,43 +210,23 @@ TEST_CASE("ResumeFlow_WindowsFeature_RebootFailures", "[Resume][windowsFeature]" TestContext context{ installOutput, std::cin }; auto previousThreadGlobals = context.SetForCurrentThread(); OverrideOpenDependencySource(context); + OverrideRegisterStartupAfterReboot(context); // Override with reboot required HRESULT. auto doesFeatureExistOverride = TestHook::SetDoesWindowsFeatureExistResult_Override(ERROR_SUCCESS); auto setEnableFeatureOverride = TestHook::SetEnableWindowsFeatureResult_Override(ERROR_SUCCESS_REBOOT_REQUIRED); + TestHook::SetRegisterForRestartResult_Override registerForRestartResultOverride(false); + TestHook::SetInitiateRebootResult_Override initiateRebootResultOverride(false); - SECTION("Register for reboot fails") - { - TestHook::SetRegisterForRestartResult_Override registerForRestartResultOverride(false); - TestHook::SetInitiateRebootResult_Override initiateRebootResultOverride(true); + const auto& testManifestPath = TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string(); + context.Args.AddArg(Execution::Args::Type::Manifest, testManifestPath); + context.Args.AddArg(Execution::Args::Type::AllowReboot); - const auto& testManifestPath = TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string(); - context.Args.AddArg(Execution::Args::Type::Manifest, testManifestPath); - context.Args.AddArg(Execution::Args::Type::AllowReboot); + InstallCommand install({}); + install.Execute(context); + INFO(installOutput.str()); - InstallCommand install({}); - install.Execute(context); - INFO(installOutput.str()); - - REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL); - REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToRegisterReboot).get()) != std::string::npos); - } - SECTION("Initiate reboot fails") - { - TestHook::SetRegisterForRestartResult_Override registerForRestartResultOverride(true); - TestHook::SetInitiateRebootResult_Override initiateRebootResultOverride(false); - - const auto& testManifestPath = TestDataFile("InstallFlowTest_WindowsFeatures.yaml").GetPath().u8string(); - context.Args.AddArg(Execution::Args::Type::Manifest, testManifestPath); - context.Args.AddArg(Execution::Args::Type::AllowReboot); - - InstallCommand install({}); - install.Execute(context); - INFO(installOutput.str()); - - REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL); - REQUIRE(installOutput.str().find(Resource::LocString(Resource::String::FailedToInitiateReboot).get()) != std::string::npos); - } + REQUIRE(context.GetTerminationHR() == APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL); } TEST_CASE("ResumeFlow_ResumeLimitExceeded", "[Resume]") diff --git a/src/AppInstallerCLITests/WorkflowCommon.cpp b/src/AppInstallerCLITests/WorkflowCommon.cpp index 33749a547b..3c35a3c104 100644 --- a/src/AppInstallerCLITests/WorkflowCommon.cpp +++ b/src/AppInstallerCLITests/WorkflowCommon.cpp @@ -709,4 +709,11 @@ namespace TestCommon { } }); } + + void OverrideRegisterStartupAfterReboot(TestContext& context) + { + context.Override({ "RegisterStartupAfterReboot", [](TestContext&) + { + } }); + } } \ No newline at end of file diff --git a/src/AppInstallerCLITests/WorkflowCommon.h b/src/AppInstallerCLITests/WorkflowCommon.h index 728c525586..95a73945b8 100644 --- a/src/AppInstallerCLITests/WorkflowCommon.h +++ b/src/AppInstallerCLITests/WorkflowCommon.h @@ -137,4 +137,6 @@ namespace TestCommon void OverrideOpenDependencySource(TestContext& context); void OverrideEnableWindowsFeaturesDependencies(TestContext& context); + + void OverrideRegisterStartupAfterReboot(TestContext& context); } \ No newline at end of file diff --git a/src/AppInstallerCommonCore/Public/winget/Reboot.h b/src/AppInstallerCommonCore/Public/winget/Reboot.h index 23e77cca37..3712a62e22 100644 --- a/src/AppInstallerCommonCore/Public/winget/Reboot.h +++ b/src/AppInstallerCommonCore/Public/winget/Reboot.h @@ -6,5 +6,12 @@ namespace AppInstaller::Reboot { bool InitiateReboot(); + // Registers the application to be restarted by Windows Error Reporting in case of an unexpected shutdown. bool RegisterApplicationForReboot(const std::string& commandLineArgs); + + // Unregisters the application from being restarted by Windows Error Reporting. + bool UnregisterApplicationForReboot(); + + // Runs a program when a user logs on. + void WriteToRunOnceRegistry(const std::string& commandLine); } \ No newline at end of file diff --git a/src/AppInstallerCommonCore/Reboot.cpp b/src/AppInstallerCommonCore/Reboot.cpp index 27ee2338f5..dd6c38442c 100644 --- a/src/AppInstallerCommonCore/Reboot.cpp +++ b/src/AppInstallerCommonCore/Reboot.cpp @@ -4,10 +4,18 @@ #include "AppInstallerLogging.h" #include "AppInstallerStrings.h" #include "Public/winget/Reboot.h" +#include "Public/winget/Registry.h" #include +using namespace AppInstaller::Registry; + namespace AppInstaller::Reboot { + namespace + { + constexpr std::wstring_view s_RunOnceRegistry = L"Software\\Microsoft\\Windows\\CurrentVersion\\RunOnce"; + } + #ifndef AICLI_DISABLE_TEST_HOOKS static bool* s_InitiateRebootResult_TestHook_Override = nullptr; @@ -85,4 +93,37 @@ namespace AppInstaller::Reboot return true; } } + + bool UnregisterApplicationForReboot() + { + HRESULT result = UnregisterApplicationRestart(); + AICLI_LOG(CLI, Info, << "Application unregistered for restart."); + + if (FAILED(result)) + { + AICLI_LOG(Core, Error, << "RegisterApplicationRestart failed with hr: " << result); + return false; + } + else + { + return true; + } + } + + void WriteToRunOnceRegistry(const std::string& commandLine) + { + THROW_HR_IF(E_UNEXPECTED, commandLine.size() > MAX_PATH); + + HKEY root = HKEY_CURRENT_USER; + std::wstring subKey = std::wstring{ s_RunOnceRegistry }; + Key key = Key::OpenIfExists(HKEY_CURRENT_USER, subKey, 0, KEY_ALL_ACCESS); + + if (key == NULL) + { + key = Key::Create(root, subKey); + } + + key.SetValue(L"ResumeWinget", Utility::ConvertToUTF16(commandLine), REG_SZ); + AICLI_LOG(CLI, Info, << "Set RunOnce registry with value: " << commandLine); + } } From 4fec0d3c617c5b610c6ceb045142b816f0e0e46c Mon Sep 17 00:00:00 2001 From: ryfu-msft Date: Thu, 16 Nov 2023 12:06:47 -0800 Subject: [PATCH 16/23] address PR feedback --- src/AppInstallerCLICore/Argument.cpp | 2 ++ .../Commands/ResumeCommand.cpp | 6 ++++-- src/AppInstallerCLICore/ExecutionArgs.h | 1 + src/AppInstallerCLICore/ExecutionContext.cpp | 18 +++++++++++++++--- src/AppInstallerCLICore/Resources.h | 1 + .../Workflows/ResumeFlow.cpp | 12 ++++++++++-- .../Shared/Strings/en-us/winget.resw | 7 +++++-- src/AppInstallerCLITests/ResumeFlow.cpp | 5 ++++- .../Public/winget/Reboot.h | 2 +- src/AppInstallerCommonCore/Reboot.cpp | 8 ++++---- .../Microsoft/CheckpointDatabase.cpp | 14 ++++++++++++++ .../Microsoft/CheckpointDatabase.h | 3 +++ .../Public/winget/Checkpoint.h | 3 +-- 13 files changed, 65 insertions(+), 17 deletions(-) diff --git a/src/AppInstallerCLICore/Argument.cpp b/src/AppInstallerCLICore/Argument.cpp index 29ff69e232..b35007008b 100644 --- a/src/AppInstallerCLICore/Argument.cpp +++ b/src/AppInstallerCLICore/Argument.cpp @@ -365,6 +365,8 @@ namespace AppInstaller::CLI return Argument{ type, Resource::String::ResumeIdArgumentDescription, ArgumentType::Standard, true }; case Args::Type::AllowReboot: return Argument{ type, Resource::String::AllowRebootArgumentDescription, ArgumentType::Flag, ExperimentalFeature::Feature::Reboot }; + case Args::Type::IgnoreResumeLimit: + return Argument{ type, Resource::String::IgnoreResumeLimitArgumentDescription, ArgumentType::Flag, ExperimentalFeature::Feature::Resume }; default: THROW_HR(E_UNEXPECTED); } diff --git a/src/AppInstallerCLICore/Commands/ResumeCommand.cpp b/src/AppInstallerCLICore/Commands/ResumeCommand.cpp index 87fc36ea07..d7d4124b60 100644 --- a/src/AppInstallerCLICore/Commands/ResumeCommand.cpp +++ b/src/AppInstallerCLICore/Commands/ResumeCommand.cpp @@ -54,6 +54,7 @@ namespace AppInstaller::CLI { return { Argument::ForType(Execution::Args::Type::ResumeId), + Argument::ForType(Execution::Args::Type::IgnoreResumeLimit), }; } @@ -101,9 +102,10 @@ namespace AppInstaller::CLI const auto& resumeCountString = automaticCheckpoint.Get(AutomaticCheckpointData::ResumeCount, {}); int resumeCount = std::stoi(resumeCountString); - if (resumeCount >= Settings::User().Get()) + if (!context.Args.Contains(Execution::Args::Type::IgnoreResumeLimit) && resumeCount >= Settings::User().Get()) { - context.Reporter.Error() << Resource::String::ResumeLimitExceeded(Utility::LocIndView{ resumeCountString }) << std::endl; + std::string manualResumeString = "winget resume -g " + std::string{ resumeId } + " --ignore-resume-limit"; + context.Reporter.Error() << Resource::String::ResumeLimitExceeded(Utility::LocIndView{ resumeCountString }) << Utility::LocIndView{ manualResumeString } << std::endl; AICLI_TERMINATE_CONTEXT(APPINSTALLER_CLI_ERROR_RESUME_LIMIT_EXCEEDED); } else diff --git a/src/AppInstallerCLICore/ExecutionArgs.h b/src/AppInstallerCLICore/ExecutionArgs.h index 0154aeab25..1b74275f3f 100644 --- a/src/AppInstallerCLICore/ExecutionArgs.h +++ b/src/AppInstallerCLICore/ExecutionArgs.h @@ -112,6 +112,7 @@ namespace AppInstaller::CLI::Execution // Resume Command ResumeId, + IgnoreResumeLimit, // Configuration ConfigurationFile, diff --git a/src/AppInstallerCLICore/ExecutionContext.cpp b/src/AppInstallerCLICore/ExecutionContext.cpp index 2be6be2c7e..3d4c194e4c 100644 --- a/src/AppInstallerCLICore/ExecutionContext.cpp +++ b/src/AppInstallerCLICore/ExecutionContext.cpp @@ -252,13 +252,26 @@ namespace AppInstaller::CLI::Execution SignalTerminationHandler::Instance().RemoveContext(context); } } + + bool ShouldRemoveCheckpointDatabase(HRESULT hr) + { + switch (hr) + { + case APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL: + case APPINSTALLER_CLI_ERROR_RESUME_LIMIT_EXCEEDED: + case APPINSTALLER_CLI_ERROR_CLIENT_VERSION_MISMATCH: + return false; + default: + return true; + } + } } Context::~Context() { if (Settings::ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::Resume)) { - if (m_checkpointManager && (!IsTerminated() || GetTerminationHR() != APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_FOR_INSTALL)) + if (m_checkpointManager && (!IsTerminated() || ShouldRemoveCheckpointDatabase(GetTerminationHR()))) { m_checkpointManager->CleanUpDatabase(); AppInstaller::Reboot::UnregisterApplicationForReboot(); @@ -456,8 +469,7 @@ namespace AppInstaller::CLI::Execution m_checkpointManager->CreateAutomaticCheckpoint(*this); // Register for restart only when we first call checkpoint to support restarting from an unexpected shutdown. - std::string commandLineArg = "resume -g " + GetResumeId(); - AppInstaller::Reboot::RegisterApplicationForReboot(commandLineArg); + AppInstaller::Reboot::RegisterApplicationForReboot("resume -g " + GetResumeId()); } // TODO: Capture context data for checkpoint. diff --git a/src/AppInstallerCLICore/Resources.h b/src/AppInstallerCLICore/Resources.h index 4511a75fa7..8f96c3c856 100644 --- a/src/AppInstallerCLICore/Resources.h +++ b/src/AppInstallerCLICore/Resources.h @@ -218,6 +218,7 @@ namespace AppInstaller::CLI::Resource WINGET_DEFINE_RESOURCE_STRINGID(HelpLinkPreamble); WINGET_DEFINE_RESOURCE_STRINGID(IdArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(IgnoreLocalArchiveMalwareScanArgumentDescription); + WINGET_DEFINE_RESOURCE_STRINGID(IgnoreResumeLimitArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(IgnoreWarningsArgumentDescription); WINGET_DEFINE_RESOURCE_STRINGID(ImportCommandLongDescription); WINGET_DEFINE_RESOURCE_STRINGID(ImportCommandReportDependencies); diff --git a/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp b/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp index bc0523a963..adb3fcc0bf 100644 --- a/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. #include "pch.h" +#include #include "ResumeFlow.h" #include "winget/Reboot.h" @@ -26,8 +27,15 @@ namespace AppInstaller::CLI::Workflow if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::RegisterResume)) { - std::string commandLine = "winget resume -g " + context.GetResumeId(); - Reboot::WriteToRunOnceRegistry(commandLine); + int argc = 0; + wil::unique_hlocal_ptr argv{ CommandLineToArgvW(GetCommandLineW(), &argc) }; + THROW_LAST_ERROR_IF_NULL(argv); + + // RunOnce registry value must start with the full path of the executable. + std::string commandLine = Utility::ConvertToUTF8(argv.get()[0]) + " resume -g " + context.GetResumeId(); + + bool isAdmin = AppInstaller::Runtime::IsRunningAsAdmin(); + Reboot::WriteToRunOnceRegistry(commandLine, isAdmin); } } } diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index bdfb2bdffd..c94b9aded5 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -2449,7 +2449,7 @@ Please specify one of them using the --source option to proceed. Restart your PC to finish installation. - + Installation failed. Restart your PC then try again. @@ -2632,7 +2632,10 @@ Please specify one of them using the --source option to proceed. Failed to initiate a reboot. - Resume operation exceeds the allowable limit of {0} resume(s). + Resume operation exceeds the allowable limit of {0} resume(s). To manually resume, run the command: {Locked="{0}"} {0} is a placeholder that is replaced by an integer number of the number of allowed resumes. + + Ignore the max limit of resuming a saved state + \ No newline at end of file diff --git a/src/AppInstallerCLITests/ResumeFlow.cpp b/src/AppInstallerCLITests/ResumeFlow.cpp index d0435bc8fd..739f300631 100644 --- a/src/AppInstallerCLITests/ResumeFlow.cpp +++ b/src/AppInstallerCLITests/ResumeFlow.cpp @@ -249,9 +249,9 @@ TEST_CASE("ResumeFlow_ResumeLimitExceeded", "[Resume]") std::filesystem::path testResumeId = L"testResumeId"; std::filesystem::path tempResumeDir = tempCheckpointRecordDirectoryPath / testResumeId; std::filesystem::path tempCheckpointDatabasePath = tempResumeDir / L"checkpoints.db"; + std::filesystem::create_directory(tempResumeDir); { - std::filesystem::create_directory(tempResumeDir); std::shared_ptr database = CheckpointDatabase::CreateNew(tempCheckpointDatabasePath.u8string(), {1, 0}); std::string_view testCheckpointName = "testCheckpoint"sv; @@ -272,5 +272,8 @@ TEST_CASE("ResumeFlow_ResumeLimitExceeded", "[Resume]") INFO(resumeOutput.str()); REQUIRE(resumeContext.IsTerminated()); REQUIRE(resumeContext.GetTerminationHR() == APPINSTALLER_CLI_ERROR_RESUME_LIMIT_EXCEEDED); + + REQUIRE(resumeOutput.str().find(Resource::LocString(Resource::String::ResumeLimitExceeded('1')).get()) != std::string::npos); + REQUIRE(resumeOutput.str().find("winget resume -g " + testResumeId.u8string() + " --ignore-resume-limit") != std::string::npos); } } diff --git a/src/AppInstallerCommonCore/Public/winget/Reboot.h b/src/AppInstallerCommonCore/Public/winget/Reboot.h index 3712a62e22..2f976fdac3 100644 --- a/src/AppInstallerCommonCore/Public/winget/Reboot.h +++ b/src/AppInstallerCommonCore/Public/winget/Reboot.h @@ -13,5 +13,5 @@ namespace AppInstaller::Reboot bool UnregisterApplicationForReboot(); // Runs a program when a user logs on. - void WriteToRunOnceRegistry(const std::string& commandLine); + void WriteToRunOnceRegistry(const std::string& commandLine, bool isAdmin); } \ No newline at end of file diff --git a/src/AppInstallerCommonCore/Reboot.cpp b/src/AppInstallerCommonCore/Reboot.cpp index dd6c38442c..3e5ec172b5 100644 --- a/src/AppInstallerCommonCore/Reboot.cpp +++ b/src/AppInstallerCommonCore/Reboot.cpp @@ -110,20 +110,20 @@ namespace AppInstaller::Reboot } } - void WriteToRunOnceRegistry(const std::string& commandLine) + void WriteToRunOnceRegistry(const std::string& commandLine, bool isAdmin) { THROW_HR_IF(E_UNEXPECTED, commandLine.size() > MAX_PATH); - HKEY root = HKEY_CURRENT_USER; + HKEY root = isAdmin ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER; std::wstring subKey = std::wstring{ s_RunOnceRegistry }; - Key key = Key::OpenIfExists(HKEY_CURRENT_USER, subKey, 0, KEY_ALL_ACCESS); + Key key = Key::OpenIfExists(root, subKey, 0, KEY_ALL_ACCESS); if (key == NULL) { key = Key::Create(root, subKey); } - key.SetValue(L"ResumeWinget", Utility::ConvertToUTF16(commandLine), REG_SZ); + key.SetValue(L"WingetResume", Utility::ConvertToUTF16(commandLine), REG_SZ); AICLI_LOG(CLI, Info, << "Set RunOnce registry with value: " << commandLine); } } diff --git a/src/AppInstallerRepositoryCore/Microsoft/CheckpointDatabase.cpp b/src/AppInstallerRepositoryCore/Microsoft/CheckpointDatabase.cpp index 744fe6f490..160a62f219 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/CheckpointDatabase.cpp +++ b/src/AppInstallerRepositoryCore/Microsoft/CheckpointDatabase.cpp @@ -77,6 +77,20 @@ namespace AppInstaller::Repository::Microsoft savepoint.Commit(); } + void CheckpointDatabase::UpdateDataValue(IdType checkpointId, int dataType, const std::string& field, const std::vector& values) + { + std::lock_guard lockInterface{ *m_interfaceLock }; + AICLI_LOG(Repo, Verbose, << "Updating checkpoint data [" << dataType << "]"); + + SQLite::Savepoint savepoint = SQLite::Savepoint::Create(m_dbconn, "CheckpointDatabase_updateDataValue"); + + m_interface->RemoveCheckpointDataType(m_dbconn, checkpointId, dataType); + m_interface->SetCheckpointDataValues(m_dbconn, checkpointId, dataType, field, values); + + SetLastWriteTime(); + savepoint.Commit(); + } + void CheckpointDatabase::RemoveDataType(IdType checkpointId, int dataType) { std::lock_guard lockInterface{ *m_interfaceLock }; diff --git a/src/AppInstallerRepositoryCore/Microsoft/CheckpointDatabase.h b/src/AppInstallerRepositoryCore/Microsoft/CheckpointDatabase.h index 5ec9ef6ff9..976cf55ff9 100644 --- a/src/AppInstallerRepositoryCore/Microsoft/CheckpointDatabase.h +++ b/src/AppInstallerRepositoryCore/Microsoft/CheckpointDatabase.h @@ -50,6 +50,9 @@ namespace AppInstaller::Repository::Microsoft // Sets the value(s) for a data type and field. void SetDataValue(IdType checkpointId, int dataType, const std::string& field, const std::vector& values); + // Updates the value(s) for a data type and field. + void UpdateDataValue(IdType checkpointId, int dataType, const std::string& field, const std::vector& values); + // Gets a single value for a data type field. std::string GetDataFieldSingleValue(IdType checkpointId, int dataType, const std::string& field); diff --git a/src/AppInstallerRepositoryCore/Public/winget/Checkpoint.h b/src/AppInstallerRepositoryCore/Public/winget/Checkpoint.h index cb83f8d672..764819be21 100644 --- a/src/AppInstallerRepositoryCore/Public/winget/Checkpoint.h +++ b/src/AppInstallerRepositoryCore/Public/winget/Checkpoint.h @@ -57,8 +57,7 @@ namespace AppInstaller::Checkpoints // Update a single existing field value for a data type. void Update(T dataType, const std::string& fieldName, const std::string& value) { - m_checkpointDatabase->RemoveDataType(m_checkpointId, dataType); - m_checkpointDatabase->SetDataValue(m_checkpointId, dataType, fieldName, { value }); + m_checkpointDatabase->UpdateDataValue(m_checkpointId, dataType, fieldName, { value }); } // Gets a single field value for a data type. From a81a803db83b41a090fd453882072b7a13ad5775 Mon Sep 17 00:00:00 2001 From: ryfu-msft Date: Thu, 16 Nov 2023 14:31:15 -0800 Subject: [PATCH 17/23] fix unit tests --- src/AppInstallerCLICore/Argument.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/AppInstallerCLICore/Argument.cpp b/src/AppInstallerCLICore/Argument.cpp index b35007008b..088155cc9b 100644 --- a/src/AppInstallerCLICore/Argument.cpp +++ b/src/AppInstallerCLICore/Argument.cpp @@ -185,6 +185,8 @@ namespace AppInstaller::CLI // Resume command case Execution::Args::Type::ResumeId: return { type, "resume-id"_liv, 'g', ArgTypeCategory::None }; + case Execution::Args::Type::IgnoreResumeLimit: + return { type, "ignore-resume-limit"_liv, ArgTypeCategory::None }; // Configuration commands case Execution::Args::Type::ConfigurationFile: From 11cbedb4d99e650f0873b9e10a3ab7c6cb5cb6c9 Mon Sep 17 00:00:00 2001 From: --global Date: Fri, 17 Nov 2023 10:45:00 -0800 Subject: [PATCH 18/23] add comment for errors --- src/AppInstallerSharedLib/Errors.cpp | 1 + src/AppInstallerSharedLib/Public/AppInstallerErrors.h | 1 + 2 files changed, 2 insertions(+) diff --git a/src/AppInstallerSharedLib/Errors.cpp b/src/AppInstallerSharedLib/Errors.cpp index 15bb868061..3e5e0e6e8a 100644 --- a/src/AppInstallerSharedLib/Errors.cpp +++ b/src/AppInstallerSharedLib/Errors.cpp @@ -87,6 +87,7 @@ namespace AppInstaller constexpr const WinGetHResultData s_wingetHResultData[] = { + // Changes to any of these errors require the corresponding resource string in winget.resw to be updated. WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_INTERNAL_ERROR, "Internal Error"), WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_INVALID_CL_ARGUMENTS, "Invalid command line arguments"), WINGET_HRESULT_INFO(APPINSTALLER_CLI_ERROR_COMMAND_FAILED, "Executing command failed"), diff --git a/src/AppInstallerSharedLib/Public/AppInstallerErrors.h b/src/AppInstallerSharedLib/Public/AppInstallerErrors.h index 5305d22220..da328acc75 100644 --- a/src/AppInstallerSharedLib/Public/AppInstallerErrors.h +++ b/src/AppInstallerSharedLib/Public/AppInstallerErrors.h @@ -17,6 +17,7 @@ #define APPINSTALLER_CLI_ERROR_FACILITY 0xA15 +// Changes to any of these errors require the corresponding resource string in winget.resw to be updated. #define APPINSTALLER_CLI_ERROR_INTERNAL_ERROR ((HRESULT)0x8A150001) #define APPINSTALLER_CLI_ERROR_INVALID_CL_ARGUMENTS ((HRESULT)0x8A150002) #define APPINSTALLER_CLI_ERROR_COMMAND_FAILED ((HRESULT)0x8A150003) From a35b2859902db293233976bde0d5b0dcc1f2738a Mon Sep 17 00:00:00 2001 From: --global Date: Fri, 1 Dec 2023 16:50:45 -0800 Subject: [PATCH 19/23] save work --- src/AppInstallerCLICore/Command.cpp | 2 +- src/AppInstallerCLICore/ExecutionContext.cpp | 4 ++-- src/AppInstallerCLICore/Workflows/ResumeFlow.cpp | 4 +--- .../Shared/Strings/en-us/winget.resw | 6 +++--- .../Public/AppInstallerRuntime.h | 2 ++ src/AppInstallerCommonCore/Public/winget/Reboot.h | 10 +++++----- src/AppInstallerCommonCore/Reboot.cpp | 13 +++++++------ src/AppInstallerCommonCore/Runtime.cpp | 3 +++ 8 files changed, 24 insertions(+), 20 deletions(-) diff --git a/src/AppInstallerCLICore/Command.cpp b/src/AppInstallerCLICore/Command.cpp index 5a324dd1b5..8983ae80ae 100644 --- a/src/AppInstallerCLICore/Command.cpp +++ b/src/AppInstallerCLICore/Command.cpp @@ -886,7 +886,7 @@ namespace AppInstaller::CLI { // RegisterResume context flag assumes we already wrote to the RunOnce registry. // Since we are about to initiate a restart, this is no longer needed as a safety net. - Reboot::UnregisterApplicationForReboot(); + Reboot::UnregisterRestartForWER(); context.ClearFlags(Execution::ContextFlag::RegisterResume); } diff --git a/src/AppInstallerCLICore/ExecutionContext.cpp b/src/AppInstallerCLICore/ExecutionContext.cpp index 87e83b2e6a..cb75fab2eb 100644 --- a/src/AppInstallerCLICore/ExecutionContext.cpp +++ b/src/AppInstallerCLICore/ExecutionContext.cpp @@ -295,7 +295,7 @@ namespace AppInstaller::CLI::Execution if (m_checkpointManager && (!IsTerminated() || ShouldRemoveCheckpointDatabase(GetTerminationHR()))) { m_checkpointManager->CleanUpDatabase(); - AppInstaller::Reboot::UnregisterApplicationForReboot(); + AppInstaller::Reboot::UnregisterRestartForWER(); } } @@ -490,7 +490,7 @@ namespace AppInstaller::CLI::Execution m_checkpointManager->CreateAutomaticCheckpoint(*this); // Register for restart only when we first call checkpoint to support restarting from an unexpected shutdown. - AppInstaller::Reboot::RegisterApplicationForReboot("resume -g " + GetResumeId()); + AppInstaller::Reboot::RegisterRestartForWER("resume -g " + GetResumeId()); } // TODO: Capture context data for checkpoint. diff --git a/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp b/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp index adb3fcc0bf..45d2d3a8de 100644 --- a/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp @@ -1,7 +1,6 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. #include "pch.h" -#include #include "ResumeFlow.h" #include "winget/Reboot.h" @@ -34,8 +33,7 @@ namespace AppInstaller::CLI::Workflow // RunOnce registry value must start with the full path of the executable. std::string commandLine = Utility::ConvertToUTF8(argv.get()[0]) + " resume -g " + context.GetResumeId(); - bool isAdmin = AppInstaller::Runtime::IsRunningAsAdmin(); - Reboot::WriteToRunOnceRegistry(commandLine, isAdmin); + Reboot::WriteToRunOnceRegistry(commandLine); } } } diff --git a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw index c94b9aded5..5004c355b4 100644 --- a/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw +++ b/src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw @@ -2632,10 +2632,10 @@ Please specify one of them using the --source option to proceed. Failed to initiate a reboot. - Resume operation exceeds the allowable limit of {0} resume(s). To manually resume, run the command: - {Locked="{0}"} {0} is a placeholder that is replaced by an integer number of the number of allowed resumes. + Resume operation exceeds the allowable limit of {0} resume(s). To manually resume, run the command `{1}`. + {Locked="{0}", "{1}"} {0} is a placeholder that is replaced by an integer number of the number of allowed resumes. {1} is a placeholder for the command to run to perform a manual resume. - Ignore the max limit of resuming a saved state + Ignore the limit on resuming a saved state \ No newline at end of file diff --git a/src/AppInstallerCommonCore/Public/AppInstallerRuntime.h b/src/AppInstallerCommonCore/Public/AppInstallerRuntime.h index 5dfd4f2567..c388a3ba24 100644 --- a/src/AppInstallerCommonCore/Public/AppInstallerRuntime.h +++ b/src/AppInstallerCommonCore/Public/AppInstallerRuntime.h @@ -51,6 +51,8 @@ namespace AppInstaller::Runtime UserProfileDownloads, // The location where checkpoints are stored. CheckpointsLocation, + // The location of the CLI executable file. + CLIExecutable, // Always one more than the last path; for being able to iterate paths in tests. Max }; diff --git a/src/AppInstallerCommonCore/Public/winget/Reboot.h b/src/AppInstallerCommonCore/Public/winget/Reboot.h index 2f976fdac3..7f6550134d 100644 --- a/src/AppInstallerCommonCore/Public/winget/Reboot.h +++ b/src/AppInstallerCommonCore/Public/winget/Reboot.h @@ -6,12 +6,12 @@ namespace AppInstaller::Reboot { bool InitiateReboot(); - // Registers the application to be restarted by Windows Error Reporting in case of an unexpected shutdown. - bool RegisterApplicationForReboot(const std::string& commandLineArgs); + // Registers the application to be restarted by Windows Error Reporting (WER) in case of an unexpected shutdown. + bool RegisterRestartForWER(const std::string& commandLineArgs); - // Unregisters the application from being restarted by Windows Error Reporting. - bool UnregisterApplicationForReboot(); + // Unregisters the application from being restarted by Windows Error Reporting (WER). + bool UnregisterRestartForWER(); // Runs a program when a user logs on. - void WriteToRunOnceRegistry(const std::string& commandLine, bool isAdmin); + void WriteToRunOnceRegistry(const std::string& commandLine); } \ No newline at end of file diff --git a/src/AppInstallerCommonCore/Reboot.cpp b/src/AppInstallerCommonCore/Reboot.cpp index 3e5ec172b5..97209424bd 100644 --- a/src/AppInstallerCommonCore/Reboot.cpp +++ b/src/AppInstallerCommonCore/Reboot.cpp @@ -5,6 +5,7 @@ #include "AppInstallerStrings.h" #include "Public/winget/Reboot.h" #include "Public/winget/Registry.h" +#include #include using namespace AppInstaller::Registry; @@ -71,7 +72,7 @@ namespace AppInstaller::Reboot return ExitWindowsEx(EWX_RESTARTAPPS, SHTDN_REASON_MINOR_INSTALLATION); } - bool RegisterApplicationForReboot(const std::string& commandLineArgs) + bool RegisterRestartForWER(const std::string& commandLineArgs) { #ifndef AICLI_DISABLE_TEST_HOOKS if (s_RegisterForRestartResult_TestHook_Override) @@ -81,7 +82,6 @@ namespace AppInstaller::Reboot #endif HRESULT result = RegisterApplicationRestart(AppInstaller::Utility::ConvertToUTF16(commandLineArgs).c_str(), 0 /* Always restart process */); - AICLI_LOG(CLI, Info, << "Register for restart with command line args: " << commandLineArgs); if (FAILED(result)) { @@ -90,11 +90,12 @@ namespace AppInstaller::Reboot } else { + AICLI_LOG(CLI, Info, << "Register for restart with command line args: " << commandLineArgs); return true; } } - bool UnregisterApplicationForReboot() + bool UnregisterRestartForWER() { HRESULT result = UnregisterApplicationRestart(); AICLI_LOG(CLI, Info, << "Application unregistered for restart."); @@ -110,15 +111,15 @@ namespace AppInstaller::Reboot } } - void WriteToRunOnceRegistry(const std::string& commandLine, bool isAdmin) + void WriteToRunOnceRegistry(const std::string& commandLine) { THROW_HR_IF(E_UNEXPECTED, commandLine.size() > MAX_PATH); - HKEY root = isAdmin ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER; + HKEY root = AppInstaller::Runtime::IsRunningAsAdmin() ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER; std::wstring subKey = std::wstring{ s_RunOnceRegistry }; Key key = Key::OpenIfExists(root, subKey, 0, KEY_ALL_ACCESS); - if (key == NULL) + if (!key) { key = Key::Create(root, subKey); } diff --git a/src/AppInstallerCommonCore/Runtime.cpp b/src/AppInstallerCommonCore/Runtime.cpp index 9af6f62d3f..455d0df651 100644 --- a/src/AppInstallerCommonCore/Runtime.cpp +++ b/src/AppInstallerCommonCore/Runtime.cpp @@ -41,6 +41,7 @@ namespace AppInstaller::Runtime constexpr std::string_view s_UserProfileEnvironmentVariable = "%USERPROFILE%"; constexpr std::string_view s_LocalAppDataEnvironmentVariable = "%LOCALAPPDATA%"; + constexpr std::string_view s_WindowsApps_Base = "Microsoft\\WindowsApps"sv; static std::optional s_runtimePathStateName; static wil::srwlock s_runtimePathStateNameLock; @@ -472,6 +473,7 @@ namespace AppInstaller::Runtime result = GetPathDetailsForPackagedContext(PathName::LocalState, forDisplay); result.Path /= s_CheckpointsDirectory; break; + // case PathName::CLIExecutable: default: THROW_HR(E_UNEXPECTED); } @@ -562,6 +564,7 @@ namespace AppInstaller::Runtime result = GetPathDetailsForUnpackagedContext(PathName::LocalState, forDisplay); result.Path /= s_CheckpointsDirectory; break; + // case PathName::CLIExecutable: default: THROW_HR(E_UNEXPECTED); } From e553fe2d9df9b2a2aa0dba02e9d9b0560aee2ab3 Mon Sep 17 00:00:00 2001 From: --global Date: Mon, 4 Dec 2023 18:39:32 -0800 Subject: [PATCH 20/23] address PR comments --- src/AppInstallerCLICore/Command.cpp | 1 + .../Workflows/ResumeFlow.cpp | 11 +++++----- .../Public/winget/Reboot.h | 2 +- src/AppInstallerCommonCore/Reboot.cpp | 4 ++-- src/AppInstallerCommonCore/Runtime.cpp | 22 +++++++++++++++++-- 5 files changed, 29 insertions(+), 11 deletions(-) diff --git a/src/AppInstallerCLICore/Command.cpp b/src/AppInstallerCLICore/Command.cpp index 8983ae80ae..6641a4bcb1 100644 --- a/src/AppInstallerCLICore/Command.cpp +++ b/src/AppInstallerCLICore/Command.cpp @@ -871,6 +871,7 @@ namespace AppInstaller::CLI ExecuteInternal(context); } + // NOTE: Reboot logic will still run even if the context is terminated (not including unhandled exceptions). if (Settings::ExperimentalFeature::IsEnabled(Settings::ExperimentalFeature::Feature::Reboot) && context.Args.Contains(Execution::Args::Type::AllowReboot) && WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::RebootRequired)) diff --git a/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp b/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp index 45d2d3a8de..c79a3002ef 100644 --- a/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp +++ b/src/AppInstallerCLICore/Workflows/ResumeFlow.cpp @@ -3,6 +3,7 @@ #include "pch.h" #include "ResumeFlow.h" #include "winget/Reboot.h" +#include namespace AppInstaller::CLI::Workflow { @@ -26,14 +27,12 @@ namespace AppInstaller::CLI::Workflow if (WI_IsFlagSet(context.GetFlags(), Execution::ContextFlag::RegisterResume)) { - int argc = 0; - wil::unique_hlocal_ptr argv{ CommandLineToArgvW(GetCommandLineW(), &argc) }; - THROW_LAST_ERROR_IF_NULL(argv); + auto executablePath = AppInstaller::Runtime::GetPathTo(AppInstaller::Runtime::PathName::CLIExecutable); // RunOnce registry value must start with the full path of the executable. - std::string commandLine = Utility::ConvertToUTF8(argv.get()[0]) + " resume -g " + context.GetResumeId(); - - Reboot::WriteToRunOnceRegistry(commandLine); + const auto& resumeId = context.GetResumeId(); + std::string commandLine = executablePath.u8string() + " resume -g " + resumeId; + Reboot::WriteToRunOnceRegistry(resumeId, commandLine); } } } diff --git a/src/AppInstallerCommonCore/Public/winget/Reboot.h b/src/AppInstallerCommonCore/Public/winget/Reboot.h index 7f6550134d..0abbd60e59 100644 --- a/src/AppInstallerCommonCore/Public/winget/Reboot.h +++ b/src/AppInstallerCommonCore/Public/winget/Reboot.h @@ -13,5 +13,5 @@ namespace AppInstaller::Reboot bool UnregisterRestartForWER(); // Runs a program when a user logs on. - void WriteToRunOnceRegistry(const std::string& commandLine); + void WriteToRunOnceRegistry(const std::string& resumeId, const std::string& commandLine); } \ No newline at end of file diff --git a/src/AppInstallerCommonCore/Reboot.cpp b/src/AppInstallerCommonCore/Reboot.cpp index 97209424bd..bc2d8d0b7c 100644 --- a/src/AppInstallerCommonCore/Reboot.cpp +++ b/src/AppInstallerCommonCore/Reboot.cpp @@ -111,7 +111,7 @@ namespace AppInstaller::Reboot } } - void WriteToRunOnceRegistry(const std::string& commandLine) + void WriteToRunOnceRegistry(const std::string& resumeId, const std::string& commandLine) { THROW_HR_IF(E_UNEXPECTED, commandLine.size() > MAX_PATH); @@ -124,7 +124,7 @@ namespace AppInstaller::Reboot key = Key::Create(root, subKey); } - key.SetValue(L"WingetResume", Utility::ConvertToUTF16(commandLine), REG_SZ); + key.SetValue(Utility::ConvertToUTF16("WingetResume-" + resumeId), Utility::ConvertToUTF16(commandLine), REG_SZ); AICLI_LOG(CLI, Info, << "Set RunOnce registry with value: " << commandLine); } } diff --git a/src/AppInstallerCommonCore/Runtime.cpp b/src/AppInstallerCommonCore/Runtime.cpp index 455d0df651..48594d0255 100644 --- a/src/AppInstallerCommonCore/Runtime.cpp +++ b/src/AppInstallerCommonCore/Runtime.cpp @@ -42,6 +42,8 @@ namespace AppInstaller::Runtime constexpr std::string_view s_UserProfileEnvironmentVariable = "%USERPROFILE%"; constexpr std::string_view s_LocalAppDataEnvironmentVariable = "%LOCALAPPDATA%"; constexpr std::string_view s_WindowsApps_Base = "Microsoft\\WindowsApps"sv; + constexpr std::string_view s_WinGetDev_Exe = "wingetdev.exe"; + constexpr std::string_view s_WinGet_Exe = "winget.exe"; static std::optional s_runtimePathStateName; static wil::srwlock s_runtimePathStateNameLock; @@ -473,7 +475,19 @@ namespace AppInstaller::Runtime result = GetPathDetailsForPackagedContext(PathName::LocalState, forDisplay); result.Path /= s_CheckpointsDirectory; break; - // case PathName::CLIExecutable: + case PathName::CLIExecutable: + result.Path = GetKnownFolderPath(FOLDERID_LocalAppData); + result.Path /= s_WindowsApps_Base; + if (IsReleaseBuild()) + { + result.Path /= s_WinGet_Exe; + } + else + { + result.Path /= s_WinGetDev_Exe; + } + result.Create = false; + break; default: THROW_HR(E_UNEXPECTED); } @@ -564,7 +578,11 @@ namespace AppInstaller::Runtime result = GetPathDetailsForUnpackagedContext(PathName::LocalState, forDisplay); result.Path /= s_CheckpointsDirectory; break; - // case PathName::CLIExecutable: + case PathName::CLIExecutable: + result.Path = GetBinaryDirectoryPath(); + result.Path /= s_WinGet_Exe; + result.Create = false; + break; default: THROW_HR(E_UNEXPECTED); } From 661432e951feb79d55c0742678b63ceb9e6127d5 Mon Sep 17 00:00:00 2001 From: --global Date: Mon, 4 Dec 2023 22:07:58 -0800 Subject: [PATCH 21/23] fix unit test --- src/AppInstallerCommonCore/Runtime.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/AppInstallerCommonCore/Runtime.cpp b/src/AppInstallerCommonCore/Runtime.cpp index 48594d0255..237f22b837 100644 --- a/src/AppInstallerCommonCore/Runtime.cpp +++ b/src/AppInstallerCommonCore/Runtime.cpp @@ -487,6 +487,7 @@ namespace AppInstaller::Runtime result.Path /= s_WinGetDev_Exe; } result.Create = false; + mayBeInProfilePath = true; break; default: THROW_HR(E_UNEXPECTED); From c847dc5aff33c22308541e35fac3aa46e52bb0b4 Mon Sep 17 00:00:00 2001 From: --global Date: Tue, 5 Dec 2023 10:05:30 -0800 Subject: [PATCH 22/23] fix path --- src/AppInstallerCommonCore/Runtime.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/AppInstallerCommonCore/Runtime.cpp b/src/AppInstallerCommonCore/Runtime.cpp index 237f22b837..7d67c3e9a9 100644 --- a/src/AppInstallerCommonCore/Runtime.cpp +++ b/src/AppInstallerCommonCore/Runtime.cpp @@ -572,18 +572,18 @@ namespace AppInstaller::Runtime result = GetPathDetailsCommon(path, forDisplay); break; case PathName::SelfPackageRoot: + case PathName::CLIExecutable: result.Path = GetBinaryDirectoryPath(); result.Create = false; + if (path == PathName::CLIExecutable) + { + result.Path /= s_WinGet_Exe; + } break; case PathName::CheckpointsLocation: result = GetPathDetailsForUnpackagedContext(PathName::LocalState, forDisplay); result.Path /= s_CheckpointsDirectory; break; - case PathName::CLIExecutable: - result.Path = GetBinaryDirectoryPath(); - result.Path /= s_WinGet_Exe; - result.Create = false; - break; default: THROW_HR(E_UNEXPECTED); } From 7f860f4bc50db73fc514c5f8d12f590bef9db07f Mon Sep 17 00:00:00 2001 From: --global Date: Tue, 5 Dec 2023 20:00:57 -0800 Subject: [PATCH 23/23] use packagefamily name in path --- src/AppInstallerCommonCore/Runtime.cpp | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/AppInstallerCommonCore/Runtime.cpp b/src/AppInstallerCommonCore/Runtime.cpp index 7d67c3e9a9..26d3de8359 100644 --- a/src/AppInstallerCommonCore/Runtime.cpp +++ b/src/AppInstallerCommonCore/Runtime.cpp @@ -478,14 +478,12 @@ namespace AppInstaller::Runtime case PathName::CLIExecutable: result.Path = GetKnownFolderPath(FOLDERID_LocalAppData); result.Path /= s_WindowsApps_Base; - if (IsReleaseBuild()) - { - result.Path /= s_WinGet_Exe; - } - else - { - result.Path /= s_WinGetDev_Exe; - } + result.Path /= GetPackageFamilyName(); +#if USE_PROD_CLSIDS + result.Path /= s_WinGet_Exe; +#else + result.Path /= s_WinGetDev_Exe; +#endif result.Create = false; mayBeInProfilePath = true; break;