Ppxlib.0.26.0 compatibility

[pitag-ha]

This is a patch PR to make the PPX compatible with ppxlib.0.26.0 which has bumped the AST to 4.14/5.00.

[pitag-ha]

Thanks for the PR! CI seems unhappy tho, any idea @pitag-ha ?

For the CI to run, we'll probably have to wait for the patch PR on ppx_sexp_conv to be merged and released first: ppx_import depends on ppx_sexp_conv, which currently has a ppxlip<2.26.0 bound.

[tatchi]

Version 0.15.1 of ppx_sexp_conv is available on opam now so it should work 😁

[ejgallego]

I tried re-running the CI , but it seems 4.05-4.09 don't find a suitable opam setup. @pitag-ha , what's the next step here?

[tatchi]

I forced a minimum version of ppx_sexp_conv to be v0.15.1 and got a better error:

  * No agreement on the version of ocaml:
    - (invariant) → ocaml = 4.09.2
    - ppx_import → ppx_sexp_conv >= v0.15.1 → base >= v0.15 → ocaml >= 4.10.0

So my understanding is that to support ppxlib>=0.26, we need ppx_sexp_conv>=0.15.1. But ppx_sexp_conv.0.15.1 has a dependency of base>=0.15 that itself only supports OCaml version greater or equal to 4.10

[ejgallego]

Thanks for the analysis @tatchi , I guess we could maybe drop support for OCaml 4.09 and lower, but that would require doing a 2.0 branch [so maybe we can also then merge #68

Edit, instead of a 2.0 branch, we could do 1.x branch, and move 1.x to maintenance mode.

[ejgallego]

So my understanding is that to support ppxlib>=0.26, we need ppx_sexp_conv>=0.15.1. But ppx_sexp_conv.0.15.1 has a dependency of base>=0.15 that itself only supports OCaml version greater or equal to 4.10

I don't indeed see a way out of this problem, seems to me that base bump on the minimal OCaml version is quite restrictive.

[ejgallego]

Well, one way to maybe make this work is only to run the test suite in OCaml 4.10 and onwards, that works, I've checked, but maybe a bit risky in terms of testing.

[gasche]

The current Debian Stable, released in August 2021, has OCaml 4.11. So maybe requiring 4.10 is okay? If it's not, I guess you need to discuss with ppxlib maintainers to see if they could support older releases.

[pitag-ha]

Thanks for having a look at this, @tatchi!

I guess you need to discuss with ppxlib maintainers to see if they could support older releases

@gasche , what are you referring to? Ppxlib supports all compiler versions from 4.04.1 on. Do you mean to see if ppxlib can stay compatible with older versions of ppx_sexp_conv? If so: I could see if the ppx_sexp_comv maintainers (i.e. Jane street) would be willing to also merge and release my patch PR on top of their 0.14 branch.

[gasche]

I misunderstood from the above that ppxlib somehow creates the dependency on a recent base. In fact we (ppx_import) depend on ppx_sexp_conv for a test where we import a type to do [@@deriving sexp] (since #54). The test was added because existing tests did not catch a sexp-related issue. I guess that @ejgallego now has to decide whether he prefers to drop support for older OCaml versions or drop the test. (Or: configure the build system to disable this test on older OCaml version, but this way lies more complexity and pain.)

[ejgallego]

I'm fine disabling the test in < 4.10 , it is not so complex, just tweak the CI file and add a new target to the test makefile (I'll be happy to help if that's an issue)

[ejgallego]

We need to tweak the opam file too as to install the stuff condictionally on the OCaml version.

[ejgallego]

Great @pitag-ha , it seems to work, thanks! I will prepare a new release. Would you like to squash the last commits so history is a bit cleaner?

[pitag-ha]

I will prepare a new release.

Perfect, thank you!

Would you like to squash the last commits so history is a bit cleaner?

Sure, I've just done that.

[pitag-ha]

Or did you want me to sqash more commits?

[ejgallego]

Or did you want me to sqash more commits?

As you prefer, IMO it makes sense that commits are structured in such a way that a particular checkout is consistent, so indeed, after a review I think it makes sense to squash them all. For example the first commit only would not pass CI, the second would miss a change, etc...

By the way, was @gasche question above resolved?

[pitag-ha]

As you prefer, IMO it makes sense that commits are structured in such a way that a particular checkout is consistent, so indeed, after a review I think it makes sense to squash them all.

Yes, I agree. I've done that now.

By the way, was @gasche question above resolved?

@gasche's proposal above is still being blocked by the fact that the compiler Types module doesn't reflect that type variables can be explicitly bount now (unless there's something I'm missing). I've just opened an issue at the compiler to ask if it might make sense to change that. I don't expect an answer right away though. I think it would make sense to merge and release this the way it is now. If we do so, ppx_import would become compatible with compilers up to 5.0 (now it's only up to 4.13) with the only caveat that folks using ppx_import can't explicitly bind type variables for now. Once we've sorted that out with the compiler (or found another solution), maybe I can just open another 2-liner PR to fix that and you could cut a point release? What do you think?

[ejgallego]

Thanks a lot, I think the plan looks good!

[pitag-ha]

Just to come back to the last question that was still kind of open: as pointed out here, explicit binders of type variables don't have any impact on the semantics of the program (and, btw, they don't even have an impact on the error reporting). So it does seem like it's ok for ppx_import to throw them away, doesn't it? The only way people might notice would be by running the PPX driver manually and inspecting the returned AST directly without passing it to the compiler. I think that wouldn't be a problem. Let me know if anyone disagrees though, please.