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

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

Closed
wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Feb 5, 2019

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.

@ghost ghost mentioned this pull request Feb 5, 2019
@alainfrisch
Copy link
Collaborator

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
Copy link
Author

ghost commented Feb 6, 2019

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
Copy link
Collaborator

Drup commented Feb 6, 2019

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
Copy link
Author

ghost commented Feb 7, 2019

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
Copy link
Collaborator

Drup commented Feb 7, 2019

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
Copy link
Author

ghost commented Feb 7, 2019

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
Copy link
Collaborator

Drup commented Feb 7, 2019

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
Copy link

let-def commented Feb 7, 2019

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:

  • lift to current parsetree
  • if the parsetree needs further manipulations, convert to a fixed version.

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

@ghost
Copy link
Author

ghost commented Feb 7, 2019

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
Copy link
Author

ghost commented Feb 13, 2019

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

@alainfrisch
Copy link
Collaborator

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
Copy link

XVilka commented Mar 11, 2019

Could be a candidate for OCaml community group then?

@nojb nojb mentioned this pull request May 10, 2019
@XVilka
Copy link

XVilka commented Jun 26, 2019

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
Copy link
Author

ghost commented Jul 1, 2019

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

@XVilka
Copy link

XVilka commented Jul 1, 2019

Ok, then I rebased myself #74

@ghost
Copy link
Author

ghost commented Jul 1, 2019

Thanks :) Closing this then

@ghost ghost closed this Jul 1, 2019
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants