| Message ID | 20230111174003.2227-1-raingloom@riseup.net |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#59487,v2,1/2] build-system/dune: Automatically deduce test-target in most cases. | expand |
Thanks for this! I think it is a valuable improvement. Csepp <raingloom@riseup.net> writes: > From: raingloom <raingloom@riseup.net> > > guix/build-system/dune.scm (dune-build): tests? defaults to #f. This should be: "test-target defaults to #f". > + (let ((program (if jbuild? "jbuilder" "dune")) > + (test-target (or test-target > + (cond > + ((file-exists? "tests") "tests") > + ((file-exists? "test") "test") > + (else "."))))) > + (apply invoke program "runtest" > + (append (if test-target (list test-target) '()) > + (if package (list "-p" package) > dune-release-flags) > test-flags)))) > #t) I think what Julien ment (and I agree) is that you can completely drop the checks for the files/directories "tests" or "test" to exist. In your patch, if test-target is #f and "test" or "tests" do not exist, then the we will run: `dune runtest -p package .` but we could (and maybe should) run just: `dune runtest -p package` In fact, we should run this even if the "test" or "tests" directories exist (otherwise we might miss running some tests placed in other directories). So this would be enough: > + (let ((program (if jbuild? "jbuilder" "dune"))) > + (apply invoke program "runtest" > + (append (if test-target (list test-target) '()) > + (if package (list "-p" package) > dune-release-flags) > test-flags))) Thinking of this, maybe we can drop the `test-target` argument completely. wdyt? -pukkamustard
pukkamustard <pukkamustard@posteo.net> writes: > Thinking of this, maybe we can drop the `test-target` argument > completely. wdyt? Yeah, I'm pretty sure we should drop the test-target argument. I don't see any reason why it should be used with the dune build-system. The two remaining packages that have a test-target that is not "test", "tests" or "." (and do not have tests disabled): - ocaml-frontc: Builds as expected when test-target is removed. - ocaml-cohttp: test-target points to a test helper library, not the tests themselves. Removing the test-target runs the tests, which currently fail. -pukkamustard
pukkamustard <pukkamustard@posteo.net> writes: > Thanks for this! I think it is a valuable improvement. > > Csepp <raingloom@riseup.net> writes: > >> From: raingloom <raingloom@riseup.net> >> >> guix/build-system/dune.scm (dune-build): tests? defaults to #f. > > This should be: "test-target defaults to #f". > >> + (let ((program (if jbuild? "jbuilder" "dune")) >> + (test-target (or test-target >> + (cond >> + ((file-exists? "tests") "tests") >> + ((file-exists? "test") "test") >> + (else "."))))) >> + (apply invoke program "runtest" >> + (append (if test-target (list test-target) '()) >> + (if package (list "-p" package) >> dune-release-flags) >> test-flags)))) >> #t) > > I think what Julien ment (and I agree) is that you can completely drop > the checks for the files/directories "tests" or "test" to exist. > > In your patch, if test-target is #f and "test" or "tests" do not exist, > then the we will run: > > `dune runtest -p package .` > > but we could (and maybe should) run just: > > `dune runtest -p package` > > In fact, we should run this even if the "test" or "tests" directories > exist (otherwise we might miss running some tests placed in other > directories). > > So this would be enough: > >> + (let ((program (if jbuild? "jbuilder" "dune"))) >> + (apply invoke program "runtest" >> + (append (if test-target (list test-target) '()) >> + (if package (list "-p" package) >> dune-release-flags) >> test-flags))) > > Thinking of this, maybe we can drop the `test-target` argument > completely. wdyt? > > -pukkamustard Hmm, makes sense. I'll test what complete removal does.
Csepp <raingloom@riseup.net> writes: > pukkamustard <pukkamustard@posteo.net> writes: > >> Thanks for this! I think it is a valuable improvement. >> >> Csepp <raingloom@riseup.net> writes: >> >>> From: raingloom <raingloom@riseup.net> >>> >>> guix/build-system/dune.scm (dune-build): tests? defaults to #f. >> >> This should be: "test-target defaults to #f". >> >>> + (let ((program (if jbuild? "jbuilder" "dune")) >>> + (test-target (or test-target >>> + (cond >>> + ((file-exists? "tests") "tests") >>> + ((file-exists? "test") "test") >>> + (else "."))))) >>> + (apply invoke program "runtest" >>> + (append (if test-target (list test-target) '()) >>> + (if package (list "-p" package) >>> dune-release-flags) >>> test-flags)))) >>> #t) >> >> I think what Julien ment (and I agree) is that you can completely drop >> the checks for the files/directories "tests" or "test" to exist. >> >> In your patch, if test-target is #f and "test" or "tests" do not exist, >> then the we will run: >> >> `dune runtest -p package .` >> >> but we could (and maybe should) run just: >> >> `dune runtest -p package` >> >> In fact, we should run this even if the "test" or "tests" directories >> exist (otherwise we might miss running some tests placed in other >> directories). >> >> So this would be enough: >> >>> + (let ((program (if jbuild? "jbuilder" "dune"))) >>> + (apply invoke program "runtest" >>> + (append (if test-target (list test-target) '()) >>> + (if package (list "-p" package) >>> dune-release-flags) >>> test-flags))) >> >> Thinking of this, maybe we can drop the `test-target` argument >> completely. wdyt? >> >> -pukkamustard > > Hmm, makes sense. I'll test what complete removal does. Well ocaml-ppxlib fails and I should be working my MirageOS thesis stuff, so I'll leave this up to you. Or I'll get back to it once I figure out how cross-toolchains are supposed to work.
diff --git a/guix/build-system/dune.scm b/guix/build-system/dune.scm index 12100fd8e8..b531d3c337 100644 --- a/guix/build-system/dune.scm +++ b/guix/build-system/dune.scm @@ -107,7 +107,7 @@ (define* (dune-build name inputs (dune-release-flags ''()) (tests? #t) (test-flags ''()) - (test-target "test") + (test-target #f) (install-target "install") (validate-runpath? #t) (patch-shebangs? #t) diff --git a/guix/build/dune-build-system.scm b/guix/build/dune-build-system.scm index e9ccc71057..e1337a3f82 100644 --- a/guix/build/dune-build-system.scm +++ b/guix/build/dune-build-system.scm @@ -42,14 +42,20 @@ (define* (build #:key (build-flags '()) (jbuild? #f) build-flags))) #t) -(define* (check #:key (test-flags '()) (test-target "test") tests? +(define* (check #:key (test-flags '()) (test-target #f) tests? (jbuild? #f) (package #f) (dune-release-flags '()) #:allow-other-keys) "Test the given package." (when tests? - (let ((program (if jbuild? "jbuilder" "dune"))) - (apply invoke program "runtest" test-target - (append (if package (list "-p" package) + (let ((program (if jbuild? "jbuilder" "dune")) + (test-target (or test-target + (cond + ((file-exists? "tests") "tests") + ((file-exists? "test") "test") + (else "."))))) + (apply invoke program "runtest" + (append (if test-target (list test-target) '()) + (if package (list "-p" package) dune-release-flags) test-flags)))) #t)
From: raingloom <raingloom@riseup.net> guix/build-system/dune.scm (dune-build): tests? defaults to #f. guix/build/dune-build-system.scm (check): Missing test-target is auto-detected. --- guix/build-system/dune.scm | 2 +- guix/build/dune-build-system.scm | 14 ++++++++++---- 2 files changed, 11 insertions(+), 5 deletions(-)