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

Use upstream graphql_ppx, upgrade ppxlib #11114

Merged
merged 28 commits into from
Jun 7, 2022
Merged

Conversation

Firobe
Copy link
Contributor

@Firobe Firobe commented May 27, 2022

Closes #11081

This:

  • removes the graphql_ppx and ppx_deriving_yojson submodules
  • upgrades ppxlib and removes old PPX tooling
  • pins graphql_ppx to upstream's master branch (until there is a release for it)
  • fixes broken PPXs across the code base broken by the ppxlib upgrade
  • fixes GraphQL queries and schemas broken by the upgrade
    • replace deprecated directives by new ones (bsDecoder -> ppxCustom, ...)
    • group decoders and encoders into modules that can be fed to ppxCustom (when one way is unavailable, failwith)
    • adapt interface around generated modules (using extend? by modifying graphql_ppx?)
    • fix CLI
    • fix Rosetta
    • fix Archiver
    • fix integration tests
    • fix frontend? doesn't seem necessary
  • fixes breakage due do js_of_ocaml upgrade
  • fixes conflict with compatbile
  • adds 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

@Firobe Firobe changed the base branch from develop to compatible May 27, 2022 10:49
@Firobe
Copy link
Contributor Author

Firobe commented May 30, 2022

Note: the new upstream graphql_ppx cannot be mixed with ppx_optcomp due to their nature. Also, ppx_jane implicitly contains ppx_optcomp, so when using graphql_ppx, ppx_jane must be deconstructed into only its needed ppxs.

@Firobe
Copy link
Contributor Author

Firobe commented May 30, 2022

PPXs seem fixed, now fixing the GraphQL interface/queries

@Firobe Firobe added the ci-build-me Add this label to trigger a circle+buildkite build for this branch label May 31, 2022
@Firobe Firobe changed the title [WIP] Use upstream graphql_ppx, upgrade ppxlib Use upstream graphql_ppx, upgrade ppxlib Jun 1, 2022
@Firobe
Copy link
Contributor Author

Firobe commented Jun 1, 2022

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 graphql_ppx with no modification, and adapts the codebase around it. CI is green (except an unrelated failure).

There is no develop PR yet, but it will be in the same vein.
Companion PR: #11159

I expect we will want to customize graphql_ppx to avoid some of the work-arounds I've used, but I believe we can start from this proof-of-concept and we should merge the ppxlib etc. update sooner rather than later. No changes to the old custom graphql_ppx were ported for now, as most do not seem necessary, but feel free to tell me otherwise.

The changes are separated in atomic commits, and I suggest to start a review with commit 93f38b6, which is representative of the rest.

@Firobe Firobe marked this pull request as ready for review June 1, 2022 17:51
@Firobe Firobe requested review from a team, bkase, psteckler, imeckler and mrmr1993 as code owners June 1, 2022 17:51
@@ -235,3 +238,36 @@ installed: [
"zarith_stubs_js.v0.14.1"
"zed.3.1.0"
]
pinned: [
Copy link
Contributor

@mimoo mimoo Jun 2, 2022

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)

Copy link
Contributor Author

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

@Firobe
Copy link
Contributor Author

Firobe commented Jun 3, 2022

So, it seems factorizing the graphql_ppx arguments cannot be done yet because

  1. The bsconfig.json file way is not working
  2. Dune cannot expand lists in ppx parameters yet.

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.

@Firobe Firobe merged commit 68c01cb into compatible Jun 7, 2022
@Firobe Firobe deleted the upgrade-graphql-ppx branch June 7, 2022 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-build-me Add this label to trigger a circle+buildkite build for this branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade graphql_ppx to use ppxlib
6 participants