Move dev-tool lock dirs into hidden folder #12671

Leonidas-from-XIV · 2025-11-04T14:55:59Z

This follows the discussion from the 29th of October where we decided that the dev-tools can (for now) live in a directory that is unlikely to be committed to git.

closes #12097

Alizter · 2025-11-04T15:09:53Z

You can interpret the statement "move dev-tool lock dirs into hidden folder" in two different ways. Either that the dev-tool.lock folder becomes hidden or the folder actually gets moved to a hidden location.

When we discussed this I initially meant the latter case. Doing something like slotting it in _build/ seems sensible.

All the issues we have with people complaining about the folder currently will not go away when we make the directory hidden IMO.

shonfeder · 2025-11-04T17:32:52Z

slotting it in _build/ seems sensible

This makes sens to me too!

davesnx · 2025-11-05T09:12:06Z

I'm out of the conversation, but I wonder if dev-tools folder could be inside dune.lock folder?

Alizter · 2025-11-05T10:59:13Z

The issue we have here is that we don't want people committing their developer tool lock files but we don't mind people committing the build locks. If we merge them, then we cause problems for both parties.

davesnx · 2025-11-05T11:55:20Z

Right, I figure it out but maybe up to them? If it's the only issue, can place a .gitignore inside dune-lock?

Place it under _build means that any rm -rf would cause issues

Leonidas-from-XIV · 2025-11-05T13:39:54Z

Place it under _build means that any rm -rf would cause issues.

It would not, as then Dune will recreate the build plan for dev-tools if it does not exist. This is what it is doing at the moment already if you delete your dev-tools.locks/<lock-dir>.

src/dune_rules/lock_dir.ml

shonfeder · 2025-11-10T15:35:08Z

I believe this is read for another round of reviews :)

rgrinberg · 2025-11-10T17:07:53Z

src/dune_rules/utop.ml

-    Fs_memo.dir_exists (In_source_dir path))
+  lazy
+    (let path = Lock_dir.dev_tool_untracked_lock_dir Utop in
+     Path.Untracked.exists path)


How would this work in watch mode without Fs_memo?

It won't, but that wasn't the goal here. Fs_memo requires Path.Outside_build_dir which is going to have to be expanded if we are to account for _build/something-that's-not-a-context.

src/dune_rules/lock_dir.ml

test/blackbox-tests/test-cases/pkg/ocamlformat/ocamlformat-dev-tool-fails-to-build.t

src/dune_rules/lock_rules.ml

src/dune_pkg/workspace.ml

rgrinberg

As far as I can tell, this PR completely breaks watch mode (not just for dev tools) in its current state. If that's the case, we should really address this problem. I think the fix should simple and just use Fs_memo rather than raw IO.

src/dune_rules/pkg_rules.ml

src/dune_rules/lock_rules.ml

src/dune_pkg/lock_dir.ml

src/dune_pkg/workspace.ml

Alizter

We can use Source_deps.files:

diff --git a/src/dune_rules/lock_rules.ml b/src/dune_rules/lock_rules.ml
index 8cf21a6305..50b076dd30 100644
--- a/src/dune_rules/lock_rules.ml
+++ b/src/dune_rules/lock_rules.ml
@@ -450,51 +450,9 @@
             ~dirs:(Path.Build.Set.singleton target))
 ;;
 
-let files dir =
-  let rec recurse dir =
-    match Path.Untracked.readdir_unsorted_with_kinds dir with
-    | Ok entries ->
-      entries
-      |> List.fold_left
-           ~init:(Path.Set.empty, Path.Set.empty)
-           ~f:(fun (files, empty_directories) (entry, kind) ->
-             let path = Path.relative dir entry in
-             match (kind : Unix.file_kind) with
-             | S_REG ->
-               let files = Path.Set.add files path in
-               files, empty_directories
-             | S_DIR ->
-               let files', empty_directories' = recurse path in
-               (match Path.Set.is_empty files', Path.Set.is_empty empty_directories' with
-                | true, true ->
-                  let empty_directories = Path.Set.add empty_directories path in
-                  files, empty_directories
-                | _, _ ->
-                  let files = Path.Set.union files files' in
-                  let empty_directories =
-                    Path.Set.union empty_directories empty_directories'
-                  in
-                  files, empty_directories)
-             | otherwise ->
-               Code_error.raise
-                 "unsupported kind of file in folder"
-                 [ "path", Path.to_dyn path; "kind", File_kind.to_dyn otherwise ])
-    | Error (ENOENT, _, _) -> Path.Set.empty, Path.Set.empty
-    | Error unix_error ->
-      User_error.raise
-        [ Pp.textf
-            "Failed to read lock dir files of %s:"
-            (Path.to_string_maybe_quoted dir)
-        ; Pp.text (Unix_error.Detailed.to_string_hum unix_error)
-        ]
-  in
-  let files, empty_directories = recurse dir in
-  Dep.Set.of_source_files ~files ~empty_directories, files
-;;
 
 let setup_copy_rules ~dir:target ~assume_src_exists ~lock_dir =
