Move dev-tool lock dirs into hidden folder

[Leonidas-from-XIV]

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]

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]

slotting it in _build/ seems sensible

This makes sens to me too!

[davesnx]

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

[Alizter]

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]

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]

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>.

[shonfeder]

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

[rgrinberg]

How would this work in watch mode without Fs_memo?

[Alizter]

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.

[rgrinberg]

String.drop_prefix maybe?

[Leonidas-from-XIV]

Yes, that's much better. I was thinking whether to extend Path.External with something like val as_source : t -> Source.t option or something like this (similar to the "drop build context" functionality) but wasn't sure if it is useful enough to have it in Path.

[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.

[Alizter]

Does the previous code work? I don't see any reason it shouldn't here. IIUC we are now using external so the build context will not be dropped which Source_deps.files does.

[Leonidas-from-XIV]

No, Source_deps.files returns (Dep.Set.empty, Path.Set.empty) as it uses Source_tree.find_dir which does (correctly) return None on that path.

[Alizter]

This kind of matching is done often. Unfortunately it is wrong. We can specify --build-dir on the CLI. You should be matching against Path.build_dir which gets set appropriately.

[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.