Skip to content

Restrict ppx_deriving_yojson dependency to yojson < 1.6#13356

Merged
kit-ty-kate merged 10 commits into
ocaml:masterfrom
NathanReb:restrict-ppx-deriving-yojson-deps-on-yojson
Feb 7, 2019
Merged

Restrict ppx_deriving_yojson dependency to yojson < 1.6#13356
kit-ty-kate merged 10 commits into
ocaml:masterfrom
NathanReb:restrict-ppx-deriving-yojson-deps-on-yojson

Conversation

@NathanReb
Copy link
Copy Markdown
Contributor

Yojson 1.6 deprecates the type json in favor of the new type t.
This shouldn't be an issue but users of ppx_deriving_yojson and
dune are gonna have a hard time working with yojson 1.6 since warning 3
is fatal in the default dune profile and they can't change
ppx_deriving_yojson output.

Yojson 1.6 deprecates the type `json` in favor of the new type `t`.
This shouldn't be an issue but users of ppx_deriving_yojson and
dune are gonna have a hard time working with yojson 1.6 since warning 3
is fatal in the default dune profile and they can't change
ppx_deriving_yojson output.
@NathanReb
Copy link
Copy Markdown
Contributor Author

Looks like the usual revdeps failures, let me know if I missed one that I actually broke!

@camelus
Copy link
Copy Markdown
Contributor

camelus commented Feb 1, 2019

☀️ All lint checks passed 4208249
  • These packages passed lint tests: ppx_deriving_yojson.1.0, ppx_deriving_yojson.1.1, ppx_deriving_yojson.2.0, ppx_deriving_yojson.2.1, ppx_deriving_yojson.2.2, ppx_deriving_yojson.2.3, ppx_deriving_yojson.2.4, ppx_deriving_yojson.3.0, ppx_deriving_yojson.3.1, ppx_deriving_yojson.3.3

☀️ Installability check (10326 → 10326)

@NathanReb
Copy link
Copy Markdown
Contributor Author

Pushed some changes to hopefully make the linter happy and fix the broken builds for the older versions of ppx_deriving_yojson.

Comment thread packages/ppx_deriving_yojson/ppx_deriving_yojson.1.0/opam
Comment thread packages/ppx_deriving_yojson/ppx_deriving_yojson.1.1/opam
@NathanReb NathanReb force-pushed the restrict-ppx-deriving-yojson-deps-on-yojson branch from fdbd0fe to c126765 Compare February 3, 2019 20:09
@NathanReb
Copy link
Copy Markdown
Contributor Author

Ouh my bad, missed the camelus notification on Friday!

@NathanReb
Copy link
Copy Markdown
Contributor Author

Hmm now there's an issue with ppx_import, I'll look into it!

@mseri
Copy link
Copy Markdown
Member

mseri commented Feb 6, 2019

Looks like the only remaining issue is with ppx_import for ppx_deriving_yojson.1.0 on ocaml-4.03.0.

The rest:

@kit-ty-kate
Copy link
Copy Markdown
Member

Everything seems fine except on 4.02.3:

