From 182f441f76d37bc05228ad975d29de02fa2045f2 Mon Sep 17 00:00:00 2001
From: Etienne Millon <me@emillon.org>
Date: Tue, 6 Jun 2023 16:57:17 +0200
Subject: [PATCH] add: skip_build in test stanza

Fixes #6938

The semantics of `(enabled_if)` in `(test)` can be confusing:
`(test)` can be seen as the combination of `(executable)` and a `(rule
(alias runtest))`; but `(enabled_if)` actually only controls the
"running" part, not the "building" one.

This adds a new `(skip_build)` field in `(test)`. When present, the
semantics of `(enabled_if)` extend to building the test executable.

In other words, if the `(enabled_if)` field of such a stanza evaluates
to `false`, it is as if the stanza was not present.

Signed-off-by: Etienne Millon <me@emillon.org>
---
 CHANGES.md                                    |  3 ++
 src/dune_rules/dune_file.ml                   |  4 ++
 src/dune_rules/dune_file.mli                  |  1 +
 src/dune_rules/gen_rules.ml                   | 20 ++++++----
 .../test-cases/test-skip-build.t              | 40 +++++++++++++++++++
 5 files changed, 61 insertions(+), 7 deletions(-)
 create mode 100644 test/blackbox-tests/test-cases/test-skip-build.t

diff --git a/CHANGES.md b/CHANGES.md
index b7d4e7525a8f..c9a143c76a2f 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -39,6 +39,9 @@ Unreleased
 - Fix RPC buffer corruption issues due to multi threading. This issue was only
   reproducible with large RPC payloads (#7418)
 
+- Add `(skip_build)` to the `(test)` stanza. When set, `(enabled_if)` also
+  controls whether the executable is built. (#...., fixes #6938, @emillon)
+
 3.8.0 (2023-05-23)
 ------------------
 
diff --git a/src/dune_rules/dune_file.ml b/src/dune_rules/dune_file.ml
index 098fdb035c7e..e1a9c84b6c36 100644
--- a/src/dune_rules/dune_file.ml
+++ b/src/dune_rules/dune_file.ml
@@ -1905,6 +1905,7 @@ module Tests = struct
     ; package : Package.t option
     ; deps : Dep_conf.t Bindings.t
     ; enabled_if : Blang.t
+    ; skip_build : bool
     ; action : Dune_lang.Action.t option
     }
 
@@ -1936,6 +1937,8 @@ module Tests = struct
               (Dune_lang.Syntax.since Stanza.syntax (2, 0)
               >>> repeat (located Lib_name.decode))
               ~default:[]
+          and+ skip_build =
+            field_b "skip_build" ~check:(Syntax.since Stanza.syntax (3, 9))
           in
           { exes =
               { Executables.link_flags
@@ -1957,6 +1960,7 @@ module Tests = struct
           ; package
           ; deps
           ; enabled_if
+          ; skip_build
           ; action
           }))
 
diff --git a/src/dune_rules/dune_file.mli b/src/dune_rules/dune_file.mli
index 36799915c907..2c594760fafc 100644
--- a/src/dune_rules/dune_file.mli
+++ b/src/dune_rules/dune_file.mli
@@ -359,6 +359,7 @@ module Tests : sig
     ; package : Package.t option
     ; deps : Dep_conf.t Bindings.t
     ; enabled_if : Blang.t
+    ; skip_build : bool
     ; action : Dune_lang.Action.t option
     }
 end
diff --git a/src/dune_rules/gen_rules.ml b/src/dune_rules/gen_rules.ml
index 575b8e0d5f4e..0ce152e3352c 100644
--- a/src/dune_rules/gen_rules.ml
+++ b/src/dune_rules/gen_rules.ml
@@ -110,14 +110,20 @@ end = struct
       let+ () = Simple_rules.alias sctx alias ~dir ~expander in
       empty_none
     | Tests tests ->
-      let+ cctx, merlin =
-        Test_rules.rules tests ~sctx ~dir ~scope ~expander ~dir_contents
+      let* enabled =
+        if tests.skip_build then Expander.eval_blang expander tests.enabled_if
+        else Memo.return true
       in
-      { merlin = Some merlin
-      ; cctx = Some (tests.exes.buildable.loc, cctx)
-      ; js = None
-      ; source_dirs = None
-      }
+      if enabled then
+        let+ cctx, merlin =
+          Test_rules.rules tests ~sctx ~dir ~scope ~expander ~dir_contents
+        in
+        { merlin = Some merlin
+        ; cctx = Some (tests.exes.buildable.loc, cctx)
+        ; js = None
+        ; source_dirs = None
+        }
+      else Memo.return empty_none
     | Copy_files { files = glob; _ } ->
       let* source_dirs =
         let loc = String_with_vars.loc glob in
diff --git a/test/blackbox-tests/test-cases/test-skip-build.t b/test/blackbox-tests/test-cases/test-skip-build.t
new file mode 100644
index 000000000000..848bfe1f7f93
--- /dev/null
+++ b/test/blackbox-tests/test-cases/test-skip-build.t
@@ -0,0 +1,40 @@
+  $ cat > dune-project << EOF
+  > (lang dune 3.8)
+  > EOF
+
+The enabled_if field evaluates to true then BUILD=true is passed.
+
+  $ cat > dune << EOF
+  > (test
+  >  (name t)
+  >  (enabled_if %{env:BUILD=false})
+  >  (skip_build))
+  > EOF
+
+  $ cat > t.ml << EOF
+  > broken
+  > EOF
+
+Version check: this only works on 3.9+.
+
+  $ dune build
+  File "dune", line 4, characters 1-13:
+  4 |  (skip_build))
+       ^^^^^^^^^^^^
+  Error: 'skip_build' is only available since version 3.9 of the dune language.
+  Please update your dune-project file to have (lang dune 3.9).
+  [1]
+
+  $ cat > dune-project << EOF
+  > (lang dune 3.9)
+  > EOF
+
+The build is attempted only when the enabled_if clause evaluates to true.
+
+  $ dune build
+  $ BUILD=true dune build
+  File "t.ml", line 1, characters 0-6:
+  1 | broken
+      ^^^^^^
+  Error: Unbound value broken
+  [1]