Skip to content

Implement Eio.Path.native as a Pi backend and add Windows support - #916

Merged
talex5 merged 1 commit into
ocaml-multicore:mainfrom
talex5:nt-path
Aug 5, 2026
Merged

Implement Eio.Path.native as a Pi backend and add Windows support#916
talex5 merged 1 commit into
ocaml-multicore:mainfrom
talex5:nt-path

Conversation

@talex5

@talex5 talex5 commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

This is split off from @avsm's #908 and rebased on top of #913 and #915.

Incorporates the approach from #738.

I haven't reviewed it yet.

@avsm

avsm commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

I think this is broken now on Windows due to the changes in step handling from #913; I'll fix it up first before you need to review.

Comment thread lib_eio/utils/test/test_paths.ml Outdated
Comment thread lib_eio/utils/test/test_paths.ml Outdated
Comment thread lib_eio/fs.ml Outdated
Comment thread lib_eio/fs.ml Outdated
Comment thread lib_eio/utils/test/test_paths.ml Outdated
@talex5

talex5 commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

I think this is broken now on Windows due to the changes in step handling from #913; I'll fix it up first before you need to review.

Should be fixed now (I spotted it too).

@talex5
talex5 force-pushed the nt-path branch 2 times, most recently from 118528e to 224e0af Compare August 3, 2026 14:53
@avsm

avsm commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

I've been reading up more on Windows paths and have a commit to slightly simplify the nt path parser, but also split out the tests. Sadly my Windows machine keeps rebooting for reasons unknown so I'll have to push it once I get back to it in person later today I hope :-)

@avsm

avsm commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

With apologies for the code churn, I've moved to mdx for the windows tests in 1fae694 which is a bit clearer to read than the denser alcotest. I'm trying to get all the portable mdx working again on Windows, but this is just the fs bit. It also drops an alcotest dep on Eio as a result.

@avsm

avsm commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

my CRLF is messed up, sorry! Can revert 1fae694 (but my Windows machine has just rebooted again!!)

@talex5

talex5 commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

No problem; I squashed them and converted the newlines in fs.md (I left the existing test files alone).

Hmm, that doesn't work (failed in CI). I've switched it back to CRLF.

@talex5
talex5 marked this pull request as ready for review August 4, 2026 13:41
@talex5
talex5 force-pushed the nt-path branch 2 times, most recently from 669cbc8 to 1621ac9 Compare August 4, 2026 13:58
@talex5

talex5 commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

I think we've hit realworldocaml/mdx#294 here.

Some git installations are configured to translate newlines, some do not. If they are translated, the LFs in this repository will be converted to CR LF and the tests will fail. Conversely this translation can also hide problems in the other direction by making CRs disappear.

I tried using .gitattributes to force it to lf, which didn't work. Then I forced it to crlf, that didn't work either.

Incorporates the approach from ocaml-multicore#738.

This also makes a behavioural change I noticed while differentially
checking Go/Rust/Python. We prefer a \ when serialising Windows paths,
even though / is technically allowed. This seems less surprising

Co-authored-by: Kento Okura <kentookura@gmail.com>
@talex5

talex5 commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

Can revert 1fae694 (but my Windows machine has just rebooted again!!)

I partly reverted this, but moved the tests of Nt_path to the main tests/fs.md, next to the POSIX ones. That means they get tested on Linux and macOS at least. The remaining tests (that use Eio_windows.run) are still using Alcotest and only run on Windows.

@talex5
talex5 merged commit 5127001 into ocaml-multicore:main Aug 5, 2026
5 checks passed
@talex5
talex5 deleted the nt-path branch August 5, 2026 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants