Skip to content

fetch: Resolve file symlinks post fetch to enable caching #14871

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
Alizter merged 1 commit into from
Jun 9, 2026
Merged

fetch: Resolve file symlinks post fetch to enable caching #14871

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions doc/changes/fixed/14871.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
- Fix caching of package sources with symlinks by resolving symlinks after
fetching the sources, similar to the directory symlinks resolution introduced
in #13792 (#14871, fixes #14509, @punchagan)
58 changes: 37 additions & 21 deletions src/dune_pkg/fetch.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,9 +259,11 @@ let is_descendant t ~of_ =
| _ -> Path.is_descendant t ~of_
;;

(* Dune can't handle symbolic links pointing to directories, so we replace them
with regular directories containing the same contents. *)
let resolve_directory_symlinks_in root =
(* Dune's engine cannot handle symlinks to directories and Dune's shared cache
rejects directory targets containing symlinks (which project sources are).
There are technical reasons for both limitations, but for the sake of
building packages, resolving them as hardlinks is good enough. *)
let resolve_symlinks_in root =
let on_symlink ~dir:raw_dir name () =
let name = Filename.to_string name in
let relative = Path.relative (Path.relative root raw_dir) name in
Expand Down Expand Up @@ -308,7 +310,7 @@ let resolve_directory_symlinks_in root =
(Path.to_string resolved)
];
(match Unix.stat (Path.to_string resolved) with
| { Unix.st_kind = S_DIR; _ } ->
| { st_kind = S_DIR; _ } ->
Fpath.unlink_exn full_name;
let _ : Fpath.mkdir_p_result = Fpath.mkdir_p full_name in
(match Readdir.read_directory_with_kinds (Path.to_string resolved) with
Expand All @@ -329,8 +331,23 @@ let resolve_directory_symlinks_in root =
Io.portable_symlink ~src ~dst)
| _ -> ());
(), Some Unix.S_DIR)
| _ ->
(* We do not care about symlinks pointing to anything but directories. *)
| { st_kind = S_REG; _ } ->
(* We turn file symlinks into hardlinks to be able to cache them. *)
Io.portable_hardlink ~src:resolved ~dst:relative;
(), None
| { st_kind = _; _ } ->
(* We do not care about symlinks pointing to anything but directories
or files. *)
Log.info
"Fetch.resolve_symlinks_in: Skipped symlink pointing to unknown kind"
[ "full_name", Path.to_dyn resolved ];
(), None
| exception Unix.Unix_error (err, a, b) ->
Log.info
"Error when checking file kind"
Comment thread
punchagan marked this conversation as resolved.
[ "full_name", Path.to_dyn resolved
; "error", Unix_error.Detailed.to_dyn (err, a, b)
];
(), None)
in
Fpath.traverse
Expand Down Expand Up @@ -379,8 +396,7 @@ let fetch ~unpack ~checksum ~target ~url:(url_loc, url) =
match fetch_result with
| Ok () ->
let target_str = Path.to_string target in
if (Unix.lstat target_str).st_kind = S_DIR
then resolve_directory_symlinks_in target;
if (Unix.lstat target_str).st_kind = S_DIR then resolve_symlinks_in target;
Ok ()
| Error e -> Error e)
;;
Expand Down Expand Up @@ -485,7 +501,7 @@ let%test_module "resolve symlink tests" =
make_file dir "other_dir/file1.txt";
printfn "before";
dump_tree dir;
resolve_directory_symlinks_in dir;
resolve_symlinks_in dir;
printfn "\nafter";
dump_tree dir;
[%expect.output]);
Expand All @@ -511,7 +527,7 @@ let%test_module "resolve symlink tests" =
make_dir dir "real_dir/sub";
make_file dir "real_dir/sub/deep.txt";
make_symlink dir ~src:"real_dir" ~dst:"link";
resolve_directory_symlinks_in dir;
resolve_symlinks_in dir;
dump_tree dir;
[%expect.output]);
[%expect
Expand All @@ -529,7 +545,7 @@ let%test_module "resolve symlink tests" =
with_temp_dir "$OUTSIDE" (fun outside ->
with_temp_dir "$DIR" (fun dir ->
make_symlink dir ~src:(Path.reach ~from:dir outside) ~dst:"escape";
resolve_directory_symlinks_in dir;
resolve_symlinks_in dir;
[%expect.output]);
[%expect.output]);
[%expect
Expand All @@ -543,13 +559,13 @@ let%test_module "resolve symlink tests" =
with_temp_dir "$DIR" (fun dir ->
make_file dir "file.txt";
make_symlink dir ~src:"file.txt" ~dst:"link";
resolve_directory_symlinks_in dir;
resolve_symlinks_in dir;
dump_tree dir;
[%expect.output]);
[%expect
{|
$DIR/link [symlink -> file.txt]
$DIR/file.txt [file]
$DIR/link [hardlink]
$DIR/file.txt [hardlink of $DIR/link]
|}]
;;

Expand All @@ -558,7 +574,7 @@ let%test_module "resolve symlink tests" =
make_file dir "keep.txt";
make_symlink dir ~src:"nonexistent" ~dst:"broken_link";
(* We could check the log here to check the message. *)
resolve_directory_symlinks_in dir;
resolve_symlinks_in dir;
dump_tree dir;
[%expect.output]);
[%expect {| $DIR/keep.txt [file] |}]
Expand All @@ -572,7 +588,7 @@ let%test_module "resolve symlink tests" =
make_file dir "dir_b/file.txt";
make_symlink dir ~src:"../dir_b" ~dst:"dir_a/link_to_b";
make_symlink dir ~src:"../dir_a" ~dst:"dir_b/link_to_a";
resolve_directory_symlinks_in dir;
resolve_symlinks_in dir;
[%expect.output]);
[%expect
{|
Expand All @@ -589,7 +605,7 @@ let%test_module "resolve symlink tests" =
make_file dir "real_dir/regular.txt";
make_symlink dir ~src:"../target_dir/file.txt" ~dst:"real_dir/inner_link";
make_symlink dir ~src:"real_dir" ~dst:"link";
resolve_directory_symlinks_in dir;
resolve_symlinks_in dir;
dump_tree dir;
[%expect.output]);
[%expect
Expand All @@ -598,10 +614,10 @@ let%test_module "resolve symlink tests" =
$DIR/real_dir/ [dir]
$DIR/link/ [dir]
$DIR/link/regular.txt [hardlink]
$DIR/link/inner_link [symlink -> ../target_dir/file.txt]
$DIR/link/inner_link [hardlink]
$DIR/real_dir/regular.txt [hardlink of $DIR/link/regular.txt]
$DIR/real_dir/inner_link [symlink -> ../target_dir/file.txt]
$DIR/target_dir/file.txt [file]
$DIR/real_dir/inner_link [hardlink of $DIR/link/inner_link]
$DIR/target_dir/file.txt [hardlink of $DIR/link/inner_link, $DIR/real_dir/inner_link]
|}]
;;

