janestreet / ppx_sexp_conv

Generation of S-expression conversion functions from type definitions
MIT License
88 stars 17 forks source link

build failure of nocrypto.0.5.9 probably caused by `ppx_sexp_conv` #20

Closed gasche closed 6 years ago

gasche commented 6 years ago

nocrypto.0.5.9 fails to build on my machine with the following error message (on a switch where base, sexplib and ppx_sexp_conv are all at version v0.11.0):

# Error: The implementation (obtained by packing)
# [...]
#        In module Hash:
#        Values do not match:
#          val hash_of_sexp : Ppx_sexp_conv_lib.Sexp.t -> hash
#        is not included in
#          val hash_of_sexp : Sexplib.Sexp.t -> hash
#        File "src/nocrypto.mli", line 217, characters 2-43:
#          Expected declaration
#        File "src/hash.ml", line 114, characters 0-86: Actual declaration

this looks like a problem of missing module equalities. (It might be caused by the fact that ppx_sexp_conv_lib.ml has no matching .mli file in base/src, but really I have no idea.)

gasche commented 6 years ago

Hm, it looks like this may be a build system issue within nocrypto, according to https://github.com/ocaml/opam-repository/pull/11628#issuecomment-375697444.

ghost commented 6 years ago

Yh, it seems to work when adding the -package flags, so I don't think there is much we can do in ppx_sexp_conv itself. I'm not sure why this is the first time such an issue shows up though.

gasche commented 6 years ago

My best guess is that this is due to the interaction between the way the .cmi of a packed-module is produced (depending on the compilation environment) and module-level equalities.

gasche commented 6 years ago

Because we all love this stuff, I build a repro case: https://gitlab.com/gasche-snippets/include-pack-and-build-repro

The repro case needs all of the following feature:

If either outlib is given a .mli with a suitably strong signature, or inlib is added to the include path when the pack is created, then the issue goes away. This means that (pack files are crazy and) both our hunches were right: this can be fixed with a build-system change, but also by adding a suitably-strong .mli to ppx_sexp_conv.