search

ocaml / ocamlfind

The OCaml findlib library manager
Other
30 stars 30 forks source link

Apply Shellcheck improvement suggestions #73

Closed Leonidas-from-XIV closed 3 weeks ago

Leonidas-from-XIV commented 9 months ago

I ran shellcheck on the configure script which suggested a lot of nice improvements, mostly about quoting and making sure spaces don't break the commands.

Other interesting improvements include removing variables that are never set (thus always empty, which made it work), so removing them makes it easier to see what is happening.

Edit: I also applied the $? suggestion since it makes the code a bit less fragile and fixed the POSIX compatibility, so it should be as portable as it was before.

zapashcanon commented 2 months ago

FYI, I tried to change the quoting stuff (i.e. ` ` vs $()) in the OCaml codebase many years ago and it supposedly break some systems in /bin/sh scripts, see this comment.

dra27 commented 2 months ago

6 years later, the quoting concerns for backticks are somewhat less of a concern - I think the intersection of "has a C11 compiler and can therefore build OCaml 5.x" and "has a non-Posix sh" is fortunately ∅ 😊

Leonidas-from-XIV commented 1 month ago

I've rebased it on top of the current master and applied @dra27's very helpful comments.

As to the $() thing, I've left it in. While the OCaml compiler can probably happily depend on $(), ocamlfind does support 3.08 so might encounter these systems. On the other hand, I would not even know where to find a system with a non-POSIX sh so I think requiring a shell that's newer than the last few decades is probably fine. We don't have a good way to test whether the rest of the script works anyway on such platforms.