-
Notifications
You must be signed in to change notification settings - Fork 574
Use upstream graphql_ppx
, upgrade ppxlib
#11114
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
Conversation
230d18a
to
7dc44c3
Compare
883beca
to
76d639f
Compare
Note: the new upstream |
PPXs seem fixed, now fixing the GraphQL interface/queries |
f3a0cb3
to
549c8d6
Compare
6bc0e00
to
3740fad
Compare
graphql_ppx
, upgrade ppxlib
graphql_ppx
, upgrade ppxlib
This PR is more or less now ready for review! I still want to see if I can use a config file to factorize the ppx parameters, but that can be done in parallel with reviewing. It uses the upstream
I expect we will want to customize The changes are separated in atomic commits, and I suggest to start a review with commit 93f38b6, which is representative of the rest. |
@@ -235,3 +238,36 @@ installed: [ | |||
"zarith_stubs_js.v0.14.1" | |||
"zed.3.1.0" | |||
] | |||
pinned: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
random thought looking at this file. I was investigating adding pins here instead of our setup script, but modifying this file by hand seems hard (no documentation on how to do it that I could find) and error prone (as it is supposed to be a machine generated file).
It would be nice to be able to list all the dependencies, all the pins, and the OCaml version we use, in a single .toml
or .json
file (similar to a Cargo.toml
in Rust)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'd really like to add all pins here in the long term, and potentially get rid of submodules. I'd argue that modifying this file by hand is feasible (this is what I've been doing thus far), but we can talk about generating it from something else.
We can have that discussion in this issue: #10982
So, it seems factorizing the
I have a PR to fix 2 in dune (ocaml/dune#5820), and I'll do the factorization in a different PR, once the dune change has landed in a release. |
Closes #11081
This:
graphql_ppx
andppx_deriving_yojson
submodulesppxlib
and removes old PPX toolinggraphql_ppx
to upstream'smaster
branch (until there is a release for it)ppxlib
upgradeppx_optcomp
(actually, removeactually, no): Merge upstream v14.3 for recentppx_optcomp
ppxlib
compatibility ppx_optcomp#8ppx_version
: Use latest ppxlib AST o1-labs/ppx_version#37ppx_coda
snarky
's PPX (both forcompatible
anddevelop
): Upgrade for latest ppxlib (coda branch) o1-labs/snarky#633 fordevelop
,compatible
no PR neededppx_util
ppx_register_event
crypto_params
genmina_base
gengen_values
bsDecoder -> ppxCustom
, ...)ppxCustom
(when one way is unavailable, failwith)extend
? by modifying)graphql_ppx
?fix frontend?doesn't seem necessaryjs_of_ocaml
upgradeadds a config file to configure the GraphQL ppx globally?Will be done in another PR, once Allow list expansion in preprocessor arguments ocaml/dune#5820 has landed in dune