-  let+ () = Memo.return () in
-  let deps, files = files lock_dir in
+  let+ deps, files = Source_deps.files lock_dir in
   let directory_targets, rules =
     match Path.Set.is_empty files with
     | true -> Path.Build.Map.empty, Rules.empty

together with

diff --git a/src/dune_rules/source_deps.ml b/src/dune_rules/source_deps.ml
index 5806dc874b..0062566636 100644
--- a/src/dune_rules/source_deps.ml
+++ b/src/dune_rules/source_deps.ml
@@ -10,6 +10,8 @@
   let prefix_with, dir =
     match (dir : Path.t) with
     | In_source_tree dir -> Path.root, dir
+    | External _ ->
+      Path.Expert.try_localize_external dir |> Path.extract_build_context_dir_exn
     | otherwise -> Path.extract_build_context_dir_exn otherwise
   in
   Source_tree.find_dir dir

or something similar.

If you run the tests, all will pass except for ocamlformat-patch-extra-files.t. I plan to fix this whole thing in a solution to #12741 so I don't mind disabling that test for now, or updating the bad output.

Leonidas-from-XIV · 2025-11-19T10:03:38Z

If you run the tests, all will pass except for ocamlformat-patch-extra-files.t. I plan to fix this whole thing in a solution to #12741 so I don't mind disabling that test for now, or updating the bad output.

I'm worried about disabling this test because it is the only test that exercises a lot of behavior like files_dir. Most of the work changing from one path type to another has been spent on making ocamlformat-patch-extra-files.t work, thus I think it exercises some important functionality and disabling it makes our test-suite notably less complete.

I am a bit wary of making Source_deps.files accept Path.External.t as an input. It seems kind of odd that a function that iterates over the sources would accept external paths. That said it already accepts (and transforms) Path.Build.t which also seems somewhat odd. @rgrinberg what do you think?

Alizter · 2025-11-19T13:55:52Z

src/dune_rules/lock_rules.ml

    | true -> Path.Build.Map.empty, Rules.empty
    | false ->
+      let deps =
+        match assume_src_exists with


Why would we assume the source exists?

Because if we don't then the build system will try to build the source (and fail as the source is not created through rules but outside the build system). Thus we have to pretend that these are not dependencies in the sense of "things the build system can create".

This used to work before because the files existed in the source directory and every file in the source directory can be used as a dependency for the build system. But now that the dev-tools are not there, the build system can't, thus we have to tell the system to just assume there will be some code executed that will place the data in _build/.dev-tools.locks that the build system is not aware of.

Once the dev tools can be solved by the build system this can go away, as the build system will be able to create them.

Let's focus on solving the issues with dev tools first then. Adding in hacks like this will just cause us more headaches in the future.

This was the original goal of #11775 however a lot of issues with dev-tools cropped up so eventually #12653 was merged which did not include automatic locking of dev-tools.

