search

ocaml-ppx / ppx_tools

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

Switch the build system to dune #74

Closed XVilka closed 4 years ago

XVilka commented 5 years ago

Rebased https://github.com/ocaml-ppx/ppx_tools/pull/67

XVilka commented 5 years ago

And merging ppx_tools_versioned into ppx_tools can be done separately, imho.

XVilka commented 5 years ago

What do you think about merging this one before the release (the one with 4.08 support)?

XVilka commented 4 years ago

Rebased one more time, please take a look.

XVilka commented 4 years ago

Maybe @kit-ty-kate or @gasche can take a look?

kit-ty-kate commented 4 years ago

This PR feels too big (adds too many new things) for me to review it properly. Could it be split into two? One for just dune and one for the "bit of glue code so that ppx_tools.metaquot can be easily used with dune"?

kit-ty-kate commented 4 years ago

Also looking at it, it seems that the .depend and META files are still present. Could they be removed in the first PR?

kit-ty-kate commented 4 years ago

Or if not in two PRs, at least two commits

XVilka commented 4 years ago

I removed the .depend and META. Regarding the metaquot changes - they are still necessary because the original buildsystem was providing the metaquot, so split would break the build. And these changes are kind of trivial anyway, wouldn't reduce the main commit size much.

kit-ty-kate commented 4 years ago

Regarding the metaquot changes - they are still necessary because the original buildsystem was providing the metaquot, so split would break the build. And these changes are kind of trivial anyway, wouldn't reduce the main commit size much.

I don't think it is strictly necessary. I've just tried it myself and ended up with this branch: https://github.com/ocaml-ppx/ppx_tools/compare/master...kit-ty-kate:dune

It is almost exactly a 1 to 1 equivalent to the Makefile version. I'd much rather have that for now than merge this PR in one go, but that's just my view on the subject.

What do you think of spliting this PR into my branch and a new PR with the change in your branch on top?

XVilka commented 4 years ago

Ok, I will rebase once your PR got merged. Thank you!

XVilka commented 4 years ago

Irrelevant then, closing.