Skip to content

Conversation

ghost
Copy link

@ghost ghost commented May 3, 2018

Remove the dependency from ppx_sexp_conv.runtime-lib to base

@camelus
Copy link
Contributor

camelus commented May 3, 2018

✅ All lint checks passed 7142f8f
  • These packages passed lint tests: msgpck.1.0, ocaml-topexpect.0.1, ocaml-topexpect.0.2, ocaml-topexpect.0.3, ppx_sexp_conv.v0.11.1, qcow.0.6.0, qcow.0.7.0, qcow.0.8.1, qcow.0.9.0, qcow.0.9.4, qcow.0.9.5, yaml.0.1.0

✅ Installability check (8793 → 8794)
  • new installable packages (1): ppx_sexp_conv.v0.11.1

@hannesm
Copy link
Member

hannesm commented May 3, 2018

thanks. are the revdeps failures expected? I suggest we merge this, and afterwards a PR similar to #11898 which (a) conflicts with ppx_sexp_conv {= "v0.11.0"} and (b) turns the build dependency ppx_sexp_conv {build} into a runtime dependency ppx_sexp_conv. if I follow mirleft/ocaml-nocrypto#144 along, there is as well the requirement to expose this runtime dependency via META!? since when does ppx_sexp_conv provide a runtime-lib (i.e. do we need as well a lower bound of ppx_sexp_conv in the respective packages?

@ghost
Copy link
Author

ghost commented May 4, 2018

Looking at the errors, some of them should already occur with ppx_sexp_conv v0.11.0, I'll add some constraints. Some are just missing a dependency on sexplib.

Basically, the runtime dependency of ppx_sexp_conv used to be sexplib, but there was a change between v0.10 and v0.11 to add a ppx_sexp_conv.runtime-lib library and make it the runtime dependency of ppx_sexp_conv. This runtime library simply re-exports some functions under a Ppx_sexp_conv_lib toplevel module and the generated code refers to it. This was part of a change that affected all our ppx rewriters and was motivated by:

  • keeping the generated code as small as possible and move more code to runtime libraries, since it's much easier to review and maintain
  • hygiene: we had some name clash problems and by systematically having a runtime library we have a better guarantee on what the identifiers produced by ppx rewriters refer too

@ghost
Copy link
Author

ghost commented May 4, 2018

For shared-block-ring and vchan-unix the issue seems to be unrelated to this PR so I didn't touch them.

@avsm and @let-def, after this PR, ocaml-topexpect and yaml needs to be updated to add an explicit dependency on sexplib. For ocaml-toplexpect, the sexplib dependency was already in the jbuild file so I just added it to the opam file, there should be no need for another release, but for yaml, I think we'll need a new release.

@ghost ghost mentioned this pull request May 4, 2018
@ghost
Copy link
Author

ghost commented May 8, 2018

Not sure what's going on with the CI here, I constrained things more so there should be less failures... The new errors look unrelated to this PR

@ghost
Copy link
Author

ghost commented May 8, 2018

Ok, it looks better now. I tested manually and ocaml-topexpect 0.1, 0.2 and 0.3 don't build with OCaml >= 4.05, so I added more constraints. The last two failures - qcow 0.6 and 0.7 - are unrelated.

I think this PR is good to merge.

@ghost
Copy link
Author

ghost commented May 8, 2018

Actually I added the missing dependency on io-page-unix for qcow 0.6 and 0.7 and the result is a bit better now. The last failures are really unrelated to this PR

@hcarty
Copy link
Member

hcarty commented May 8, 2018

Agreed that this seems ready to merge.

@hannesm Do you want to take a look since you were reviewing this?

@trefis
Copy link
Contributor

trefis commented May 10, 2018

Ping, it would be nice to unsplit the world ASAP.

(x-ref mirage/ocaml-uri#120 )

@trefis
Copy link
Contributor

trefis commented May 10, 2018

Forget about the "unsplit the world part", apparently my opam has just gone crazy on its own.
Still, it would be nice to get this released.

@hannesm
Copy link
Member

hannesm commented May 10, 2018

sorry for the late reply. This is good to go. to unsplit the universe we'll still need to carefully adapt packages to the new ppx_sexp_conv world (thanks @diml for explaining the design considerations).

@hannesm hannesm merged commit 19027c6 into ocaml:master May 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants