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

allows newer ppx_yojson_conf version #833

Merged
merged 2 commits into from
Jul 24, 2024

Conversation

CodiePP
Copy link
Contributor

@CodiePP CodiePP commented Jul 16, 2024

in newer ppx_yojson_conv versions apparently for deriving yojson types of primitives one has to include some declarations.
I was using v0.17 and this solved the compilation errors.

the opam file had a duplicate entry for the ppx_yojson_conv library. 🍤

@gares
Copy link
Member

gares commented Jul 16, 2024

There was an upper bound, dunno why, but I'd be happy to remove it. Unfortunately Ci is not picking the version you mention up, maybe because ocaml is 4.13? We can surely switch to 4.14 in the opam job. Would you mind trying?

CodiePP added 2 commits July 23, 2024 09:43
Signed-off-by: Alexander Diemand <[email protected]>
Signed-off-by: Alexander Diemand <[email protected]>
@CodiePP CodiePP force-pushed the update_newer_ppx_yojson_conv branch from b2526c5 to 0904539 Compare July 23, 2024 07:43
@CodiePP
Copy link
Contributor Author

CodiePP commented Jul 23, 2024

yes, seems to work fine. I recompiled using OCaml 4.14.2

ocaml                 4.14.2      The OCaml compiler (virtual package)
ppx_yojson_conv       v0.16.0     [@@deriving] plugin to generate Yojson conversion functions
ppx_yojson_conv_lib   v0.16.0     Runtime lib for ppx_yojson_conv

and it picked up v0.16.0 which let the dependent packages compile.
also, rebased on main.

@@ -11,7 +11,7 @@ build: [
[ "dune" "build" "-p" name "-j" jobs ]
]
depends: [
"ocaml" { >= "4.13.1" }
"ocaml" { >= "4.14" }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the new lower bound ? Do your changes not work with ocaml < 4.14 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it requires at least 4.14.0 (see https://ocaml.org/p/ppx_yojson_conv/v0.16.0)
and, now it is more consistent in the repo among flake.nix and ci, cd Github actions

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gares I think that sounds find ! You can make the final call to merge !

@gares gares merged commit 762363a into coq:main Jul 24, 2024
23 checks passed
@CodiePP CodiePP deleted the update_newer_ppx_yojson_conv branch July 24, 2024 16:43
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