[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
?