-
Notifications
You must be signed in to change notification settings - Fork 401
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(pkg): dune fmt
missing extra files from the locks.
#10951
base: main
Are you sure you want to change the base?
Conversation
26bb033
to
3b49b47
Compare
bin/build_cmd.ml
Outdated
@@ -82,12 +82,18 @@ let run_build_system ~common ~request = | |||
Fiber.return ()) | |||
;; | |||
|
|||
let run_build_command_poll_eager ~(common : Common.t) ~config ~request : unit = | |||
let run_build_command_poll_eager ~pre_request ~(common : Common.t) ~config ~request : unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no "hooks" please. They make the code unreadable.
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
|
I came with this idea at some point and it turns out running twice the scheduler wasn't accepted. |
You can do this in |
60ed31a
to
972d275
Compare
A refactoring that avoid |
Another last review would be great. The hooks are removed and the scheduler runs only once. |
bin/build_cmd.ml
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: can you fix the indentation here so the comment text lines up on each line
;; | ||
|
||
let run_build_command_poll_eager | ||
~with_lock_ocamlformat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. A flag is certainly better than a hook, but we should try to do better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could at least agree upon 69ea9cf or start from that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also OK to run the scheduler once because it avoid loading the project twice than having a refactoring preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
93ffd26
to
69ea9cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with the flag solution provided that there's a comment explaining why it's necessary.
69ea9cf
to
ba0d481
Compare
Signed-off-by: Alpha DIALLO <[email protected]>
ba0d481
to
ad7c1f2
Compare
Refactored on the flag solution and rebased. ping @rgrinberg. |
I'll take a look tomorrow |
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.