Fixes - Githubissues

mirage / ocaml-crunch

Convert a filesystem into a static OCaml module

Other

74 stars 21 forks source link

Fixes #58

Closed MisterDA closed 2 years ago

MisterDA commented 2 years ago

A bunch of fixes for ocaml-crunch.

Add GitHub Actions for Windows and macOS;
Fixes #57 by always using unix-style paths as keys for files;
Fix tests running on Windows, add expected tests for the generated output;
Fix documentation (usage of SOURCE_DATE_EPOCH, readme);
Use Unix.realpath if available (requires Dune 2.5);
Run ocamlformat on the code.

Perhaps best reviewed on a per-commit basis, but the diff isn't too big anyway. You can see that the GHA workflow succeeded at https://github.com/MisterDA/ocaml-crunch/actions/runs/2621639339.

MisterDA commented 2 years ago

Removed the ocamlformat reformat to ease review. cc @jonahbeckford as a Windows user, what do you think of having unix-style filepaths for keys?

jonahbeckford commented 2 years ago

Unix style paths as keys are a great idea. Thanks!

MisterDA commented 2 years ago

Thanks! I'm not sure why, but now the CI on Windows is failing and I cannot reproduce the failure locally. https://github.com/MisterDA/ocaml-crunch/runs/7215696643

dinosaure commented 2 years ago

I'm not sure why, but now the CI on Windows is failing and I cannot reproduce the failure locally. https://github.com/MisterDA/ocaml-crunch/runs/7215696643

Seems related to \r\n?

jonahbeckford commented 2 years ago

That seems to me to be a CRLF problem ... I usually run into two things that cause that problem:

Git can insert CRLF. I usually have something like https://github.com/diskuv/diskuvbox/blob/2430e85a3b47275eb2be9d671dba267308349f70/.gitattributes to avoid it.
Opening a file but not using binary mode. Anytime you use \n with open_out on Windows it will insert \r\n. I try to use open_out_bin exclusively.

MisterDA commented 2 years ago

Using open_out_bin fixes the problem, thanks. https://github.com/MisterDA/ocaml-crunch/runs/7231290939 It's still strange because the diff command should have ignored CR here:

D:\cygwin\bin\git.exe --no-pager diff --no-index --color=always -u --ignore-cr-at-eol _build/default/test/t1.expected.mli _build/default/test/t1.mli

IMO the PR is good to go now.

hannesm commented 2 years ago

To me this looks fine, thanks for working on the windows support thereof @MisterDA.