Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enabled_if doesn't work in (test) #6938

Closed
talex5 opened this issue Jan 26, 2023 · 6 comments · Fixed by #7899 or ocaml/opam-repository#24013
Closed

enabled_if doesn't work in (test) #6938

talex5 opened this issue Jan 26, 2023 · 6 comments · Fixed by #7899 or ocaml/opam-repository#24013
Milestone

Comments

@talex5
Copy link

talex5 commented Jan 26, 2023

Expected Behavior

We have:

(test
 (name test)
 (package eio_linux)
 (enabled_if (= %{system} "linux"))
 (modules test)
 (libraries alcotest eio_linux))

I expect this stanza to be ignored on macos. However, dune tries to build test.ml anyway. If I delete the stanza, it skips the file, as expected (reported by a macos user and tested on Linux by changing "linux" to "foobar" and confirming that it still tries to build it).

I consulted the dune documention for (test):

  • (test) says "the test stanza is the singular form of tests"
  • (tests) says "The optional fields supported are a subset of the alias and executables fields. In particular, all fields except for public_names are supported from the executables stanza. Alias fields apart from name are allowed."
  • (executables) says "(enabled_if <blang expression>) is the same as the corresponding field of library."
  • (library) says "The condition is specified using the Boolean Language, and the field allows for the %{os_type} variable, which is expanded to the type of OS being targeted by the current build."

However, I believe system is supposed to work. If I try a different variable name, dune says Error: Unknown variable %{baz}, which I infer to mean that it does understand system.

/cc @polytypic

Reproduction

$ ls
dune  dune-project  test.ml

$ cat dune
(test
 (name test)
 (enabled_if (= %{system} "foobar"))
 (modules test))

$ cat dune-project 
(lang dune 3.0)

$ cat test.ml 
BROKEN

$ dune build @all
File "test.ml", line 1, characters 0-6:
1 | BROKEN
    ^^^^^^
Error: Unbound constructor BROKEN

Specifications

  • Version of dune (output of dune --version): 3.6.1
  • Version of ocaml (output of ocamlc --version): 5.0.0
  • Operating system (distribution and version): Debian 11.6
@talex5
Copy link
Author

talex5 commented Jan 26, 2023

Looks related to #5505 from last March, except that there it is claimed that:

I had a look at this, this is only caused in the presence of stubs.

However, the test case above does not use stubs.

@talex5
Copy link
Author

talex5 commented Jan 26, 2023

You can almost work around this by splitting the (test) into separate (executable) and (alias) stanzas, except that then you hit dune's "This field is useless without a (public_name ...) field" bug (#5621).

talex5 pushed a commit to talex5/eio that referenced this issue Feb 27, 2023
talex5 pushed a commit to talex5/eio that referenced this issue Feb 27, 2023
emillon added a commit to emillon/dune that referenced this issue Jun 6, 2023
Fixes ocaml#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>
emillon added a commit to emillon/dune that referenced this issue Jun 6, 2023
Fixes ocaml#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>
emillon added a commit to emillon/dune that referenced this issue Jun 7, 2023
Fixes ocaml#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>
emillon added a commit to emillon/dune that referenced this issue Jun 7, 2023
Fixes ocaml#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 <me@emillon.org>
emillon added a commit to emillon/dune that referenced this issue Jun 9, 2023
Fixes ocaml#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 <me@emillon.org>
emillon added a commit to emillon/dune that referenced this issue Jun 12, 2023
Fixes ocaml#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 <me@emillon.org>
emillon added a commit to emillon/dune that referenced this issue Jun 12, 2023
Fixes ocaml#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 <me@emillon.org>
emillon added a commit to emillon/dune that referenced this issue Jun 19, 2023
Fixes ocaml#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 <me@emillon.org>
emillon added a commit that referenced this issue Jun 19, 2023
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 <me@emillon.org>
@rgrinberg rgrinberg added this to the 3.9.0 milestone Jun 20, 2023
emillon added a commit to emillon/opam-repository that referenced this issue Jun 23, 2023
CHANGES:

