[PATCH] lint: Verify if #:tests? is respected in the 'check' phase.

  • Done
  • quality assurance status badge
Details
2 participants
  • Maxime Devos
  • Mathieu Othacehe
Owner
unassigned
Submitted by
Maxime Devos
Severity
normal
M
M
Maxime Devos wrote on 9 May 2021 20:02
(address . guix-patches@gnu.org)
2b0fee1845a66e1fb126b4bbf1c9892b7c648a3a.camel@telenet.be
Hi guix,

There have been a few patches to the mailing list lately not
respecting this, and this linter detects 325 package definitions
that could be modified to support the --without-tests package
transformation.

Copyright lines were added in the previous patch I sent to guix-patches
today.

Greetings,
Maxime
Attachment: file
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYJgjpRccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7mS2AQDE6GmX2rgPP04WQhiWSFAe7vuJ
l058vc5A5t814RtvLQD/R89rFFBIwrYcoPdn7HacGBy5xnb711tZMLirBAsvWQw=
=V/0D
-----END PGP SIGNATURE-----


M
M
Mathieu Othacehe wrote on 18 Jun 2021 14:15
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 48320@debbugs.gnu.org)
87wnqrbc7d.fsf@gnu.org
Hello Maxime,

Toggle quote (8 lines)
> + (`(,(or 'lambda 'lambda*) ,_ (invoke . ,_) . ,_)
> + (list (make-warning package
> + ;; TRANSLATORS: check and #:tests? are a Scheme
> + ;; symbol and keyword respectively and should not
> + ;; be translated.
> + (G_ "the 'check' phase should respect #:tests?")
> + #:field 'arguments)))

I like the idea behind this patch. However I think the detection pattern
could be improved for instance, here are a few unreported packages:

- dejagnu
- python-dateutil
- eigen

Maybe we should check directly if the tests? variable is used within the
'check replace phase?

Thanks,

Mathieu
M
M
Maxime Devos wrote on 18 Jun 2021 17:34
(name . Mathieu Othacehe)(address . othacehe@gnu.org)(address . 48320@debbugs.gnu.org)
2ecfe7d292264ff24a80fa02c47dd566c831776a.camel@telenet.be
Mathieu Othacehe schreef op vr 18-06-2021 om 14:15 [+0200]:
Toggle quote (10 lines)
> Hello Maxime,
>
> > + (`(,(or 'lambda 'lambda*) ,_ (invoke . ,_) . ,_)
> > + (list (make-warning package
> > + ;; TRANSLATORS: check and #:tests? are a Scheme
> > + ;; symbol and keyword respectively and should not
> > + ;; be translated.
> > + (G_ "the 'check' phase should respect #:tests?")
> > + #:field 'arguments)))

I just noticed the following test case in (tests lint) is somewhat bogus:

Toggle quote (3 lines)
> + '((replace 'check+
> + (lambda (#:key tests? #:allow-other-keys?)

Instead of 'lambda', this should be 'lambda*'.

Also, the value for #:phases can now be a G-expression,
so the usage of 'package-arguments' in the patch would need to be adjusted
as well.

Toggle quote (10 lines)
> I like the idea behind this patch. However I think the detection pattern
> could be improved for instance, here are a few unreported packages:
>
> - dejagnu
> - python-dateutil
> - eigen
>
> Maybe we should check directly if the tests? variable is used within the
> 'check replace phase?

So, basically, test if applying the following procedure to the body
succeeds?

(define (sexp-uses-tests?? sexp)
(sexp-contains-atom? sexp 'tests?))

(define (sexp-contains-atom? sexp atom)
; atoms are compared with eq? and vectors are currently not supported
(if (pair? sexp)
(or (sexp-contains? sexp atom)
(sexp-contains? sexp atom))
(eq? sexp atom)))

That seems a good improvement for a v2.

Thanks,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYMy8+RccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7ssVAQDJwL8ffq8IaVa/aF0TbF/5h8u4
tDvV4B2gtF1orxOsqAEAjItEOgvCVnDrcR0Dr/Cij64KNUoo4PEygxjsBRmvWgM=
=SCrC
-----END PGP SIGNATURE-----


M
M
Maxime Devos wrote on 28 Jun 2021 23:15
[PATCH v2] lint: Verify if #:tests? is respected in the 'check' phase.
(address . 48320@debbugs.gnu.org)(name . Mathieu Othacehe)(address . othacehe@gnu.org)
44f0c8b823b0f6f8e5388ff6c1d90e76fa09bf2c.camel@telenet.be
Hi Guix,

This is a v2. It detects some more cases
(e.g. python-dateutil dejagnu and eigen).
It also allows letting '#:phases' be
a G-exp.

With thanks to Mathieu Othacehe.

Greetings,
Maxime.
From 8e898a6c0f3dfa086f1414115fb2f58fe36224b1 Mon Sep 17 00:00:00 2001
From: Maxime Devos <maximedevos@telenet.be>
Date: Mon, 28 Jun 2021 19:24:44 +0200
Subject: [PATCH v2 1/2] guix: gexp: Define gexp->approximate-sexp.
To: 48320@debbugs.gnu.org
Cc: Mathieu Othacehe <othacehe@gnu.org>

It will be used in the 'optional-tests' linter.

* guix/gexp.scm (gexp->approximate-sexp): New procedure.
* tests/gexp.scm
("no references", "unquoted gexp", "unquoted gexp (native)")
("spliced gexp", "unspliced gexp, approximated")
("unquoted gexp, approximated"): Test it.
* doc/gexp.scm ("G-Expressions"): Document it.
---
doc/guix.texi | 11 +++++++++++
guix/gexp.scm | 19 +++++++++++++++++++
tests/gexp.scm | 31 +++++++++++++++++++++++++++++++
3 files changed, 61 insertions(+)

Toggle diff (116 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index 15e8999447..cc81c417a0 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -10038,6 +10038,17 @@ corresponding to @var{obj} for @var{system}, cross-compiling for
has an associated gexp compiler, such as a @code{<package>}.
@end deffn
+@deffn {Procedure} gexp->approximate-sexp @var{gexp}
+Sometimes, it may be useful to convert a G-exp into a S-exp.
+For example, some linters (@pxref{Invoking guix lint})
+peek into the build phases of a package to detect potential
+problems. This conversion can be achieved with this
+procedure. However, some information can be lost in the
+process. More specifically, lowerable objects will be silently
+replaced with some arbitrary object -- currently the list
+@code{(*approximate*)}, but this may change.
+@end deffn
+
@node Invoking guix repl
@section Invoking @command{guix repl}
diff --git a/guix/gexp.scm b/guix/gexp.scm
index 187f5c5e85..f3d278b3e6 100644
--- a/guix/gexp.scm
+++ b/guix/gexp.scm
@@ -4,6 +4,7 @@
;;; Copyright © 2018 Jan Nieuwenhuizen <janneke@gnu.org>
;;; Copyright © 2019, 2020 Mathieu Othacehe <m.othacehe@gmail.com>
;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
+;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -42,6 +43,7 @@
with-imported-modules
with-extensions
let-system
+ gexp->approximate-sexp
gexp-input
gexp-input?
@@ -157,6 +159,23 @@
"Return the source code location of GEXP."
(and=> (%gexp-location gexp) source-properties->location))
+(define* (gexp->approximate-sexp gexp)
+ "Return the S-expression corresponding to GEXP, but do not lower anything.
+As a result, the S-expression will be approximate if GEXP has references."
+ (define (gexp-like? thing)
+ (or (gexp? thing) (gexp-input? thing)))
+ (apply (gexp-proc gexp)
+ (map (lambda (reference)
+ (match reference
+ (($ <gexp-input> thing output native)
+ (if (gexp-like? thing)
+ (gexp->approximate-sexp thing)
+ ;; Simply returning 'thing' won't work in some
+ ;; situations; see 'write-gexp' below.
+ '(*approximate*)))
+ (_ '(*approximate*))))
+ (gexp-references gexp))))
+
(define (write-gexp gexp port)
"Write GEXP on PORT."
(display "#<gexp " port)
diff --git a/tests/gexp.scm b/tests/gexp.scm
index 834e78b9a0..39a47d4e8c 100644
--- a/tests/gexp.scm
+++ b/tests/gexp.scm
@@ -1,5 +1,6 @@
;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2014, 2015, 2016, 2017, 2018, 2019, 2020, 2021 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -89,6 +90,36 @@
(test-begin "gexp")
+(test-equal "no references"
+ '(display "hello gexp->approximate-sexp!")
+ (gexp->approximate-sexp #~(display "hello gexp->approximate-sexp!")))
+
+(test-equal "unquoted gexp"
+ '(display "hello")
+ (let ((inside #~"hello"))
+ (gexp->approximate-sexp #~(display #$inside))))
+
+(test-equal "unquoted gexp (native)"
+ '(display "hello")
+ (let ((inside #~"hello"))
+ (gexp->approximate-sexp #~(display #+inside))))
+
+(test-equal "spliced gexp"
+ '(display '(fresh vegetables))
+ (let ((inside #~(fresh vegetables)))
+ (gexp->approximate-sexp #~(display '(#$@inside)))))
+
+(test-equal "unspliced gexp, approximated"
+ ;; (*approximate*) is really an implementation detail
+ '(display '(*approximate*))
+ (let ((inside (file-append coreutils "/bin/hello")))
+ (gexp->approximate-sexp #~(display '(#$@inside)))))
+
+(test-equal "unquoted gexp, approximated"
+ '(display '(*approximate*))
+ (let ((inside (file-append coreutils "/bin/hello")))
+ (gexp->approximate-sexp #~(display '#$inside))))
+
(test-equal "no refs"
'(display "hello!")
(let ((exp (gexp (display "hello!"))))
--
2.32.0
Attachment: file
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYNo7/hccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7p0vAP9OpY+Vuo5yymS1BQeAodl01J/3
Xr/764zjJkqajwaWYgEA6nlIgBrcpfZSZidFaElQDQKuVEUCyp+hwpxGNtt7GAs=
=GuJE
-----END PGP SIGNATURE-----


M
M
Mathieu Othacehe wrote on 29 Jun 2021 12:34
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 48320@debbugs.gnu.org)
87h7hhvu0n.fsf@gnu.org
Hello Maxime,

Thanks for the new revision.

Toggle quote (4 lines)
> +@deffn {Procedure} gexp->approximate-sexp @var{gexp}
> +Sometimes, it may be useful to convert a G-exp into a S-exp.
> +For example, some linters (@pxref{Invoking guix lint})

You can write longer sentences here, up to 78 columns. If you are using
Emacs, fill-paragraph does the right thing.

Toggle quote (10 lines)
> + (define (sexp-uses-tests?? sexp)
> + "Test if SEXP contains the symbol 'tests?'."
> + (sexp-contains-atom? sexp 'tests?))
> + (define (sexp-contains-atom? sexp atom)
> + "Test if SEXP contains ATOM."
> + (if (pair? sexp)
> + (or (sexp-contains-atom? (car sexp) atom)
> + (sexp-contains-atom? (cdr sexp) atom))
> + (eq? sexp atom)))

It would make more sense to define "sexp-uses-tests??" later as it uses
"sexp-contains-atom" that is defined afterwards.

Toggle quote (3 lines)
> + (or (check-phases-delta head)
> + (check-phases-deltas tail)))

I think it should be "append" instead of "or". Otherwise, it fails to
detect package which 'replace is not the first phase, see mkvtoolnix for
instance.

Otherwise looks fine :)

Thanks,

Mathieu
M
M
Maxime Devos wrote on 30 Jun 2021 12:31
[PATCH v3] lint: Verify if #:tests? is respected in the 'check' phase.
(name . Mathieu Othacehe)(address . othacehe@gnu.org)(address . 48320@debbugs.gnu.org)
db3aff7f3315f7092c50ded3fe9dab5d025e1041.camel@telenet.be
Mathieu Othacehe schreef op di 29-06-2021 om 12:34 [+0200]:
Toggle quote (11 lines)
> Hello Maxime,
>
> Thanks for the new revision.
>
> > +@deffn {Procedure} gexp->approximate-sexp @var{gexp}
> > +Sometimes, it may be useful to convert a G-exp into a S-exp.
> > +For example, some linters (@pxref{Invoking guix lint})
>
> You can write longer sentences here, up to 78 columns. If you are using
> Emacs, fill-paragraph does the right thing.

I did a "fill-paragraph" in the v3.

Toggle quote (13 lines)
> > + (define (sexp-uses-tests?? sexp)
> > + "Test if SEXP contains the symbol 'tests?'."
> > + (sexp-contains-atom? sexp 'tests?))
> > + (define (sexp-contains-atom? sexp atom)
> > + "Test if SEXP contains ATOM."
> > + (if (pair? sexp)
> > + (or (sexp-contains-atom? (car sexp) atom)
> > + (sexp-contains-atom? (cdr sexp) atom))
> > + (eq? sexp atom)))
>
> It would make more sense to define "sexp-uses-tests??" later as it uses
> "sexp-contains-atom" that is defined afterwards.

Indeed. I switched these two procedures around in the v3.

Toggle quote (7 lines)
> > + (or (check-phases-delta head)
> > + (check-phases-deltas tail)))
>
> I think it should be "append" instead of "or". Otherwise, it fails to
> detect package which 'replace is not the first phase, see mkvtoolnix for
> instance.

Indeed. I added a test case and replaced "or" with "append". The linter
now detects about 300 additional cases.

Greetings,
Maxime.
From 5835b32d916681db73fb2d91b3646d915bfbd0a8 Mon Sep 17 00:00:00 2001
From: Maxime Devos <maximedevos@telenet.be>
Date: Mon, 28 Jun 2021 19:24:44 +0200
Subject: [PATCH v3 1/2] guix: gexp: Define gexp->approximate-sexp.

It will be used in the 'optional-tests' linter.

* guix/gexp.scm (gexp->approximate-sexp): New procedure.
* tests/gexp.scm
("no references", "unquoted gexp", "unquoted gexp (native)")
("spliced gexp", "unspliced gexp, approximated")
("unquoted gexp, approximated"): Test it.
* doc/gexp.scm ("G-Expressions"): Document it.
---
doc/guix.texi | 10 ++++++++++
guix/gexp.scm | 19 +++++++++++++++++++
tests/gexp.scm | 31 +++++++++++++++++++++++++++++++
3 files changed, 60 insertions(+)

Toggle diff (115 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index 15e8999447..f051373571 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -10038,6 +10038,16 @@ corresponding to @var{obj} for @var{system}, cross-compiling for
has an associated gexp compiler, such as a @code{<package>}.
@end deffn
+@deffn {Procedure} gexp->approximate-sexp @var{gexp}
+Sometimes, it may be useful to convert a G-exp into a S-exp. For
+example, some linters (@pxref{Invoking guix lint}) peek into the build
+phases of a package to detect potential problems. This conversion can
+be achieved with this procedure. However, some information can be lost
+in the process. More specifically, lowerable objects will be silently
+replaced with some arbitrary object -- currently the list
+@code{(*approximate*)}, but this may change.
+@end deffn
+
@node Invoking guix repl
@section Invoking @command{guix repl}
diff --git a/guix/gexp.scm b/guix/gexp.scm
index 187f5c5e85..f3d278b3e6 100644
--- a/guix/gexp.scm
+++ b/guix/gexp.scm
@@ -4,6 +4,7 @@
;;; Copyright © 2018 Jan Nieuwenhuizen <janneke@gnu.org>
;;; Copyright © 2019, 2020 Mathieu Othacehe <m.othacehe@gmail.com>
;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
+;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -42,6 +43,7 @@
with-imported-modules
with-extensions
let-system
+ gexp->approximate-sexp
gexp-input
gexp-input?
@@ -157,6 +159,23 @@
"Return the source code location of GEXP."
(and=> (%gexp-location gexp) source-properties->location))
+(define* (gexp->approximate-sexp gexp)
+ "Return the S-expression corresponding to GEXP, but do not lower anything.
+As a result, the S-expression will be approximate if GEXP has references."
+ (define (gexp-like? thing)
+ (or (gexp? thing) (gexp-input? thing)))
+ (apply (gexp-proc gexp)
+ (map (lambda (reference)
+ (match reference
+ (($ <gexp-input> thing output native)
+ (if (gexp-like? thing)
+ (gexp->approximate-sexp thing)
+ ;; Simply returning 'thing' won't work in some
+ ;; situations; see 'write-gexp' below.
+ '(*approximate*)))
+ (_ '(*approximate*))))
+ (gexp-references gexp))))
+
(define (write-gexp gexp port)
"Write GEXP on PORT."
(display "#<gexp " port)
diff --git a/tests/gexp.scm b/tests/gexp.scm
index 834e78b9a0..39a47d4e8c 100644
--- a/tests/gexp.scm
+++ b/tests/gexp.scm
@@ -1,5 +1,6 @@
;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2014, 2015, 2016, 2017, 2018, 2019, 2020, 2021 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -89,6 +90,36 @@
(test-begin "gexp")
+(test-equal "no references"
+ '(display "hello gexp->approximate-sexp!")
+ (gexp->approximate-sexp #~(display "hello gexp->approximate-sexp!")))
+
+(test-equal "unquoted gexp"
+ '(display "hello")
+ (let ((inside #~"hello"))
+ (gexp->approximate-sexp #~(display #$inside))))
+
+(test-equal "unquoted gexp (native)"
+ '(display "hello")
+ (let ((inside #~"hello"))
+ (gexp->approximate-sexp #~(display #+inside))))
+
+(test-equal "spliced gexp"
+ '(display '(fresh vegetables))
+ (let ((inside #~(fresh vegetables)))
+ (gexp->approximate-sexp #~(display '(#$@inside)))))
+
+(test-equal "unspliced gexp, approximated"
+ ;; (*approximate*) is really an implementation detail
+ '(display '(*approximate*))
+ (let ((inside (file-append coreutils "/bin/hello")))
+ (gexp->approximate-sexp #~(display '(#$@inside)))))
+
+(test-equal "unquoted gexp, approximated"
+ '(display '(*approximate*))
+ (let ((inside (file-append coreutils "/bin/hello")))
+ (gexp->approximate-sexp #~(display '#$inside))))
+
(test-equal "no refs"
'(display "hello!")
(let ((exp (gexp (display "hello!"))))
--
2.32.0
Attachment: file
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYNxIHRccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7lQfAP9IgkU3DnISH3cWxQCopB3aJh3i
DL5fPJcnW41fT5k3xAEAiD21lY7yA4HxQibRtNg9iFhNhmuNaasPo4nPInrfQQM=
=2wb0
-----END PGP SIGNATURE-----


M
M
Mathieu Othacehe wrote on 30 Jun 2021 13:55
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 48320-done@debbugs.gnu.org)
87pmw3lg7f.fsf@gnu.org
Hey,

Toggle quote (3 lines)
> Indeed. I added a test case and replaced "or" with "append". The linter
> now detects about 300 additional cases.

Great, pushed on master. We now have some work to fix those ~600
packages!

Thanks,

Mathieu
Closed
?
Your comment

This issue is archived.

To comment on this conversation send an email to 48320@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 48320
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch