PowerShell: Fix assumption that $LASTEXITCODE is always defined

[nrusch]

#1778 added support for surfacing the exit code from aliased commands to the rez shell. However, it assumes $LASTEXITCODE will always be defined after any command is run, which is not safe.

On Windows, GUI applications will detach from their parent process at startup unless explicitly written to (re)attach. In PowerShell, this manifests as the command execution returning immediately, and $LASTEXITCODE being left unmodified, which means it may not be defined all depending on what has previously been run in the shell.

When running PowerShell with Set-StrictMode enabled, attempting to access an undefined variable is treated as an error, rather than returning 0 or $null.

Thus, launching a GUI application using a rez-env call (e.g. rez-env some_package -- some_app.exe) in a "strict" PowerShell session leads to an error like the following:

InvalidOperation: S:\Temp\rez_context_qwzoucq1\rez-shell.ps1:5
Line |
   5 |  if(! $? -or $LASTEXITCODE) {
     |              ~~~~~~~~~~~~~
     | The variable '$LASTEXITCODE' cannot be retrieved because it has not been set.

This MR fixes this particular error by ensuring $LASTEXITCODE is actually defined before trying to read it.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.30%. Comparing base (7727d35) to head (b8a5183).
⚠️ Report is 24 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1962      +/-   ##
==========================================
+ Coverage   59.28%   59.30%   +0.01%     
==========================================
  Files         126      126              
  Lines       17218    17219       +1     
  Branches     3017     3017              
==========================================
+ Hits        10208    10211       +3     
+ Misses       6325     6323       -2     
  Partials      685      685              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JeanChristopheMorinPerso]

Hi @nrusch, thanks for creating this PR. Unfortunately, it looks like your change is breaking the tests for pwsh on macOS. Can you take a look at that please? Also, do you think there would be a way to test this specific behavior? I would guess that a test would probably look something like https://github.com/AcademySoftwareFoundation/rez/blob/main/src/rez/tests/test_shells.py#L594-L608.

[nrusch]

Hey @JeanChristopheMorinPerso, I've been poking around with this, and I think there may actually be a "bug" in the whack test package itself.

Specifically, its build_command is set to python {root}/build.py {install}, but the package does not require python in any way. Compare that to all of the other packages in data/tests/builds that use python in their build_command: they all also specify private_build_requires = ["build_util", "python"].

So, what is the whack package actually supposed to be testing? An invalid build_command, or an erroring build command?

Even if I install rez from main on Windows and run rez-selftest --only-shell pwsh --build, I can see the whack-1 test "soft-failing" with:
Python was not found; run without arguments to install from the Microsoft Store, or disable this shortcut from Settings > Apps > Advanced app settings > App execution aliases.

However, this does not actually end up failing the test.

[JeanChristopheMorinPerso]

Interesting finding!

I'm kind of surprised that it's failing. I went through those tests very carefully, one by one. But maybe I forgot one and missed something... From the look of it, whack is expected to fail, but not this way. It should fail in the build script. So I guess you can add python as a private build requirement.

[nrusch]

OK, I've got the baseline tests working now after an update to the whack package. I can't test locally on MacOS, so I can only trust what the CI pipeline does here...

I will look at adding a new test for the behavior change... I'll just have to think about what shape it might take.

[JeanChristopheMorinPerso]

Thanks! I can give it a try tomorrow. But in general, I trust our tests (only because I spent days last year making sure they were testing what they were supposed to).

As for the new test, I have an idea, but it's probably a "bad" one... Basically, I'm wondering if creating a "gui" executable from https://github.com/AcademySoftwareFoundation/rez/blob/main/src%2Frez%2Fbind%2Fhello_world.py#L52 would allow to replicate the scenario you want to test. This could be done using gui_scripts instead of console_scripts. Doing that use a different exe for the command... Would that help?

[nrusch]

That's more or less what I had in mind, and I've been able to confirm that a basic auto-generated "GUI script" entrypoint can be used to reproduce the error on Windows before this patch is applied. I guess the one question is just whether a GUI entrypoint will run as expected in CI, but we'll find out.

[nrusch]

In the course of creating a reproducible test failure to A/B my fix against, I realized that one of the critical ingredients for hitting this error is to be running PowerShell in "strict" mode.

From the PowerShell documentation:

When Set-StrictMode is off, PowerShell has the following behaviors:

• Uninitialized variables are assumed to have a value of 0 (zero) or $null, depending on type
  [...]

We run PowerShell with Set-StrictMode enabled, which may explain why we ran into this issue when others have not.

I have manufactured a test case using a Windows GUI entrypoint and confirmed that I can reproduce the error I originally reported here prior to my patch using a test with Set-StrictMode enabled, and that the error no longer occurs after my patch it applied, so I just need to distill all of that down into some extra changes to push up.

[nrusch]

OK, I've added a (perhaps somewhat contrived) test for the GUI application return behavior. Let me know what you think.

[JeanChristopheMorinPerso]

Thank you for all the hard work on this! I left a small comment that should be easy to address. Apart from that, I'm ready to approve this.