From 027303059afaff5f8d705455fe449665f91d0acf Mon Sep 17 00:00:00 2001 From: Alpha DIALLO Date: Mon, 23 Sep 2024 12:04:56 +0200 Subject: [PATCH 1/2] fix(pkg): `dune fmt` missing extra files from the locks. Signed-off-by: Alpha DIALLO --- bin/build_cmd.ml | 54 +++++++++++++++++-- .../ocamlformat-patch-extra-files.t | 27 ++++------ 2 files changed, 58 insertions(+), 23 deletions(-) diff --git a/bin/build_cmd.ml b/bin/build_cmd.ml index 19fa78f0b20..b5e6518a51d 100644 --- a/bin/build_cmd.ml +++ b/bin/build_cmd.ml @@ -82,12 +82,40 @@ let run_build_system ~common ~request = Fiber.return ()) ;; -let run_build_command_poll_eager ~(common : Common.t) ~config ~request : unit = +let lock_ocamlformat () = + 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. *) + Lock_dev_tool.lock_ocamlformat () |> Memo.run + else Fiber.return () +;; + +let run_build_command_poll_eager + ~with_lock_ocamlformat + ~(common : Common.t) + ~config + ~request + : unit + = + let open Fiber.O in Scheduler.go_with_rpc_server_and_console_status_reporting ~common ~config (fun () -> - Scheduler.Run.poll (run_build_system ~common ~request)) + Scheduler.Run.poll + (let* () = if with_lock_ocamlformat then lock_ocamlformat () else Fiber.return () in + run_build_system ~common ~request)) ;; -let run_build_command_poll_passive ~(common : Common.t) ~config ~request:_ : unit = +let run_build_command_poll_passive + ~with_lock_ocamlformat + ~(common : Common.t) + ~config + ~request:_ + : unit + = (* CR-someday aalekseyev: It would've been better to complain if [request] is non-empty, but we can't check that here because [request] is a function.*) let open Fiber.O in @@ -99,16 +127,20 @@ let run_build_command_poll_passive ~(common : Common.t) ~config ~request:_ : uni Scheduler.go_with_rpc_server_and_console_status_reporting ~common ~config (fun () -> Scheduler.Run.poll_passive ~get_build_request: - (let+ (Build (targets, ivar)) = Dune_rpc_impl.Server.pending_build_action rpc in + (let* () = + if with_lock_ocamlformat then lock_ocamlformat () else Fiber.return () + in + let+ (Build (targets, ivar)) = Dune_rpc_impl.Server.pending_build_action rpc in let request setup = Target.interpret_targets (Common.root common) config setup targets in run_build_system ~common ~request, ivar)) ;; -let run_build_command_once ~(common : Common.t) ~config ~request = +let run_build_command_once ~with_lock_ocamlformat ~(common : Common.t) ~config ~request = let open Fiber.O in let once () = + let* () = if with_lock_ocamlformat then lock_ocamlformat () else Fiber.return () in let+ res = run_build_system ~common ~request in match res with | Error `Already_reported -> raise Dune_util.Report_error.Already_reported @@ -122,6 +154,18 @@ let run_build_command ~(common : Common.t) ~config ~request = | Yes Eager -> run_build_command_poll_eager | Yes Passive -> run_build_command_poll_passive | No -> run_build_command_once) + ~with_lock_ocamlformat:false + ~common + ~config + ~request +;; + +let run_build_command_fmt ~(common : Common.t) ~config ~request = + (match Common.watch common with + | Yes Eager -> run_build_command_poll_eager + | Yes Passive -> run_build_command_poll_passive + | No -> run_build_command_once) + ~with_lock_ocamlformat:true ~common ~config ~request diff --git a/test/blackbox-tests/test-cases/pkg/ocamlformat/ocamlformat-patch-extra-files.t b/test/blackbox-tests/test-cases/pkg/ocamlformat/ocamlformat-patch-extra-files.t index 79d1ef751c1..d909909fd35 100644 --- a/test/blackbox-tests/test-cases/pkg/ocamlformat/ocamlformat-patch-extra-files.t +++ b/test/blackbox-tests/test-cases/pkg/ocamlformat/ocamlformat-patch-extra-files.t @@ -3,17 +3,21 @@ ocamlformat and its dependencies during the same run of the "dune fmt" command. loaded before the lock files are produced, this is why any 'patch' file inside 'dev-tools.locks/ocmalformat' is not copied inside the 'build' directory when a rule depends on it. -In the case of this issue, there is a rule that depends on an 'patch' file in order to copy the file +The issue was that there is a rule that depends on an 'patch' file in order to copy the file inside '_private/default/..' directory, since the file could not be copied, the rule is not activated. So it fails when 'dune' trying to apply the patch. After any follwing run of 'dune fmt', it works because the 'patch' file is already present. +The issue is now fixed. + + $ . ./helpers.sh $ mkrepo Make a fake ocamlformat: $ make_fake_ocamlformat "0.1" +A patch that changes the version from "0.1" to "0.26.2": $ cat > patch-for-ocamlformat.patch < diff a/ocamlformat.ml b/ocamlformat.ml > --- a/ocamlformat.ml @@ -55,29 +59,16 @@ Make the ocamlformat opam package which uses a patch: Make a project that uses the fake ocamlformat: $ make_project_with_dev_tool_lockdir -First run of 'dune fmt' - $ DUNE_CONFIG__LOCK_DEV_TOOL=enabled dune fmt 2>&1 | sed -E 's#.*.sandbox/[^/]+#.sandbox/$SANDBOX#g' +First run of 'dune fmt' is supposed to format the fail. + $ DUNE_CONFIG__LOCK_DEV_TOOL=enabled dune fmt Solution for dev-tools.locks/ocamlformat: - ocamlformat.0.26.2 - Error: - .sandbox/$SANDBOX/_private/default/.dev-tool/ocamlformat/ocamlformat/source/patch-for-ocamlformat.patch: - No such file or directory - -> required by - _build/_private/default/.dev-tool/ocamlformat/ocamlformat/target/bin/ocamlformat - -> required by _build/default/.formatted/foo.ml - -> required by alias .formatted/fmt - -> required by alias fmt - -The second run will works because the 'patch' is already in the source. - $ ls dev-tools.locks/ocamlformat/ocamlformat.files - patch-for-ocamlformat.patch - -Second run of 'dune fmt' - $ DUNE_CONFIG__LOCK_DEV_TOOL=enabled dune fmt File "foo.ml", line 1, characters 0-0: Error: Files _build/default/foo.ml and _build/default/.formatted/foo.ml differ. Promoting _build/default/.formatted/foo.ml to foo.ml. [1] + +The foo.ml file is now formatted with the patched version of ocamlformat. $ cat foo.ml formatted with version 0.26.2 From 1a4ee5f23d55f7bec4710f4aaa61dbb30a54c6dd Mon Sep 17 00:00:00 2001 From: Alpha DIALLO Date: Tue, 5 Nov 2024 15:48:11 +0100 Subject: [PATCH 2/2] pkg: Suggestions applied Signed-off-by: Alpha DIALLO --- bin/build_cmd.ml | 54 +++++------------------------------------------- bin/fmt.ml | 40 ++++++++++++++++++++++------------- 2 files changed, 31 insertions(+), 63 deletions(-) diff --git a/bin/build_cmd.ml b/bin/build_cmd.ml index b5e6518a51d..19fa78f0b20 100644 --- a/bin/build_cmd.ml +++ b/bin/build_cmd.ml @@ -82,40 +82,12 @@ let run_build_system ~common ~request = Fiber.return ()) ;; -let lock_ocamlformat () = - 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. *) - Lock_dev_tool.lock_ocamlformat () |> Memo.run - else Fiber.return () -;; - -let run_build_command_poll_eager - ~with_lock_ocamlformat - ~(common : Common.t) - ~config - ~request - : unit - = - let open Fiber.O in +let run_build_command_poll_eager ~(common : Common.t) ~config ~request : unit = Scheduler.go_with_rpc_server_and_console_status_reporting ~common ~config (fun () -> - Scheduler.Run.poll - (let* () = if with_lock_ocamlformat then lock_ocamlformat () else Fiber.return () in - run_build_system ~common ~request)) + Scheduler.Run.poll (run_build_system ~common ~request)) ;; -let run_build_command_poll_passive - ~with_lock_ocamlformat - ~(common : Common.t) - ~config - ~request:_ - : unit - = +let run_build_command_poll_passive ~(common : Common.t) ~config ~request:_ : unit = (* CR-someday aalekseyev: It would've been better to complain if [request] is non-empty, but we can't check that here because [request] is a function.*) let open Fiber.O in @@ -127,20 +99,16 @@ let run_build_command_poll_passive Scheduler.go_with_rpc_server_and_console_status_reporting ~common ~config (fun () -> Scheduler.Run.poll_passive ~get_build_request: - (let* () = - if with_lock_ocamlformat then lock_ocamlformat () else Fiber.return () - in - let+ (Build (targets, ivar)) = Dune_rpc_impl.Server.pending_build_action rpc in + (let+ (Build (targets, ivar)) = Dune_rpc_impl.Server.pending_build_action rpc in let request setup = Target.interpret_targets (Common.root common) config setup targets in run_build_system ~common ~request, ivar)) ;; -let run_build_command_once ~with_lock_ocamlformat ~(common : Common.t) ~config ~request = +let run_build_command_once ~(common : Common.t) ~config ~request = let open Fiber.O in let once () = - let* () = if with_lock_ocamlformat then lock_ocamlformat () else Fiber.return () in let+ res = run_build_system ~common ~request in match res with | Error `Already_reported -> raise Dune_util.Report_error.Already_reported @@ -154,18 +122,6 @@ let run_build_command ~(common : Common.t) ~config ~request = | Yes Eager -> run_build_command_poll_eager | Yes Passive -> run_build_command_poll_passive | No -> run_build_command_once) - ~with_lock_ocamlformat:false - ~common - ~config - ~request -;; - -let run_build_command_fmt ~(common : Common.t) ~config ~request = - (match Common.watch common with - | Yes Eager -> run_build_command_poll_eager - | Yes Passive -> run_build_command_poll_passive - | No -> run_build_command_once) - ~with_lock_ocamlformat:true ~common ~config ~request diff --git a/bin/fmt.ml b/bin/fmt.ml index e10cefc6cad..d77c0e4337b 100644 --- a/bin/fmt.ml +++ b/bin/fmt.ml @@ -13,6 +13,31 @@ let man = ] ;; +let lock_ocamlformat () = + 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. *) + Lock_dev_tool.lock_ocamlformat () |> Memo.run + else Fiber.return () +;; + +let run_fmt_command ~(common : Common.t) ~config ~request = + let open Fiber.O in + let once () = + let* () = lock_ocamlformat () in + let+ res = Build_cmd.run_build_system ~common ~request in + match res with + | Error `Already_reported -> raise Dune_util.Report_error.Already_reported + | Ok () -> () + in + Scheduler.go ~common ~config once +;; + let command = let term = let+ builder = Common.Builder.term @@ -32,24 +57,11 @@ let command = in let common, config = Common.init builder in 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 in - Build_cmd.run_build_command ~common ~config ~request + run_fmt_command ~common ~config ~request in Cmd.v (Cmd.info "fmt" ~doc ~man ~envs:Common.envs) term ;;