Generic ppx driver #576

ghost · 2018-03-03T13:44:47Z

This PR removes the hard-coded knowledge of ppx_driver and ocaml-migrate-parsetree and their specific details. Instead we know have this in the jbuild files of ocaml-migrate-parsetree and ppx_driver:

(library
 ((name migrate_parsetree)
  [...]
  (ppx.driver
   ((main       Migrate_parsetree.Driver.run_main)
    (flags      (--dump-ast))
    (lint_flags (--null))))))

(library
 ((name ppx_driver)
  [...]
  (ppx.driver
   ((main       Ppx_driver.standalone)
    (replaces   (ocaml-migrate-parsetree))
    (flags      (-corrected-suffix ${corrected-suffix} -diff-cmd - -dump-ast))
    (lint_flags (-corrected-suffix ${corrected-suffix} -diff-cmd - -null    ))))
  ))

This makes things a bit cleaner and in particular we no longer have to synchronise the releases of Dune and these packages when adding new features.

To test, it is necessary to use the generic-ppx-driver branch of ocaml-migrate-parsetree and ppx_driver

rgrinberg · 2018-03-04T17:38:04Z

Looks good but it's a shame the error when the driver is missing is so opaque. Previously, we'd tell the user they needed to install something. It would be nice to have something more helpful than "No ppx driver found"

ghost · 2018-03-05T17:26:18Z

Indeed. Technically, we should only get this error in two cases:

when the (pps ()) list is empty.
when none of the listed preprocessor depends on a driver
the user updated jbuilder but not ocaml-migrate-parsetree or ppx_driver

For (1), no pps should simply not be allowed. We can make the switch when we switch to Dune. For (2) maybe we can say something like: none of the listed name is a ppx rewriter.

ghost · 2018-03-05T17:30:37Z

BTW, I'm trying to think what is the best way to release this. We need to add dependency constraints on released ppx_driver and ocaml-migrate-parsetree packages, but then we have the choice between:

option1:

releasing jbuilder beta19
releasing new versions of ppx_driver and ocaml-migrate-parsetree

and in between, users won't be able to upgrade to jbuilder beta19

option 2:

releasing jbuilder beta19 that only accept the new ppx.driver field but ignores it
releasing a version of ppx_driver and ocaml-migrate-parsetree with the updated jbuild files
releasing jbuilder beta20 with this PR in

rgrinberg · 2018-03-05T17:35:18Z

I think option #1 is doable if we batch together the releases of jbuilder, omp, and driver. But we'd also like to avoid delaying beta19 as it contains some important fixes. So unless those other releases are imminent, I'd favour #2.

ghost · 2018-03-05T17:43:34Z

Yh, the other releases are ready

ghost · 2018-03-05T17:47:30Z

But maybe 2 is better anyway. For instance the latest ppx_driver is not compatible with older version of OCaml so we will need multiple minor releases of ppx_driver.

rgrinberg · 2018-03-05T17:55:43Z

Let's go with #2 then

rgrinberg · 2018-03-18T15:36:17Z

src/inline_tests.ml

+                 | None ->
+                   Error (Loc.exnf loc "%S is not an %s" name
+                            (desc ~plural:false))
+                 | Some t -> Ok t))


Would it be possible to move these changes into their own PR if possible?

Ok, I'll cherry-pick directly some of the simpler commit to master directly, since we can't create dependent PRs on github

rgrinberg · 2018-04-18T02:57:50Z

@diml what's the status of this? You think you'll have enough time to polish this for the next release?

ghost · 2018-04-18T12:24:45Z

I improved the error messages, now it suggests to upgrade ocaml-migrate-parsetree, ppxlib or ppx_driver if the pps depend on them. Otherwise it suggests that they are not compatible with jbuilder.

ghost · 2018-04-18T12:25:04Z

But we need to wait for a new version of ppxlib

ghost · 2018-05-15T08:48:36Z

The recent problems we had with ppx_sexp_conv makes me think that this is going to cause more pain than I expected. I think I will change this feature so that the new behavior is only activated in dune projects. This way nothing will change for existing released projects.

ghost · 2018-06-02T17:06:01Z

I added a commit on top of the previous changes to restore the old behavior when the file is a jbuild file. To know if the file is a jbuild file, we pass a dir_kind value around that is either Dune or Jbuild. The old ppx drivers are stored in _build/default/.ppx/jbuild.

The code is not exactly the same as what we had before, but it should work for all existing projects.

We shouldn't need to do anything special for the next release. Existing projects will continue to work, and if there is an issue with a pure dune project because ppxlib or omp was reinstalled, Dune prints an error message inviting the user to reinstall omp or ppxlib.

@rgrinberg could you review the last changes? I think this is now safe to merge.

rgrinberg · 2018-06-03T05:33:16Z

src/install_rules.ml

+          match Preprocessing.get_ppx_driver sctx ~scope ~dir_kind pps with
+          | Ok    x -> [x]
+          | Error _ ->
+            [Preprocessing.get_ppx_driver_for_public_lib sctx ~name ~dir_kind]


Could you explain this bit? What kind of errors are we ignoring here?

Looking at the code again, I believe the only possible failure is if ppxlib.runner or ppx_driver.runner doesn't exist. This might be the case at some point in the future and this code is not very clear anyway. I reworked it to make things more explicit and avoid the error case here.

rgrinberg · 2018-06-03T06:29:41Z

Those last (recent) commits look good. Btw, how would this work for mixed setups with both dune and jbuild files? So we're not going to be able to use both the new and old drivers in the same workspace?

ghost · 2018-06-04T10:15:18Z

It should work fine. Basically when we are in a jbuild directory, the new driver stuff is not used at all and we still use the hardcoded values. The old drivers are built in a separate directory .ppx/jbuild, so it should be possible to use both method without issues.

ghost · 2018-06-04T11:23:30Z

I changed the message in case of error to:

No ppx driver found.
It seems that these ppx rewriters are not compatible with jbuilder.
It seems that these ppx rewriters are not compatible with Dune.
Hint: Examples of ppx rewriters that are compatible with Dune are
ones using ocaml-migrate-parsetree, ppxlib or ppx_driver.

- remove hard-coded knowledge of ocaml-migrate-parsetree and ppx_driver - get the exact driver parameters directly from the driver itself Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

I just spent an hour debugging a stupid bug caused by this... Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

rgrinberg · 2018-06-05T07:13:56Z

src/preprocessing.ml

@@ -203,7 +203,9 @@ let no_driver_error pps =
  | None ->
    sprintf
      "No ppx driver found.\n\
-       It seems that these ppx rewriters are not compatible with jbuilder."
+       It seems that these ppx rewriters are not compatible with Dune.\n\


Rather than "these", should we just include the pps here in the error message (which also has locs)

Indeed. We don't always have the location at this point because we might reach this program point when generating the rules for _build/default/.ppx/<key>/ppx.exe.

I'm refactoring a bit this code because it's becoming messy.

ghost · 2018-06-05T12:36:39Z

I improved the error messages and added tests.

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

ghost · 2018-06-05T18:19:50Z

I'm glad to have finished this one!

ghost requested a review from rgrinberg March 3, 2018 13:44

ghost mentioned this pull request Mar 5, 2018

Accept and ignore ppx.driver fields #588

Merged

ghost mentioned this pull request Mar 13, 2018

Use the generic ppx driver support of Dune ocaml-ppx/ocaml-migrate-parsetree#40

Merged

rgrinberg reviewed Mar 18, 2018

View reviewed changes

ghost mentioned this pull request Apr 23, 2018

CMI and PPX generation ordering #193

Closed

ghost added this to the 1.0.0 milestone Jun 2, 2018

ghost mentioned this pull request Jun 2, 2018

Change the name of a few variables #842

Closed

4 tasks

ghost self-assigned this Jun 2, 2018

rgrinberg reviewed Jun 3, 2018

View reviewed changes

Jeremie Dimino and others added 6 commits June 4, 2018 16:26

Abstract the ppx driver system

f580abb

- remove hard-coded knowledge of ocaml-migrate-parsetree and ppx_driver - get the exact driver parameters directly from the driver itself Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

Add List.assoc

b05ea02

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

Restore old ppx behavior for directories with jbuild files

5f47100

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

Add a warning for with_lib_deps

278d300

I just spent an hour debugging a stupid bug caused by this... Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

Improve the code for building and installing the compat ppx.exe program

cc85a2a

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

Update gen-opam-install-file test

d0581b4

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

Add a hint for ppx rewriters that are not compatible

16d8270

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

rgrinberg reviewed Jun 5, 2018

View reviewed changes

Refactor handling of backend selection errors + add tests

bf9b25c

Signed-off-by: Jeremie Dimino <jdimino@janestreet.com>

rgrinberg approved these changes Jun 5, 2018

View reviewed changes

Merge branch 'master' into generic-ppx-driver

35706cc

ghost merged commit ec6ca4b into ocaml:master Jun 5, 2018

ghost mentioned this pull request Jul 10, 2018

[new release] dune (1.0.0) ocaml/opam-repository#12323

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic ppx driver #576

Generic ppx driver #576

ghost commented Mar 3, 2018

rgrinberg commented Mar 4, 2018

ghost commented Mar 5, 2018

ghost commented Mar 5, 2018

rgrinberg commented Mar 5, 2018

ghost commented Mar 5, 2018

ghost commented Mar 5, 2018

rgrinberg commented Mar 5, 2018

rgrinberg Mar 18, 2018

ghost Mar 18, 2018

ghost Mar 18, 2018

rgrinberg commented Apr 18, 2018

ghost commented Apr 18, 2018

ghost commented Apr 18, 2018

ghost commented May 15, 2018

ghost commented Jun 2, 2018

rgrinberg Jun 3, 2018

ghost Jun 4, 2018

rgrinberg commented Jun 3, 2018

ghost commented Jun 4, 2018

ghost commented Jun 4, 2018

rgrinberg Jun 5, 2018

ghost Jun 5, 2018

ghost commented Jun 5, 2018

ghost commented Jun 5, 2018

Generic ppx driver #576

Generic ppx driver #576

Conversation

ghost commented Mar 3, 2018

rgrinberg commented Mar 4, 2018

ghost commented Mar 5, 2018

ghost commented Mar 5, 2018

rgrinberg commented Mar 5, 2018

ghost commented Mar 5, 2018

ghost commented Mar 5, 2018

rgrinberg commented Mar 5, 2018

rgrinberg Mar 18, 2018

Choose a reason for hiding this comment

ghost Mar 18, 2018

Choose a reason for hiding this comment

ghost Mar 18, 2018

Choose a reason for hiding this comment

rgrinberg commented Apr 18, 2018

ghost commented Apr 18, 2018

ghost commented Apr 18, 2018

ghost commented May 15, 2018

ghost commented Jun 2, 2018

rgrinberg Jun 3, 2018

Choose a reason for hiding this comment

ghost Jun 4, 2018

Choose a reason for hiding this comment

rgrinberg commented Jun 3, 2018

ghost commented Jun 4, 2018

ghost commented Jun 4, 2018

rgrinberg Jun 5, 2018

Choose a reason for hiding this comment

ghost Jun 5, 2018

Choose a reason for hiding this comment

ghost commented Jun 5, 2018

ghost commented Jun 5, 2018