Skip to content

ppx_sexp_conv v0.11.1 - #11901

Merged
hannesm merged 4 commits into
ocaml:masterfrom
janestreet:ppx_sexp_conv-v0.11.1
May 10, 2018
Merged

ppx_sexp_conv v0.11.1#11901
hannesm merged 4 commits into
ocaml:masterfrom
janestreet:ppx_sexp_conv-v0.11.1

Conversation

@ghost

@ghost ghost commented May 3, 2018

Copy link
Copy Markdown

Remove the dependency from ppx_sexp_conv.runtime-lib to base

@camelus

camelus commented May 3, 2018

Copy link
Copy Markdown
Contributor
✅ 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

hannesm commented May 3, 2018

Copy link
Copy Markdown
Member

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

ghost commented May 4, 2018

Copy link
Copy Markdown
Author

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

ghost commented May 4, 2018

Copy link
Copy Markdown
Author

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

ghost commented May 8, 2018

Copy link
Copy Markdown
Author

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

ghost commented May 8, 2018

Copy link
Copy Markdown
Author

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

ghost commented May 8, 2018

Copy link
Copy Markdown
Author

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

hcarty commented May 8, 2018

Copy link
Copy Markdown
Member

Agreed that this seems ready to merge.

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

@trefis

trefis commented May 10, 2018

Copy link
Copy Markdown
Contributor

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

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

@trefis

trefis commented May 10, 2018

Copy link
Copy Markdown
Contributor

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

hannesm commented May 10, 2018

Copy link
Copy Markdown
Member

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.

6 participants