#=== ERROR while compiling ppx_deriving_yojson.1.0 ============================#
# context     2.0.0 | linux/x86_64 | ocaml-base-compiler.4.02.3 | file:///home/travis/build/ocaml/opam-repository
# path        ~/.opam/ocaml-base-compiler.4.02.3/.opam-switch/build/ppx_deriving_yojson.1.0
# command     ~/.opam/opam-init/hooks/sandbox.sh build ocamlbuild -classic-display -use-ocamlfind src_test/test_ppx_yojson.byte --
# exit-code   10
# env-file    ~/.opam/log/ppx_deriving_yojson-15051-03b1a4.env
# output-file ~/.opam/log/ppx_deriving_yojson-15051-03b1a4.out
### output ###
# ocamlfind ocamldep -package oUnit -package yojson -modules src_test/test_ppx_yojson.ml > src_test/test_ppx_yojson.ml.depends
# ocamlfind ocamlc -c -g -bin-annot -safe-string -ppx 'ocamlfind ppx_import/ppx_import' -ppx 'ocamlfind ppx_deriving/ppx_deriving src/ppx_deriving_yojson.cma' -package oUnit -package yojson -w @5@8@10@11@12@14@23@24@26@29@40 -I src_test -I src -o src_test/test_ppx_yojson.cmo src_test/test_ppx_yojson.ml
# + ocamlfind ocamlc -c -g -bin-annot -safe-string -ppx 'ocamlfind ppx_import/ppx_import' -ppx 'ocamlfind ppx_deriving/ppx_deriving src/ppx_deriving_yojson.cma' -package oUnit -package yojson -w @5@8@10@11@12@14@23@24@26@29@40 -I src_test -I src -o src_test/test_ppx_yojson.cmo src_test/test_ppx_yojson.ml
# ocamlfind: Package `ppx_import' not found
# File "src_test/test_ppx_yojson.ml", line 1:
# Error: Error while running external preprocessor
# Command line: ocamlfind ppx_import/ppx_import '/tmp/camlppxf2b7c6' '/tmp/camlppx741f78'
# 
# Command exited with code 2.

I'm not sure how this change introduces a failure as it seems that it was working in 3ae7241: http://check.ocamllabs.io/4.02.3/good/ppx_deriving_yojson.1.0

@kit-ty-kate
Copy link
Copy Markdown
Member

ah, got it.. this package tests during normal compilation

@NathanReb
Copy link
Copy Markdown
Contributor Author

Ouh sorry, got carried away, will have a look at it, thanks for the help!

@NathanReb
Copy link
Copy Markdown
Contributor Author

I added a missing test dependency in the 1.1 package but that won't solve the issue. At this point I don't see what I could do beside removing the test build since the tests appear to be broken.

@NathanReb
Copy link
Copy Markdown
Contributor Author

I tried running opam install -t for various versions of ppx_deriving_yojson on a local 4.02.3 switch and several of them are somehow broken so this is indeed unrelated to this PR.

I'll disable the test builds for the broken versions.

@NathanReb
Copy link
Copy Markdown
Contributor Author

There's still an issue with 3.x which tests won't build on 4.02 for some reason but will for later versions of ocaml.

Not sure what the best solution is here, maybe add an ocaml >= 4.03 lower bound on 3.0.

Considering that the package was already broken before this PR it might be worth considering merging it. I'm happy to fix those old versions but not at the expense of the important part (ie the upper bound on yojson).

@mseri
Copy link
Copy Markdown
Member

mseri commented Feb 7, 2019

If it does not compile, why not adding ocaml >= 4.03?
Then I'd be happy to merge. Thanks for the help

@NathanReb
Copy link
Copy Markdown
Contributor Author

It does compile, the tests don't ie opam install will work but opam install -t won't. That's why the CI fails from what I understood.

@kit-ty-kate
Copy link
Copy Markdown
Member

it looks like this works. I'll let the CI complete but it seems good

@NathanReb
Copy link
Copy Markdown
Contributor Author

I'd expect the 4.02.3 build to still fail, just on a later version, 2.x ones have being fixed indeed. If I'm wrong and it actually goes through then that's even better !

@kit-ty-kate
Copy link
Copy Markdown
Member

I'd expect the 4.02.3 build to still fail, just on a later version, 2.x ones have being fixed indeed. If I'm wrong and it actually goes through then that's even better !

well no, the build is already finished and everything is fine on 4.02.3 ^^

@kit-ty-kate
Copy link
Copy Markdown
Member

all green. Thanks a lot

@kit-ty-kate kit-ty-kate merged commit 49b71f2 into ocaml:master Feb 7, 2019
@NathanReb NathanReb deleted the restrict-ppx-deriving-yojson-deps-on-yojson branch February 21, 2019 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants