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

4.06 support #66

Merged
merged 2 commits into from
Nov 24, 2017
Merged

4.06 support #66

merged 2 commits into from
Nov 24, 2017

Conversation

gasche
Copy link
Contributor

@gasche gasche commented Nov 13, 2017

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.

@shonfeder
Copy link

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:

The following dependencies couldn't be met:
  - ppx_deriving_yojson -> ppx_deriving < 5.0
Your request can't be satisfied:
  - ppx_deriving<5.0 is not available because your system doesn't comply with ocaml-version > "4.03.0" & opam-version >= "1.2" & ocaml-version < "4.06.0".

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!

@gasche
Copy link
Contributor Author

gasche commented Nov 22, 2017

Have you done opam update recently? It may be that you don't see the new ppx_deriving release because your repository copy is not up-to-date.

@shonfeder
Copy link

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:

$ opam pin add ppx_deriving_yojson https://github.com/gasche/ppx_deriving_yojson\#4.06-support
[NOTE] Package ppx_deriving_yojson is already http-pinned to https://github.com/gasche/ppx_deriving_yojson#4.06-support.
       This will erase any previous custom definition.
Proceed ? [Y/n] y

Processing: [ppx_deriving_yojson: http]
[ppx_deriving_yojson] https://github.com/gasche/ppx_deriving_yojson#4.06-support downloaded

ppx_deriving_yojson needs to be installed.
[ERROR] ppx_deriving_yojson.3.0 is not available because your system doesn't comply with ocaml-version < "4.05.0".
[NOTE] Pinning command successful, but your installed packages may be out of sync.

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.

@vogler
Copy link

vogler commented Nov 23, 2017

@shonfeder It needs to be (notice the .git):

opam pin add ppx_deriving_yojson https://github.com/gasche/ppx_deriving_yojson.git\#4.06-support

It would be nice if opam complained about it. Can I also pin pull requests directly?
Also, thank you @gasche! ppx_deriving_yojson and ppx_import are both working for me on 4.06.

@gasche
Copy link
Contributor Author

gasche commented Nov 23, 2017

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.

@gasche
Copy link
Contributor Author

gasche commented Nov 24, 2017

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

@gasche gasche merged commit d846d39 into ocaml-ppx:master Nov 24, 2017
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.

3 participants