diff mbox series

[bug#59487,1/2] build-system/dune: Automatically deduce test-target in most cases.

Message ID 20221122194712.31515-1-raingloom@riseup.net
State New
Headers show
Series [bug#59487,1/2] build-system/dune: Automatically deduce test-target in most cases. | expand

Commit Message

Csepp Nov. 22, 2022, 7:47 p.m. UTC 
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 | 9 +++++++--
 2 files changed, 8 insertions(+), 3 deletions(-)

Comments

Julien Lepiller Nov. 22, 2022, 8:26 p.m. UTC | #1 
Hi!

This LGTM, but maybe put test-target inside the cond too, so we can
remove the or?

I've looked at how opam does things, and this seems to be the norm:

["dune" "runtest" "-p" name "-j" jobs]

Maybe this would work better:

(apply invoke program "runtest"
       (append (if test-target (list test-target) '())
               ...))

WDYT?

Le Tue, 22 Nov 2022 20:47:11 +0100,
raingloom <raingloom@riseup.net> a écrit :

> 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 | 9 +++++++--
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> 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..8a3725a4e9 100644
> --- a/guix/build/dune-build-system.scm
> +++ b/guix/build/dune-build-system.scm
> @@ -42,12 +42,17 @@ (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")))
> +    (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" test-target
>               (append (if package (list "-p" package)
>                           dune-release-flags)
pukkamustard Jan. 10, 2023, 8:24 p.m. UTC | #2 
Hi! Sorry, I missed these patches and only found them after realizing
that your Mirage patches require these.

I'm having trouble applying the second patch (of V2), maybe there have
been changes that cause conflicts? Could you rebase on master or just
resend the V2 patches to 59487@debbugs.gnu.org? Maybe with a short
comment if and how they address Julien's suggestion?

-pukkamustard

raingloom <raingloom@riseup.net> writes:

> 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 | 9 +++++++--
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> 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..8a3725a4e9 100644
> --- a/guix/build/dune-build-system.scm
> +++ b/guix/build/dune-build-system.scm
> @@ -42,12 +42,17 @@ (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")))
> +    (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" test-target
>               (append (if package (list "-p" package)
>                           dune-release-flags)
Csepp Jan. 11, 2023, 5:41 p.m. UTC | #3 
pukkamustard <pukkamustard@posteo.net> writes:

> Hi! Sorry, I missed these patches and only found them after realizing
> that your Mirage patches require these.
>
> I'm having trouble applying the second patch (of V2), maybe there have
> been changes that cause conflicts? Could you rebase on master or just
> resend the V2 patches to 59487@debbugs.gnu.org? Maybe with a short
> comment if and how they address Julien's suggestion?
>
> -pukkamustard

Sent a rebased V2 just now.  The runtest invocation looks like Julien's
suggestion, so I think I addressed it.
diff mbox series

Patch

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..8a3725a4e9 100644
--- a/guix/build/dune-build-system.scm
+++ b/guix/build/dune-build-system.scm
@@ -42,12 +42,17 @@  (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")))
+    (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" test-target
              (append (if package (list "-p" package)
                          dune-release-flags)