fix: remove [loc] from rule digest (#9052)

Signed-off-by: Rudi Grinberg <[email protected]>
ocaml · Nov 1, 2023 · 29b25ac · 29b25ac
1 parent 72bb20c
commit 29b25ac
Show file tree

Hide file tree

Showing 9 changed files with 46 additions and 25 deletions.
diff --git a/doc/changes/9052.md b/doc/changes/9052.md
@@ -0,0 +1 @@
+- Do not re-run rules when their location changes (#9052, @rgrinberg)
diff --git a/src/dune_engine/build_system.ml b/src/dune_engine/build_system.ml
@@ -235,7 +235,7 @@ end = struct
 
   (* The current version of the rule digest scheme. We should increment it when
      making any changes to the scheme, to avoid collisions. *)
-  let rule_digest_version = 18
+  let rule_digest_version = 19
 
   let compute_rule_digest
     (rule : Rule.t)
@@ -716,7 +716,7 @@ end = struct
     let digest =
       let { Rule.Anonymous_action.action =
               { action; env; locks; can_go_in_shared_cache; sandbox }
-          ; loc
+          ; loc = _
           ; dir
           ; alias
           }
@@ -743,7 +743,6 @@ end = struct
         , Dep.Set.digest deps
         , Action.for_shell action
         , List.map locks ~f:Path.to_string
-        , loc
         , dir
         , alias
         , capture_stdout

diff --git a/test/blackbox-tests/test-cases/dune-cache/mode-copy.t b/test/blackbox-tests/test-cases/dune-cache/mode-copy.t
@@ -40,9 +40,9 @@ never built [target1] before.
   $ dune build --config-file=config target1 --debug-cache=shared,workspace-local \
   >   2>&1 | grep '_build/default/source\|_build/default/target'
   Workspace-local cache miss: _build/default/source: never seen this target before
-  Shared cache miss [43284c58c2079faf9e5421c4d82a28c2] (_build/default/source): not found in cache
+  Shared cache miss [4c8aba9580c271d7ac111bf2d72a147a] (_build/default/source): not found in cache
   Workspace-local cache miss: _build/default/target1: never seen this target before
-  Shared cache miss [d92cafbd0f0c19a3a6c99407b959dc72] (_build/default/target1): not found in cache
+  Shared cache miss [68e477811b0e612a0cc0bb83c205420a] (_build/default/target1): not found in cache
 
   $ dune_cmd stat hardlinks _build/default/source
   1

diff --git a/test/blackbox-tests/test-cases/dune-cache/mode-hardlink.t b/test/blackbox-tests/test-cases/dune-cache/mode-hardlink.t
@@ -35,9 +35,9 @@ never built [target1] before.
   $ dune build --config-file=config target1 --debug-cache=shared,workspace-local \
   >   2>&1 | grep '_build/default/source\|_build/default/target'
   Workspace-local cache miss: _build/default/source: never seen this target before
-  Shared cache miss [afae5cd4afe24e40e88a52ffd372da14] (_build/default/source): not found in cache
+  Shared cache miss [4a1c82562ca4c3348fe36436814a9842] (_build/default/source): not found in cache
   Workspace-local cache miss: _build/default/target1: never seen this target before
-  Shared cache miss [8dd10495c1458bf12c3c55807797879c] (_build/default/target1): not found in cache
+  Shared cache miss [3ffbc9519f4ca5e44997bf8ba62de0bb] (_build/default/target1): not found in cache
 
   $ dune_cmd stat hardlinks _build/default/source
   3

diff --git a/test/blackbox-tests/test-cases/dune-cache/repro-check.t b/test/blackbox-tests/test-cases/dune-cache/repro-check.t
@@ -67,7 +67,7 @@ Set 'cache-check-probability' to 1.0, which should trigger the check
   > EOF
   $ rm -rf _build
   $ dune build --config-file config reproducible non-reproducible
-  Warning: cache store error [f533fad74b151f0d6cbf8c3f5a91fa30]: ((in_cache
+  Warning: cache store error [761869532e88535d64e09b60102c4416]: ((in_cache
   ((non-reproducible 1c8fc4744d4cef1bd2b8f5e915b36be9))) (computed
   ((non-reproducible 6cfaa7a90747882bcf4ffe7252c1cf89)))) after executing
   (echo 'build non-reproducible';cp dep non-reproducible)
@@ -119,7 +119,7 @@ Test that the environment variable and the command line flag work too
 
   $ rm -rf _build
   $ DUNE_CACHE_CHECK_PROBABILITY=1.0 dune build --cache=enabled reproducible non-reproducible
-  Warning: cache store error [f533fad74b151f0d6cbf8c3f5a91fa30]: ((in_cache
+  Warning: cache store error [761869532e88535d64e09b60102c4416]: ((in_cache
   ((non-reproducible 1c8fc4744d4cef1bd2b8f5e915b36be9))) (computed
   ((non-reproducible 6cfaa7a90747882bcf4ffe7252c1cf89)))) after executing
   (echo 'build non-reproducible';cp dep non-reproducible)
@@ -131,7 +131,7 @@ Test that the environment variable and the command line flag work too
 
   $ rm -rf _build
   $ dune build --cache=enabled --cache-check-probability=1.0 reproducible non-reproducible
-  Warning: cache store error [f533fad74b151f0d6cbf8c3f5a91fa30]: ((in_cache
+  Warning: cache store error [761869532e88535d64e09b60102c4416]: ((in_cache
   ((non-reproducible 1c8fc4744d4cef1bd2b8f5e915b36be9))) (computed
   ((non-reproducible 6cfaa7a90747882bcf4ffe7252c1cf89)))) after executing
   (echo 'build non-reproducible';cp dep non-reproducible)

diff --git a/test/blackbox-tests/test-cases/dune-cache/trim.t b/test/blackbox-tests/test-cases/dune-cache/trim.t
@@ -78,8 +78,8 @@ entries uniformly.
 
   $ (cd "$PWD/.xdg-cache/dune/db/meta/v5"; grep -rws . -e 'metadata' | sort ) > out
   $ cat out
-  ./8c/8c4aba72abad331531e3af2fb9f9cfaa:((8:metadata)(5:files(8:target_b32:8a53bfae3829b48866079fa7f2d97781)))
-  ./df/df307ded076980fa7dc2f5dbd563dbc1:((8:metadata)(5:files(8:target_a32:5637dd9730e430c7477f52d46de3909c)))
+  ./50/50673e014878b6b71ee39f1f32ca4726:((8:metadata)(5:files(8:target_a32:5637dd9730e430c7477f52d46de3909c)))
+  ./8f/8f658cc1fc1f083f42e98bbcd5a6ce2f:((8:metadata)(5:files(8:target_b32:8a53bfae3829b48866079fa7f2d97781)))
 
   $ digest="$(awk -F: '/target_b/ { digest=$1 } END { print digest }' < out)"
 

diff --git a/test/blackbox-tests/test-cases/patch-back-source-tree.t b/test/blackbox-tests/test-cases/patch-back-source-tree.t
@@ -201,7 +201,7 @@ produced in the sandbox and copied back:
 This is the internal stamp file:
 
   $ ls _build/.actions/default/blah*
-  _build/.actions/default/blah-182327d6e04fe09497790fcdbab8ca83
+  _build/.actions/default/blah-e7a0efae1209023d2186a50341cd25fa
 
 And we check that it isn't copied in the source tree:
 

diff --git a/test/blackbox-tests/test-cases/rule/digest.t/run.t b/test/blackbox-tests/test-cases/rule/digest.t/run.t
@@ -48,23 +48,41 @@ Now the same but with an alias.
   running...
   digest: $2
 
-Let's add a comment to the dune file. One might think that it doesn't affect
-the rule digest, but it does because all the locations gets shifted.
+Let's add a comment to the dune file. Dune does not re-run the rule because it
+has the same digest:
+
+  $ changelocation() {
+  > sed -i.bak '1s/^/;; spurious location change\n/' dune
+  > }
+
+  $ changelocation
+
+Now we make sure that failed rules re-run when the location changes:
 
   $ cat >dune <<EOF
-  > ; hello
   > (rule
-  >  (alias default)
-  >  (deps (sandbox always) pwd.ml)
-  >  (action (run ocaml pwd.ml)))
+  >  (alias foo)
+  >  (action (system "echo failing; exit 1")))
   > EOF
 
-... but it does! It would be nice to encode the locations in a way that would
-make them more resilient to non semantic changes.
+  $ dune build @foo
+  File "dune", line 1, characters 0-61:
+  1 | (rule
+  2 |  (alias foo)
+  3 |  (action (system "echo failing; exit 1")))
+  failing
+  [1]
+
+  $ changelocation
 
-# CR-someday amokhov: Remove actual digests from this test so that we don't
-# need to update it when rule digest version changes.
+This should re-run the action
+
+  $ dune build @foo
+  File "dune", line 2, characters 0-61:
+  2 | (rule
+  3 |  (alias foo)
+  4 |  (action (system "echo failing; exit 1")))
+  failing
+  [1]
 
   $ dune build @default
-  running...
-  digest: $3
diff --git a/test/blackbox-tests/test-cases/rule/dune b/test/blackbox-tests/test-cases/rule/dune
@@ -0,0 +1,3 @@
+(cram
+ (applies_to digest)
+ (deps %{bin:sed} %{bin:mktemp}))
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		- Do not re-run rules when their location changes (#9052, @rgrinberg)