Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

I marked this commit as my own (and manually added the CLA Signed tag)

I did not find much info on this.
As this concerns a dune file, @emillon can I have your opinion on this?

The approach works (it will call sh manually and register a dependency to the shell script). I'm not sure about the double percent though - (run) will not use system, so I don't think it's necessary to quote it on windows. It will probably add a literal %.

@emillon - it's an ocaml-mode file, the escaping is for printf's benefit

All good then! Maybe a way to improve this on dune's side is to add a (sh ...) action like we have for (bash).

Actually, the better option will be for me to carve out ocaml/opam#3348 to a library, and have Dune vendor it!

Yes, that would work as well. We have a similar problem with make which has a different name depending on systems (and opam has a builtin solution for this as well).

True - that's mostly an orthogonal issue, unless you have a Cygwin-symlink gmake or something annoying like that!

By the way @emillon is this correct to make this change in ocamlformat instead of doing it in dune?
(cc @jberdine )

@gpetiot - it's not correct to assume that shell scripts can be executed directly on Windows. If Dune gets that then, like opam, it would be a largely unique feature (I'm not aware of any other native Windows program which tries to do this and, given the amount of code it takes to do it, it's not too surprising!). It would remain correct for ocamlformat's build system to invoke sh directly, even if Dune gained the ability to call the script.

Of course, converting the script to OCaml is ultimately the correct move!

(I'm a Dune dev too, btw, if rather less active!)

I agree with @dra27. It might be end up as a nice helper in dune, but the "workaround" is the portable way to do it.

Alright, thank you for your help!

I don't intend to be grumpy, but I'm really not convinced. Looking at this code change, it looks just plain worse. Am I missing something? I mean, to understand that the version proposed is not just a roundabout way to write the existing version, the reader has to understand that windows can't handle seemingly innocuous invocations of scripts in dune files, and also some fiddly business about what dune considers to be a dependency. The solution to the windows problem cannot be to complicate everything.

@jberdine - it’s either %{dep:...} or including it in the deps field, but that’s code duplication with the usual entailed brittleness.

I’m not particularly clear why sh script is so much more complicated than ./script - it’s a quite common idiom to avoid issues where the x attribute gets lost in a distribution archive too.

It is also worth noting that the manual for run states “to execute a program”!

:-( Such nigh invisible fiddly details are very easy to break unintentionally, and don't help readability.

I suppose if gen_version.sh was rewritten in ocaml, we still couldn't use (run gen_version.ml) in the dune file, and we'd need the same dep fiddling?

It would need to be run ocaml %%{dep:gen_version.ml}, but otherwise yes. I could more readily get on board with Dune automatically knowing on Windows that ./foo.ml means to run ocaml foo.ml, though.

Note that you can also avoid this by compiling gen_version at that point (more lines of dune, but fewer “tricks”)

Thanks for the info.

Re compiling gen_version, I agree, but the catch is that it needs to be run from dune (when building distributed archives), opam (when pinning), and make (when building during development). I don't know if it is feasible to compile it in particular when opam pinning.