Allow use of ppx in dune

[NathanReb]

Fixes #8586

This adds an (include_preprocessed_sources) extension which generates rules to promote preprocessed version of source files. It's used in a ppx/ folder at the project root. For any library or executable stanza with a (preprocess (pps ...)) field in , it will generate promotion rules for preprocessed versions in ppx/<path>.

It also modifies duneboot.ml to use ppx/<path>.ml(i) instead of <path>.ml(i) if it exists, allowing us to use ppx in development without making it a build dependency.

[NathanReb]

TODO

• [x] restrict this feature to dune's codebase only
• [x] ignore ppx/ for diffs
• [x] switch from a dir inclusion to a dir exclusion mechanism
• [x] document the feature for dune devs

This is still a draft at the moment so I'm adding this todo list.

Restrict feature to dune's codebase only

The first part is to restrict this feature so it can only be used by dune itself as we agreed we did not want to make it public or at least not just yet. I'm not sure how to proceed with this so I'll gladly take any pointer. I wrote this feature as an extension because that was the simplest way to prototype it for me but this can be changed as well, especially if we don't want to make it public it probably doesn't make much sense to keep it that way.

Ignore ppx/ for diffs

We need to mark the ppx folder so it doesn't pollute git commands or PRs.

Switch from a dir inclusion to a dir exclusion mechanism

There are already uses of ppx in the repo in bench/micro or in some test folders in otherlibs/. These are not necessary to build the main dune executable and therefore should not have there preprocessed sources included. I initially wrote the feature with a dir inclusion mechanism, i.e. it needs a list of directories to scan for sources to preprocess, because it made testing it easier. We do need a way to tell it where to and not to look but it is likely that excluding directories will be easier than the other way around.

Document feature for dune devs

The feature needs a bit of documentation as it's not entirely straightforward to use. In particular, whenever a dune dev wants to use a ppx in a new location, they will have to determine whether the preprocessed sources need to be included and if so, they will have to add the proper directories in ppx/.

[Alizter]

Some comments addressing the version bumping which can be resolved later:

• Bumping the version to >3.11 has issues with our bench job. See my attempt in https://github.com/ocaml/dune/pull/9212. Hopefully the bench team will resolve this soon enough.
• Since this will miss the 3.12 release, you might as well target 3.13. This will happen in a PR soon when somebody extends the syntax. However if you would like to do it, preferably in another PR, you will need to bump the version which lives in otherlibs/dune-rpc/private/types.ml from 3, 11 to 3, 13. 3.12 didn't have any syntax changes hence we are still in 3.11. Edit: I bumped 3.13.

[rgrinberg]

The first part is to restrict this feature so it can only be used by dune itself as we agreed we did not want to make it public or at least not just yet. I'm not sure how to proceed with this so I'll gladly take any pointer. I wrote this feature as an extension because that was the simplest way to prototype it for me but this can be changed as well, especially if we don't want to make it public it probably doesn't make much sense to keep it that way.

Simplest way is just to only allow it if the project name is equal to "dune". This is what we do for our bootstrapping extension.

There are already uses of ppx in the repo in bench/micro or in some test folders in otherlibs/. These are not necessary to build the main dune executable and therefore should not have there preprocessed sources included. I initially wrote the feature with a dir inclusion mechanism, i.e. it needs a list of directories to scan for sources to preprocess, because it made testing it easier. We do need a way to tell it where to and not to look but it is likely that excluding directories will be easier than the other way around.

You can add an appropriate field in the env stanza. That seems like the simplest thing.

[NathanReb]

You can add an appropriate field in the env stanza. That seems like the simplest thing.

Is there any particular reason to add something to the env stanza rather than using a field of (include_preprocessed_sources)?

[rgrinberg]

With the env stanza, you can set it for entire directories. Either one works fine though

[NathanReb]

I modified the stanza to scan the whole project except for the given dirs to exclude. I added bench/ for now but if you can think of other dirs that contain uses of ppx and shouldn't be accounted for by the stanza please let me know.

[rgrinberg]

I modified the stanza to scan the whole project except for the given dirs to exclude. I added bench/ for now but if you can think of other dirs that contain uses of ppx and shouldn't be accounted for by the stanza please let me know.

I think everything in otherlibs should be excluded.

[Alizter]

@rgrinberg Don't we want to use this for stdune?

[rgrinberg]

No, we don't want to vendor anything for libraries that we publish.

[NathanReb]

I added otherlibs/ to the list of excluded directories.

I also restricted the feature to dune only. While doing it I looked at dune-bootstrap-info and I think the restriction mechanism there is bugged, I tried using it in a separate project and dune did not complain. I'm happy to fix it in a separate PR if the implementation for include_preprocessed_sources seems right to you!

[NathanReb]

I completed all the items from my todo but there's a couple things I'd like to do before undrafting the PR:

1. I'd like to create branch on top of this one where I'm using ppx_deriving_dyn to generate Dyn.t converters and make sure it all works properly
2. Update the CI builds to include a check that ppx/ is always kept up to date.

I can easily take care of 1 by myself but I'd like some guidance on how to proceed with 2.

[Alizter]

Is there a target/alias that is built to check this? We could setup something similar to our format jobs? Or even just include said target in that job.

[rgrinberg]

Update the CI builds to include a check that ppx/ is always kept up to date.

You can make it part of the @check alias for development and a ppx alias for CI.

[NathanReb]

At the moment we generate auto promote rules so for the CI we'd need an extra git diff --exit-code command for the build to fail when ppx/ is out of sync. What's the best place to add this? A Makefile target?

[rgrinberg]

We could also just add a mode to dune that would fail on promotions. For now your workaround is enough though.

[NathanReb]

