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

ppx_deriving.4.4, with 4.08 support #14454

Merged
merged 5 commits into from
Jul 9, 2019
Merged

Conversation

gasche
Copy link
Member

@gasche gasche commented Jul 8, 2019

@camelus
Copy link
Contributor

camelus commented Jul 8, 2019

🌤️ opam-lint warnings 587c1be
  • ppx_deriving_rpc.5.9.0 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • visitors.20171124 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • visitors.20180306 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • visitors.20180513 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • These packages passed lint tests: js_of_ocaml-ppx_deriving_json.3.1.0, js_of_ocaml-ppx_deriving_json.3.2.0, js_of_ocaml-ppx_deriving_json.3.3.0, nocrypto.0.5.4-1, ppx_deriving.4.4, ppx_deriving_argparse.0.0.4, ppx_deriving_argparse.0.0.5, ppx_deriving_protobuf.2.6, ppx_deriving_yojson.3.3, ppx_deriving_yojson.3.4


☀️ Installability check (+1)
  • new installable packages (1): ppx_deriving.4.4

@gasche
Copy link
Member Author

gasche commented Jul 8, 2019

The revdeps look good (there are few 4.08-available dependencies to test, but some like lens are and they seem to work), so I'll cut a real release and un-draft this PR.

@kit-ty-kate
Copy link
Member

Once OCaml 4.08 enabled there was a lot of revdeps failures. I fixed most of them but this one seems rather weird:

#=== ERROR while compiling nocrypto.0.5.4-1 ===================================#
# context              2.0.4 | linux/x86_64 | ocaml-base-compiler.4.08.0 | git+file:///home/opam/opam-repository
# path                 ~/.opam/4.08/.opam-switch/build/nocrypto.0.5.4-1
# command              ~/.opam/4.08/bin/ocaml pkg/pkg.ml build --pinned false --tests false --jobs 1 --with-lwt true --xen false --freestanding false
# exit-code            1
# env-file             ~/.opam/log/nocrypto-17-c7ac5d.env
# output-file          ~/.opam/log/nocrypto-17-c7ac5d.out
### output ###
[...]
# + ocamlfind ocamlopt -pack -g -bin-annot -package ppx_sexp_conv -package bytes -package sexplib -package cstruct -I src src/native.cmx src/uncommon.cmx src/base64.cmx src/hash.cmx src/cipher_stream.cmx src/ccm.cmx src/numeric.cmx src/gcm.cmx src/cipher_block.cmx src/fortuna.cmx src/hmac_drgb.cmx src/rng.cmx src/rsa.cmx src/dsa.cmx src/dh.cmx -o src/nocrypto.cmx
# Fatal error: exception Invalid_argument("index out of bounds")
# Command exited with code 2.

There are a few broken packages to deal with but let's leave it like that for today

@mseri
Copy link
Member

mseri commented Jul 8, 2019

That's quite interesting, in principle it should not depend on ppx_deriving but just on ppxlib, right?

@gasche
Copy link
Member Author

gasche commented Jul 8, 2019

@kit-ty-kate thanks! I think that the failure that you are seeing on nocrypto is a 4.08.0 compiler bug that we have fixed in the 4.08 branch upstream, ocaml/ocaml#8770

@gasche
Copy link
Member Author

gasche commented Jul 9, 2019

I studied more revdeps failures, and I didn't find anything very suspicious. (ppx_deriving_argparse is failing because of a hardcoded .cmxs path that changed when we moved to a Dune build, I can send a fix upstream and ask for a new release.) There are some unrelated failures and some plugins that really need a 4.08 upgrade.

I will complete the release and rebase this PR to point to the definitive archive. Thanks again @kit-ty-kate for the work triaging the issues and marking 4.08 incompatibility in dependent packages.

@gasche
Copy link
Member Author

gasche commented Jul 9, 2019

