fix(pkg): dune fmt missing extra files from the locks.

[moyodiallo]

This PR follows #10947 in which we could see the reproduction of the bug from the issue #10903.

It turns out, the issue is not a race condition, the auto-locking/solve for the dev-tool save the lock files after the setup of the build. Since dune setup once in the same run, the patch files ends up missing during the build.

[rgrinberg]

no "hooks" please. They make the code unreadable.

[gridbugs]

I think a simpler fix is to run a separate fiber scheduler to lock ocamlformat before running the build system. E.g.:

diff --git a/bin/build_cmd.ml b/bin/build_cmd.ml
index fec619125..66cf397cb 100644
--- a/bin/build_cmd.ml
+++ b/bin/build_cmd.ml
@@ -230,20 +230,17 @@ let fmt =
       Common.Builder.set_promote builder (if no_promote then Never else Automatically)
     in
     let common, config = Common.init builder in
+    if Lazy.force Lock_dev_tool.is_enabled
+    then
+      (* Note that generating the ocamlformat lockdir here means that
+         it will be created when a user runs `dune fmt` but not when a
+         user runs `dune build @fmt`. It's important that this logic
+         remain outside of `dune build`, as `dune build` is intended
+         to only build targets, and generating a lockdir is not
+         building a target. *)
+      Scheduler.go ~common ~config (fun () ->
+        Memo.run (Lock_dev_tool.lock_ocamlformat ()));
     let request (setup : Import.Main.build_system) =
-      let open Action_builder.O in
-      let* () =
-        if Lazy.force Lock_dev_tool.is_enabled
-        then
-          (* Note that generating the ocamlformat lockdir here means
-             that it will be created when a user runs `dune fmt` but not
-             when a user runs `dune build @fmt`. It's important that
-             this logic remain outside of `dune build`, as `dune
-             build` is intended to only build targets, and generating
-             a lockdir is not building a target. *)
-          Action_builder.of_memo (Lock_dev_tool.lock_ocamlformat ())
-        else Action_builder.return ()
-      in
       let dir = Path.(relative root) (Common.prefix_target common ".") in
       Alias.in_dir ~name:Dune_rules.Alias.fmt ~recursive:true ~contexts:setup.contexts dir
       |> Alias.request

[moyodiallo]

I think a simpler fix is to run a separate fiber scheduler to lock ocamlformat before running the build system. E.g.:

I came with this idea at some point and it turns out running twice the scheduler wasn't accepted.

[rgrinberg]

I think a simpler fix is to run a separate fiber scheduler to lock ocamlformat before running the build system. E.g.:

You can do this in dune fmt as long as you don't change dune build.

[moyodiallo]

A refactoring that avoid dune build @fmt to lock ocamlformat and also avoid to run the scheduler twice.

[moyodiallo]

Another last review would be great. The hooks are removed and the scheduler runs only once.

@gridbugs @rgrinberg.

[gridbugs]

nitpick: can you fix the indentation here so the comment text lines up on each line

[gridbugs]

I'm not a fan of introducing special behaviour into all these functions when a flag is passed. I think it muddies what was once a fairly clean interface with a special case which makes it harder to conceptualize what these functions do.

[rgrinberg]

Agreed. A flag is certainly better than a hook, but we should try to do better.

[moyodiallo]

I think we could at least agree upon 69ea9cf or start from that.

[gridbugs]

What was the issue with running the scheduler multiple times? It seems like that would also solve this problem and IMO would be preferable to either a hook or a flag.

[moyodiallo]

I'm also OK to run the scheduler once because it avoid loading the project twice than having a refactoring preference.

[gridbugs]

If we're not going to solve this with a hook, a flag, or with running the scheduler twice, then I'm not sure what other options are possible. If/when we make solving dependencies into a build rule/action then this will become simpler. In the meantime it sounds like a flag is the least bad option. @rgrinberg how do you feel about merging this with the flag for now?

[rgrinberg]

I looked at the code again and I don't think this is adequate. Here's a plan that addresses most of my concerns:

1. Remove watch mode support from fmt. There's no point to it as it's essentially racing against the user to edit source files
2. Implement dune fmt by running the scheduler and then running the lock + build inside.

[moyodiallo]

There's no point to it as it's essentially racing against the user to edit source files

Is it because no one using fmt on watch mode, this is not addressed before ?

[rgrinberg]

Yes, probably nobody is using fmt in watch mode. It would be rather frustrating as it's always racing to update the file as you're editing it, possibly overwriting your changes.

[gridbugs]

I'm happy with the flag solution provided that there's a comment explaining why it's necessary.

[moyodiallo]

Refactored on the flag solution and rebased.

ping @rgrinberg.

[rgrinberg]

I'll take a look tomorrow

[rgrinberg]

I looked at the code again and I don't think this is adequate. Here's a plan that addresses most of my concerns:

1. Remove watch mode support from fmt. There's no point to it as it's essentially racing against the user to edit source files
2. Implement dune fmt by running the scheduler and then running the lock + build inside.

[moyodiallo]

1. Remove watch mode support from fmt. There's no point to it as it's essentially racing against the user to edit source files
2. Implement dune fmt by running the scheduler and then running the lock + build inside

The 2. won't solve this issue because what we want is to run the lock and then run the scheduler.
But I've got what the suggestions 1 & 2 means in the case of the PR and some of the work is done here #11044.

The suggestions are applied.

[moyodiallo]

@gridbugs What do you think about this simplification ?

ping @rgrinberg.

[gridbugs]

Suggested change

	First run of 'dune fmt' is supposed to format the fail.
	First run of 'dune fmt' is supposed to format the file.

[gridbugs]

I don't think we should write this sort of comment in our tests. The test should describe the behaviour we're trying to preserve, but it's not necessary to describe the original problem or the fact that it was fixed. If anything, include a link to the github issue so people can read more about it there.

[gridbugs]

I'm happy with the state of this PR. I've left some comments about some minor stylistic fixes to the test.