From a5285d7ef379a2af610beaf5bd6babfdff64a72c Mon Sep 17 00:00:00 2001 From: Rudi Grinberg Date: Thu, 26 Oct 2023 16:22:53 -0600 Subject: [PATCH] fix: remove [loc] from rule digest Signed-off-by: Rudi Grinberg --- doc/changes/9052.md | 1 + src/dune_engine/build_system.ml | 5 +-- .../test-cases/dune-cache/mode-copy.t | 4 +- .../test-cases/dune-cache/mode-hardlink.t | 4 +- .../test-cases/dune-cache/repro-check.t | 6 +-- .../test-cases/dune-cache/trim.t | 4 +- .../test-cases/patch-back-source-tree.t | 2 +- .../test-cases/rule/digest.t/run.t | 42 +++++++++++++------ test/blackbox-tests/test-cases/rule/dune | 3 ++ 9 files changed, 46 insertions(+), 25 deletions(-) create mode 100644 doc/changes/9052.md create mode 100644 test/blackbox-tests/test-cases/rule/dune diff --git a/doc/changes/9052.md b/doc/changes/9052.md new file mode 100644 index 00000000000..e9e5c613b24 --- /dev/null +++ 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 index 344f49a4ad8..d3158e2d075 100644 --- 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 index 6b31aff2f47..c28308fc6d9 100644 --- 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 index 05c13bd1bca..87b9dc76a39 100644 --- 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 index 536a892db22..7523d91b923 100644 --- 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 index 5540fe30c98..499dbb881ca 100644 --- 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 index 000510adef6..07222bc9633 100644 --- 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 index b0cf36295bb..b4ccfc8c962 100644 --- 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 < ; 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 new file mode 100644 index 00000000000..cf948030e44 --- /dev/null +++ b/test/blackbox-tests/test-cases/rule/dune @@ -0,0 +1,3 @@ +(cram + (applies_to digest) + (deps %{bin:sed} %{bin:mktemp}))