search

ocaml-multicore / eio

Effects-based direct-style IO for multicore OCaml
Other
560 stars 72 forks source link

WIP: use build_if in test stanzas #550

Closed emillon closed 1 year ago

emillon commented 1 year ago

This is making sure that ocaml/dune#7899 fits the build for eio. For now I'm checking that CI passes, I'll update this with questions later.

emillon commented 1 year ago

( actually if someone can enable the workflow so that I can see the CI results :) )

emillon commented 1 year ago

It seems to work: the disabled tests do not run. Other than what's currently in the diff, is there something else that's not satisfactory at the moment?

The current situation with (allow_empty) does not look great to me. Maybe this warning should only trigger when -p thispackage is passed, I think that this would solve most issues.

talex5 commented 1 year ago

Thanks - this looks much better! Would be very happy to get rid of the allow_empty bits too, indeed.

talex5 commented 1 year ago

Other than what's currently in the diff, is there something else that's not satisfactory at the moment?

One minor annoyance is that we can't attach a (package eio_main) to the benchmark executable. As a work-around we define it as a (test) instead, which does take a package, and then disable the test part using (action (progn)): https://github.com/ocaml-multicore/eio/blob/264a7d0751f2a1cc9b16ecdf71480ea5c9ad068a/bench/dune#L1-L8

(reported at https://github.com/ocaml/dune/issues/5621, though maybe it should be a separate issue)

avsm commented 1 year ago

@emillon just wanted to check on the status of this -- will this feature be in the dune 2.9.0 release, and have you had a chance to look at @talex5's comment above regarding the benchmark executable?

emillon commented 1 year ago

(build_if) is part of 3.9.0 which has been just submitted to opam-repository.

As for the benchmark executable, yes that seems valid as well. Maybe we can have a system to override which alias (test) adds the rule to; in which case dune runtest would do nothing but dune build @bench would run the benchmark. I believe that it would work?

talex5 commented 1 year ago

Maybe we can have a system to override which alias (test) adds the rule to; in which case dune runtest would do nothing but dune build @bench would run the benchmark. I believe that it would work?

That sounds useful for this case. But it would be nice to fix the problem with executable too (for the reasons given in https://github.com/ocaml/dune/issues/5621).

talex5 commented 1 year ago

Fixed in #582 - thanks!