Expand All @@ -613,7 +629,7 @@ let%test_module "resolve symlink tests" =
Unix.mkfifo (Path.to_string (Path.relative dir "real_dir/my_pipe")) 0o644;
make_symlink dir ~src:"my_pipe" ~dst:"real_dir/pipelink";
make_symlink dir ~src:"real_dir" ~dst:"link";
resolve_directory_symlinks_in dir;
resolve_symlinks_in dir;
dump_tree dir;
[%expect.output]);
[%expect
Expand Down
19 changes: 8 additions & 11 deletions test/blackbox-tests/test-cases/pkg/fetch-cache-symlink.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,21 +23,18 @@ disabling the download of the source a second time.

$ build_pkg test

We see cache store events for our targets in the trace:
We see no cache store errors and the cache store events for the fetched dir,
pkg-$DIGEST/source and pkg-$DIGEST/target in the trace:

$ dune trace cat | jq -s '[.[] | select(.args.message == "cache store target creation errors") ] | length'
Comment thread
punchagan marked this conversation as resolved.
1
0
$ dune trace cat | jq -s '[.[] | select(.args.message == "cache store success") ] | length'
3

Cleaning the project to force rebuilding. This triggers an attempt to
re-download the source, since it contains a symlink and wasn't cached:
Cleaning the project to force rebuilding. This no longer triggers an attempt to
re-download the source after the change to turn symlinks in the fetched source
into hardlinks:

$ dune clean
$ export DUNE_CACHE=enabled
$ build_pkg test
File "dune.lock/test.pkg", line 4, characters 7-25:
4 | (url http://localhost:1)
^^^^^^^^^^^^^^^^^^
Error: Download failed with code 404

[1]

Loading