ocaml-ppx / ppx_import

Less redundancy in type declarations and signatures
MIT License
89 stars 29 forks source link

[build] Migrate to `dune` / `ocaml-migrate-parsetree` / OPAM 2.0 #27

Closed ejgallego closed 6 years ago

ejgallego commented 6 years ago

Fixes #26. Changes:

We had to raise dependencies on quite a few packages, thus ppx_import now requires OCaml >= 4.04.2.

The question of whether we should use ppx_tools_versioned or ppxlib does remain, but it can be addressed at a later point.

ejgallego commented 6 years ago

typedtree mismatches:

ejgallego commented 6 years ago

Ok, so this seems very close to be ready to support a few OCaml versions after I added a small compatibility layer.

A pitfall remains, I had to comment out the following snippet: let oval = Primitive.print p oval in

I guess this should be solvable too, I'll have a look to see where the mismatch happens.

ghost commented 6 years ago

FTR, I submitted new releases of ocaml-migrate-parsetree and ppxlib with the necessary fixes:

We'll make a new dune release this week

ejgallego commented 6 years ago

We'll make a new dune release this week

Thanks a lot @diml , that should allow us to use the CI here.

ghost commented 6 years ago

All the new dependencies are now in opam.

ejgallego commented 6 years ago

All the new dependencies are now in opam.

Finally!!! That's great, thanks! I wonder tho, will the new packages arrive to OPAM 1.2, or it is too late?

In that case, I'll update the Travis setup to use OPAM 2.0 but that will take me more time.

ghost commented 6 years ago

No problem. I don't think they'll arrive to opam 1.2, IIRC the plan is that the opam 1 repo is now freezed

ejgallego commented 6 years ago

Et voilà ! There are still minor questions to solve, but this seems to be working.

ejgallego commented 6 years ago

See updated description.

gasche commented 6 years ago

I'm not sure what should be the review process for this (cc @whitequark who may have an opinion). My inclination would be to try the branch, check that it works on my machine and that I am comfortable with the workflow. If it does and I am, then I would assume that it would also work fine for other people, and merge the PR. Any comments?

ejgallego commented 6 years ago

that I am comfortable with the workflow.

Indeed the release workflow should need some adjustment, unfortunately dune-release doesn't seem to support OPAM 2.0 well, so maybe indeed opam-publish is the way to go for now.

whitequark commented 6 years ago

@gasche Looks good to me, please merge as soon as you think it's ready. I have so far been very happy with your stewardship of these packages, I couldn't have done it better.

ejgallego commented 6 years ago

Thanks a lot @gasche @whitequark ; code wise I think the only question remaining is what is the role of Primitive.print [which I had to disable as not to mess a bit with conversion]

ejgallego commented 6 years ago

Indeed the release workflow should need some adjustment, unfortunately dune-release doesn't seem to support OPAM 2.0 well, so maybe indeed opam-publish is the way to go for now.

Dune release now supports OPAM 2.

shonfeder commented 6 years ago

Just thought I'd ping on this PR, since it sounds ready to go. I've hit a couple cases where I would I could have used ppx_import in dune project recently.

ejgallego commented 6 years ago

Indeed thanks @shonfeder . There is a nit to solve maybe but otherwise the approach seems to work fine.

ejgallego commented 6 years ago

Dear @gasche , @whitequark , this seems stuck.

I'd be great if we could move it forward as it would allow a significant part of the toolchain we use to move to Dune.

What would you propose to try advance it? Thanks!

ejgallego commented 6 years ago

I would be happy to try to look for a ppx expert that could act as a shepherd if you think that may be an option and you both are busy.

gasche commented 6 years ago

Yes, I've been too busy to properly deal with this (I did try to build from your branch and it failed on my machine and I didn't have the time to understand what was broken in my build environment), and I think your idea of getting an external reviewer is good. (Usually I would ask @let-def, who loves the ppx stuff, but I don't know for this one.)

I think Primitive.print does need to be fixed. What it does is to handle external foo : ... = ... in signatures, and a ppx-import plugin may rely on those. Do you think it would be possible to keep OCaml-version-dependent logic just for this feature? (You could isolate it in a small submodule that would be the only one to use cppo.)

ejgallego commented 6 years ago

Hi @gasche , OK, I'll ask around.

I don't think the Primitive.print will need cppo, but let's see how it goes.

gasche commented 6 years ago

I asked myself whether it could be just copied over within ppx-import, but the logic is actually quite subtle now with the [@unboxed] attributes and in general attribute handling, so I don't think it is very reasonable to duplicate the standard implementation on versions 4.03 or above.

ejgallego commented 6 years ago

Thanks @gasche , it turns out the Outcometree problem was a silly capturing one; I realized after writing the useless migration function.

IMHO this is ready and the review seems pretty straightforward. The workflow should also be pretty easy; dune-release works for the package and seems to produce working packages.

ejgallego commented 6 years ago

I'd like to say I've tested this with my 2 packages and it works very beautifully.

gasche commented 6 years ago

I had given a very quick try to an earlier version of the PR, and it didn't build on my machine (I didn't have any time to investigate so I kept quiet about it). I just gave it another try and it seems to be working perfectly on either 4.02.2 and 4.07.1.

I'll merge now. This has waited too long, and it brings a very nice simplification by removing the version-specific code, enabling merlin, and bringing the nice dune features. Thanks a lot for your patience, @ejgallego.

ejgallego commented 6 years ago

Thanks to you @gasche , and of course to all that helped!

ejgallego commented 6 years ago

I will try to upload a OPAM package generated with dune-release to OPAM, let's see how CI goes for this one, then maybe it would be convenient to do a release.

ejgallego commented 6 years ago

https://github.com/ocaml/opam-repository/pull/12959