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

• restrict this feature to dune's codebase only
• ignore ppx/ for diffs
• switch from a dir inclusion to a dir exclusion mechanism
• 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 chore: update dune-project version to 3.11 #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.

[Alizter]

We now have 3.13 in main if you want to use that.

[NathanReb]

Awesome, I'll switch to it! I rolled it back to 3.10 while trying to fix an issue I had with merlin locally.

[NathanReb]

I did bump it to 3.13 but since the PR also enables the extension it makes the whole repo depend on dune >= 3.13 and I therefore get CI builds failure with:

Switch invariant: ["ocaml" {>= "4.05.0"}]
[ERROR] Could not determine which packages to install for this switch:
  * Missing dependency:
Switch initialisation failed: clean up? ('n' will leave the switch partially installed) [Y/n] y
    - dune >= 3.13
    no matching version

This is expected I guess. Is there any way I can tell the CI builds to pin dune.3.13.0 instead of 3.11.1?

[Alizter]

Maybe its best to keep it 3.10 then. I don't think its worth spending time resolving these CI issues. It's not a user facing feature anyway yet.

[Alizter]

To be clear, the issue is not because we are adding a field to a new dune lang version, but because the CI works by coincidence. The actual dune-project version Dune itself uses, always lags behind the latest dune language version and our CI setup uses an older Dune to do some unrelated checks. If a newer version is required then these checks fail. (Ideally we shouldn't be doing them, but that isn't for this PR).

[NathanReb]

I could also not enable it in this PR, simply adding the feature and enabling it later if that helps.

I'm also happy to use an older version of the language as lower bound for the feature since, as you said, it's not user facing.

[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]

@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!

[NathanReb]

I have a draft version of this that builds but when I try to use it I get the following errors:

$ make check
Error: No rule found for ppx/src/dune_cache/trimmer.pp.ml
-> required by alias ppx/check
Error: No rule found for ppx/src/dune_cache/trimmer.pp.mli
-> required by alias ppx/check
...

and so on, for each file in the library that uses a ppx. I'm guessing this is an error somewhat similar to what I encountered originally with rules being defined in a different folder than their targets. That required that the addition this function, used here.

I expect this is a slightly different flavor of the same issue.

[NathanReb]

Nevermind, it seems to be something much simpler: the formatting action I'm using must be declaring a dependency on the preprocessed file which is the optional output of the driver invocation and is never declared as a target of any rule.

I think I got it wrong when building the combined action. I'll look into it!