Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

All cmd shims created for Invoke-Build have a mistake #339

Closed
nightroman opened this issue Apr 17, 2015 · 8 comments
Closed

All cmd shims created for Invoke-Build have a mistake #339

nightroman opened this issue Apr 17, 2015 · 8 comments

Comments

@nightroman
Copy link
Contributor

The problem may be common for all shims created for PowerShell scripts. But I
have noticed it for Invoke-Build, so I explain it just for Invoke-Build.

Let's take a look at invoke-build.cmd

@powershell -noprofile -ex unrestricted "& '...\scoop\apps\invoke-build\2.9.14\Invoke-Build.ps1' %*;exit $lastexitcode"

For Invoke-Build the correct shim would be

@powershell -noprofile -ex unrestricted "& '...\scoop\apps\invoke-build\2.9.14\Invoke-Build.ps1' %*"

Because when IB fails it throws a terminating error and PowerShell in this case
exists with code 1. If IB works then PowerShell exits with code 0 itself. This
is a normal scenario.

But the extra part exit $lastexitcode is a mistake and it may break things.

Same as above, if IB fails it throws a terminating error. But this means that
the next statement exit $lastexitcode is not called at all. So it is not
needed.

The case when IB succeeds is more interesting. The automatic variable
$lastexitcode is changed when a native application is called.

If no native command is called this variable is not even defined. In some cases
this will be treated as null and converted to 0. In other cases it may cause a
failure. Try to run this script in a fresh PowerShell session:

Set-StrictMode -Version 2
exit $LASTEXITCODE

It fails: The variable '$LASTEXITCODE' cannot be retrieved because it has not been set.
So that if a build script actually sets Set-StrictMode -Version 2 and calls
no native commands then it always fails due to the shim exit $LASTEXITCODE.

Then, if a native command was called then its exit code tells nothing about IB
success or failure. Suppose a build script invoked by IB is

task copy {
    exec {robocopy source destination /mir} (0..3)
}

robocopy codes 0..3 are for success. Note that the function exec is told to
take this into account and to not fail on 0..3 codes. But PowerShell still sets
$lastexitcode to one of this values when robocopy is called. And then the
shim code uses it for exit. So in spite of the fact that IB succeeds the shim
exists with non zero code. This is the mistake.

As the author of Invoke-Build, I am glad and proud it is noticed by scoop.
Of course I would like Invoke-Build to be installed properly.

@nightroman
Copy link
Contributor Author

From the top of my head, I would simply remove exit $lastexitcode from PowerShell cmd shims. Leave this to scripts. It's their responsibility to care of the exit code if this matters. They have plenty of ways

  • Write an error or throw for exit code 1.
  • Work normally for exit code 0.
  • Use exit X to exit with X.
  • Use $host.SetShouldExit(X).

@lukesampson
Copy link
Member

Hi Roman, thanks for looking at this.

I've just done some tests to try to confirm the behaviour you're talking about, and the potential problems didn't actually cause problems in practice, whereas removing the exit $exitcode did reduce the functionality.

There are a couple of things to point out:

  • The .cmd shim should only be invoked from DOS sessions. Powershell sessions should favour the .ps1 shim and invoke that instead. The .ps1 shim doesn't do anything with exit codes, as they'll be available anyway if they're needed.
  • The exit $lastexitcode is for powershell scripts that want to mimic the old school exit code convention, so if the target script follows PS conventions like Invoke-Build does, then it doesn't really apply.
  • We're not trying to get exact 0=success, non-zero=failure results from the .cmd shim, only to transfer whatever exit code remained in the powershell session back to the DOS session.

If I remove exit $lastexitcode from the .cmd shim, and the .ps1 script that is the target script for the shim calls exit X, then this exit code isn't available from the user scope. The exit code isn't passed from the powershell session back to the DOS session.

If a native app like robocopy changes the exit code inside a PS target script I think that's ok—if the target script hasn't reset the exit code then it's unlikely that anything calling the target script will be looking for an exit code.

Finally, even if the target powershell script sets strict mode, I don't think this affects the calling scope where the exit $lastexitcode is—so there is no error, and the shim will just exit with 0.

@nightroman
Copy link
Contributor Author

Mixing PowerShell and CMD is pain at times. Nevertheless, it would be useful to
investigate and come to a correct approach. I made three tests.

They show that in PowerShell exit X does not exactly gets X. But
SetShouldExit(5) does. So there is a way to set the exit code exactly.
The third test shows that this correctly set 5 is eliminated by exit $LASTEXITCODE.

exit 5 results in 1, no exit $LASTEXITCODE - not exactly OK

exit-5.ps1

'hello 5'
exit 5

exit-5.ps1.cmd

powershell -nop "& .\exit-5.ps1"

test-exit-5.ps1.cmd

call exit-5.ps1.cmd
echo %errorlevel%

SetShouldExit(5) results in 5, no exit $LASTEXITCODE - OK

exit-5.ps1

'hello 5'
$host.SetShouldExit(5)

exit-5.ps1.cmd

powershell -nop "& .\exit-5.ps1"

test-exit-5.ps1.cmd

call exit-5.ps1.cmd
echo %errorlevel%

SetShouldExit(5) results in 0 with exit $LASTEXITCODE - not OK

exit-5.ps1

'hello 5'
$host.SetShouldExit(5)

exit-5.ps1.cmd

powershell -nop "& .\exit-5.ps1; exit $lastexitcode"

test-exit-5.ps1.cmd

call exit-5.ps1.cmd
echo %errorlevel%

@lukesampson
Copy link
Member

Sorry, I'm not really following why we're talking about $host.setShouldExit(x)—from what I can tell this is a request to terminate the host runspace, not to return an exit code for a script.

We want people to be able to write powershell scripts without needing to worry about whether they're executing in the context of a scoop shim or not, so calling exit x is what needs to work I think. And in this case, the exit $lastexitcode seems to work fine.

Am I missing something here?

@nightroman
Copy link
Contributor Author

Probably we are talking about different scenarios. Let's forget then about $host.setShouldExit(x). Personally, I never use it.

I just explained how exit $lastexitcode may get incorrect result for Invoke-Build (read some PowerShell scripts). It is not a problem for me, I am not going to use cmd shims. But some people may use them and rely on their exit codes. They may be incorrect, namely, not zero on successful calls.

@nightroman
Copy link
Contributor Author

If a native app like robocopy changes the exit code inside a PS target script I think that's ok—if the target script hasn't reset the exit code then it's unlikely that anything calling the target script will be looking for an exit code.

It's not ok. The last exit code may be not zero but script works just fine (i.e. with robocopy). I do not think many scripts care of resetting exit codes. I am not sure what it is. What do you mean?

@nightroman
Copy link
Contributor Author

P.S. I do not want to disturb too much. If you think all is fine for designed
scenarios then just close the issue. As I said, for me the issue is academic.
But it was useful to think of this once again. In particular I have found one
more PowerShell oddity.

@lukesampson
Copy link
Member

Hi Roman, thanks again for looking at this. I will close it for now, but if we can find a case where it's causing problems then we can re-open it again. It's an interesting behaviour that you've found—thanks for pointing it out.

With the robocopy thing, I just meant that if a target script that calls robocopy doesn't care about setting it's own exit code, then it's ok if we don't clean that up. We just want to preserve whatever exit code the script finished with, so that it's transparent to the caller that the script executed in a completely different shell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants