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

WIP: use build_if in test stanzas #550

Closed
wants to merge 2 commits into from

Conversation

emillon
Copy link

@emillon emillon commented Jun 7, 2023

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
Copy link
Author

emillon commented Jun 7, 2023

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

@emillon
Copy link
Author

emillon commented Jun 7, 2023

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
Copy link
Collaborator

talex5 commented Jun 7, 2023

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

@talex5
Copy link
Collaborator

talex5 commented Jun 7, 2023

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

eio/bench/dune

Lines 1 to 8 in 264a7d0

; This should be an executable, but dune won't let us associate non-installed executables
; to packages, so we use this work-around.
(test
(name main)
(package eio_main)
(deps ./main.exe)
(action (progn)) ; Don't run as a test
(libraries eio_main yojson))

(reported at ocaml/dune#5621, though maybe it should be a separate issue)

@avsm
Copy link
Contributor

avsm commented Jun 28, 2023

@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
Copy link
Author

emillon commented Jun 28, 2023

(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
Copy link
Collaborator

talex5 commented Jun 29, 2023

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 ocaml/dune#5621).

@talex5
Copy link
Collaborator

talex5 commented Nov 6, 2023

Fixed in #582 - thanks!

@talex5 talex5 closed this Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants