janestreet / sexplib

Automated S-expression conversion
MIT License
147 stars 27 forks source link

sexplib.113.33.00 produces invalid `sexplib.install` in MinGW environment #21

Closed camlspotter closed 8 years ago

camlspotter commented 8 years ago

remove_cwd of js-utils/gen_install.ml assumes the environment is unix and directory separator is /.

In MinGW, the separator is not / but \ and therefore remove_cwd does not properly remove the cwd, which results in a invalid sexp.install.

Following patch fixes the issue, and I have verified Cygwin OPAM + MinGW OCaml successfully installs the library:

diff -r -c -N sexplib.113.33.00/js-utils/gen_install.ml sexplib.113.33.00-fix/js-utils/gen_install.ml
*** sexplib.113.33.00/js-utils/gen_install.ml   Fri Apr  1 12:06:04 2016
--- sexplib.113.33.00-fix/js-utils/gen_install.ml   Fri Apr  1 12:10:51 2016
***************
*** 31,37 ****
    |> List.map (fun line -> Scanf.sscanf line "%[^=]=%S" (fun k v -> (k, v)))

  let remove_cwd =
!   let prefix = Sys.getcwd () ^ "/" in
    let len_prefix = String.length prefix in
    fun fn ->
      let len = String.length fn in
--- 31,37 ----
    |> List.map (fun line -> Scanf.sscanf line "%[^=]=%S" (fun k v -> (k, v)))

  let remove_cwd =
!   let prefix = Sys.getcwd () ^ Filename.dir_sep in
    let len_prefix = String.length prefix in
    fun fn ->
      let len = String.length fn in

If you guys use this js-utils for other libraries the patch is also usable I guess.

camlspotter commented 8 years ago

Ooops, a typo in a patch. I fixed it in the above comment.

ghost commented 8 years ago

Thanks, I integrated the change internally so that it will be present in the next release

ghost commented 8 years ago

Fixed in 113.33.03, this script now lives there: https://github.com/janestreet/js-build-tools