Set SHELL in tests that use shell/2

[bakaq]

The shell/2 predicate always runs its argument in the user's configured shell. This is not always POSIX compliant, because some people use stuff like fish and Nushell. This sets the $SHELL env var to /bin/sh, which is a POSIX compliant shell in any reasonable Unix.

I wasn't being able to run the tests because of this (I use Nushell).

[bakaq]

Of course Windows would fail. (The mystery is why it even passed before)

[bakaq]

Oh no, I understand why now. The Windows runners have Git Bash installed, and all the GNU coreutils (like test and rmdir) in the PATH. Stuff like test -d path_canonical_test && rmdir path_canonical_test || true is valid cmd.exe syntax and works as expected. This is incredibly cursed, and not something I think we should be depending on.

[guregu]

I have the Windows tests/build for Trealla use the msys2 shell, which seems to be reasonably POSIX-y: https://github.com/guregu/trealla/blob/c3216a11ae2c6c987201b2dfd02620d8c2d8d3dc/.github/workflows/build.yaml#L59

[bakaq]

The problem here isn't getting a POSIX-y into the Windows runner, there's many ways to do that. The problem is that shell/2 behavior is completely dependent on what the user has configured as SHELL (or COMSPEC I guess). I don't think we should depend on that for our test suite.

The setenv("SHELL", "/bin/sh") trick is the best I can think of to actually guarantee a consistent shell syntax (POSIX shell), and it works for all Unixes, but doesn't work for Windows. It's what I've been doing in Bakage to actually have some portability, with the same caveats. The fact that the shell/2 invocations here also run as expected on the default Windows runner is a coincidence, and will probably bite us in the future if we don't deal with this soon.

I believe we should either:

1. Do the /bin/sh trick and skip the tests that use shell/2 in Windows (and other non-POSIX platforms if we support them in the future).
2. Make the tests branch or have to also support a standard Windows shell that we can also set with setenv/2 (like PowerShell).

2 sounds like a lot of work to maintain as most contributors here don't use Windows machines and are probably not familiar with cmd.exe or PowerShell peculiarities, so I'll be going with 1 for now.

We could just always use library(process) instead too. That solves the shell syntax situation, but it doesn't really solve the portability problem because the set of "coreutils" programs is also not guaranteed. Invoking the POSIX test command on a vanilla Windows install will still fail even if we do it through library(process) instead of shell/2. We also need to have tests for shell/2 anyway.

[Skgland]

I haven't checked how these tests are run, but one thing to keep in mind when using setenv/2 is that it is not safe to be used in multithreaded context¹ (due to underlying rust/C APIs not being thread safe). So care must be taken that the tests aren't being run in parallel in the same process.

Edit: Though it appears all tests were already using setenv/2, so this wouldn't be a new issue.
Edit2: And they appear to be all marked #[serial] in ‎tests/scryer/issues.rs

Footnotes

1. https://doc.rust-lang.org/std/env/fn.set_var.html#safety ↩

[bakaq]

I'm aware of this issue. I thought it shouldn't be a problem because the Rust tests run in different processes, but turns out that's not true!!.

As you mentioned though, this was already a problem there before. And these tests run sequentially so the problems should be reduced (not sure if completely avoided).

I'll try to turn these tests into trycmd tests in another PR, so that they are indeed run in another process and we don't have problems. This should get merged first though.