The consensus to go forward for the MSP was that we move the dev-tool lock files out of the users source dir. Originally we wanted to move all lock dirs (#12648) but then we decided that we will just move the dev-tool lock dirs instead, while keeping the project lock dirs as-is. This is exactly what this PR does. Lets not change the focus, given the work here is done, has tests and does not prevent a better solution in the future.

I too would have preferred if the dev-tool issues in #11775 could've been solved (hence then the build system could have just created the dependencies) but solving the issues with dev-tool lock dirs as targets is a bigger can of worms and we don't have this work scheduled.

Signed-off-by: Marek Kubica <[email protected]>

Leonidas-from-XIV added this to the [pkg]: Minimal Stable Product milestone Nov 4, 2025

Leonidas-from-XIV added the package management label Nov 4, 2025

Leonidas-from-XIV force-pushed the dev-tool-dotfiles branch 4 times, most recently from 7a2d49f to 4c44f66 Compare November 7, 2025 14:09

Leonidas-from-XIV marked this pull request as ready for review November 7, 2025 14:09

Leonidas-from-XIV requested review from Alizter, ElectreAAS, Sudha247 and punchagan November 7, 2025 14:09

Alizter reviewed Nov 7, 2025

View reviewed changes

src/dune_rules/lock_dir.ml Outdated Show resolved Hide resolved

shonfeder mentioned this pull request Nov 7, 2025

Rename lockdirs to discourage checking them into version control #12648

Closed

shonfeder assigned Leonidas-from-XIV Nov 10, 2025

Leonidas-from-XIV force-pushed the dev-tool-dotfiles branch 2 times, most recently from 57e78aa to 8cc6508 Compare November 10, 2025 13:47

Leonidas-from-XIV requested a review from Alizter November 10, 2025 15:14

Leonidas-from-XIV force-pushed the dev-tool-dotfiles branch from 8cc6508 to 0e622f0 Compare November 10, 2025 15:28

rgrinberg reviewed Nov 10, 2025

View reviewed changes

src/dune_rules/lock_dir.ml Outdated Show resolved Hide resolved

Leonidas-from-XIV force-pushed the dev-tool-dotfiles branch 2 times, most recently from 7a26281 to 9e80516 Compare November 12, 2025 16:33

Alizter reviewed Nov 17, 2025

View reviewed changes

test/blackbox-tests/test-cases/pkg/ocamlformat/ocamlformat-dev-tool-fails-to-build.t Outdated Show resolved Hide resolved

Leonidas-from-XIV requested a review from Alizter November 17, 2025 15:16

shonfeder requested a review from rgrinberg November 17, 2025 20:21

rgrinberg reviewed Nov 17, 2025

View reviewed changes

src/dune_rules/lock_rules.ml Outdated Show resolved Hide resolved

rgrinberg reviewed Nov 17, 2025

View reviewed changes

src/dune_pkg/workspace.ml Outdated Show resolved Hide resolved

rgrinberg requested changes Nov 17, 2025

View reviewed changes

src/dune_rules/pkg_rules.ml Outdated Show resolved Hide resolved

Alizter reviewed Nov 18, 2025

View reviewed changes

src/dune_rules/lock_rules.ml Show resolved Hide resolved

Leonidas-from-XIV force-pushed the dev-tool-dotfiles branch from 8d5db71 to 4edba3d Compare November 18, 2025 10:39

Alizter reviewed Nov 18, 2025

View reviewed changes

src/dune_pkg/lock_dir.ml Outdated Show resolved Hide resolved

Alizter reviewed Nov 18, 2025

View reviewed changes

src/dune_pkg/workspace.ml Outdated Show resolved Hide resolved

Alizter reviewed Nov 18, 2025

View reviewed changes

src/dune_pkg/workspace.ml Outdated Show resolved Hide resolved

Alizter requested changes Nov 18, 2025

View reviewed changes

Leonidas-from-XIV force-pushed the dev-tool-dotfiles branch from c0a81ed to 341cd12 Compare November 19, 2025 10:04

Leonidas-from-XIV requested a review from rgrinberg November 19, 2025 10:04

Alizter reviewed Nov 19, 2025

View reviewed changes

Leonidas-from-XIV added 13 commits November 19, 2025 15:07

Move dev-tool lock dirs from source into into build folder

a8307e9

Signed-off-by: Marek Kubica <[email protected]>

Use Path type

9c4f505

Signed-off-by: Marek Kubica <[email protected]>

Turn the dev-tool path to a Path.External.t

9814f37

Signed-off-by: Marek Kubica <[email protected]>

Fix after rebase

c0b6fc8

Signed-off-by: Marek Kubica <[email protected]>

Remove unused indirection

ea1fa36

Signed-off-by: Marek Kubica <[email protected]>

Rewrite absolute external paths into relative paths

63858d1

Signed-off-by: Marek Kubica <[email protected]>

Bring back Fs_memo

008bffe

Signed-off-by: Marek Kubica <[email protected]>

Improve implementation of external to source mapping

0f41c42

Signed-off-by: Marek Kubica <[email protected]>

Closer to style of main

377a859

Signed-off-by: Marek Kubica <[email protected]>

Add validation that path to be deleted is in fact managed

235b1d0

Signed-off-by: Marek Kubica <[email protected]>

Use try_localize instead of rolling my own version of it

d48ecd0

Signed-off-by: Marek Kubica <[email protected]>

Do not assume build dir name

def60d8

Signed-off-by: Marek Kubica <[email protected]>

Add cram test to show that custom build dirs work

7b71f8d

Signed-off-by: Marek Kubica <[email protected]>

Leonidas-from-XIV force-pushed the dev-tool-dotfiles branch from 31fc686 to 7b71f8d Compare November 19, 2025 14:07

fixup! Add cram test to show that custom build dirs work

55ee4c9

Move dev-tool lock dirs into hidden folder #12671

Are you sure you want to change the base?

Move dev-tool lock dirs into hidden folder #12671

Uh oh!

Conversation

Leonidas-from-XIV commented Nov 4, 2025 • edited by Alizter Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alizter commented Nov 4, 2025

Uh oh!

shonfeder commented Nov 4, 2025

Uh oh!

davesnx commented Nov 5, 2025

Uh oh!

Alizter commented Nov 5, 2025

Uh oh!

davesnx commented Nov 5, 2025

Uh oh!

Leonidas-from-XIV commented Nov 5, 2025

Uh oh!

Uh oh!

shonfeder commented Nov 10, 2025

Uh oh!

rgrinberg Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Alizter Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgrinberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Alizter left a comment

Choose a reason for hiding this comment

Uh oh!

Leonidas-from-XIV commented Nov 19, 2025

Uh oh!

Alizter Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Leonidas-from-XIV Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Alizter Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Leonidas-from-XIV Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Leonidas-from-XIV commented Nov 4, 2025 •

edited by Alizter

Loading