opened
diml
username_0
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:

```scheme
(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
Generic ppx driver
issue
created
rgrinberg
username_1
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"
comment
created
diml
username_0
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
comment
created
rgrinberg
username_1
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.
comment
created
diml
username_0
Yh, the other releases are ready
comment
created
diml
username_0
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.
comment
created
rgrinberg
username_1
Let's go with #2 then
comment
created
rgrinberg
username_1
@username_0 what's the status of this? You think you'll have enough time to polish this for the next release?
comment
created
diml
username_0
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.
comment
created
diml
username_0
But we need to wait for a new version of ppxlib
comment
created
diml
username_0
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.
comment
created
diml
username_0
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.

@username_1 could you review the last changes? I think this is now safe to merge.
comment
created
rgrinberg
username_1
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?
comment
created
diml
username_0
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.
comment
created
diml
username_0
I improved the error messages and added tests.
comment
created
diml
username_0
I'm glad to have finished this one!
comment