Re-hash generated files less often

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

bin/common.ml

@@ -40,6 +40,7 @@ type t =
   ; debug_backtraces : bool
   ; debug_artifact_substitution : bool
   ; debug_digests : bool
+  ; wait_for_filesystem_clock : bool
   ; profile : Profile.t option
   ; workspace_file : Arg.Path.t option
   ; root : Workspace_root.t
@@ -134,6 +135,7 @@ let set_common ?log_file c =
   Clflags.debug_backtraces c.debug_backtraces;
   Clflags.debug_artifact_substitution := c.debug_artifact_substitution;
   Clflags.debug_digests := c.debug_digests;
+  Clflags.wait_for_filesystem_clock := c.wait_for_filesystem_clock;
   Clflags.capture_outputs := c.capture_outputs;
   Clflags.diff_command := c.diff_command;
   Clflags.promote := c.promote;
@@ -765,6 +767,19 @@ let term =
              ])
           Detect_external
       & info [ "file-watcher" ] ~doc)
+  and+ wait_for_filesystem_clock =
+    Arg.(
+      value & flag
+      & info
+          [ "wait-for-filesystem-clock" ]
+          ~doc:
+            "Dune digest file contents for better incrementally. These digests \
+             are themselves cached. In some cases, Dune needs to drop some \
+             digest cache entries in order for things to be reliable. This \
+             option makes Dune wait for the file system clock to advance so \
+             that it doesn't need to drop anything. You should probably not \
+             care about this option, it is mostly useful for Dune velopers to \
+             make Dune tests of the digest cache more reproducible.")
   in
   let build_dir = Option.value ~default:default_build_dir build_dir in
   let root = Workspace_root.create ~specified_by_user:root in
@@ -805,6 +820,7 @@ let term =
   ; debug_backtraces
   ; debug_artifact_substitution
   ; debug_digests
+  ; wait_for_filesystem_clock
   ; profile
   ; capture_outputs = not no_buffer
   ; workspace_file

src/dune_engine/cached_digest.ml

@@ -84,8 +84,17 @@ let get_current_filesystem_time () =
   Io.write_file special_path "<dummy>";
   (Path.stat special_path).st_mtime
 
+let wait_for_fs_clock_to_advance () =
+  let t = get_current_filesystem_time () in
+  while get_current_filesystem_time () <= t do
+    (* This is a blocking wait but we don't care too much. This code is only
+       used in the test suite. *)
+    Unix.sleepf 0.01
+  done
+
 let delete_very_recent_entries () =
   let cache = Lazy.force cache in
+  if !Clflags.wait_for_filesystem_clock then wait_for_fs_clock_to_advance ();
   let now = get_current_filesystem_time () in
   match Float.compare cache.max_timestamp now with
   | Lt -> ()
@@ -151,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
 

src/dune_engine/clflags.ml

@@ -12,6 +12,8 @@ let debug_artifact_substitution = ref false
 
 let debug_digests = ref false
 
+let wait_for_filesystem_clock = ref false
+
 let capture_outputs = ref true
 
 let debug_backtraces b =

src/dune_engine/clflags.mli

@@ -18,6 +18,10 @@ val debug_artifact_substitution : bool ref
 (** Print debug info for cached digests *)
 val debug_digests : bool ref
 
+(** Wait for the filesystem clock to advance rather than dropping cached digest
+    entries *)
+val wait_for_filesystem_clock : bool ref
+
 (** Command to use to diff things *)
 val diff_command : string option ref
 

test/blackbox-tests/test-cases/digest-cache/README

@@ -0,0 +1,7 @@
+This directory has various tests for the digest cache of Dune.
+
+In most of the tests, we use --wait-for-filesystem-clock to make the
+tests more reproducible. Without it, Dune might drop some entries or
+not depending on whether the time lap between targets being produced
+and dune saving the digest cache is smaller than the file system
+granularity.

test/blackbox-tests/test-cases/digest-cache/debug-option.t

@@ -0,0 +1,13 @@
+Test the --debug-digests command line option
+
+  $ echo '(lang dune 3.0)' > dune-project
+  $ touch x
+  $ dune build x --wait-for-filesystem-clock
+  $ echo 1 > x
+  $ dune build x --wait-for-filesystem-clock --debug-digests 2>&1 | sed 's/stats =.*/stats = XXX/'
+  Re-digested file x because its stats changed:
+  { old_digest = digest "b83631c134a9649ec383d0eb9c356803"
+  ; new_digest = digest "705e0d7e5030b1831b18211b1e398faf"
+  ; old_stats = XXX
+  ; new_stats = XXX
+  }

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

@@ -0,0 +1,16 @@
+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
+
+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; ... }/'