Ahem. I force-pushed to my github clone out of habit, and lost the extra commits that @kit-ty-kate had pushed. Apologies... @kit-ty-kate, can I find them in your own github fork? (I just looked but I don't see the branch.)

@gasche
Copy link
Member Author

gasche commented Jul 9, 2019

@kit-ty-kate
Copy link
Member

I had them locally. It was pushed on your branch directly. You might want to use git push --force-with-lease by default, it's very handy

ppx_deriving.4.3 installs each plugin in a separate subdirectory,
which breaks the hardcoded paths in the build system of the "make
test" rule of ppx_deriving_argparse.
@kit-ty-kate
Copy link
Member

kit-ty-kate commented Jul 9, 2019

oops sorry my bad, I thought there was a mistake

@gasche
Copy link
Member Author

gasche commented Jul 9, 2019

Wow, --force-with-lease sounds very handy, thanks! (I wonder if there is a way to make it the default --force behavior.)

I just pushed an updated version of the branch that should be final (CI can always tell otherwise). I restricted the package to (< 4.09), so that we avoid use-side build failures in the future. I restricted ppx_deriving_argparse to 4.3 instead of 4.4 (the failure is not new).

@gasche gasche marked this pull request as ready for review July 9, 2019 12:56
@kit-ty-kate
Copy link
Member

I restricted ppx_deriving_argparse to 4.3 instead of 4.4 (the failure is not new).

Are you sure? check.ocamllabs.io tells me otherwise: http://check.ocamllabs.io/log/1562505242-7152ba482366e28003d2457509dd618295bc655d/4.07.1/good/ppx_deriving_argparse.0.0.4 (but that's not really important anyway)

@gasche
Copy link
Member Author

gasche commented Jul 9, 2019

The failure is only in the make test rule, but I thought I reproduced it locally under 4.3 and 4.07. Let me try again.

@gasche
Copy link
Member Author

gasche commented Jul 9, 2019

+ ocamlfind ocamlc -c -g -bin-annot -safe-string -ppx 'ocamlfind ppx_deriving/ppx_deriving src/ppx_deriving_argparse.cma /home/gasche/.opam/4.07.1/lib/ppx_deriving/ppx_deriving_show.cma' -I /home/gasche/.opam/4.07.1/lib/ppx_deriving/ -package result -package oUnit -w @5@8@10@11@12@14@23@24@26@29@40 -I src_test -I src -o src_test/test.cmo src_test/test.ml
File "_none_", line 1:
Error: Cannot load /home/gasche/.opam/4.07.1/lib/ppx_deriving/ppx_deriving_show.cmxs: cannot find file /home/gasche/.opam/4.07.1/lib/ppx_deriving/ppx_deriving_show.cmxs in search path

@kit-ty-kate kit-ty-kate merged commit 36df785 into ocaml:master Jul 9, 2019
@kit-ty-kate
Copy link
Member

thanks a lot!

@kit-ty-kate
Copy link
Member

Ping to maintainers of packages of which latest version is not available with OCaml 4.08:

@fpottier
Copy link
Contributor

fpottier commented Jul 10, 2019

Well, someone (@kit-ty-kate or @gasche?) marked visitors as incompatible with 4.08, without telling me. Would you care to explain what the problem is? Is it a problem with my code, or with some package that visitors depends upon, maybe ppx_deriving or ppx_tools?

@gasche
Copy link
Member Author

gasche commented Jul 10, 2019

That's part of the maintenance operations that we (most of that work is done by @kit-ty-kate) perform when the build of a package breaks (the idea is to mark it incompatible as soon as possible to avoid end-users seeing build failures). You were just "told" by the cc above: -- done by @kit-ty-kate when the change to the visitors description was merged in the main repository.

Here is what I get if I try to build visitors on 4.08:

+ ocamlfind ocamlc -c -g -safe-string -package compiler-libs.common -package ppx_deriving.api -w A-4-44 -package result -o VisitorsCompatibility.cmo VisitorsCompatibility.ml
File "VisitorsCompatibility.cppo.ml", line 128, characters 6-21:
Error: The constructor Otag expects 2 argument(s),
       but is applied here to 3 argument(s)

@gasche gasche changed the title [WIP] ppx_deriving.4.4 release preparation branch ppx_deriving.4.4, with 4.08 support Jul 10, 2019
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