CRAM tests on windows

[pirbo]

Expected Behavior

I'm not sure... Low hanging fruit: a warning saying "CRAM tests skipped, sh cannot be found in the path."

Actual Behavior

Internal error, please report upstream including the contents of _build/log.
Description:
  ("Option.value_exn", {})
Raised at Stdune__Code_error.raise in file
  "otherlibs/stdune/src\\code_error.ml", line 10, characters 30-62
Called from Dune_rules__Cram_exec.run in file
  "src/dune_rules\\cram\\cram_exec.ml", line 427, characters 6-45
Called from Fiber__Scheduler.exec in file "vendor/fiber/src\\scheduler.ml",
  line 76, characters 8-11
Re-raised at Stdune__Exn.raise_with_backtrace in file
  "otherlibs/stdune/src\\exn.ml" (inlined), line 38, characters 27-56
[...]

Line 427 of src/dune_rules/cram/cram_exec.ml is Option.value_exn (Bin.which ~path "sh") :)

Reproduction

@sudha247 discovered the bug while testing if the "ocaml platform" was windows yet (and I investigated it). It is raised by, at least, ocamlformat.0.26.0 and odoc.2.4.2 but I mean any cram test anywhere would raise it

1. open a Powershell or cmd.exe
2. opam source ocamlformat
3. cd ocamlformat.0.26.0
4. dune runtest

CAUTION: dont be confused as I've been at first: opam install -t ocamlformat is ~working~ failing with a different set of errors (a ton of diffs where /s have been replaced by some \s...) I mean (cygwin) sh IS found in this case because "opam install" extends your environment with enough of the cygwin_opam_installs for that)

PS: nope sadly opam exec -- dune runtest is not sufficient to get (cygwin) sh in your path... If/When cygwin executable are added to the path by opam is a mystery to me, I would redirect the question to @dra27 ;)

Specifications

• Version of dune (output of dune --version): 3.16.0 and 3.15.2
• Version of ocaml (output of ocamlc --version): 5.2.0
• Operating system (distribution and version): Windows (11 VM) opam.2.2.1 install by winget

[nojb]

"CRAM tests skipped, sh cannot be found in the path."

Sounds reasonable. PR welcome!

[emillon]

Thanks for the bug report. I think it would be more correct for dune itself to to declare a dependency on %{bin:sh} for any cram action. This will improve the error message without silently skipping tests. The package author might then want to set (enabled_if) for the corresponding test, but I don't think dune should do it by itself.

[rgrinberg]

Adding a dependency on sh sounds good independently, but it doesn't help with this issue. To add a dependency on a binary, we need to resolve it first. And the resolving step is where this bug is rearing its head at the moment.

[emillon]

oh yes of course I missed this, I assumed that the resolution was delayed in dependencies.

[dra27]

PS: nope sadly opam exec -- dune runtest is not sufficient to get (cygwin) sh in your path... If/When cygwin executable are added to the path by opam is a mystery to me, I would redirect the question to @dra27 ;)

The core idea is that portable systems should be being written portably - sh is not portable (in terms of Unix/Windows portability). The executables from Cygwin which are exposed are limited to the C compiler - i.e. the commands which OCaml needs to run. When using MSYS2's opam, nothing needs doing precisely because it ships GCC as a set of native Windows executables.

I don't think it's too bad when developing packages to have to say that their testsuites must be run from a Cygwin/MSYS2 bash prompt (though it's obviously in general to have testsuites which don't require it). It's very bad to require a Cygwin/MSYS2 bash prompt in order to use those packages (i.e. one should expect, as one can, be able to install and run dune.exe and ocamlformat.exe from Cmd/PowerShell, but one should not necessarily be able to compile either dune or ocamlformat from sources from Cmd/PowerShell directly)

[maiste]

Would changing the error message be a first step? Like matching on the option value and raise an exception from dune instead of an error from OCaml. If yes, I can work on it.

[rgrinberg]

I think that's reasonable. The next to actually fix the issue is to just delay the said error until it doesn't interfere with loading the rules.