Build dependency on ppx_sexp_conv should be runtime dependency?

[paurkedal]

The ipaddr package installs a META file with a dependency on ppx_sexp_conv.runtime-lib, but ppx_sexp_conv is only declared a build dependency in opam. This can cause dependent packages to fail building when the ipaddr itself is not rebuilt, due since opam does not know that ppx_sexp_conv must be rebuilt first. An example is show below.

Note that ipaddr has a runtime dependency on sexplib, so the ppx_sexp_conv.runtime-lib may have been an unexpected dependency due to the requires(-ppx_driver) = "ppx_sexp_conv.runtime-lib" line in the META of ppx_sexp_conv.

#=== ERROR while compiling mirage-stack-lwt.1.2.0 =============================#
# context      2.0.0~rc | linux/x86_64 | base-bigarray.base base-threads.base base-unix.base ocaml-base-compiler.4.06.1 | https://opam.ocaml.org/2.0#6927894a
# path         ~/.opam/4.06.1/.opam-switch/build/mirage-stack-lwt.1.2.0
# command      ~/.opam/4.06.1/bin/jbuilder build -p mirage-stack-lwt -j 6
# exit-code    1
# env-file     ~/.opam/log/mirage-stack-lwt-23800-575881.env
# output-file  ~/.opam/log/mirage-stack-lwt-23800-575881.out
### output ###
# File "/home/urkedal/.opam/4.06.1/lib/ipaddr/META", line 1, characters 0-0:
# Error: Library "ppx_sexp_conv.runtime-lib" not found.
# -> required by library "ipaddr" in /home/urkedal/.opam/4.06.1/lib/ipaddr
# Hint: try: jbuilder external-lib-deps --missing -p mirage-stack-lwt @install


#=== ERROR while compiling mirage-runtime.3.0.7 ===============================#
# context      2.0.0~rc | linux/x86_64 | base-bigarray.base base-threads.base base-unix.base ocaml-base-compiler.4.06.1 | https://opam.ocaml.org/2.0#6927894a
# path         ~/.opam/4.06.1/.opam-switch/build/mirage-runtime.3.0.7
# command      ~/.opam/4.06.1/bin/jbuilder build -p mirage-runtime -j 6
# exit-code    1
# env-file     ~/.opam/log/mirage-runtime-23800-406bbc.env
# output-file  ~/.opam/log/mirage-runtime-23800-406bbc.out
### output ###
# File "/home/urkedal/.opam/4.06.1/lib/ipaddr/META", line 1, characters 0-0:
# Error: Library "ppx_sexp_conv.runtime-lib" not found.
# -> required by library "ipaddr" in /home/urkedal/.opam/4.06.1/lib/ipaddr
# Hint: try: jbuilder external-lib-deps --missing -p mirage-runtime @install


#=== ERROR while compiling mirage-protocols-lwt.1.3.0 =========================#
# context      2.0.0~rc | linux/x86_64 | base-bigarray.base base-threads.base base-unix.base ocaml-base-compiler.4.06.1 | https://opam.ocaml.org/2.0#6927894a
# path         ~/.opam/4.06.1/.opam-switch/build/mirage-protocols-lwt.1.3.0
# command      ~/.opam/4.06.1/bin/jbuilder build -p mirage-protocols-lwt -j 6
# exit-code    1
# env-file     ~/.opam/log/mirage-protocols-lwt-23800-4bf7c2.env
# output-file  ~/.opam/log/mirage-protocols-lwt-23800-4bf7c2.out
### output ###
# File "/home/urkedal/.opam/4.06.1/lib/ipaddr/META", line 1, characters 0-0:
# Error: Library "ppx_sexp_conv.runtime-lib" not found.
# -> required by library "ipaddr" in /home/urkedal/.opam/4.06.1/lib/ipaddr
# Hint: try: jbuilder external-lib-deps --missing -p mirage-protocols-lwt @install


#=== ERROR while compiling mirage-net-lwt.1.1.0 ===============================#
# context      2.0.0~rc | linux/x86_64 | base-bigarray.base base-threads.base base-unix.base ocaml-base-compiler.4.06.1 | https://opam.ocaml.org/2.0#6927894a
# path         ~/.opam/4.06.1/.opam-switch/build/mirage-net-lwt.1.1.0
# command      ~/.opam/4.06.1/bin/jbuilder build -p mirage-net-lwt -j 6
# exit-code    1
# env-file     ~/.opam/log/mirage-net-lwt-23800-c97fa7.env
# output-file  ~/.opam/log/mirage-net-lwt-23800-c97fa7.out
### output ###
# File "/home/urkedal/.opam/4.06.1/lib/ipaddr/META", line 1, characters 0-0:
# Error: Library "ppx_sexp_conv.runtime-lib" not found.
# -> required by library "ipaddr" in /home/urkedal/.opam/4.06.1/lib/ipaddr
# Hint: try: jbuilder external-lib-deps --missing -p mirage-net-lwt @install

My conclusion is that, as things are, ppx_sexp_conv must always be a runtime dependency in opam. The following packages lists ppx_sexp_conv as a build dependency:

