Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto-detect ppx_sexp_conv runtime dependencies #146

Closed
wants to merge 1 commit into from
Closed

Auto-detect ppx_sexp_conv runtime dependencies #146

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented May 11, 2018

This PR allows to automatically detect the runtime dependencies of ppx_sexp_conv in order to store them in the META file. This allows ocaml-nocrypto to be compatible with both ppx_sexp_conv < v0.11 and >= v0.11.

The method used should work with any ppx rewriter, not just ppx_sexp_conv.

@hannesm
Copy link
Member

hannesm commented May 23, 2018

thanks @diml, I looked into this locally, and discovered that this expands on my system to base.caml base.shadow_stdlib sexplib0 base ppx_sexp_conv.runtime-lib result ppx_deriving.runtime -- this means ppx_sexp_conv depends at runtime on base (similar to sexplib=v0.11.0), is this the desired behaviour (again, similar to sexplib, this is not a good option for MirageOS due to binary size)!?

@hannesm
Copy link
Member

hannesm commented May 23, 2018

oops, sorry, please ignore my comment, I was stuck with a sexplib v0.11.0, upgrading to v0.11.1 solved this issue.

@samoht
Copy link
Contributor

samoht commented May 23, 2018

Maybe we don't need to add this complexity to the build system and depend unconditionally on the latest version of ppx_sexp_conv?

@copy
Copy link

copy commented May 26, 2018

@samoht That's what #144 does.

copy added a commit to copy/opam-repository that referenced this pull request May 26, 2018
ppx_sexp_conv v0.11.0 compiles successfully, but contains an undesired
dependency on base, and is thus still marked as conflicting. This is
fixed in ppx_sexp_conv v0.11.1.

This commit submits @gasche's fixes from
mirleft/ocaml-nocrypto#144 and @diml's fixes
from mirleft/ocaml-nocrypto#146
copy added a commit to copy/opam-repository that referenced this pull request May 27, 2018
ppx_sexp_conv v0.11.0 compiles successfully, but contains an undesired
dependency on base, and is thus still marked as conflicting. This is
fixed in ppx_sexp_conv v0.11.1.

This commit submits @gasche's fixes from
mirleft/ocaml-nocrypto#144 and @diml's fixes
from mirleft/ocaml-nocrypto#146
copy added a commit to copy/opam-repository that referenced this pull request May 27, 2018
ppx_sexp_conv v0.11.0 compiles successfully, but contains an undesired
dependency on base, and is thus still marked as conflicting. This is
fixed in ppx_sexp_conv v0.11.1.

This commit submits @gasche's fixes from
mirleft/ocaml-nocrypto#144 and @diml's fixes
from mirleft/ocaml-nocrypto#146
copy added a commit to copy/opam-repository that referenced this pull request Jun 26, 2018
ppx_sexp_conv v0.11.0 compiles successfully, but contains an undesired
dependency on base, and is thus still marked as conflicting. This is
fixed in ppx_sexp_conv v0.11.1.

This commit submits @gasche's fixes from
mirleft/ocaml-nocrypto#144 and @diml's fixes
from mirleft/ocaml-nocrypto#146
@hcarty
Copy link

hcarty commented Aug 17, 2018

Where does this patch/fix stand currently? From what I can see, the current state in opam prevents the use of ppxlib and nocrypto (and therefore tls) in the same project.

@copy
Copy link

copy commented Aug 17, 2018

Where does this patch/fix stand currently? From what I can see, the current state in opam prevents the use of ppxlib and nocrypto (and therefore tls) in the same project.

The patches are in opam now, so nocrypto and ppxlib can be used together. The tls issue is being tracked here: mirleft/ocaml-tls#379, and we need to summon @hannesm for that.

@hcarty
Copy link

hcarty commented Aug 17, 2018

Thank you @copy!

@cfcs
Copy link

cfcs commented Feb 5, 2019

master got rid of the sexp stuff now, so I think this PR can be closed? a58c653

@XVilka
Copy link

XVilka commented Sep 15, 2019

Ping? Sounds like indeed should be closed.

@ghost
Copy link
Author

ghost commented Sep 16, 2019

Sure, let's close this

@ghost ghost closed this Sep 16, 2019
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

7 participants