I opened a draft PR here to show how it would look like to use the new ppx_deriving_dyn with this work on dune's codebase.

I initially wanted to do it all before showing it to you but there's a significant amount of function to replace and each of them needs a bit of attention to determine whether a derived to_dyn would fit or it's too specific and we should keep the hand written version.

My first impression is that it adds a lot of files to the repo and some of them could probably be spared as it currently preprocesses the whole library no matter what. This could probably be improved by using a per-module pp spec or by detecting whether a preprocessed file actually differs from the original. The latter is of course harder to implement but requires less configuration if we can manage it.

Let me know what you think.

[rgrinberg]

This could probably be improved by using a per-module pp spec or by detecting whether a preprocessed file actually differs from the original. The latter is of course harder to implement but requires less configuration if we can manage it.

I would very much prefer this option.

EDIT: it should also be fairly simple to implement if we use the diff? action.

[NathanReb]

I would very much prefer this option.

EDIT: it should also be fairly simple to implement if we use the diff? action.

I'll look into this. We'll need two things for this to work:

1. to format the preprocessed file before diffing it
2. to make sure the driver doesn't add stuff when there's no transformation

2 is the hard part here obviously as I'm not sure we can configure the driver to drop any generic addition to the AST at the moment and even if we could it's likely we don't want to disable them unconditionally but only if the AST is untouched which would require changes in ppxlib.

[NathanReb]

I'm quite surprised but after formatting it seems to work out of the box. I was sure the driver added some attributes by default but it appears it doesn't, or at least doesn't always add them.

I'll try to change the rules so it doesn't promote a preprocessed file if it's identical to the source!

[NathanReb]

I just added the formatting of promoted files, now I need to implement the conditional promotion mechanism.

What we would like to have here would be:

1. generate the formatted, preprocessed version of x.ml, which should be _build/default/ppx/x.ml
2. if _build/default/ppx/x.ml differs from x.ml, we promote it to ppx/x.ml. If it's identical we promote nothing.

This is quite different from diff?'s behaviour if I understand it correctly because we promote to a different file. It also won't clean up files if at some point they cease to differ from the source one but that can easily be dealt with by cleaning up the ppx subdirs in make ppx I guess.

Does this sound like a good behaviour to you and if so, do you have any pointer or tips on how to best implement this?

[rgrinberg]

Tbh, I saw it working like this:

1. If the driver encounters a ppx annotation, it will produce a preprocessed target. If there's no ppx's, no target would be produced.
2. We format the target in step 1. (if it's produced)
3. The diff? action picks up the optionally produced file and promotes it.

What you suggest sounds like it would work too. I suppose it relies on the driver pretty printer not introducing any spurious changes?

[NathanReb]

Having an option to use the driver that way would be nice indeed but we have to be aware that some ppx are written as full AST rewriting and won't work with such a feature, i.e. the driver has no way to tell whether it's going to modify anything and therefore would have to always output a file. That's a minority of ppx so it shouldn't be a problem but that's still something to keep in mind.

I'll open an issue on ppxlib to see if we can add this feature.

The other method does not depend on a new feature in the driver so we could get it in sooner but yeah it relies on the driver's printer (modulo formatting).

Regardless of the method we choose, I think diff? is not exactly what we're looking for here since the promotion target is different from the file we want to diff with the preprocessed version is it?

[rgrinberg]

Regardless of the method we choose, I think diff? is not exactly what we're looking for here since the promotion target is different from the file we want to diff with the preprocessed version is it?

If we're going to rely on the driver's pretty printer using diff? isn't going to work. Using diff? only works when the producing action knows whether the promotion should occur which the driver doesn't support yet. Inserting a formatting step on an optionally produced target isn't a problem.

[NathanReb]

I opened ocaml-ppx/ppxlib#461.

From the top of your head, do you have an idea of which ppx we'd like to use in dune that we wouldn't write ourselves?

It'd be good to take a look and see if those would be compatible with the driver optional output thing before going forward with this.

[NathanReb]

I haven't had a lot of time to work on this lately but should be able to resume this week, starting with a prototype for the ppxlib driver output mode!

[rgrinberg]

@NathanReb is this ready for a round of review?

[NathanReb]

No sorry, I still have to change the rules to make use of the new driver mode I added in ppxlib. I'll work on it next week!

[NathanReb]

I'm looking into how to rewrite the rules with the new driver mode.

For context, the idea was to make the ppx driver not write to the target file if there is no ppx transformation to apply so we can use it with a diff? to promote only relevant files in the ppx/ folder.

I'm struggling with how to write such a rule and think I could use some guidance here. It seems that it should be used in a progn but I can't get to build one with both the command I already defined [here] (https://github.com/ocaml/dune/pull/9223/files#diff-bd1c262fd559824b1d88f7b9d4646d71ee76a97460e1b31a9e4caae5537635c9R182) and a simple optional diff action. I tried looking for other uses of the diff action in the codebase but except for the one in cram_rules I couldn't find something close enough to what I'm trying to achieve here.

I'm probably missing something very simple but if anyone can point me in the right direction that would probably ease things a lot!

[rgrinberg]

I pushed a quick sketch of how I imagined it to work. You were indeed pretty close. The key here is that we don't need any intermediate targets for our rule. We need two ingredients:

1. An action that will produce the preprocessed + formatted file
2. Attach this action to an alias so that the user can execute it. I chose @check.

The only missing piece is that to correctly create the action in 1., we need a way to run the formatter only if the file produced by the ppx exists. So we need an action like:

val run_if_exists : Path.t -> Action.t -> Action.t

I inlined a comment on how you would go about implementing such an action.

Let me know if this isn't clear.

[NathanReb]

Thanks @rgrinberg! I just got back from vacation and will start working on this!