Fix issues exposed in previous commit

Signed-off-by: Jeremie Dimino <[email protected]>

CHANGES.md

@@ -75,6 +75,9 @@ Unreleased
 - Fix a bug where dune would always re-run all actions that produce symlinks,
   even if their dependencies did not change. (#4405, @aalekseyev)
 
+- Fix a bug that was causing Dune to re-hash generated files more
+  often than necessary (#4419, @jeremiedimino)
+
 2.8.5 (28/03/2021)
 ------------------
 

src/dune_engine/cached_digest.ml

@@ -160,17 +160,28 @@ let refresh fn = refresh_internal (Path.build fn)
 let refresh_and_chmod fn =
   let fn = Path.build fn in
   let stats = Path.lstat fn in
-  let () =
-    (* We remove write permissions to uniformize behavior regardless of whether
-       the cache is activated. No need to be zealous in case the file is not
-       cached anyway. See issue #3311. *)
-    if Cache.cachable stats.st_kind then
-      Path.chmod ~stats:(Some stats) ~mode:0o222 ~op:`Remove fn
-  in
   let stats =
     match stats.st_kind with
-    | S_LNK -> Path.stat fn
-    | _ -> stats
+    | S_LNK ->
+      (* If the path is a symbolic link, we don't try to remove write
+         permissions. For two reasons:
+
+         - if the destination was not a build path (i.e. in the build
+         directory), then it would definitely be wrong to do so
+
+         - if it is in the build directory, then we expect that the rule
+         producing this file will have taken core of chmodding it *)
+      Path.stat fn
+    | _ -> (
+      match Cache.cachable stats.st_kind with
+      | true ->
+        (* We remove write permissions to uniformize behavior regardless of
+           whether the cache is activated. No need to be zealous in case the
+           file is not cached anyway. See issue #3311. *)
+        let perm = stats.st_perm land lnot 0o222 in
+        Path.chmod ~stats:(Some stats) ~mode:perm ~op:`Set fn;
+        { stats with st_perm = perm }
+      | false -> stats)
   in
   refresh_ stats fn
 

test/blackbox-tests/test-cases/digest-cache/stats-used-in-db.t

@@ -1,32 +1,16 @@
-This test expose the following problem: inside the digest cache, we
-store parts of the file stats to detect when it changes and should be
-re-hashed. However, for targets we use the stats before removing the
-write permissions, which means that during the second run, dune always
-re-hash all the targets.
+Reproduction case for a bug we discovered in the past; Dune was
+storing the wrong stats for generated files in the digest cache and as
+a result was always re-hashing all files in the build directory on the
+second run.
+
+More precisely, we were using the stats of the file before making it
+read-only.
 
 
   $ echo '(lang dune 3.0)' > dune-project
   $ touch x y z
   $ dune build x y z --wait-for-filesystem-clock
 
-The following shouldn't rehash anything:
+Check that the next build rehashes nothing:
 
   $ dune build x y z --wait-for-filesystem-clock --debug-digests 2>&1 | sed 's/stats = { .*\( perm = [0-9]*\).*}/stats = {\1; ... }/'
-  Re-digested file _build/default/x because its stats changed:
-  { old_digest = digest "b83631c134a9649ec383d0eb9c356803"
-  ; new_digest = digest "b83631c134a9649ec383d0eb9c356803"
-  ; old_stats = { perm = 436; ... }
-  ; new_stats = { perm = 292; ... }
-  }
-  Re-digested file _build/default/y because its stats changed:
-  { old_digest = digest "b83631c134a9649ec383d0eb9c356803"
-  ; new_digest = digest "b83631c134a9649ec383d0eb9c356803"
-  ; old_stats = { perm = 436; ... }
-  ; new_stats = { perm = 292; ... }
-  }
-  Re-digested file _build/default/z because its stats changed:
-  { old_digest = digest "b83631c134a9649ec383d0eb9c356803"
-  ; new_digest = digest "b83631c134a9649ec383d0eb9c356803"
-  ; old_stats = { perm = 436; ... }
-  ; new_stats = { perm = 292; ... }
-  }