Run windows tests in multiple shells

[blairconrad]

From pawamoy/duty#23, which identified problems running failprint on Windows, specifically when not run under a bash-like shell.

The current CI builds run the Windows tests using git bash and so don't surface the problem.

[pawamoy]

Great, so it passes with powershell. Could you now remove the fix (setting the env var) to see if it actually fails?

[pawamoy]

Hmmm actually it looks like the shell variable of the matrix is not used anywhere. Unless it's implicit?

[pawamoy]

OK it's not implicit, because as you can see both the bash and pwsh jobs use pwsh to run the make test command. You'll probably have to use the matrix shell variable like this: https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsshell.

[pawamoy]

Thank you for all this 🚀

[blairconrad]

Ha, so many comments. Yes, I am an inexpert GitHub action person. I intended to flail about privately before embarrassing myself, but now I'm in the public eye. Expect way too many updates here. You might want to mute the thread!

[blairconrad]

I actually slightly fear that running make might pollute the results, as it could be forcing us into a "*nixy" situation. But that's just wild speculation at this point.

[pawamoy]

Valid concern 🤔 Well, we'll know for sure if you manage to get one test job to fail on PowerShell. If you can't make it fail, you might want to customize the step indeed, to run Python directly, without make.

[pawamoy]

Flushing issue. You can try to add flush=True to the print call. For sys.stdout.write, you can try to add sys.stdout.flush() right after it.

Or you could move the print and sys.stdout line below the two other lines but the flush variant is more explicit.

[pawamoy]

Too bad, tests passing even without the fix.

[blairconrad]

Hey. Work was busy. Sorry you had to turn off the fix. I'm flummoxed by the results I see. With the fix, I still have a poor user experience. My hand-rolled "smoketest" fails.

With this code:

import failprint
import sys
import time

def passing():
    time.sleep(1)
    print("I'm passing")
    return 0

def failing():
    time.sleep(1)
    print("I'm failing")
    return 1

failprint.run(vars()[sys.argv[1]], capture=sys.argv[2])

• uv run smoketest.py passing both exits with code 120 and no output
• uv run smoketest.py failing both exits with code 120 and no output
• uv run smoketest.py passing none exits with code 0 and shows good output (including fancy glyphs)
• uv run smoketest.py failing none exits with code 0 and shows good output (including fancy glyphs)

If I set PYTHONLEGACYWINDOWSSTDIO="1" first, then all combinations above exit with code 1 and prints error indicating that "UnicodeEncodeError: 'charmap' codec can't encode character '\u2717' in position 5: character maps to " (or \u2713)

