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?

[gridbugs]

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