Incompatible with sexplib/ppx_sexp_conv v0.11.0

[copy]

After upgrading sexplib and ppx_sexp_conv to v0.11.0 the following error appears when compiling nocrypto:

Error: The implementation (obtained by packing)
       does not match the interface src/nocrypto.mli:
       ...
       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

[gasche]

I just noticed the same error and reported a bug upstream at ppx_sexp_conv: janestreet/ppx_sexp_conv#20 .

[hannesm]

FWIW: diml has an explanation for this in ocaml/opam-repository#11628 (comment)

I've "fixed" this issue in ocaml/opam-repository#11636 by adding an upper bound of sexplib (<"v0.11.0") to nocrypto releases. of course a proper fix and a release of nocrypto would be welcome.

[gasche]

Of course my curiosity was piqued as it revealed to be a build system issue. I have two bugfix branches, master-ocamlbuild-pack and v0.5.4-ocamlbuild-pack.

@hannesm: if you know how to get one of these in an OPAM-available release of nocrypto, that could be nice (the v0.5.4 branch would be suitable for a patch/-level fix, but I understand that we are not excited with opam-level patching anymore). I will get the ocamlbuild-side issue fixed, but an ocamlbuild update will not fix the nocrypto build, it still needs a change to the _tags file.

[hcarty]

Would a patch/-level fix be acceptable for now? This unfortunately prevents packages like tls from being installable alongside the master branch of ocamlformat, for example.

[gasche]

@hcarty: yes, that seems acceptable to me given nocrypto's release management style. Would you be willing to submit a PR to opam-repository?

[hannesm]

(no, I'm neither author nor maintainer of nocrypto) tbh, I'm really against depending on the ppx machinery at runtime, since this will increase the binary size of all my unikernels tremendously. in the past I've been removing runtime dependencies of various ppx in the MirageOS ecosystem. I'm not sure how to move forward, to me it looks like getting rid of ppx_sexp_conv entirely is the only viable solution.

[gasche]

If you already depend on Base, there is no additional size cost. If you don't, then yes, you may want to use a serializer/deserializer that comes with smaller runtime dependencies (it may be possible to have something that is drop-in compatible with with sexp but with a smaller runtime).

[hannesm]

Dear @gasche, there is no dependency on base in the MirageOS ecosystem (sexplib 0.9.0 required base, which increased the binary size by 5MB of every unikernel, which was fixed by the sexplib maintainers).

[gasche]

Well I guess that you should complain to the sexplib maintainers again, as I suppose that they have some control over how ppx_sexplib sets its runtime dependencies -- it should be possible to only depend on sexplib.

[gasche]

cc @trefis :-)

[ansiwen]

I want to mention that my project uses the serializers, so please don’t remove them without replacement. Isn’t it possible to put them in a separate package, so they are only added if used?

Anyway I don’t understand, why the linker can’t remove unused code?

[gasche]

The linker can remove unused compilation units (toplevel modules), but doesn't go at a finer-grained level, and it is easy to have (unused) definitions in your (used) module pull out extra undesirable dependency. A global tree-shaker / dead code remover link-time pass could be implemented in theory (and is an interesting subject, particularly for unikernels), but it is difficult (in theory and in practice) due the presence of side-effect-performing expressions at the toplevel of modules (can those initializing an otherwise-unused datastructure be also removed, or is their effect observable by other parts of the codebase?).

[ansiwen]

Interesting. I was totally depending on the linker when I wrote C++ code for an embedded linux platform, and with statically linked libraries that worked very well. I though a decision, if a certain code-path is referenced somewhere in the binary, and if not to cut it out, would be similarly easy in ocaml. (Initialization code which is called can't of course just cut out without more sophisticated analysis.)

[hcarty]

@gasche Based on a simple local test, I still get the original build issue reported by @copy with your patch against 0.5.4. Sadly I can't spend more time on this currently.

[cfcs]

Has anyone pinged @pqwy about this? I don't know how to contact him now that he also stopped responding to emails, but this is an annoying issue to have to work around.

[j0sh]

As far as I can tell, we can't use nocrypto along with a ppxlib driven rewriter. The alternative is using the deprecated ppx_type_conv for rewriters. Would really appreciate a long-term fix here.

[copy]

This has been fixed using opam patches in ocaml/opam-repository#12062 and released as nocrypto 0.5.4-1.

Will leave this issue open for a fix in this repository.

[cfcs]

should be fixed in latest master: a58c653