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

fix for ocamlbuild's pack+ocamlfind handling #144

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gasche
Copy link

@gasche gasche commented Mar 26, 2018

This PR fixes the issue that causes the build failure discussed in

ocaml/opam-repository#11628 (comment)

and

#143

I have another branch v0.5.4-ocamlbuild-pack against the v0.5.4 tag, where the _tags changes are slightly different.

ocamlbuild should pass -package(...) flags to ocamlfind when building
a -pack-ed file, see

  ocaml/opam-repository#11628 (comment)
Binaries in <bench/*>, <tests/*> depend on the runtime libraries
within ppx_sexp_conv and ocplib-endian.

The packed modules <src/nocrypto.cm{x,o}> also depend on the package
ppx_sexp_conv: its presence at pack-creation time influences the
generated .cmi interface, see

  ocaml/opam-repository#11628 (comment)

Note: the package ppx_sexp_conv.runtime-lib would suffice, but it is
only available as such under recent ppx_sexp_conv versions, so its
explicit use would make the build description (needlessly)
incompatible with older ppx_sexp_conv versions.
@hannesm
Copy link
Member

hannesm commented Mar 28, 2018

hmm, IIUC, this makes ppx_sexp_conv (which depends on ocaml-migrate-parsetree etc.) a runtime dependency of nocrypto!? my intuition is that these should better stay build-time only dependencies, is there another way?

@gasche
Copy link
Author

gasche commented Mar 28, 2018

ppx_sexp_conv (maybe only recent versions?) has a runtime library that I suppose they use for hygiene (to alias things from the global environment that code generators use), so I don't think we can avoid having it as a runtime dependency. There is a more precise ppx_sexp_conv.runtime-lib ocamlfind package, but I don't think using it improves things noticeably, and it breaks on older versions of the library.

@copy
Copy link

copy commented Apr 14, 2018

Note: the package ppx_sexp_conv.runtime-lib would suffice, but it is
only available as such under recent ppx_sexp_conv versions, so its
explicit use would make the build description (needlessly)
incompatible with older ppx_sexp_conv versions.

Wouldn't ppx_sexp_conv also pull in ppx_sexp_conv.expander, which in turn contains the whole build-time ppx machinery? sexplib v0.10 already included the runtime library, so I'd be in favour of constraining to sexplib>=v0.10 and using ppx_sexp_conv.runtime-lib rather than using ppx_sexp_conv.

@gasche
Copy link
Author

gasche commented Apr 14, 2018

You should test this, but I think from the META file that the .expander dependency should only exists under the ppx_driver predicate.

@copy
Copy link

copy commented Apr 14, 2018

You should test this, but I think from the META file that the .expander dependency should only exists under the ppx_driver predicate.

Ah, indeed it doesn't get linked. (tested by adding module X = Ppx_sexp_conv_expander in the code and making sure it doesn't compile).

@copy
Copy link

copy commented May 26, 2018

@gasche In the v0.5.4-ocamlbuild-pack branch the ocamlfind_and_pack commit is reverted. Is this due to the fix already being in ocamlbuild? I re-applied the commit and it compiles fine. I'm about to submit these commits as patch files to opam-repository, including the reverted commit.

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
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
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.

3 participants