- Validate file extension for `$ dune ocaml top-module`. (ocaml/dune#8005, fixes ocaml/dune#8004, @3Rafal)

- Include the time it takes to read/write state files when `--trace-file` is
  enabled (ocaml/dune#7960, @rgrinberg)

- Add `dune show` command group which is an alias of `dune describe`. (ocaml/dune#7946,
  @Alizter)

- Include source tree scans in the traces produced by `--trace-file` (ocaml/dune#7937,
  @rgrinberg)

- Cinaps: The promotion rules for cinaps would only offer one file at a time no
  matter how many promotions were available. Now we offer all the promotions at
  once (ocaml/dune#7901, @rgrinberg)

- Do not re-run OCaml syntax files on every iteration of the watch mode. This
  is too memory consuming. (ocaml/dune#7894, fix ocaml/dune#6900, @rgrinberg)

- Remove some compatibility code for old version of dune that generated
  `.merlin` files. Now dune will never remove `.merlin` files automatically
  (ocaml/dune#7562)

- Add `dune show env` command and make `dune printenv` an alias of it. (ocaml/dune#7985,
  @Alizter)

- Add additional metadata to the traces provided by `--trace-file` whenever
  `--trace-extended` is passed (ocaml/dune#7778, @rleshchinskiy)

- Extensions used in `(dialect)` can contain periods (e.g., `cppo.ml`). (ocaml/dune#7782,
  fixes ocaml/dune#7777, @nojb)

- Allow `(include_subdirs qualified)` to be used when libraries define a
  `(modules ...)` field (ocaml/dune#7797, fixes ocaml/dune#7597, @anmonteiro)

- `$ dune describe` is now a command group, so arguments to subcommands must be
  passed after subcommand itself. (ocaml/dune#7919, @Alizter)

- The `interface` and `implementation` fields of a `(dialect)` are now optional
  (ocaml/dune#7757, @gpetiot)

- Add commands `dune show targets` and `dune show aliases` that display all the
  available targets and aliases in a given directory respectively. (ocaml/dune#7770,
  grants ocaml/dune#265, @Alizter)

- Allow multiple globs in library's `(stdlib (internal_modules ..))`
  (@anmonteiro, ocaml/dune#7878)

- Attach melange rules to the default alias (ocaml/dune#7926, @haochenx)

- In opam constraints, reject `(and)` and `(or)` with no arguments at parse
  time (ocaml/dune#7730, @emillon)

- Compute digests and manage sandboxes in background threads (ocaml/dune#7947,
  @rgrinberg)

- Add `(build_if)` to the `(test)` stanza. When it evaluates to false, the
  executable is not built. (ocaml/dune#7899, fixes ocaml/dune#6938, @emillon)

- Add necessary parentheses in generated opam constraints (ocaml/dune#7682, fixes ocaml/dune#3431,
  @Lucccyo)
emillon added a commit to emillon/opam-repository that referenced this issue Jun 28, 2023
CHANGES:

- Validate file extension for `$ dune ocaml top-module`. (ocaml/dune#8005, fixes ocaml/dune#8004, @3Rafal)

- Include the time it takes to read/write state files when `--trace-file` is
  enabled (ocaml/dune#7960, @rgrinberg)

- Add `dune show` command group which is an alias of `dune describe`. (ocaml/dune#7946,
  @Alizter)

- Include source tree scans in the traces produced by `--trace-file` (ocaml/dune#7937,
  @rgrinberg)

- Cinaps: The promotion rules for cinaps would only offer one file at a time no
  matter how many promotions were available. Now we offer all the promotions at
  once (ocaml/dune#7901, @rgrinberg)

- Do not re-run OCaml syntax files on every iteration of the watch mode. This
  is too memory consuming. (ocaml/dune#7894, fix ocaml/dune#6900, @rgrinberg)

- Add `--all` option to `dune rpc status` to show all Dune RPC servers running.
  (ocaml/dune#8011, fix ocaml/dune#7902, @Alizter)

- Remove some compatibility code for old version of dune that generated
  `.merlin` files. Now dune will never remove `.merlin` files automatically
  (ocaml/dune#7562)

- Add `dune show env` command and make `dune printenv` an alias of it. (ocaml/dune#7985,
  @Alizter)

- Add additional metadata to the traces provided by `--trace-file` whenever
  `--trace-extended` is passed (ocaml/dune#7778, @rleshchinskiy)

- Extensions used in `(dialect)` can contain periods (e.g., `cppo.ml`). (ocaml/dune#7782,
  fixes ocaml/dune#7777, @nojb)

- Allow `(include_subdirs qualified)` to be used when libraries define a
  `(modules ...)` field (ocaml/dune#7797, fixes ocaml/dune#7597, @anmonteiro)

- `$ dune describe` is now a command group, so arguments to subcommands must be
  passed after subcommand itself. (ocaml/dune#7919, @Alizter)

- The `interface` and `implementation` fields of a `(dialect)` are now optional
  (ocaml/dune#7757, @gpetiot)

- Add commands `dune show targets` and `dune show aliases` that display all the
  available targets and aliases in a given directory respectively. (ocaml/dune#7770,
  grants ocaml/dune#265, @Alizter)

- Allow multiple globs in library's `(stdlib (internal_modules ..))`
  (@anmonteiro, ocaml/dune#7878)

- Attach melange rules to the default alias (ocaml/dune#7926, @haochenx)

- In opam constraints, reject `(and)` and `(or)` with no arguments at parse
  time (ocaml/dune#7730, @emillon)

- Compute digests and manage sandboxes in background threads (ocaml/dune#7947,
  @rgrinberg)

- Add `(build_if)` to the `(test)` stanza. When it evaluates to false, the
  executable is not built. (ocaml/dune#7899, fixes ocaml/dune#6938, @emillon)

- Add necessary parentheses in generated opam constraints (ocaml/dune#7682, fixes ocaml/dune#3431,
  @Lucccyo)
@davesnx
Copy link
Contributor

davesnx commented Jan 24, 2024

I would re-open this issue since It's still holds true, enabled_if doesn't work in test stanza.

@emillon
Copy link
Collaborator

emillon commented Jan 26, 2024

What do you mean by "enabled_if doesn't work with test stanzas"?

Enabling a test can have several meanings:

  • if you want to conditionally run a test, use enabled_if
  • if you want to conditionally build a test, use build_if (added in add: build_if in test stanza #7899, which has closed this issue)

@davesnx
Copy link
Contributor

davesnx commented Jan 26, 2024

TIL about build_if

I set enabled_if and the build was still passing, my bad. I'm unsure how to educate users without the need to look for the documentation on this case.

@emillon
Copy link
Collaborator

emillon commented Jan 26, 2024

It's mentioned in https://dune.readthedocs.io/en/stable/dune-files.html#tests and users might also see this discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants