ocaml-ppx / ppx_tools

Tools for authors of ppx rewriters
MIT License
134 stars 39 forks source link

Upgrade to opam 2, switch the build system to Dune and make ppx_tools.metaquot usable with dune #67

Closed ghost closed 5 years ago

ghost commented 5 years ago

This PR switches the build system to dune, upgrades the opam file to opam 2 and adds a bit of glue code so that ppx_tools.metaquot can be easily used with dune.

alainfrisch commented 5 years ago

I'm all for such modernization, but I'm not familiar with the tools involved here, so I cannot really review this. @diml, if you are confident enough, I can merge blindly, but perhaps someone else (@Drup, also a maintainer) wants to review?

ghost commented 5 years ago

I looked at what is installed by the package and tested with reverse dependencies, so I'm confident enough. I noticed one issue with one package (ppx_deriving) due to changes to the META file, though the fix in ppx_deriving is trivial.

I'm also happy to be added as maintainer of ppx_tools so that I can quickly release a fix if needed.

Drup commented 5 years ago

I wonder if we shouldn't just merge ppx_tools and ppx_tools_versionned. users should go with the later anyway, and it uses omp, which avoids the need for this micro-driver.

ghost commented 5 years ago

Yeah, that make sense. @alainfrisch and @let-def what do you think?

Maybe we should reuse the name ppx_tools as well, which is shorter. i.e. we instead merge ppx_tools_versioned into ppx_tools and make ppx_tools_versioned an alias for ppx_tools.

Drup commented 5 years ago

I propose this because the micro-driver module you added bother me a little bit, and I want to avoid yet-another-ppx-driver. omp has one and it's clearly going to be the main non-jst driver, going forward, so it's better to use that one.

Apart from that particular point, the patch looks good, and I trust @diml to know how to write dune files. :)

ghost commented 5 years ago

Ok. As a first step, I can also update this PR to use the omp driver and get rid of the micro driver. Are you happy with ppx_tools depending on omp?

Drup commented 5 years ago

I'm of the opinion that every ppx should use omp, so yes. If @alainfrisch agrees in making ppx_tools and ppx_tools_versionned converge, then we can proceed.

let-def commented 5 years ago

I agree that ppx_tools_versioned and ppx_tools should be merged. However, when I started ppx_tools_versioned, I didn't put genlifter in it. The reason is that it is "one meta-level" above the other tools: (1) it parses the compiler data structures to produce (2) a code that will produce (3) a parsetree when executed.

I don't think it makes sense to put genlifter in a versioned ppx_tools (unless someone find a clear semantics for that, that could not be achieved by simpler means). The intuitive version-agnostic behavior can be recovered by composing a version-specific genlifter with omp:

So what about merging ppx_tools_versioned into ppx_tools and making genlifter a separate project?

ghost commented 5 years ago

That seems fine to me.

In ppxlib we have something that serves a purpose slightly similar to genlifter: [@@deriving traverse_lift]:

utop # #require "ppxlib.traverse";;
utop # type t = A of string [@@deriving traverse_lift];;
type t = A of string
class virtual ['res] lift :
  object
    method virtual constr : string -> 'res list -> 'res
    method virtual string : string -> 'res
    method t : t -> 'res
  end
# let x = object(self)
inherit [_] lift as super
method constr name args = `Constr (name, args)
method string x = `String x
end;;
val x : (_[> `Constr of string * 'a list | `String of string ] as 'a) lift =
  <obj>
# x#t (A "hello");;
- : _[> `Constr of string * 'a list | `String of string ] as 'a =
`Constr ("A", [`String "hello"])

The part that it doesn't cover is importing a type from an external library. Though this can be covered by other tools such as ppx_import. We also have a small piece of code to do that in ppx_custom_printf, which relies on reading the output of the toplevel.

ghost commented 5 years ago

Gentle ping. @alainfrisch what do you think of the idea of merging ppx_tools_versioned and ppx_tools?

alainfrisch commented 5 years ago

Yes, we should definitely merge ppx_tools and ppx_tools_versioned. Btw, I'm more than happy to completely hand over maintenance of ppx_tools.

XVilka commented 5 years ago

Could be a candidate for OCaml community group then?

XVilka commented 5 years ago

Are there any recent developments on this one? It would be a good thing to include for a new release, along with OCaml 4.08 support.

ghost commented 5 years ago

Not on my side. I'm now focusing most of my ppx time towards building the future of ppx.

XVilka commented 5 years ago

Ok, then I rebased myself https://github.com/ocaml-ppx/ppx_tools/pull/74

ghost commented 5 years ago

Thanks :) Closing this then