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

fixes following core suite v0.11 #11628

Merged
merged 3 commits into from
Mar 22, 2018
Merged

fixes following core suite v0.11 #11628

merged 3 commits into from
Mar 22, 2018

Conversation

trefis
Copy link
Contributor

@trefis trefis commented Mar 22, 2018

I left the following packages broken:

  • camlhighlight (a failed build is logged here)
  • nocrypto (a failed build is logged here)
  • yaml

That last one doesn't have any constraint at the moment, looking at the code it opens Sexplib.Conv without depending on sexplib in the opam file: someone else will have to look at it.

For the other two, I'm not sure what's going on. The build fails because some cmi files are not available (because the right -I haven't been passed).
It might be an issue with the content of some of our META files (in which case it's a jbuilder issue I guess), or it might just be that their build system is confused: IDK.

@avsm
Copy link
Member

avsm commented Mar 22, 2018

i'll fix yaml, thanks :)

@camelus
Copy link
Contributor

camelus commented Mar 22, 2018

❗ opam-lint warnings 77f9d92
  • frag.0.1.0 has some warnings:

    • warning 27: No field 'remove' while a field 'install' is present, uncomplete uninstallation suspected
    • warning 36: Missing field 'bug-reports'
    • warning 37: Missing field 'dev-repo'
  • osm_xml.0.0.1 has some warnings:

    • warning 36: Missing field 'bug-reports'
  • These packages passed lint tests: async_inotify.v0.10.0, async_inotify.v0.9.0, bistro.0.3.0, bitcoinml.0.2.4, bitcoinml.0.2, bitcoinml.0.3.0, bitcoinml.0.3.1, boomerang.1.1.0, capnp.3.2.0, cfstream.1.2.3, conduit-async.1.0.3, core-lwt.0.2.0, netml.0.1.0, prettiest.0.0.1, prettiest.0.0.2, session-postgresql-async.0.4.0, trie.0.1.1


✅ Installability check (8602 → 8602)

@avsm
Copy link
Member

avsm commented Mar 22, 2018

#=== ERROR while installing prettiest.0.0.1 ===================================#
# opam-version         1.2.2 (aa258ecc06d3aea5a67f442a4ffd23f2a457180b)
# os                   linux
# command              jbuilder build -p prettiest -j 2
# path                 /home/opam/.opam/4.05.0/build/prettiest.0.0.1
# compiler             4.05.0
# exit-code            1
# env-file             /home/opam/.opam/4.05.0/build/prettiest.0.0.1/prettiest-7-eecdc3.env
# stdout-file          /home/opam/.opam/4.05.0/build/prettiest.0.0.1/prettiest-7-eecdc3.out
# stderr-file          /home/opam/.opam/4.05.0/build/prettiest.0.0.1/prettiest-7-eecdc3.err
### stderr ###
#       ocamlc lib/.prettiest.objs/prettiest.{cmo,cmt} (exit 2)
# (cd _build/default && /home/opam/.opam/4.05.0/bin/ocamlc.opt -w -40 -g -bin-annot -I lib/.prettiest.objs -I /home/opam/.opam/4.05.0/lib/base -I /home/opam/.opam/4.05.0/lib/base/caml -I /home/opam/.opam/4.05.0/lib/base/shadow_stdlib -I /home/opam/.opam/4.05.0/lib/ppx_compare/runtime-lib -I /home/opam/.opam/4.05.0/lib/sexplib/0 -no-alias-deps -o lib/.prettiest.objs/prettiest.cmo -c -impl lib/prettiest.pp.ml)
# File "lib/prettiest.ml", line 68, characters 43-48:
# Error: Unbound value force
#     ocamlopt lib/.prettiest.objs/prettiest.{cmx,o} (exit 2)
# (cd _build/default && /home/opam/.opam/4.05.0/bin/ocamlopt.opt -w -40 -g -I lib/.prettiest.objs -I /home/opam/.opam/4.05.0/lib/base -I /home/opam/.opam/4.05.0/lib/base/caml -I /home/opam/.opam/4.05.0/lib/base/shadow_stdlib -I /home/opam/.opam/4.05.0/lib/ppx_compare/runtime-lib -I /home/opam/.opam/4.05.0/lib/sexplib/0 -no-alias-deps -o lib/.prettiest.objs/prettiest.cmx -c -impl lib/prettiest.pp.ml)
# File "lib/prettiest.ml", line 68, characters 43-48:
# Error: Unbound value force


@trefis
Copy link
Contributor Author

trefis commented Mar 22, 2018

That failure is happening with base v0.9, indeed force was added in v0.10.
I guess this package needs a lower bound as well...

@kit-ty-kate
Copy link
Member

Good enough. I'll add the lower-bound in a followup pr

@kit-ty-kate kit-ty-kate merged commit e7296aa into ocaml:master Mar 22, 2018
@kit-ty-kate
Copy link
Member

Ping @pqwy & @hannesm for nocrypto (see the first message)

@kit-ty-kate
Copy link
Member

@trefis
Copy link
Contributor Author

trefis commented Mar 23, 2018

Ping @diml as well: could the failures we don't understand be caused by some changes in how jbuilder generates META files?

For the record: Ppx_sexp_conv_lib is defined as:

include Base.Exported_for_specific_use.Ppx_sexp_conv_lib

And in there, Sexp.t is = Sexplib0.Sexp.t = Atom of string | List of t list so I don't understand what's going on.

@ghost
Copy link

ghost commented Mar 23, 2018

I don't think so, there have been no changes in generated META file for a while.

@ghost
Copy link

ghost commented Mar 23, 2018

I think it's a problem with the hand-written META file for ipaddr.2.7.2 that is the problem:

version = "2.7.2"
description =
"A library for manipulation of IP (and MAC) address representations"
requires = "bytes sexplib"

i.e. it doesn't mention ppx_sexp_conv.runtime-lib, as a result the compiler doesn't see ppx_sexp_conv_lib.cmi when compiling mirage. Ipaddr.2.8.0 is using jbuilder so its META file will be correct, I suggest to simply bump the version constraint on ipaddr.

@kit-ty-kate
Copy link
Member

@diml according to this log this is not the issue: https://travis-ci.org/ocaml/opam-repository/jobs/355944133

@ghost
Copy link

ghost commented Mar 23, 2018

It definitely was for mirage, I reproduced the error on my machine, and upgrading ipaddr to 2.8.0 fixed the issue

@ghost
Copy link

ghost commented Mar 23, 2018

I found the issue for nocrypto: when creating the packed module (with ocamlfind ocamlopt -pack), the build system doesn't pass the -package .. options. If I run the same command with -package ppx_sexp_conv,sexplib the build succeeds.

@pqwy you should try jbuilder for nocrypto :)

gasche added a commit to gasche/ocaml-nocrypto that referenced this pull request Mar 26, 2018
ocamlbuild should pass -package(...) flags to ocamlfind when building
a -pack-ed file, see

  ocaml/opam-repository#11628 (comment)
gasche added a commit to gasche/ocaml-nocrypto that referenced this pull request Mar 26, 2018
Binaries in <bench/*>, <tests/*> depend on ppx_sexp_conv's runtime
library within ppx_sexp_conv.

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.
gasche added a commit to gasche/ocaml-nocrypto that referenced this pull request Mar 26, 2018
ocamlbuild should pass -package(...) flags to ocamlfind when building
a -pack-ed file, see

  ocaml/opam-repository#11628 (comment)
gasche added a commit to gasche/ocaml-nocrypto that referenced this pull request Mar 26, 2018
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.
gasche added a commit to gasche/ocamlbuild that referenced this pull request Mar 26, 2018
Instead of a separate category, "pack" is now another form of linking
like "program", "library", "toplevel" and "output_obj". This fixes the
issue that package(...) tags where not passed at pack-production time,
spotted by Jérémie Dimino in
  <ocaml/opam-repository#11628 (comment)>
  <janestreet/ppx_sexp_conv#20>
More generally, this extends the meaning of all "link" flags to
"pack", which seems to be the correct behavior for all the rules we
inspected.

I tested that this change fixes the original 'nocrypto' issue
(once their _tags is also fixed to provide the right package(..)
option at pack-time).

This is a somewhat invasive change to the behavior of "pack" and
"link" tags, which may affect all user-defined flag declarations. An
alternative to this change would be to just add a specific "package"
rule when the "pack" tag is present, but my understanding is that
linking options quite generally apply to "pack" productions
(that really behave mostly like library-archive production already
under the ("link";"library") regime), so this more invasive change is
more correct: the extra cases where it applies were likely to be small
bugs before.

(I checked that ocb-stubblr is orthogonal to this change and does not
need any update.)
gasche added a commit to gasche/ocamlbuild that referenced this pull request Mar 26, 2018
Instead of a separate category, "pack" is now another form of linking
like "program", "library", "toplevel" and "output_obj". This fixes the
issue that package(...) tags where not passed at pack-production time,
spotted by Jérémie Dimino in
  <ocaml/opam-repository#11628 (comment)>
  <janestreet/ppx_sexp_conv#20>
More generally, this extends the meaning of all "link" flags to
"pack", which seems to be the correct behavior for all the rules we
inspected.

I tested that this change fixes the original 'nocrypto' issue
(once their _tags is also fixed to provide the right package(..)
option at pack-time).

This is a somewhat invasive change to the behavior of "pack" and
"link" tags, which may affect all user-defined flag declarations. An
alternative to this change would be to just add a specific "package"
rule when the "pack" tag is present, but my understanding is that
linking options quite generally apply to "pack" productions
(that really behave mostly like library-archive production already
under the ("link";"library") regime), so this more invasive change is
more correct: the extra cases where it applies were likely to be small
bugs before.

(I checked that ocb-stubblr is orthogonal to this change and does not
need any update.)
hannesm pushed a commit to hannesm/ocaml-nocrypto that referenced this pull request May 23, 2018
ocamlbuild should pass -package(...) flags to ocamlfind when building
a -pack-ed file, see

  ocaml/opam-repository#11628 (comment)
hannesm pushed a commit to hannesm/ocaml-nocrypto that referenced this pull request May 23, 2018
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 pushed a commit to hannesm/ocaml-nocrypto that referenced this pull request Jan 12, 2019
ocamlbuild should pass -package(...) flags to ocamlfind when building
a -pack-ed file, see

  ocaml/opam-repository#11628 (comment)
hannesm pushed a commit to hannesm/ocaml-nocrypto that referenced this pull request Jan 12, 2019
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.
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.

4 participants