If I then also set PYTHONIOENCODING="UTF-8" (as the github actions have) and repeat, the smoketests behave as you'd hope ("I'm failing" always printed, "I'm passing" only with capture=none), but the fancy glyphs are mangled, with what's probably their 3 UTF-8 bytes rendered directly as CP1252 characters (I'm too lazy to look, but "Γ£ô" and "Γ£ù". Feels right.)

I think running in pytest or possibly on github machines is confounding the results. I'm going to amend the job to also run the smoketests.

Update
I did it. I think the tests appear to do what we'd want, even without your fix. (And no degradation in output when I use the legacy stdio, but that could also be the github console helping us out.)

Now that I'm saying it out loud, it seems reasonable that stdout and stderr aren't what we've come to expect on Windows even when running in pwsh on a Windows GitHub Action runner. They're capturing the output somehow. Even preprending timestamps if you look close enough.

[pawamoy]

but the fancy glyphs are mangled

Oh OK, I thought the result was "worse" than that. I'd consider it solved on failprint's side with PYTHONLEGACYWINDOWSSTDIO=1, and on the user side with PYTHONUTF8=1 or PYTHONIOENCODING=UTF8. You might want to look deeper into this unicode encode/decode error on Windows to fix the glyphs display 🤷

[blairconrad]

I did not expect a reply at 03:00! But anyhow, we currently seem to be at a point where user-side, we can workaround by setting PYTHONLEGACYWINDOWSSTDIO and maybe PYTHONIOENCODING (or just the former and using a different template). But so far failprint changes don't seem to be doing much good for your average Windows user...

[pawamoy]

🤫

I'll probably add the env var to failprint anyway and add a small paragraph in the docs of both failprint and duty to recommend setting the UTF8 env vars for Windows users.

[blairconrad]

Sorry, I am a little lost. The environment variable, when set from within failprint, does nothing, as far as I can tell. I guess it would affect further pythons spawned from within failprint. But is there a real advantage to setting it within failprint?

(I also wonder if the naive trick of swapping out sys.stdout and sys.stderr would help. Coupled with duplicating the file descriptors, it might mean that both suprocess comamnds and callable commands that don't further spawn a subprocess would work... Of course this may very well be getting to be too much work to support an OS that you don't even use. And that is so far impossible to test on GitHub Actions.)

I am on the side investigating the cp1252 issue, but work interferes. Also, there's no stacktrace. You go from print to the complaint about the encoding!

[pawamoy]

The environment variable, when set from within failprint, does nothing, as far as I can tell.

My understanding was that this would solve the "invalid handle" error, but since I can't try for myself, I can't say for sure. It does fix it for you, though you probably set the env var higher in the stack. I thought setting it right before doing the os.dup2 hackery would have the same effect, i.e. fixing the "invalid handle" issue, but maybe it needs to be set before the Python process is started.

What our tests here have shown is not that setting the env var in failprint does nothing, but that we can't reproduce the "invalid handle" issue in GitHub Actions.

If you can confirm this "invalid handle" issue is indeed fixed by failprint setting the env var (and you not setting it yourself), I'll push and release this change. If it doesn't fix the "invalid handle" issue for you, there's indeed no point in making the change, and I'll only document the env var in a troubleshooting section.

Let me know if anything is unclear!

[blairconrad]

Hey, if I take the current branch and revert 418defa, so failprint sets PYTHONLEGACYWINDOWSSTDIO in CaptureManager.__enter__ (the change you're proposing to merge, I think), then the invalid handle problem is not fixed for me. Running uv run .\tests\smoketest.py failing both results in the same 120 exit code as before, and no output printed. As far as I can tell, setting the environment variable after Python is running did not have the desired effect.

Setting PYTHONLEGACYWINDOWSSTDIO explicitly before running python avoids the problem with the os.dup2 (leaving us with the encoding problem, which is lesser).

To sum up: I don't think merging your change to the CaptureManager helps.

[blairconrad]

More on the environment variable. I've never read the python source before, but for CPython it seems to me that PYTHONLEGACYWINDOWSSTDIO is read to establish initial configuration, right at startup, and then a config object is passed around to affect later behaviour:

https://github.com/python/cpython/blob/e545ead66ce725aae6fb0ad5d733abe806c19750/Python/initconfig.c#L1767-L1770

which would support the finding that setting the environment variable after startup made no difference

[pawamoy]

I see, thank you for confirming! Then yeah, no need to change anything in failprint, and I'll just document this in a troubleshooting section 🙂

[blairconrad]

Thanks. And I'm sorry it hasn't worked out better. I had high hopes, but it seems you're doing things that Windows just doesn't want you to.

Me, for now I'm probably going to try using duty with subprocess-based duties instead of callable. It's not satisfying, but will probably work well enough that I'll not notice any problems.

Out of curiosity, are you interested in either

1. adjustments to make it possible to run the failprint tests on Windows? I have been sitting on some workarounds for missing executables (test, fail, true, echo) that the GitHub Actions environment just seems to supply. I have a commit or so that shouldn't interfere with your current setup and would make for a better experience for Windows-based potential contributors
2. adjustments to your uv-copier template along the same lines? I was slowed down because I don't have make, and updates to the instructions might ease someone else's way. Also, a change to (e.g.) the vscode duty to not rely on cp.

I realize Windows isn't your target platform so if you'd rather not even see PRs, I will drop the idea right now. Otherwise, I'm happy to submit them for you to reject!

Again, sorry nothing more useful has come from the bug reports/PRs. Regardless, I've had a good time trying. You've been very accommodating.

[pawamoy]

1. Sure, we have a specific cross-platform use-case in failprint, so I'm OK to make it easier to test on all supported systems.
2. If these are small adjustments, why not. make is not actually required, only Python: you can run python scripts/make to obtain the command/task interface. So, OK for getting rid of cp in the vscode task. Other adjustments would have to be discussed 🙂

[pawamoy]

And no worries, I enjoyed our interactions too and learned a few things, so all good!

[blairconrad]

Closing. As discovered, the GitHub Actions prevent us from testing with proper Windows streams, so the behaviour that prompted this PR cannot be reproduced. See pawamoy/duty#23 (comment) for a summary of the findings.