[PATCH] Check if #:tests? is unconditionally #t; not acceptable when cross-compiling

  • 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 14:35
(address . guix-patches@gnu.org)
ede44afaa1f52d80c0b6967524197fd0776b5ec2.camel@telenet.be
Hi guix,

This patch defines a linter detecting

(arguments `(#:tests? #t et cetera))

in package definitions. This is unlikely to work well when
cross-compiling (some exceptions apply, e.g. take a look
at the 'fennel' package). Also, it is simply unnecessary
when compiling natively.

Fortunately, I failed to find a package that fails to cross-compile
due to this particular reason. (They failed to cross-compile due to
other reasons, or the 'check' target did nothing.)

I didn't try all packages flagged by the new linter, though.

On top of 75af43162e58a0b3fdc804963809ecb801fb81b7.

Greetings,
Maxime.
From daf537fe6e99b308424cb89106d254efa9ff0781 Mon Sep 17 00:00:00 2001
From: Maxime Devos <maximedevos@telenet.be>
Date: Sun, 9 May 2021 14:08:12 +0200
Subject: [PATCH] lint: tests-true: Check if tests are enabled when
cross-compiling.

* guix/lint.scm
(check-tests-true): New linter.
(%local-checkers)[tests-true]: Add it.
* tests/lint.scm ("tests-true: #:tests? does not need to be set to #t")
("tests-true: absent #:tests? is acceptable")
("tests-true: #:tests? #f is acceptable")
("tests-true: #:tests? #t acceptable when compiling natively"): Test it.
---
guix/lint.scm | 29 +++++++++++++++++++++++++++++
tests/lint.scm | 23 +++++++++++++++++++++++
2 files changed, 52 insertions(+)

Toggle diff (97 lines)
diff --git a/guix/lint.scm b/guix/lint.scm
index 1bebfe03d3..d1cbc9d300 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -12,6 +12,7 @@
;;; Copyright © 2020 Chris Marusich <cmmarusich@gmail.com>
;;; Copyright © 2020 Timothy Sample <samplet@ngyro.com>
;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz>
+;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -95,6 +96,7 @@
check-archival
check-profile-collisions
check-haskell-stackage
+ check-tests-true
lint-warning
lint-warning?
@@ -190,6 +192,29 @@
#:field 'name)))
(else '()))))
+(define (check-tests-true package)
+ "Check whether PACKAGE explicitly requests to run tests, which is
+superfluous when building natively and incorrect when cross-compiling."
+ (define (tests-explicitly-enabled?)
+ (apply (lambda* (#:key tests? #:allow-other-keys)
+ (eq? tests? #t))
+ (package-arguments package)))
+ (if (and (tests-explicitly-enabled?)
+ ;; Some packages, e.g. gnutls, set #:tests?
+ ;; differently depending on whether it is being
+ ;; cross-compiled.
+ (parameterize ((%current-target-system "aarch64-linux-gnu"))
+ (tests-explicitly-enabled?)))
+ ;; Actually, #:tests? *should* not be (unconditionally) set to #t,
+ ;; but that wording would suggest that tests should be disabled,
+ ;; which is not the case.
+ (list (make-warning package
+ ;; TRANSLATORS: #:tests? and #t are Scheme constants
+ ;; and must not be translated.
+ (G_ "#:tests? does not need to be explicitly set to #t")
+ #:field 'arguments))
+ '()))
+
(define (properly-starts-sentence? s)
(string-match "^[(\"'`[:upper:][:digit:]]" s))
@@ -1481,6 +1506,10 @@ them for PACKAGE."
(name 'name)
(description "Validate package names")
(check check-name))
+ (lint-checker
+ (name 'tests-true)
+ (description "Check if tests are explicitly enabled")
+ (check check-tests-true))
(lint-checker
(name 'description)
(description "Validate package descriptions")
diff --git a/tests/lint.scm b/tests/lint.scm
index a2c8665142..46830aad01 100644
--- a/tests/lint.scm
+++ b/tests/lint.scm
@@ -277,6 +277,29 @@
(let ((pkg (dummy-package "under_score")))
(check-name pkg))))
+(test-equal "tests-true: #:tests? does not need to be set to #t"
+ "#:tests? does not need to be explicitly set to #t"
+ (single-lint-warning-message
+ (let ((pkg (dummy-package "x" (arguments '(#:tests? #t)))))
+ (check-tests-true pkg))))
+
+(test-equal "tests-true: absent #:tests? is acceptable"
+ '()
+ (let ((pkg (dummy-package "x")))
+ (check-tests-true pkg)))
+
+(test-equal "tests-true: #:tests? #f is acceptable"
+ '()
+ (let ((pkg (dummy-package "x" (arguments '(#:tests? #f)))))
+ (check-tests-true pkg)))
+
+(test-equal "tests-true: #:tests? #t acceptable when compiling natively"
+ '()
+ (let ((pkg (dummy-package "x"
+ (arguments
+ `(#:tests? ,(not (%current-target-system)))))))
+ (check-tests-true pkg)))
+
(test-equal "inputs: pkg-config is probably a native input"
"'pkg-config' should probably be a native input"
(single-lint-warning-message
--
2.31.1
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYJfXJhccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7vXdAP9VY5O1SqEBkx+Lt6Sv/nYnG3+a
yVLIev+QOTuKmQGfRwD8DkuTnnyDdum6oMn2Ed//AnGfWKNunc1C/Bcm3C7RQwA=
=cIWd
-----END PGP SIGNATURE-----


M
M
Mathieu Othacehe wrote on 4 Jun 2021 14:50
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 48319@debbugs.gnu.org)
878s3peqtu.fsf@gnu.org
Hello Maxime,

Toggle quote (5 lines)
> + ;; TRANSLATORS: #:tests? and #t are Scheme constants
> + ;; and must not be translated.
> + (G_ "#:tests? does not need to be explicitly set to #t")
> + #:field 'arguments))

This patch looks fine, and I remember fixing multiple packages failing
to cross-compile because #:tests was hard-coded to #t.

I'm not sure about the warning message though. What about:

"#:tests? must not be explicitly set to #t"

as "does not need" implies it could be sometimes correct, whereas it is
always a mistake.

WDYT?

Thanks,

Mathieu
M
M
Maxime Devos wrote on 5 Jun 2021 11:56
(name . Mathieu Othacehe)(address . othacehe@gnu.org)(address . 48319@debbugs.gnu.org)
a3f734ce21b294f258eeee86b18d74f00b2602ff.camel@telenet.be
Mathieu Othacehe schreef op vr 04-06-2021 om 14:50 [+0200]:
Toggle quote (17 lines)
> Hello Maxime,
>
> > + ;; TRANSLATORS: #:tests? and #t are Scheme constants
> > + ;; and must not be translated.
> > + (G_ "#:tests? does not need to be explicitly set to #t")
> > + #:field 'arguments))
>
> This patch looks fine, and I remember fixing multiple packages failing
> to cross-compile because #:tests was hard-coded to #t.
>
> I'm not sure about the warning message though. What about:
>
> "#:tests? must not be explicitly set to #t"
>
> as "does not need" implies it could be sometimes correct, whereas it is
> always a mistake.

The new warning message "#:tests? must not be explicitly set to #t"
seems reasonable to me. I'm currently working on other things though
(guix and other things), so don't expect a revised patch soon.

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYLtKSRccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7mQmAQDrjya7FeZ4pZWYgEPrS02/i8/1
B0zOse6Wpg8WOX2XTwD/ZnotLo4DHuKauAD1qmOTq48oJt6kmKuM3Yc7Ifs+CQE=
=tj3W
-----END PGP SIGNATURE-----


M
M
Mathieu Othacehe wrote on 6 Jun 2021 18:51
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 48319-done@debbugs.gnu.org)
87a6o3os1m.fsf_-_@gnu.org
Hey,

Toggle quote (4 lines)
> The new warning message "#:tests? must not be explicitly set to #t"
> seems reasonable to me. I'm currently working on other things though
> (guix and other things), so don't expect a revised patch soon.

I edited the message, removed the associated comment and fixed the
relevant test accordingly. Pushed as
82b0e27de109b38ed44f67434a96460c4a7f9217.

Thanks,

Mathieu
Closed
?