• amf
• async-zmq
• charrua-core
• cohttp
• cohttp-lwt
• conduit
• conduit-async
• conduit-lwt
• conduit-lwt-unix
• coq-serapi
• datakit-ci
• dns-forward
• dockerfile
• ipaddr
• mecab
• mirage-conduit
• mirage-net-lwt
• mirage-net-xen
• mirage-protocols-lwt
• mirage-runtime
• mirage-stack-lwt
• msgpck
• netchannel
• nocrypto
• ocaml-logicalform
• ocaml-topexpect
• otr
• protocol-9p
• protocol-9p-unix
• qcow
• qcow-format
• shared-block-ring
• sslconf
• tls
• uri
• vchan
• vchan-unix
• vchan-xen
• vmnet
• wamp
• x509
• yaml

[hcarty]

@trefis Any suggestions for the right way to address this?

[trefis]

What @paurkedal said sounds correct to me indeed, the opam file of these packages should be fixed.

[hannesm]

an excerpt of the META file of ppx_sexp_conv:

package "runtime-lib" (
  requires = "base"
)

this means that -- as in sexplib v0.9.0 -- a dependency onto base (which cmxa is >6MB (6267306 bytes) in size) is added. with v0.10.0 this is not the case. given that I mostly compile my libraries to MirageOS unikernels, I'm not eager to accept this additional dependency (please see further discussion in mirleft/ocaml-nocrypto#143).

if, as proposed in the nocrypto PR/issue by @gasche, a runtime dependency to ppx_sexp_conv is added (i.e. not only runtime-lib), this will imply requires = "base ppxlib", which includes the compiler libraries and ppx utilities and increases the code size even further.

for any of the packages in the above list which I maintain, I'm instead in favour of adding an upper bound to sexplib* v0.10.0. this is not a good solution because it means a split universe in the opam repository, but I honestly don't understand the motivation behind the added dependencies (maybe @trefis can enlighten us).

EDIT: I added upper bounds for otr, tls, x509 in #11880

[cfcs]

Would meta_conv or similar work as an alternative to if Jane Street refuses to remove the added bloat dependency?

[trefis]

I'll let @xclerc or @diml look at the runtime dependency of ppx_sexp_conv on base.

[hannesm]

thanks, earlier discussion (from March 2017 when sexplib v0.9.0 was released with a dependency on base) at https://www.mail-archive.com/mirageos-devel@lists.xenproject.org/msg02638.html

[ghost]

We did some work to remove the sexplib->Base dependency but didn't consider ppx_sexp_conv. We can remove the latter as well. @hannesm, can you open a ticket on ppx_sexp_conv so that we can track this issue?

Regarding this PR, yes, ppx_sexp_conv should definitely be a runtime dependency as well. I think it is safe to always make ppx rewriters be runtime dependencies since they often have runtime libraries.

[hannesm]

which I did in janestreet/ppx_sexp_conv#22

[hannesm]

I also constrained lots of packages in #11898 with ppx_sexp_conv {< "v0.11.0"}. from the list above (thx @paurkedal) there are some false positives, i.e. libraries which depend on ipaddr (such as mirage-net-lwt), and thus fail, but don't have a direct dependency to ppx_sexp_conv. I'm pretty sure I missed some packages from the above list in #11898.

[ghost]

BTW, if I understand the meaning of {build} correctly, then ppx rewriters should never be flagged as build only dependencies, even when they don't have runtime dependencies. The reason is that if the code they generate changes, then all their reverse dependencies must be rebuilt as well.

[cfcs]

@diml nothing should ever be {build} unless it has no consequence for the built package.

[paurkedal]

Yes, [T]aking correctness to it's conclusion would evict most {build} annotations, so I think the decision must be pragmatic. Ppx rewriters are quite different from build systems, since their primary purpose is to generate content, whereas build systems mostly affect the result indirectly though flags and build strategies. So, while in the latter the {build} is in good faith for real build dependences, I realise @diml is right about ppx rewriters. One case I find hard to dismiss is that if there is a bug in the code generator of a ppx, reverse dependencies won't be rebuild when a fixed version is released.

(Declaring {build} for build infrastructure isn't strictly safe either, since they may provide content for substitutions or even generate code directly.)

[cfcs]

@paurkedal Yeah, I wish I know how to make opam disregard the {build}.
It's awkward that a fresh install will differ from a freshly updated+upgraded installation of opam due to this. It also means that newcomers will face a lot of bugs that people with "old" opam installations won't notice (and vice versa).
I'm really starting to regard {build} as a misfeature, even if it speeds up things.

[paurkedal]

@xclerc This issue affects datakit-ci.1.0.0. Should we remove the {build} flag from the ppx_sexp_conv of that package? I can send a PR if you are busy.

Apart from that, I can't find any package which in its latest version has a build dependency on ppx_sexp_conv.

[avsm]

I'd be fine with a PR removing the {build} flag from all ppx packages. Our experience with dune has shown us it's not a safe flag, and I strongly prefer correctness over minor efficiencies with this repository.

[paurkedal]

Thanks for the call, I sent a PR proposal.