Add experimental feature for initiating reboot for single package installs #3631
Conversation
| @@ -730,6 +781,11 @@ namespace AppInstaller::CLI::Workflow | |||
| } | |||
| } | |||
|
|
|||
| if (shouldReboot) | |||
| { | |||
| InitiateRebootIfApplicable(context, true); | |||
There was a problem hiding this comment.
Maybe we should be setting a flag on the context rather than initiating a reboot at arbitrary locations in the workflow. We could then ensure that the reboot happens at the end the workflow, rather than in the middle. It would also make it easier to ensure that we know exactly what can initiate a reboot (maybe we want to forever block it from COM).
There was a problem hiding this comment.
Changed to setting a context flag so that indicates whether we should initiate a reboot so that it only takes place at the end of all the install workflows and is contained inside a single workflow function.
| <value>Allows a reboot if applicable</value> | ||
| </data> | ||
| <data name="InitiatingReboot" xml:space="preserve"> | ||
| <value>Initiating reboot to complete installation...</value> |
There was a problem hiding this comment.
This is not quite true for APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL until you hook up resume. I assume that is the plan: _TO_INSTALL reboots with a resume but _TO_FINISH reboots without a resume?
Side thought, we might already have an issue with _TO_FINISH results being treated as an error and thus not recorded properly. They should probably be treated as a success for post-install purposes.
There was a problem hiding this comment.
Made changes to ReportInstallerResult so that APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_FINISH is treated as a success and no longer reported as a failed installer result.
Also added a context flag that indicates whether we should register for a restart if the return code is APPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL for resume
| @@ -1204,3 +1204,68 @@ TEST_CASE("InstallFlow_InstallAcquiresLock", "[InstallFlow][workflow]") | |||
| REQUIRE(installResultStr.find("/custom") != std::string::npos); | |||
| REQUIRE(installResultStr.find("/silentwithprogress") != std::string::npos); | |||
| } | |||
|
|
|||
| TEST_CASE("InstallFlow_InstallMultipleWithReboot", "[InstallFlow][workflow][MultiQuery][reboot]") | |||
There was a problem hiding this comment.
Not a huge fan of a test that, if it "fails", reboots my machine...
Can we also have an override that replaces the reboot operation with a callback and you can just set a bool? That would also allow for tests that actually "reboot".
There was a problem hiding this comment.
Changed to suggested and added tests that verifies if a reboot actually succeeds
src/AppInstallerCLITests/TestData/InstallFlowTest_ExpectedReturnCodes.yaml
Show resolved
Hide resolved
Related to: #3165
This PR adds support for the
rebootexperimental feature. This only applies to single package and multiple package installs. This does not apply to any package dependencies that may require a restart.Changes:
AllowRebootargument flag. When provided, if the return code of an installer is eitherAPPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_FINISHorAPPINSTALLER_CLI_ERROR_INSTALL_REBOOT_REQUIRED_TO_INSTALL, winget will automatically begin restarting your machine so that the installation can be completed.Tests:
Microsoft Reviewers: Open in CodeFlow