Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get main test-suite running on Windows #761

Open
talex5 opened this issue Oct 4, 2024 · 5 comments
Open

Get main test-suite running on Windows #761

talex5 opened this issue Oct 4, 2024 · 5 comments
Labels
good first issue Good for newcomers windows

Comments

@talex5
Copy link
Collaborator

talex5 commented Oct 4, 2024

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 realworldocaml/mdx#393 would be really helpful for Eio.

@talex5 talex5 added good first issue Good for newcomers windows labels Oct 4, 2024
@talex5 talex5 mentioned this issue Oct 4, 2024
9 tasks
@patricoferris
Copy link
Collaborator

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 ocaml/ocaml#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
Copy link
Collaborator

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
Copy link
Collaborator Author

talex5 commented Oct 4, 2024

Ah, realworldocaml/mdx#433 got merged, so you can do <!-- $MDX os_type<>Win32 --> now.

@RichardChukwu
Copy link

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

Thank you

@patricoferris
Copy link
Collaborator

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 :))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers windows
Projects
None yet
Development

No branches or pull requests

3 participants