Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 18, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
94950b7
fix(pkg): `dune fmt` missing extra files from the locks.
moyodiallo Sep 23, 2024
4c27e2c
pkg: Suggestions applied
moyodiallo Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 26 additions & 14 deletions bin/fmt.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
;;
27 changes: 9 additions & 18 deletions test/blackbox-tests/test-cases/pkg/ocamlformat/ocamlformat-patch-extra-files.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



$ . ./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 <<EOF
> diff a/ocamlformat.ml b/ocamlformat.ml
> --- a/ocamlformat.ml
Expand Down Expand Up @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$ 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
Loading