From 0d482e9bdb105d7fc660b187e8f7eac3869b99f2 Mon Sep 17 00:00:00 2001 From: Etienne Millon Date: Mon, 19 Jun 2023 15:40:48 +0200 Subject: [PATCH] add: build_if in test stanza (#7899) 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 `(build_if)` field in `(test)`. When it evaluates to false, the test stanza is bypassed (no build is attempted). Signed-off-by: Etienne Millon --- CHANGES.md | 3 + doc/stanzas/test.rst | 4 + src/dune_rules/dune_file.ml | 6 ++ src/dune_rules/dune_file.mli | 1 + src/dune_rules/enabled_if.ml | 45 +++++------ src/dune_rules/enabled_if.mli | 6 ++ src/dune_rules/gen_rules.ml | 19 +++-- .../test-cases/test-build-if/feature.t | 76 +++++++++++++++++++ .../test-cases/test-build-if/package.t | 42 ++++++++++ .../test-cases/test-build-if/version.t | 25 ++++++ 10 files changed, 197 insertions(+), 30 deletions(-) create mode 100644 test/blackbox-tests/test-cases/test-build-if/feature.t create mode 100644 test/blackbox-tests/test-cases/test-build-if/package.t create mode 100644 test/blackbox-tests/test-cases/test-build-if/version.t diff --git a/CHANGES.md b/CHANGES.md index 6e995537b3c..6122f211678 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -69,6 +69,9 @@ Unreleased - Compute digests and manage sandboxes in background threads (#7947, @rgrinberg) +- Add `(build_if)` to the `(test)` stanza. When it evaluates to false, the + executable is not built. (#7899, fixes #6938, @emillon) + 3.8.1 (2023-06-05) ------------------ diff --git a/doc/stanzas/test.rst b/doc/stanzas/test.rst index c4ef281a422..3f1529a3a1d 100644 --- a/doc/stanzas/test.rst +++ b/doc/stanzas/test.rst @@ -37,6 +37,10 @@ In particular, all fields except for ``public_names`` are supported from the :ref:`executables stanza `. Alias fields apart from ``name`` are allowed. +The ``(enabled_if)`` field has special semantics: when present, it only applies +to running the tests. The test executable is always built by default. +If you need to restrict building the test executable, use ``(build_if)`` instead. + By default, the test binaries are run without options. The ``action`` field can override the test binary invocation, i.e., if you're using Alcotest and wish to see all the test failures on the standard output. When running Dune ``runtest`` diff --git a/src/dune_rules/dune_file.ml b/src/dune_rules/dune_file.ml index e6bbb1ed8e2..eb6cd8993d1 100644 --- a/src/dune_rules/dune_file.ml +++ b/src/dune_rules/dune_file.ml @@ -1892,6 +1892,7 @@ module Tests = struct ; package : Package.t option ; deps : Dep_conf.t Bindings.t ; enabled_if : Blang.t + ; build_if : Blang.t ; action : Dune_lang.Action.t option } @@ -1923,6 +1924,10 @@ module Tests = struct (Dune_lang.Syntax.since Stanza.syntax (2, 0) >>> repeat (located Lib_name.decode)) ~default:[] + and+ build_if = + field "build_if" ~default:Blang.true_ + (Syntax.since Stanza.syntax (3, 9) + >>> Enabled_if.decode_value ~allowed_vars:Any ()) in { exes = { Executables.link_flags @@ -1944,6 +1949,7 @@ module Tests = struct ; package ; deps ; enabled_if + ; build_if ; action })) diff --git a/src/dune_rules/dune_file.mli b/src/dune_rules/dune_file.mli index 36799915c90..8d915031b66 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 + ; build_if : Blang.t ; action : Dune_lang.Action.t option } end diff --git a/src/dune_rules/enabled_if.ml b/src/dune_rules/enabled_if.ml index 075b077a3f2..dd5d8d85c83 100644 --- a/src/dune_rules/enabled_if.ml +++ b/src/dune_rules/enabled_if.ml @@ -40,30 +40,31 @@ let emit_warning allowed_vars is_error var = (Dune_lang.Template.Pform.name var) ] -let decode ~allowed_vars ?(is_error = true) ~since () = - let decode = - match allowed_vars with - | Any -> Blang.decode - | Only allowed_vars -> - Blang.decode_manually (fun env var -> - match Dune_lang.Template.Pform.payload var with - | Some _ -> +let decode_value ~allowed_vars ?(is_error = true) () = + match allowed_vars with + | Any -> Blang.decode + | Only allowed_vars -> + Blang.decode_manually (fun env var -> + match Dune_lang.Template.Pform.payload var with + | Some _ -> + emit_warning allowed_vars is_error var; + Pform.Env.parse env var + | None -> ( + let name = Dune_lang.Template.Pform.name var in + match List.assoc allowed_vars name with + | None -> emit_warning allowed_vars is_error var; Pform.Env.parse env var - | None -> ( - let name = Dune_lang.Template.Pform.name var in - match List.assoc allowed_vars name with - | None -> - emit_warning allowed_vars is_error var; - Pform.Env.parse env var - | Some min_ver -> - let current_ver = Pform.Env.syntax_version env in - if min_ver > current_ver then - let loc = Dune_lang.Template.Pform.loc var in - let what = Dune_lang.Template.Pform.describe var in - Dune_lang.Syntax.Error.since loc Stanza.syntax min_ver ~what - else Pform.Env.unsafe_parse_without_checking_version env var)) - in + | Some min_ver -> + let current_ver = Pform.Env.syntax_version env in + if min_ver > current_ver then + let loc = Dune_lang.Template.Pform.loc var in + let what = Dune_lang.Template.Pform.describe var in + Dune_lang.Syntax.Error.since loc Stanza.syntax min_ver ~what + else Pform.Env.unsafe_parse_without_checking_version env var)) + +let decode ~allowed_vars ?is_error ~since () = + let decode = decode_value ?is_error ~allowed_vars () in let decode = match since with | None -> decode diff --git a/src/dune_rules/enabled_if.mli b/src/dune_rules/enabled_if.mli index 70749be99c7..e181dfa0c80 100644 --- a/src/dune_rules/enabled_if.mli +++ b/src/dune_rules/enabled_if.mli @@ -12,3 +12,9 @@ val decode : -> since:Dune_lang.Syntax.Version.t option -> unit -> Blang.t Dune_lang.Decoder.fields_parser + +val decode_value : + allowed_vars:allowed_vars + -> ?is_error:bool + -> unit + -> Blang.t Dune_lang.Decoder.t diff --git a/src/dune_rules/gen_rules.ml b/src/dune_rules/gen_rules.ml index 575b8e0d5f4..ec7a2ba0b00 100644 --- a/src/dune_rules/gen_rules.ml +++ b/src/dune_rules/gen_rules.ml @@ -110,14 +110,17 @@ 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 - in - { merlin = Some merlin - ; cctx = Some (tests.exes.buildable.loc, cctx) - ; js = None - ; source_dirs = None - } + let* enabled = Expander.eval_blang expander tests.build_if in + 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-build-if/feature.t b/test/blackbox-tests/test-cases/test-build-if/feature.t new file mode 100644 index 00000000000..b8dc8745e27 --- /dev/null +++ b/test/blackbox-tests/test-cases/test-build-if/feature.t @@ -0,0 +1,76 @@ +enabled_if has a limitation: it attempts building even if enabled_if evaluates to false. + + $ cat > dune-project << EOF + > (lang dune 3.9) + > EOF + + $ cat > dune << EOF + > (test + > (name t) + > (enabled_if %{env:ENABLED=false})) + > EOF + + $ touch t.ml + +We test the various combinations: + + $ test_one () { + > dune clean + > output=$( dune build "$1" --display short 2>&1 ) + > echo When building $1 with ENABLED=${ENABLED:-unset}: + > if echo $output|grep -q ocamlopt ; then + > echo ' build was done: YES' + > else + > echo ' build was done: NO' + > fi + > if echo $output|grep -q "alias runtest" ; then + > echo ' test did run: YES' + > else + > echo ' test did run: NO' + > fi + > } + + $ test_all () { + > test_one @all + > test_one @runtest + > ENABLED=true test_one @all + > ENABLED=true test_one @runtest + > } + + $ test_all + When building @all with ENABLED=unset: + build was done: YES + test did run: NO + When building @runtest with ENABLED=unset: + build was done: NO + test did run: NO + When building @all with ENABLED=true: + build was done: YES + test did run: NO + When building @runtest with ENABLED=true: + build was done: YES + test did run: YES + +Now with build_if: + + $ cat > dune << EOF + > (test + > (name t) + > (build_if %{env:ENABLED=false})) + > EOF + +Notice that in the first case, nothing is done at all: + + $ test_all + When building @all with ENABLED=unset: + build was done: NO + test did run: NO + When building @runtest with ENABLED=unset: + build was done: NO + test did run: NO + When building @all with ENABLED=true: + build was done: YES + test did run: NO + When building @runtest with ENABLED=true: + build was done: YES + test did run: YES diff --git a/test/blackbox-tests/test-cases/test-build-if/package.t b/test/blackbox-tests/test-cases/test-build-if/package.t new file mode 100644 index 00000000000..d9d66423c62 --- /dev/null +++ b/test/blackbox-tests/test-cases/test-build-if/package.t @@ -0,0 +1,42 @@ +build_if is compatible with package. + +This is important to test because in that case, (test) can not be split into two stanzas: + + $ cat > dune-project << EOF + > (lang dune 3.9) + > + > (package (name a) (allow_empty)) + > EOF + + $ cat > dune << EOF + > (test + > (name t) + > (package a) + > (build_if %{env:ENABLED=false})) + > EOF + + $ touch t.ml + + $ dune runtest + +If we try to split it we get an error: + + $ cat > dune << EOF + > (executable + > (name t) + > (package a) + > (enabled_if %{env:ENABLED=false})) + > + > (rule + > (alias runtest) + > (action (run ./t.exe)) + > (package a) + > (enabled_if %{env:ENABLED=false})) + > EOF + + $ dune runtest + File "dune", line 3, characters 1-12: + 3 | (package a) + ^^^^^^^^^^^ + Error: This field is useless without a (public_name ...) field. + [1] diff --git a/test/blackbox-tests/test-cases/test-build-if/version.t b/test/blackbox-tests/test-cases/test-build-if/version.t new file mode 100644 index 00000000000..e5abbbd9d47 --- /dev/null +++ b/test/blackbox-tests/test-cases/test-build-if/version.t @@ -0,0 +1,25 @@ + $ cat > dune-project << EOF + > (lang dune 3.8) + > EOF + + $ cat > dune << EOF + > (test + > (name t) + > (build_if true)) + > EOF + + $ touch t.ml + + $ dune build + File "dune", line 3, characters 1-16: + 3 | (build_if true)) + ^^^^^^^^^^^^^^^ + Error: 'build_if' 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 + + $ dune build