From 8e6c4f4a639625db204f8650a116359437f73f22 Mon Sep 17 00:00:00 2001 From: Rudi Grinberg Date: Sun, 29 Oct 2023 18:32:05 -0600 Subject: [PATCH] fix: allow internal dune actions to be sandboxed Internal actions that that don't need to be sandboxed would prevent the entire rule from being sandboxed. Now we just sandbox them even if it's not necessary. Signed-off-by: Rudi Grinberg --- src/dune_engine/build_system.ml | 20 ++++------------- test/blackbox-tests/test-cases/corrections.t | 23 +------------------- test/blackbox-tests/test-cases/sandboxing.t | 17 +++++++-------- 3 files changed, 13 insertions(+), 47 deletions(-) diff --git a/src/dune_engine/build_system.ml b/src/dune_engine/build_system.ml index 83fe57cb47d9..10d0a9b54612 100644 --- a/src/dune_engine/build_system.ml +++ b/src/dune_engine/build_system.ml @@ -470,22 +470,10 @@ end = struct let* () = Targets.maybe_async (fun () -> Path.mkdir_p (Path.build dir)) in let is_action_dynamic = Action.is_dynamic action.action in let sandbox_mode = - match Action.is_useful_to_sandbox action.action with - | Clearly_not -> - if Sandbox_config.mem action.sandbox Sandbox_mode.none - then Sandbox_mode.none - else - User_error.raise - ~loc - [ Pp.text - "Rule dependencies are configured to require sandboxing, but the rule \ - has no actions that could potentially require sandboxing." - ] - | Maybe -> - select_sandbox_mode - ~loc - action.sandbox - ~sandboxing_preference:config.sandboxing_preference + select_sandbox_mode + ~loc + action.sandbox + ~sandboxing_preference:config.sandboxing_preference in (* CR-someday amokhov: More [always_rerun] and [can_go_in_shared_cache] to [Rule_cache] too. *) diff --git a/test/blackbox-tests/test-cases/corrections.t b/test/blackbox-tests/test-cases/corrections.t index 454373357cb5..5b005d3e9ab0 100644 --- a/test/blackbox-tests/test-cases/corrections.t +++ b/test/blackbox-tests/test-cases/corrections.t @@ -103,28 +103,7 @@ One fix is to not look at it, and flag its existence as an error. differ. [1] -Sandboxing doesn't necessarily help either because dune thinks -[diff?] is not worth sandboxing. +Sandboxing helps since dune will sandbox all actions: $ dune build text-file-corrected $ dune build @correction1 --sandbox copy - File "text-file", line 1, characters 0-0: - Error: Files _build/default/text-file and _build/default/text-file-corrected - differ. - [1] - -Sandboxing does help if the command producing the -correction is non-trivial. - - $ cat > dune < (rule (alias correction1) - > (deps) - > (action - > (progn - > (bash "true") - > (diff? text-file text-file-corrected))) - > ) - > EOF - - $ dune build @correction1 --sandbox copy - diff --git a/test/blackbox-tests/test-cases/sandboxing.t b/test/blackbox-tests/test-cases/sandboxing.t index f15f511f023b..18901deb63b6 100644 --- a/test/blackbox-tests/test-cases/sandboxing.t +++ b/test/blackbox-tests/test-cases/sandboxing.t @@ -105,18 +105,17 @@ If rule fails to generate targets, we give a good error message, even with sandb - t [1] -If rule is configured to require sandboxing, but clearly needs none, -we give an error message: +If rule is configured to require sandboxing, but clearly needs none, we sandbox +anyway. $ true > dune - $ echo '(rule (target t) (deps (sandbox always)) (action (echo "")))' >> dune + $ cat >dune < (rule + > (target t) + > (deps (sandbox always)) + > (action (write-file t ""))) + > EOF $ dune build t - File "dune", line 1, characters 0-60: - 1 | (rule (target t) (deps (sandbox always)) (action (echo ""))) - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - Error: Rule dependencies are configured to require sandboxing, but the rule - has no actions that could potentially require sandboxing. - [1] If an action [chdir]s to a non-existing directory, it is created.