ocaml-multicore / eio

Effects-based direct-style IO for multicore OCaml
Other
559 stars 72 forks source link

Get main test-suite running on Windows #761

Open talex5 opened 1 month ago

talex5 commented 1 month ago

Most of Eio's tests are in the tests directory, which isn't tested on Windows (due to the (enabled_if (<> %{os_type} "Win32")) in tests/dune).

A good first step would be to find out what happens if you remove that. Probably a few of the tests will fail (and should be disabled on Windows for now). Or possibly there are problems using MDX on Windows. Either way, it would be good to document the current situation.

Implementing https://github.com/realworldocaml/mdx/issues/393 would be really helpful for Eio.

patricoferris commented 1 month ago

Indeed! I took a little look at the feasibility of this and MDX seems to be in good shape with the caveat that the MDX files use LF and not CRLF (I believe we can force Git to do something about that). Windows + VSCode users can also tell VSCode to use LF in the bottom right corner. CRLF files hang in MDX...

Unfortunately though, it looks like we actually need compiler changes to make this work. We run into https://github.com/ocaml/ocaml/issues/12999 because MDX is using a top-level environment IIUC. I spoke to @dra27 about this offline and a short-term solution is to make caml_unix_error a CAMLextern (although this may lead to other issues...) or to use -fvisibility which also means making compiler changes. If I find the time I'll look at both, but until then unfortunately I think we have to wait or continue with making tests non-bytecode for Windows ://

We could also perhaps remove uses of unix errors in our Windows stubs and use our own exception as a workaround perhaps... but that feels slightly annoying given this is just so we can test the library in MDX ! But maybe worth it... I don't understand the issue fully enough though as loading eio_windows in ocaml or utop works fine...

patricoferris commented 1 month ago

To be clear though, working on the sections MDX work would be great and then we could disable all the tests inside the MDX files so that come the fateful day that we're just turning off tests because of implementation issues in eio_windows (and not issues in ocaml/ocaml or elsewhere) we can do that easily by uncommenting a line or however it will support "turning off" parts of the markdown file for Windows!

talex5 commented 1 month ago

Ah, https://github.com/realworldocaml/mdx/pull/433 got merged, so you can do <!-- $MDX os_type<>Win32 --> now.

RichardChukwu commented 1 month ago

I'd like this to be assigned to me please @patricoferris @talex5

Thank you

patricoferris commented 1 month ago

Hey @RichardChukwu,

I think it would be best to focus on a single issue and it's PR (namely #763) at a time. Unfortunately, that does mean waiting for a review but I think that is only fair for other applicants. We'll do our best to review it in a timely manner. Thanks :))

RichardChukwu commented 1 month ago

Hello @patricoferris and@talex5 can I be assigned this issue please instead since it's windows specific and the other issue #763 depends on getting this solved first?

patricoferris commented 1 month ago

Hey @RichardChukwu,

763 doesn't depend on this issue because it is not a test inside a Markdown file. Would you prefer to work on this issue or the other one in light of this?

RichardChukwu commented 1 month ago

@patricoferris I'd prefer this please instead since it's more windows specific for the purpose of this Contrbution phase if that's okay?

oyenuga17 commented 1 month ago

Hey @RichardChukwu, how's it going over here, do you have any blockers?

RichardChukwu commented 1 month ago

@talex5 @patricoferris @oyenuga17 tried running the Eio tests in tests/dune after uncommenting the ; (build_if (= %{os_type} "Win32")) using Utop on windows.

Here are some erros encountered for each test:

For test.ml image

For test_fs.ml image

For test_net.ml image

Not sure exactly how to proceed from here, from what I understand, the unix, Formatters and eio_unix respectively modules are not recognized, tried installing using opam install fmt but it says the package is already installed. Eio_unix installation gives [ERROR] No package named eio_unix found.

patricoferris commented 1 month ago

To run the tests you will have to invoke them using dune which will handle all of the libraries that are needed. Unfortunately, you can't just load them into utop.

There's not an easy way to do this file by file, but I recommend altering the tests/dune file to incrementally test the various markdown files on Windows, disabling those that don't work as you go (or doing it all at once).

(mdx
  (package eio_main)
  (files :standard \ buf_reader.md)
  (enabled_if (<> %{os_type} "Win32"))
  (deps
    (env_var "EIO_BACKEND")
    (package eio_main)))

; Enabled on Windows
(mdx
  (files buf_reader.md)
  (package eio_main)
  (deps
    (env_var "EIO_BACKEND")
    (package eio_main)))

Read about dune's "set" notation here: https://dune.readthedocs.io/en/stable/reference/ordered-set-language.html

RichardChukwu commented 3 weeks ago

Hello @patricoferris @talex5 Kindly check my submitted PR on this issue. I ran the tests successfully on windows after removing the (enabled_if (<> %{os_type} "Win32")) in tests/dune) and noted some observations.