-
Notifications
You must be signed in to change notification settings - Fork 47
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
4.06 support #66
4.06 support #66
Conversation
I tried to pin this branch (as my current mini-project is running into build failures left and right beyond my control), but I ended up with the same constraint error as indicated in the CI test:
I went digging in the source code, hoping I could remove this constraint, but I couldn't find anywhere in the source where such a constraint was specified! I am afraid I am simply hitting against my own ignorance (again), but I risk chiming in here any how; for I would really like to see this compatibility achieved quickly (even if I have to patch the code myself or pin a dev branch branch), and I am trying to find ways of contributing constructively. @gasche Would you happen to know where I should search for the source of this constraint? I'd be happy to make a PR into your branch with the fix if I can figure out where I should be looking! |
Have you done |
Oh shoot, I didn't read that compilation output correctly. Sorry about that! The output pasted is actually from the travis-ci build failure for this PR (which I assume must just need it's opam updated?). I apologize for troubling you with the same noise as on the other PR. What I meant to report was the constraint failure I get when trying to pin your branch:
This is likely just my own ignorance again, but I thought it may be worth mentioning, as it looks to me like it should work. Thank for your work here, and, again, for the corrections. |
@shonfeder It needs to be (notice the .git):
It would be nice if opam complained about it. Can I also pin pull requests directly? |
On 4.06, the Travis fails because it uses opam-available versions of ppx_import, so I need ocaml-ppx/ppx_import#19 to be turned into a release before this can work. On 4.05, it seems that the problem is that 4.2 and 4.2.1 have incompatible APIs due to the change in variable types. I had not foreseen that issue (I worked on the patches that went into 4.2.1 on top of the 4.1 branch, which didn't support 4.05 and thus didn't have this issue), and I think it would have been better to call 4.2.1 a 4.3.0 release for this reason. I am not sure what is best to do. The plan is to resolve both shortly and do a release of ppx_deriving_yojson on opam that works well with 4.06 and ppx_deriving.4.2.1. |
ppx_import 1.4 (with 4.06 support) just reached the public opam repository. This means that 4.06 should not be testable by the CI here. The 4.05.0 issue remains. (It seems that the Travis build doesn't see the updated ppx_import yet, I don't know what is causing latency or how much latency there is, so I'll wait until tomorrow to relaunch the CI.) |
I tested on top of ocaml-ppx/ppx_deriving#159 (not merged in ppx_deriving yet), this should be the only additional change required for ppx_deriving_yojson to work under 4.06.
This is an alternative to #64, which is a sensibly more invasive change.