Add (format-dune-file) action

[nojb]

Context is #10892 (comment).

This exposes a user action (format-dune-file file) which outputs the formatted contents of file (assumed to contain S-expressions) to standard output. It uses the version for the formatting specified by the current project.

This action should be used instead of calling dune format-dune-file from inside a user rule, which does not work well (calling Dune recursively is not really well supported).

cc @maiste @Leonidas-from-XIV

[maiste]

Shouldn't it be this?

Suggested change

	; "format-dune-file", Field
	; "format-dune-file", Since ((3,18), Field)

[nojb]

Thanks, fixed.

[rgrinberg]

There's already a format action defined in Format_rules. You can just use that instead of extending the engine.

[nojb]

There's already a format action defined in Format_rules. You can just use that instead of extending the engine.

Thanks, I had missed this. This is now done: a7abf53. For simplicity, the action now has the form (format-dune-file <src> <dst>) and outputs the formatted contents to <dst> (a file), instead of standard output.

[rgrinberg]

Maybe add a test demonstraating how the action fails when given an invalid dune file?

[nojb]

Done, thanks.

[rgrinberg]

Would be appreciated if the update to dune lang 3.18 was done in a separate PR

[nojb]

Would be appreciated if the update to dune lang 3.18 was done in a separate PR

Good idea, done in #11175

[nojb]

Thanks for the review @rgrinberg. I think all issues have been addressed.

[nojb]

Planning to merge soon if the CI passes. Yell if you object!

[mbarbin]

Hi, I just discovered this new feature in the dune changelog for 3.18, sounds nice!

I'm using dune format-dune-file in action in this project as part of a pattern to generate dune.inc files see this file for example.

This works well, so I am not sure which issue does it cause to call dune recursively from an action. At any rate, I am not able to directly translate this to the new construct, as I am using the format command in the mode where it reads from stdin and emit the fmt to stdout (in a pipi-stdout construct). Do you think the action could be extended to support this, or should I just leave this alone and not pay attention? Thanks!

[nojb]

This works well, so I am not sure which issue does it cause to call dune recursively from an action.

Even if it works today, there's no guarantee that this will continue to work in the future. In general, Dune is not really designed to be reentrant. For example, it is planned to modify dune format-dune-file to read the Dune project metadata in order to take into account the version of the Dune language used by the project. I am not sure, but I think that this change will make it more difficult to invoke Dune recursively from within a Dune action.

At any rate, I am not able to directly translate this to the new construct, as I am using the format command in the mode where it reads from stdin and emit the fmt to stdout (in a pipi-stdout construct). Do you think the action could be extended to support this, or should I just leave this alone and not pay attention? Thanks!

It would add a bit of complexity, so we will probably not do it unless there is a clear need for it. You should be able to use the new construct by introducing intermediate files in your Dune rules. If you can share the code somewhere, I can try to suggest some pointers.

[mbarbin]

If you can share the code somewhere, I can try to suggest some pointers.

Do you mean for me to link you to code where I am using this? If so, there are some instances of this pattern I'm using in this repo: https://github.com/mbarbin/bopkit/blob/cfc30b189b4bb852ddd92bcc40178f03b2e24927/stdlib/pulse/dune#L14 (this is taken from this file)

(rule
 (target dune.inc)
 (deps
  (glob_files *.bop))
 (alias runtest)
 (mode promote)
 (action
  (with-stdout-to
   %{target}
   (pipe-stdout
    (run %{bin:bopkit} fmt gen-dune "\%{bin:bopkit}" fmt file)
    (run dune format-dune-file)))))

I'm ok to try and fix the rules according to your guidance if you think that's best. Thanks for your help!

[nojb]

(rule
 (target dune.inc)
 (deps
  (glob_files *.bop))
 (alias runtest)
 (mode promote)
 (action
  (with-stdout-to
   %{target}
   (pipe-stdout
    (run %{bin:bopkit} fmt gen-dune "\%{bin:bopkit}" fmt file)
    (run dune format-dune-file)))))

Something along the following lines should do the trick:

(rule
 (target dune.inc.raw)
 (deps (glob_files *.bop))
 (action
  (with-stdout-to %{target}
   (run %{bin:bopkit} fmt gen-dune "\%{bin:bopkit}" fmt file))))

(rule
 (alias runtest)
 (mode promote)
 (action (format-dune-file dune.inc.raw dune.inc)))

I'm ok to try and fix the rules according to your guidance if you think that's best. Thanks for your help!

Yes, the motivation to introduce the new action was to help people migrate away from calling dune format-dune-file from within Dune rules, so it would be nice if you could give it a shot and report back any issues you encounter (if any!).

[mbarbin]

Thanks I'll try the change with the intermediate raw file.

there's no guarantee that this will continue to work in the future

I should note here that I find it very useful for dune to export a command that can be used to auto-format dune files. I am using this in the dunolint project (see here). It would be nice if you could commit to having a command that could remain usable in the future for this. For my use case I'd be OK if that command is made a bit less user-friendly, and adds more parameter (such as the lang version). Alternatively, that'd work quite well for that project if the functionality is exposed from within an OCaml lib directly. I'm happy to make this into a discussion or separate issue if you'd like.

[nojb]

I should note here that I find it very useful for dune to export a command that can be used to auto-format dune files.

Good to know. Our plan is not to get rid of this command, but to improve it, by having it read the Dune metadata and format according to the version of the Dune lang used by the project containing the Dune file being formatted (right now, dune format-dune-file does not take this into account). But the point remains that calling Dune recursively is probably not a good idea in general, so using the dedicated action from within Dune files is probably better.

[mbarbin]

Yes, the motivation to introduce the new action was to help people migrate away from calling dune format-dune-file from within Dune rules, so it would be nice if you could give it a shot and report back any issues you encounter (if any!).

I gave it a shot in this pr based on your suggestion of splitting the rule in 2. There was no issue, I just wanted to say thank you!

There was a pre-existing "issue" with this repo, which is made visible again by this change (but unrelated) : the construct that I am changing is repeated many times, and thus I had to repeat the fix many times. I opened this discussion with a potential idea to improve, if you have other ideas, please let me know! Thanks!

[nojb]

@mbarbin Thanks for the confirmation. Cheers!