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.

Sign up for GitHub

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

Jump to bottom

add: build_if in test stanza #7899

Merged
merged 1 commit into from Jun 19, 2023
Merged

add: build_if in test stanza #7899

merged 1 commit into from Jun 19, 2023

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Jun 6, 2023 •
edited

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).

@emillon emillon requested a review from rgrinberg June 6, 2023 11:40
@emillon
Copy link
Collaborator Author

emillon commented Jun 6, 2023 •
edited

Remarks:

  • I'm not sure about the syntax and field name; I also contemplated extending the enabled_if field with something like (enabled_if (build (...)))
  • this does not allow having a different predicate for "build if" and "test if" but I don't think we need that nuance.

@Alizter
Copy link
Collaborator

Alizter commented Jun 6, 2023

Why don't we want this semantics for enabled_if to begin with?

@emillon
Copy link
Collaborator Author

emillon commented Jun 6, 2023 •
edited

TODO:

  • test interaction with (package)
  • confirm that this works on eio and ctypes
  • write doc

@emillon
Copy link
Collaborator Author

emillon commented Jun 6, 2023

xrefs: #5529 #6134 #5505

@rgrinberg
Copy link
Member

rgrinberg commented Jun 6, 2023 •
edited

How about build_if? It's a little more in-line with our other names. Also, it gives you strictly more flexibility by allowing you to control both conditions.

@emillon
Copy link
Collaborator Author

emillon commented Jun 6, 2023

How about build_if? It's a little more in-line with our other names. Also, it gives you strictly more flexibility by allowing you to control both conditions.

do you mean that build_if would be an extra blang field? I can try that. I think there should be a shorthand for when the condition is the same though. what do you think about (build_if X) and no (enabled_if) be a shortcut for (build_if X) (enabled_if X)?

@rgrinberg
Copy link
Member

do you mean that build_if would be an extra blang field? I can try that

Exactly.

I think there should be a shorthand for when the condition is the same though. what do you think about (build_if X) and no (enabled_if) be a shortcut for (build_if X) (enabled_if X)?

Honestly, I wouldn't bother. We have the exact same issue in dozens of different places. We should just implement variables and solve it once and for all.

@emillon
Copy link
Collaborator Author

emillon commented Jun 6, 2023

Actually, it's not even necessary: (enabled_if) defaults to true so replacing (enabled_if) by (build_if) does the expected thing.

@rgrinberg rgrinberg added this to the 3.9.0 milestone Jun 6, 2023
@Alizter
Copy link
Collaborator

Alizter commented Jun 6, 2023

Will you update the docs?

@emillon
Copy link
Collaborator Author

emillon commented Jun 7, 2023

Will you update the docs?

Yes, it's literally in the TODO list above :)

@emillon emillon mentioned this pull request Jun 7, 2023
Closed
@emillon emillon changed the title add: skip_build in test stanza add: build_if in test stanza Jun 7, 2023
@emillon emillon mentioned this pull request Jun 7, 2023
Closed
18 tasks
@emillon
Copy link
Collaborator Author

emillon commented Jun 7, 2023

This works on the ctypes port; I also created an eio branch that uses that and seems to be working.

@tmattio
Copy link
Collaborator

tmattio commented Jun 7, 2023

This is super useful, thanks @emillon! I have the same question as @Alizter: why not change the semantics of enabled_if instead? It seems to me that if you don't want to run your test, it's not worth building it?

@rgrinberg
Copy link
Member

There's some situations where a test is only executable on a particular platform but it's still useful to make sure it at least builds on every platform for all developers. We have some tests like that in dune.

@emillon
Copy link
Collaborator Author

emillon commented Jun 8, 2023

Yes, exactly. For example, something that only runs if another package is available, or if you have an API key available. I would say it depends on what is in the enabled_if clause.

@tmattio
Copy link
Collaborator

tmattio commented Jun 8, 2023

That makes sense, thanks!

@avsm avsm mentioned this pull request Jun 9, 2023
Merged
@emillon emillon force-pushed the tests-skip-build branch 3 times, most recently from 09dc529 to e7eba3b Compare June 12, 2023 12:11
@nguermond nguermond mentioned this pull request Jun 13, 2023
Open
@emillon
add: build_if in test stanza
8f0183d 
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 emillon linked an issue Jun 19, 2023 that may be closed by this pull request
enabled_if not masking a library target #5505
Closed
@emillon emillon merged commit 0d482e9 into ocaml:main Jun 19, 2023
19 of 21 checks passed
@emillon emillon deleted the tests-skip-build branch June 19, 2023 13:40
@emillon emillon mentioned this pull request Jun 23, 2023
Closed
emillon added a commit to emillon/opam-repository that referenced this pull request Jun 23, 2023
@emillon
[new release] dune (15 packages) (3.9.0~alpha1)
e84760b 
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 emillon mentioned this pull request Jun 28, 2023
Merged
emillon added a commit to emillon/opam-repository that referenced this pull request Jun 28, 2023
@emillon
[new release] dune (15 packages) (3.9.0)
2e2aa14 
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)
@emillon emillon mentioned this pull request Jan 26, 2024
Closed
@jchavarri jchavarri mentioned this pull request Mar 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrinberg rgrinberg rgrinberg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.9.0
Development

Successfully merging this pull request may close these issues.

enabled_if doesn't work in (test) enabled_if not masking a library target
4 participants
@emillon @Alizter @rgrinberg @tmattio