diff --git a/doc/changes/fixed/16675.md b/doc/changes/fixed/16675.md new file mode 100644 index 00000000000..bde743b0b35 --- /dev/null +++ b/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). diff --git a/src/dune_engine/sandbox.ml b/src/dune_engine/sandbox.ml index 253a7fde075..988dd033ddf 100644 --- a/src/dune_engine/sandbox.ml +++ b/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,13 +140,26 @@ 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 = @@ -154,7 +167,7 @@ let copy_recursively = 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 = diff --git a/test/blackbox-tests/test-cases/pkg/package-deps-run-binary.t b/test/blackbox-tests/test-cases/pkg/package-deps-run-binary.t index 88c842b9a79..e6c2fe43703 100644 --- a/test/blackbox-tests/test-cases/pkg/package-deps-run-binary.t +++ b/test/blackbox-tests/test-cases/pkg/package-deps-run-binary.t @@ -27,19 +27,14 @@ 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 @@ -47,7 +42,6 @@ Removing [(deps (package provider))] fixes the problem > (with-stdout-to out (run mybin)))) > EOF - $ dune build @all 2>&1 | censor - + $ dune build @all $ cat _build/default/out from provider diff --git a/test/blackbox-tests/test-cases/sandbox/copy-dir-deps.t b/test/blackbox-tests/test-cases/sandbox/copy-dir-deps.t new file mode 100644 index 00000000000..77790ac6a3f --- /dev/null +++ b/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 diff --git a/test/blackbox-tests/test-cases/sandbox/overlapping-dir-deps.t b/test/blackbox-tests/test-cases/sandbox/overlapping-dir-deps.t index 94442f07050..4b793f3284e 100644 --- a/test/blackbox-tests/test-cases/sandbox/overlapping-dir-deps.t +++ b/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 diff --git a/test/blackbox-tests/test-cases/sandbox/sandbox-hardlink-cyclic-symlink.t b/test/blackbox-tests/test-cases/sandbox/sandbox-hardlink-cyclic-symlink.t index e21f48b8ff3..03e7b65fc61 100644 --- a/test/blackbox-tests/test-cases/sandbox/sandbox-hardlink-cyclic-symlink.t +++ b/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]