fix(sandbox): share directory traversal across sandbox modes

doc/changes/fixed/16675.md

@@ -0,0 +1,2 @@
+- Support directory dependencies in hardlink sandboxes by linking their files
+  rather than the directories themselves (#16675, @Alizter).

src/dune_engine/sandbox.ml

@@ -114,7 +114,7 @@ let map_path t p =
   | No_sandbox _ -> p
 ;;
 
-let copy_recursively =
+let link_function ~(mode : Sandbox_mode.some) =
   let chmod_file = Permissions.add Permissions.write in
   let chmod_dir p =
     Permissions.add Permissions.execute p |> Permissions.add Permissions.write
@@ -125,7 +125,7 @@ let copy_recursively =
         [ Pp.text "Re-run Dune to delete the stale artifact, or manually delete this file"
         ]
       [ Pp.textf
-          "Failed to copy file %s of kind %S while creating a copy sandbox"
+          "Failed to sandbox file %s of kind %S"
           (Path.to_string_maybe_quoted src)
           (File_kind.to_string_hum kind)
       ]
@@ -140,21 +140,34 @@ let copy_recursively =
               "Re-run Dune to delete the stale artifact, or manually delete this file"
           ]
         [ Pp.textf
-            "Failed to copy file %s because it is a broken symbolic link while creating \
-             a copy sandbox"
+            "Failed to sandbox file %s because it is a broken symbolic link"
             (Path.to_string_maybe_quoted src)
         ]
   in
-  let copy_file ~src ~dst =
-    Io.copy_file ~overwrite:false ~chmod:chmod_file ~src ~dst ()
+  let copy_file =
+    match mode with
+    | Hardlink -> Io.portable_hardlink
+    | Copy | Patch_back_source_tree ->
+      (* We need to let the action modify its dependencies, so we copy
+         dependencies and make them writable. *)
+      fun ~src ~dst -> Io.copy_file ~overwrite:false ~chmod:chmod_file ~src ~dst ()
+    | Symlink ->
+      (match Sys.win32 with
+       | true ->
+         User_error.raise
+           [ Pp.text
+               "Sandboxing mode symlink is not supported on Windows. Use copy or \
+                hardlink instead."
+           ]
+       | false -> Io.portable_symlink)
   in
   let mkdir_with_perms ~src ~dst =
     let perms =
       (Unix.stat (Path.to_string src)).st_perm |> Permissions.Mode.of_int |> chmod_dir
     in
     Path.mkdir_p ~perms dst
   in
-  fun ~src ~dst ->
+  let copy_recursively ~src ~dst =
     Tree_copy.copy
       ~src
       ~dst
@@ -163,6 +176,13 @@ let copy_recursively =
       ~on_unsupported:raise_other_kind
       ~on_symlink:(`Call resolve_symlink_kind)
       ()
+  in
+  Staged.stage
+    (match mode with
+     | Patch_back_source_tree ->
+       (* CR-someday: this doesn't work with directory targets *)
+       copy_file
+     | Copy | Symlink | Hardlink -> copy_recursively)
 ;;
 
 let create_dir t dir =
@@ -183,31 +203,6 @@ let create_dir t dir =
 
 let create_dirs t dirs = Path.Build.Set.iter dirs ~f:(fun dir -> create_dir t dir)
 
-let link_function ~(mode : Sandbox_mode.some) =
-  let win32_error mode =
-    let mode = Sandbox_mode.to_string (Some mode) in
-    User_error.raise
-      [ Pp.textf
-          "Sandboxing mode %s is not supported on Windows. Use copy or hardlink instead."
-          mode
-      ]
-  in
-  Staged.stage
-    (match mode with
-     | Symlink ->
-       (match Sys.win32 with
-        | true -> win32_error mode
-        | false -> fun src dst -> Io.portable_symlink ~src ~dst)
-     | Copy -> fun src dst -> copy_recursively ~src ~dst
-     | Hardlink -> fun src dst -> Io.portable_hardlink ~src ~dst
-     | Patch_back_source_tree ->
-       (* We need to let the action modify its dependencies, so we copy
-          dependencies and make them writable. *)
-       (* CR-someday: this doesn't work with directory targets *)
-       let chmod = Permissions.add Permissions.write in
-       fun src dst -> Io.copy_file ~overwrite:false ~src ~dst ~chmod ())
-;;
-
 let link_deps t ~deps =
   let link = Staged.unstage (link_function ~mode:t.mode) in
   Path.Set.iter deps ~f:(fun path ->
@@ -221,7 +216,7 @@ let link_deps t ~deps =
           "Action depends on source tree. All actions should depend on the copies in the \
            build directory instead."
           [ "path", Path.to_dyn path ]
-    | Some p -> link path (Path.build (map_real_path t p)))
+    | Some p -> link ~src:path ~dst:(Path.build (map_real_path t p)))
 ;;
 
 let unlinked_deps t deps =

test/blackbox-tests/test-cases/pkg/package-deps-run-binary.t

@@ -27,27 +27,21 @@ correctly under the default sandbox.
   >   (with-stdout-to out (run mybin))))
   > EOF
 
-This should print "from provider", but fails instead:
-
-  $ dune build @all 2>&1 | censor
-  Error:
-  symlink(_build/.sandbox/$DIGEST1/_private/default/.pkg/provider.0.0.1-$DIGEST2/target): File exists
-  -> required by _build/default/out
-  -> required by alias all
-  [1]
+The overlapping package-directory and executable dependencies are both
+available to the action.
+
+  $ dune build @all
   $ cat _build/default/out
-  cat: _build/default/out: No such file or directory
-  [1]
+  from provider
 
-Removing [(deps (package provider))] fixes the problem
+Removing [(deps (package provider))] produces the same result.
 
   $ cat >dune <<'EOF'
   > (rule
   >  (action
   >   (with-stdout-to out (run mybin))))
   > EOF
 
-  $ dune build @all 2>&1 | censor
-
+  $ dune build @all
   $ cat _build/default/out
   from provider

test/blackbox-tests/test-cases/sandbox/copy-dir-deps.t

@@ -0,0 +1,49 @@
+A copy action on a directory dependency should produce regular files in every
+sandbox mode. This case has no overlapping dependencies.
+
+  $ make_dune_project 3.25
+  $ cat > dune <<'EOF'
+  > (rule
+  >  (target (dir tree))
+  >  (action (system "mkdir tree && echo contents > tree/file")))
+  > (rule
+  >  (target (dir copied))
+  >  (deps tree (sandbox always))
+  >  (action (no-infer (copy tree copied))))
+  > EOF
+
+First check that the directory dependency contains a regular file.
+
+  $ dune build tree/file
+  $ dune_cmd stat kind _build/default/tree/file
+  regular file
+
+CR: Shared traversal creates real sandbox directories containing file links.
+The recursive copy action rejects these links in symlink mode. This should
+succeed: the failure demonstrates why copy actions need to recognize
+sandbox-generated file links.
+
+  $ dune build copied --sandbox symlink
+  File "dune", lines 4-7, characters 0-99:
+  4 | (rule
+  5 |  (target (dir copied))
+  6 |  (deps tree (sandbox always))
+  7 |  (action (no-infer (copy tree copied))))
+  Error: Symlink file is not allowed here
+  [1]
+
+Copy mode produces a regular file with the expected contents.
+
+  $ dune build copied --sandbox copy
+  $ cat _build/default/copied/file
+  contents
+  $ dune_cmd stat kind _build/default/copied/file
+  regular file
+
+Hardlink mode also produces a regular file with the expected contents.
+
+  $ dune build copied --sandbox hardlink
+  $ cat _build/default/copied/file
+  contents
+  $ dune_cmd stat kind _build/default/copied/file
+  regular file

test/blackbox-tests/test-cases/sandbox/overlapping-dir-deps.t

@@ -23,16 +23,13 @@ First check that the directory target produces the expected input.
   $ cat _build/default/tree/file
   contents
 
-Symlink mode: the directory alone works, but adding an explicit file dependency
-fails.
+Symlink mode: both dependencies expose the expected contents. This does not
+establish that overlapping files are populated only once.
 
   $ dune build @directory --sandbox symlink
   contents
-  $ dune build @overlap --sandbox symlink 2>&1 | censor
-  Error:
-  symlink(_build/.sandbox/$DIGEST/default/tree): File exists
-  -> required by alias overlap in dune:8
-  [1]
+  $ dune build @overlap --sandbox symlink
+  contents
 
 Copy mode: reject the second attempt to populate an already copied file.
 This exposes the overlap rather than silently overwriting its contents. Normalize
@@ -49,24 +46,9 @@ both syscall errors and the portable copier's wrapped diagnostic.
   Error: _build/.sandbox/$DIGEST/default/tree/file: File exists
   -> required by alias overlap in dune:8
 
-Hardlink mode: a directory-only dependency already fails.
-
-  $ dune build @directory --sandbox hardlink 2>&1 | censor
-  Error:
-  link(_build/.sandbox/$DIGEST/default/tree): Operation not permitted
-  -> required by alias directory in dune:4
-  [1]
-
-With an overlapping file dependency, Linux reports EEXIST from link, then EISDIR
-from unlink. macOS rejects the directory directly with EPERM from link. Normalize
-these two diagnostics, checking Dune's exit status separately from the filter.
+Hardlink mode: both dependencies also expose the expected contents.
 
-  $ dune build @overlap --sandbox hardlink > hardlink.log 2>&1
-  [1]
-  $ sed -E \
-  >   -e 's/^link\((.*)\): Operation not permitted$/hardlink(\1): rejected/' \
-  >   -e 's/^unlink\((.*)\): Is a directory$/hardlink(\1): rejected/' \
-  >   hardlink.log | censor
-  Error:
-  hardlink(_build/.sandbox/$DIGEST/default/tree): rejected
-  -> required by alias overlap in dune:8
+  $ dune build @directory --sandbox hardlink
+  contents
+  $ dune build @overlap --sandbox hardlink
+  contents

test/blackbox-tests/test-cases/sandbox/sandbox-hardlink-cyclic-symlink.t

@@ -73,15 +73,10 @@ So, it seems like we must play dirty to create a symbolic link loop.
 
   $ (cd ..; rm link; ln -s file link)
 
-Finally, we get to see the error message printed out at sandbox creation.
+The shared traversal catches the cycle with [stat] during sandbox population,
+before attempting to create a hardlink.
 
   $ dune build t --sandbox hardlink
-  File "dune", lines 8-11, characters 0-103:
-   8 | (rule
-   9 |  (targets t)
-  10 |  (deps link dirty-rule)
-  11 |  (action (bash "cp %{deps} t; dune_cmd stat kind %{deps}")))
-  Error: Sandbox creation error: cannot resolve symbolic link
-  "_build/default/link".
-  Reason: Too many indirections; is this a cyclic symbolic link?
+  Error: stat(_build/default/link): Too many levels of symbolic links
+  -> required by _build/default/t
   [1]