Uri.sexp as an optional sub-package

mirage / ocaml-uri

RFC3986 URI parsing library for OCaml

Other

97 stars 57 forks source link

Uri.sexp as an optional sub-package #136

Closed dinosaure closed 4 years ago

dinosaure commented 5 years ago

Currently, uri as an OPAM package needs ppx_sexp_conv. For many packages (like Syndic or git), they don't use uri.sexp (or any PPX things). By transitivity, these packages depends on ppx_sexp_conv and are constrained to follow release cycle of ppx_sexp_conv.

The current status is, ppx_sexp_conv is not available on 4.08.0+beta3 and by this way Syndic and git are not available too - just for this reason.

A way to break this hard dependency (where we don't use it) is to switch uri.sexp as an optional package (with (optional true) in the dune file). Then, remove the ppx_sexp_conv dependency from the OPAM file and release a new uri.

So, it's more a question to @mirage/core if we should do that or not - then, we anybody agree with this, I will happy to make a PR.

hannesm commented 5 years ago

I'd instead, similar to what was done in cstruct, create a new opam package uri-sexp, and not use ocamlfind sublibs for that. the optional (and optional dependencies in general) are in my experience more confusing than they do good. same applies to ipaddr. @avsm - any reason you decided to use ocamlfind sublibds instead of separate opam packages (this would be another breaking change, IMHO worth it)?

Julow commented 5 years ago

I submitted a PR a while ago: https://github.com/mirage/ocaml-uri/pull/134

avsm commented 5 years ago

I did the subpackage first since it introduced the minimum churn, and then did it more comprehensively in ipaddr afterwards.

At this stage, there have been enough requests to minimise the opam dependency cone that it doesn't seem wise to deny them any more :-)

@Julow - I am ok with resurrecting #134, but it needs:

a CHANGES entry to explain how to migrate to the new (just replace uri.sexp -> uri-sexp)
add a META.template as a compat layer that requires: uri-sexp so that revdeps dont break
constrain the tests in uri to only run on uri-sexp with a (package) entry. You can test this with dune-release distrib

I could really use help with submitted patches to upstream projects to remove uri.sexp references once this package is released... any volunteers for that? :-)

dinosaure commented 5 years ago

@Julow and I will work on that and certainly on ipaddr.

avsm commented 4 years ago

fixed in v3.0.0 release