Update ComputerUnix.cs to fix Restart- and Stop-Computer on Linux/Mac #19780

mark-wilson-spc · 2023-06-10T22:35:42Z

Fix Restart-Computer on Linux and Stop-Computer on MacOS by modifying RunCommand to respect command and args inputs.

PR Summary

Fix Restart-Computer on Linux systems and Stop-Computer on MacOS systems.

See issues #14684 and #19738

Made minimal changes necessary to correct behavior.

I am new at contributing to projects and have not done a build or testing. Perhaps someone in the Community or on the PowerShell team can validate functionality.

PR Context

Restart-Computer on Linux has been shutting down systems instead of restarting them..
Stop-Computer on MacOS has been incorrectly throwing errors due to missing arguments.

PR Checklist

… Mac Fix RunCommand to respect 'command' and 'args' inputs.

Can't use string.empty as a default parameter value. Changed to "".

iSazonov · 2023-06-12T15:49:53Z

src/Microsoft.PowerShell.Commands.Management/commands/management/ComputerUnix.cs

        /// Run a command.
        /// </summary>
-        protected void RunCommand(String command, String args) {
+        protected void RunCommand(String command = "/sbin/shutdown", String args = "") {


Why do we need the defaults?

Suggested change

protected void RunCommand(String command = "/sbin/shutdown", String args = "") {

protected void RunCommand(string command, string args) {

Please replace String with string.

Note that the args string for macOS needs changing from "now" to "-h now", but note the concerns raised in #19738 (comment)

I suggest don't complicate the PR and only fix the code with right shutdown for Linux and Mac. Then open new issue to enhance the cmdlet to support Force (and grace shutdown) on Unix.

Good point, @iSazonov - I've opened:

Stop-Computer, Restart-Computer on macOS: support graceful shutdown by default, implement -Force support #19791

@iSazonov defaults have been removed. I had put them in there just because those were the previous hardcoded values. Changed String to string per your request.

@mark-wilson-spc, note that the restart fix would work for macOS too, but shutdown would still be broken, if "now" isn't changed to "-h now".

My personal preference is to include the latter change, and then deal with the following aspects separately:

graceful shutdown/reboot on macOS: Stop-Computer, Restart-Computer on macOS: support graceful shutdown by default, implement -Force support #19791

inability to programmatically detect failure: Stop-Computer, Restart-Computer on Unix: inability to detect failure. #19802

Given my discussion of the tests above: Is it possible that the macOS test failure you saw was transient, unrelated to your changes? If so, including the "-h now" too shouldn't be a problem.

Thank you for the clarification, @mklement0 . My thought was that we could go ahead and push out the working fix (Restart-Computer for Linux and MacOs) immediately since it is ready to go and has a broad benefit, and then address the thornier shutdown issue separately.

As far as the testing goes, I am new at this and am not in a position to speak to the results of the automated testing (especially where MacOS is concerned). I had assumed all tests were valid and robust, and that it was necessary to pass all tests for merge approval. I'm not sure who can best speak to your question regarding the possible transient or false nature of a test result.

Thanks, @mark-wilson-spc - I'll have to defer to others too with respect to the tests.

As for whether to hold off on the macOS "-h now" fix to also make Stop-Computer work:

As noted, even without the fix, Restart-Computer will start to work on macOS, because -r now works on macOS too.

If we proceed without the shutdown fix for macOS, we'll have a somewhat working Restart-Computer there - one that forces instant shutdown with potential data loss - and a still broken Stop-Computer.

If we include the shutdown fix for macOS we'll have somewhat working Restart-Computer and Stop-Computer cmdlets on macOS, with the caveats noted.

If either approach gets into a stable release before the graceful approach is implemented on macOS (I'm not holding my breath), we'll have a backward-compatibility problem on our hand: users may by then have come to rely on the forceful and therefore guaranteed shutdown/reboot, even without having specified -Force.

Therefore, the most conservative approach would be: fix the problem for Linux only for now (which already had a working Stop-Computer, but a broken Restart-Computer), which would require deliberately setting the arguments to pass to shutdown to something invalid for Restart-Computer too.

I hope that lays out the options clearly, and I'll leave the decision to others.

In any case we can not really stop/reboot CI system and have to use test mosc/hook. So we must check the cmdlets manually before merge.

As for code, I suggest to implement "-h now" for macOs as @mklement0 proposed. As result after the PR we should get the cmdlets working well on Linux and macOS but without (1) grace scenario, (2) error processing - both are for follow PRs.

I am working on an implementation and should have it ready later today.

iSazonov

@mklement0 Could you please check the build on Mac and Linux?

mklement0 · 2023-06-12T19:01:23Z

@iSazonov, unfortunately, I'm not set up for building on Linux.
As for macOS: see my comment above and therefore also #19738 (comment)

Remove default parameter assignments and change 'String' to 'string'.

Change MacOs 'Stop-Computer' args from 'now' to '-h now' per PR directive.

mklement0 · 2023-06-14T15:28:31Z

src/Microsoft.PowerShell.Commands.Management/commands/management/ComputerUnix.cs

        /// Run a command.
        /// </summary>
-        protected void RunCommand(String command, String args) {
+        protected void RunCommand(string command, string args) {


The way /sbin/shutdown is invoked makes it impossible to detect failure on the PowerShell side, though that's probably for a separate PR; see

Stop-Computer, Restart-Computer on Unix: inability to detect failure. #19802

Added handling for MacOS and Unix Force and NoForce restarts and shutdowns.

iSazonov · 2023-06-16T03:39:40Z

src/Microsoft.PowerShell.Commands.Management/commands/management/ComputerUnix.cs

+        /// Force the operation to take place if possible.
+        /// </summary>
+        [Parameter]
+        public SwitchParameter Force { get; set; } = false;


No need to init to default.

Suggested change

public SwitchParameter Force { get; set; } = false;

public SwitchParameter Force { get; set; }

I will remove it, but just an fyi I used Computer.cs as my style template and it is defaulted there.

Implemented.

Removing the default assignment false on force params may have caused validation tests to fail. Re-added as a test.

No, it doesn't affect the tests.

iSazonov · 2023-06-16T03:41:55Z

src/Microsoft.PowerShell.Commands.Management/commands/management/ComputerUnix.cs

+            string unixRestartCommand = "/sbin/shutdown";
+            string unixRestartArgs = "-r now";
+
+            string macOSRestartCommand = "osascript";
+            string macOSRestartArgs = @"-e 'tell application ""System Events"" to restart'";
+
+            string macOSForceRestartCommand = "/sbin/shutdown";
+            string macOSForceRestartArgs = "-r now";            


Please use string const for all.

Implemented.

…switch default value Implement requested changes from ISazonov. Change string to string const Remove unnecessary default value of false on Force parameter declarations.

Removing the assignment seems to have caused validation testing to fail.

iSazonov · 2023-06-16T07:09:15Z

src/Microsoft.PowerShell.Commands.Management/commands/management/ComputerUnix.cs

        /// </summary>
        protected override void BeginProcessing()
        {
+            string const unixRestartCommand = "/sbin/shutdown";


Build failed on the wrong syntax.

iSazonov · 2023-06-16T16:59:15Z

@mark-wilson-spc Please sign CLA (see instructions above) if you agree. We can not merge without CLA.

iSazonov · 2023-06-16T17:05:20Z

@mklement0 Please review last build.
We should also implement -WhatIf] / -Confirm support. I hope osascript doesn't ask user?

mark-wilson-spc · 2023-06-16T20:46:39Z

@microsoft-github-policy-service agree company="SierraSpace.com"

ghost · 2023-06-24T02:00:54Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

JamesWTruher

I like this (I think we could add sleep too - in a separate PR).
I just have 1 question and a concern about more validation.

JamesWTruher · 2023-07-17T16:02:26Z

src/Microsoft.PowerShell.Commands.Management/commands/management/ComputerUnix.cs

+                command = unixRestartCommand;
+                args = unixRestartArgs;
+            }
+


now that you've changed the way shutdown is being called (and adding the more gentle osascript approach, I would be happier if we boosted the tests here, at least to have validation between force and not-force on mac

In CI, a test hook will have to be needed to verify intended behavior

JamesWTruher · 2023-07-17T16:07:31Z

src/Microsoft.PowerShell.Commands.Management/commands/management/ComputerUnix.cs

+            const string unixRestartArgs = "-r now";
+
+            const string macOSRestartCommand = "osascript";
+            const string macOSRestartArgs = @"-e 'tell application ""System Events"" to restart'";


if this is the more gentle approach, would it be a good idea to pop UI for confirmation?

ShouldProcess might be appropriate.

microsoft-github-policy-service · 2023-08-01T20:10:26Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment.

microsoft-github-policy-service · 2023-08-01T20:10:38Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment.

microsoft-github-policy-service · 2023-08-01T20:11:27Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment.

microsoft-github-policy-service · 2023-08-01T20:11:41Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment.

microsoft-github-policy-service · 2023-08-01T20:11:48Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment.

pull-request-quantifier-deprecated · 2023-12-20T17:25:59Z

This PR has 55 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Small
Size       : +48 -7
Percentile : 22%

Total files changed: 1

Change summary by file extension:
.cs : +48 -7

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detected.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

SteveL-MSFT · 2023-12-20T17:26:32Z

WIll see if someone on the team can finish this PR

SteveL-MSFT

I'm ok with the changes as there's an ask to continue to improve this in subsequent PRs

Update ComputerUnix.cs to fix Restart- and Stop-Computer on Linux and…

1fe8294

… Mac Fix RunCommand to respect 'command' and 'args' inputs.

mark-wilson-spc requested review from adityapatwardhan and daxian-dbw as code owners June 10, 2023 22:35

pull-request-quantifier-deprecated bot added the Extra Small label Jun 10, 2023

mark-wilson-spc mentioned this pull request Jun 10, 2023

Restart-Computer performs shutdown on Ubuntu 18.04 #14684

Open

Change RunCommand args default value to constant

1ec7d7f

Can't use string.empty as a default parameter value. Changed to "".

iSazonov reviewed Jun 12, 2023

View reviewed changes

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Jun 12, 2023

iSazonov assigned daxian-dbw Jun 12, 2023

mklement0 mentioned this pull request Jun 13, 2023

Stop-Computer, Restart-Computer on macOS: support graceful shutdown by default, implement -Force support #19791

Open

mark-wilson-spc added 2 commits June 13, 2023 09:25

Update RunCommand params per PR input from iSazinov

36f334c

Remove default parameter assignments and change 'String' to 'string'.

Update ComputerUnix.cs change MacOs Stop- args to -h now

439f210

Change MacOs 'Stop-Computer' args from 'now' to '-h now' per PR directive.

iSazonov requested a review from SteveL-MSFT June 14, 2023 05:45

mklement0 reviewed Jun 14, 2023

View reviewed changes

Update ComputerUnix.cs with MacOs and Unix force and noforce args

a721390

Added handling for MacOS and Unix Force and NoForce restarts and shutdowns.

pull-request-quantifier-deprecated bot added Small and removed Extra Small labels Jun 16, 2023

iSazonov reviewed Jun 16, 2023

View reviewed changes

mark-wilson-spc added 2 commits June 16, 2023 00:23

Update ComputerUnix.cs - add const to string declarations and remove …

fe7f66a

…switch default value Implement requested changes from ISazonov. Change string to string const Remove unnecessary default value of false on Force parameter declarations.

Update ComputerUnix.cs Restore default value FALSE to FORCE params

d2c064f

Removing the assignment seems to have caused validation testing to fail.

iSazonov reviewed Jun 16, 2023

View reviewed changes

mark-wilson-spc added 2 commits June 16, 2023 10:34

Update ComputerUnix.cs fix const string syntax error

1190c86

Update ComputerUnix.cs fix syntax error

fa1cc99

daxian-dbw mentioned this pull request Jun 19, 2023

Computer cmdlets should fail with error when not run via sudo on Unix #19824

Merged

22 tasks

ghost added the Review - Needed The PR is being reviewed label Jun 24, 2023

StevenBucher98 added the PowerShell-Docs not needed The PR was reviewed and doesn't appear to require a PowerShell Docs update label Jun 26, 2023

JamesWTruher reviewed Jul 17, 2023

View reviewed changes

daxian-dbw added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed labels Jul 17, 2023

microsoft-github-policy-service bot added the Stale label Aug 1, 2023

microsoft-github-policy-service bot closed this Aug 12, 2023

SteveL-MSFT reopened this Dec 20, 2023

microsoft-github-policy-service bot removed the Stale label Dec 20, 2023

SteveL-MSFT added the CommunityDay-Small A small PR that the PS team has identified to prioritize to review label Dec 20, 2023

microsoft-github-policy-service bot added the Stale label Jan 4, 2024

SteveL-MSFT approved these changes Jan 8, 2024

View reviewed changes

microsoft-github-policy-service bot removed the Stale label Jan 8, 2024

microsoft-github-policy-service bot added the Stale label Jan 24, 2024

microsoft-github-policy-service bot closed this Feb 3, 2024

microsoft-github-policy-service bot removed the Stale label Feb 3, 2024

	protected void RunCommand(String command = "/sbin/shutdown", String args = "") {
	protected void RunCommand(string command, string args) {

	public SwitchParameter Force { get; set; } = false;
	public SwitchParameter Force { get; set; }

Update ComputerUnix.cs to fix Restart- and Stop-Computer on Linux/Mac #19780

Update ComputerUnix.cs to fix Restart- and Stop-Computer on Linux/Mac #19780

Uh oh!

Conversation

mark-wilson-spc commented Jun 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

mklement0 commented Jun 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Jun 16, 2023

Uh oh!

iSazonov commented Jun 16, 2023

Uh oh!

mark-wilson-spc commented Jun 16, 2023

Uh oh!

ghost commented Jun 24, 2023

Uh oh!

JamesWTruher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

microsoft-github-policy-service bot commented Aug 1, 2023

Uh oh!

microsoft-github-policy-service bot commented Aug 1, 2023

Uh oh!

microsoft-github-policy-service bot commented Aug 1, 2023

Uh oh!

mark-wilson-spc commented Jun 10, 2023 •

edited

Loading