[PATCH 0/4] scripts: style: Sort more kinds of package

  • Open
  • quality assurance status badge
Details
2 participants
  • Herman Rimm
  • Ludovic Courtès
Owner
unassigned
Submitted by
Herman Rimm
Severity
normal
H
H
Herman Rimm wrote on 19 Dec 2024 20:31
(address . guix-patches@gnu.org)
cover.1734636205.git.herman@rimm.ee
Hello,

The warnings added in [PATCH 4/4] are emitted multiple times. How
should I prevent that? Or should I put them behind a --verbose option?

Cheers,
Herman

Herman Rimm (4):
scripts: style: Refactor order-packages.
scripts: style: Sort more kinds of package definitions.
scripts: style: Only sort packages with string literal name.
scripts: style: Warn about unmatched package definitions.

guix/scripts/style.scm | 57 ++++++++++++++++++++++--------------------
1 file changed, 30 insertions(+), 27 deletions(-)


base-commit: 07b4b1d055c36c6c61d39273c26974771dbfe805
--
2.45.2
H
H
Herman Rimm wrote on 19 Dec 2024 20:33
[PATCH 1/4] scripts: style: Refactor order-packages.
(address . 74979@debbugs.gnu.org)
bccdc8fe4884bf1f114a1f07eac3a3a3b25081e0.1734636205.git.herman@rimm.ee
* guix/scripts/style.scm (order-packages): Combine package-name and
package-version procedures into package-fields.
(format-whole-file): Do not sort copyright headers or module definition.

Change-Id: I5507bf8ed221f7017f972f0e0e64d149bea4854b
---
guix/scripts/style.scm | 36 +++++++++++++++---------------------
1 file changed, 15 insertions(+), 21 deletions(-)

Toggle diff (68 lines)
diff --git a/guix/scripts/style.scm b/guix/scripts/style.scm
index 51234952e9..4b704ddfb7 100644
--- a/guix/scripts/style.scm
+++ b/guix/scripts/style.scm
@@ -43,6 +43,7 @@ (define-module (guix scripts style)
#:use-module (ice-9 match)
#:use-module (srfi srfi-1)
#:use-module (srfi srfi-9)
+ #:use-module (srfi srfi-11)
#:use-module (srfi srfi-26)
#:use-module (srfi srfi-34)
#:use-module (srfi srfi-37)
@@ -500,31 +501,19 @@ (define (order-packages lst)
"Return LST, a list of top-level expressions and blanks, with
top-level package definitions in alphabetical order. Packages which
share a name are placed with versions in descending order."
- (define (package-name pkg)
+ (define (package-fields pkg)
(match pkg
((('define-public _ expr) _ ...)
(match expr
- ((or ('package _ ('name name) _ ...)
- ('package ('name name) _ ...))
- name)
- (_ #f)))
- (_ #f)))
-
- (define (package-version pkg)
- (match pkg
- ((('define-public _ expr) _ ...)
- (match expr
- ((or ('package _ _ ('version version) _ ...)
- ('package _ ('version version) _ ...))
- version)
- (_ #f)))
- (_ #f)))
+ ((or ('package _ ('name name) ('version version) _ ...)
+ ('package ('name name) ('version version) _ ...))
+ (values name version))
+ (_ (values #f #f))))
+ (_ (values #f #f))))
(define (package>? lst1 lst2)
- (let ((name1 (package-name lst1))
- (name2 (package-name lst2))
- (version1 (package-version lst1))
- (version2 (package-version lst2)))
+ (let-values (((name1 version1) (package-fields lst1))
+ ((name2 version2) (package-fields lst2)))
(and name1 name2 (or (string>? name1 name2)
(and (string=? name1 name2)
version1
@@ -550,7 +539,12 @@ (define* (format-whole-file file order? #:rest rest)
(let* ((lst (call-with-input-file file read-with-comments/sequence
#:guess-encoding #t))
(lst (if order?
- (order-packages lst)
+ (let loop ((lst lst))
+ (match lst
+ (((? blank? blank) rest ...)
+ (cons blank (loop rest)))
+ ((module rest ...)
+ (cons module (order-packages rest)))))
lst)))
(with-atomic-file-output file
(lambda (port)
--
2.45.2
H
H
Herman Rimm wrote on 19 Dec 2024 20:33
[PATCH 2/4] scripts: style: Sort more kinds of package definitions.
(address . 74979@debbugs.gnu.org)
addf23b3b65e09a039a11a2d9dddeb2c6109a9a1.1734636205.git.herman@rimm.ee
* guix/scripts/style.scm (order-packages): Match comments before package
S-exp. and its fields. Match in let. Match package/inherit.

Change-Id: I48a5976930501c20415b5413966b5294958bc23b
---
guix/scripts/style.scm | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)

Toggle diff (27 lines)
diff --git a/guix/scripts/style.scm b/guix/scripts/style.scm
index 4b704ddfb7..6f07f6c3b9 100644
--- a/guix/scripts/style.scm
+++ b/guix/scripts/style.scm
@@ -503,13 +503,14 @@ (define (order-packages lst)
share a name are placed with versions in descending order."
(define (package-fields pkg)
(match pkg
- ((('define-public _ expr) _ ...)
+ ((('define-public pkg _ ... (or ('let _ expr) expr)) _ ...)
(match expr
- ((or ('package _ ('name name) ('version version) _ ...)
- ('package ('name name) ('version version) _ ...))
- (values name version))
- (_ (values #f #f))))
- (_ (values #f #f))))
+ (((or 'package 'package/inherit) fields ...)
+ (let ((name (and=> (assoc-ref fields 'name) first))
+ (version (and=> (assoc-ref fields 'version) first)))
+ (values name version)))
+ (_ (and (values #f #f)))))
+ (_ (and (values #f #f)))))
(define (package>? lst1 lst2)
(let-values (((name1 version1) (package-fields lst1))
--
2.45.2
H
H
Herman Rimm wrote on 19 Dec 2024 20:33
[PATCH 3/4] scripts: style: Only sort packages with string literal name.
(address . 74979@debbugs.gnu.org)
48fb3a7d236d39e90107931893cbddf2f16a761f.1734636205.git.herman@rimm.ee
* guix/scripts/style.scm (order-packages): Only match string literals.

Change-Id: I48a5976930501c20415b5413966b5294958bc23b
---
guix/scripts/style.scm | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)

Toggle diff (25 lines)
diff --git a/guix/scripts/style.scm b/guix/scripts/style.scm
index 6f07f6c3b9..4801529f7e 100644
--- a/guix/scripts/style.scm
+++ b/guix/scripts/style.scm
@@ -515,11 +515,13 @@ (define (order-packages lst)
(define (package>? lst1 lst2)
(let-values (((name1 version1) (package-fields lst1))
((name2 version2) (package-fields lst2)))
- (and name1 name2 (or (string>? name1 name2)
- (and (string=? name1 name2)
- version1
- version2
- (version>? version2 version1))))))
+ (and (string? name1)
+ (string? name2)
+ (or (string>? name1 name2)
+ (and (string=? name1 name2)
+ (string? version1)
+ (string? version2)
+ (version>? version2 version1))))))
;; Group define-public with preceding blanks and defines.
(let ((lst (fold2 (lambda (expr tail head)
--
2.45.2
H
H
Herman Rimm wrote on 19 Dec 2024 20:33
[PATCH 4/4] scripts: style: Warn about unmatched package definitions.
(address . 74979@debbugs.gnu.org)
6986ddba422272423a928f2d0b40cd8aeb8752ea.1734636205.git.herman@rimm.ee
* guix/scripts/style.scm (order-packages): Warn.

Change-Id: Iddbf979ee9ee5ed1ebada63776a390db024154fa
---
guix/scripts/style.scm | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

Toggle diff (24 lines)
diff --git a/guix/scripts/style.scm b/guix/scripts/style.scm
index 4801529f7e..81fe1141e2 100644
--- a/guix/scripts/style.scm
+++ b/guix/scripts/style.scm
@@ -508,9 +508,15 @@ (define (order-packages lst)
(((or 'package 'package/inherit) fields ...)
(let ((name (and=> (assoc-ref fields 'name) first))
(version (and=> (assoc-ref fields 'version) first)))
+ (if (and name version)
+ (unless (and (string? name) (string? version))
+ (warning (G_ "non-string name/version for ~a~%") pkg))
+ (warning (G_ "package fields not found for ~a~%") pkg))
(values name version)))
- (_ (and (values #f #f)))))
- (_ (and (values #f #f)))))
+ (_ (and (warning (G_ "package record missing for ~a~%") pkg)
+ (values #f #f)))))
+ (_ (and (info (G_ "not sorting top-level S-exp.: ~a~%") pkg)
+ (values #f #f)))))
(define (package>? lst1 lst2)
(let-values (((name1 version1) (package-fields lst1))
--
2.45.2
L
L
Ludovic Courtès wrote on 23 Dec 2024 18:28
Re: [bug#74979] [PATCH 0/4] scripts: style: Sort more kinds of package
(name . Herman Rimm)(address . herman@rimm.ee)
87ttau8g1k.fsf@gnu.org
Hi Herman,

Herman Rimm <herman@rimm.ee> skribis:

Toggle quote (3 lines)
> The warnings added in [PATCH 4/4] are emitted multiple times. How
> should I prevent that? Or should I put them behind a --verbose option?

Not sure, do you have an example on how to trigger it?

Toggle quote (17 lines)
> +++ b/guix/scripts/style.scm
> @@ -508,9 +508,15 @@ (define (order-packages lst)
> (((or 'package 'package/inherit) fields ...)
> (let ((name (and=> (assoc-ref fields 'name) first))
> (version (and=> (assoc-ref fields 'version) first)))
> + (if (and name version)
> + (unless (and (string? name) (string? version))
> + (warning (G_ "non-string name/version for ~a~%") pkg))
> + (warning (G_ "package fields not found for ~a~%") pkg))
> (values name version)))
> - (_ (and (values #f #f)))))
> - (_ (and (values #f #f)))))
> + (_ (and (warning (G_ "package record missing for ~a~%") pkg)
> + (values #f #f)))))
> + (_ (and (info (G_ "not sorting top-level S-exp.: ~a~%") pkg)
> + (values #f #f)))))

You shouldn’t rely on the return value of ‘info’, ‘warning’, etc.:
they’re not specified (that’s generally the case for procedures called
for side effects only).

So I’d recommend:

(begin
(info …)
(values #f #f))

(You don’t even need ‘begin’ in this context.)

The other patches LGTM, though perhaps ‘tests/guix-style.sh’ could be
augmented a bit to cover the new cases?

Thanks,
Ludo’.
H
H
Herman Rimm wrote on 24 Dec 2024 11:42
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 74979@debbugs.gnu.org)
mnyemf53orphuunwlle7vfpljqqrlv2kujextlh4pifllv3b6w@uhlhq6ycuohc
Hello,

On Mon, Dec 23, 2024 at 06:28:39PM +0100, Ludovic Court�s wrote:
Toggle quote (9 lines)
> Hi Herman,
>
> Herman Rimm <herman@rimm.ee> skribis:
>
> > The warnings added in [PATCH 4/4] are emitted multiple times. How
> > should I prevent that? Or should I put them behind a --verbose option?
>
> Not sure, do you have an example on how to trigger it?

Yeah, for example:

$ guix style -fA gnu/packages/crates-io.scm
guix style: warning: package record missing for rust-version-compare-0.0
guix style: warning: package record missing for rust-version-compare-0.0
guix style: warning: package record missing for rust-version-compare-0.0
guix style: warning: package record missing for rust-version-compare-0.0
guix style: warning: package record missing for rust-version-compare-0.0
guix style: warning: package fields not found for rust-indoc-0.3
guix style: warning: package fields not found for rust-indoc-0.3
guix style: warning: package fields not found for rust-indoc-0.3
guix style: warning: package fields not found for rust-indoc-0.3
guix style: warning: package fields not found for rust-indoc-0.3
guix style: warning: package fields not found for rust-indoc-0.3
guix style: warning: package fields not found for rust-indoc-0.3
guix style: warning: package fields not found for rust-indoc-0.3
guix style: warning: package fields not found for rust-indoc-0.3
guix style: warning: package record missing for rust-version-compare-0.0
guix style: warning: package fields not found for rust-indoc-0.3
guix style: warning: package fields not found for rust-indoc-0.3
guix style: warning: package fields not found for rust-indoc-0.3
guix style: warning: package fields not found for rust-indoc-0.3
guix style: warning: package fields not found for rust-indoc-0.3

Cheers,
Herman
H
H
Herman Rimm wrote on 24 Dec 2024 12:28
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 74979@debbugs.gnu.org)
yj4mzm422sh2kmjacpw2pcgjbykrzzipmvll5c6ygyqvuhnim6@spykgwusvgyf
Hi again,

On Tue, Dec 24, 2024 at 11:42:58AM +0100, Herman Rimm wrote:
Toggle quote (6 lines)
> > Not sure, do you have an example on how to trigger it?
>
> Yeah, for example:
>
> $ guix style -fA gnu/packages/crates-io.scm

Just to be clear, this will only work if you are using a Guix channel
with this patch series already applied. Instead, you probably want to
run (in a Git checkout with the patch series applied):

./pre-inst-env guix style -fA gnu/packages/crates-io.scm

Cheers,
Herman
H
H
Herman Rimm wrote on 21 Jan 22:43 +0100
[PATCH v2 1/4] scripts: style: Refactor order-packages.
(address . 74979@debbugs.gnu.org)
a7bb27cee522c0f89aba50b5b1ff412a8fcf1249.1737495587.git.herman@rimm.ee
* guix/scripts/style.scm (order-packages): Combine package-name and
package-version procedures into package-fields.
(format-whole-file): Do not sort copyright headers or module definition.

Change-Id: I5507bf8ed221f7017f972f0e0e64d149bea4854b
---
guix/scripts/style.scm | 36 +++++++++++++++---------------------
1 file changed, 15 insertions(+), 21 deletions(-)

Toggle diff (70 lines)
diff --git a/guix/scripts/style.scm b/guix/scripts/style.scm
index 51234952e91..4b704ddfb7e 100644
--- a/guix/scripts/style.scm
+++ b/guix/scripts/style.scm
@@ -43,6 +43,7 @@ (define-module (guix scripts style)
#:use-module (ice-9 match)
#:use-module (srfi srfi-1)
#:use-module (srfi srfi-9)
+ #:use-module (srfi srfi-11)
#:use-module (srfi srfi-26)
#:use-module (srfi srfi-34)
#:use-module (srfi srfi-37)
@@ -500,31 +501,19 @@ (define (order-packages lst)
"Return LST, a list of top-level expressions and blanks, with
top-level package definitions in alphabetical order. Packages which
share a name are placed with versions in descending order."
- (define (package-name pkg)
+ (define (package-fields pkg)
(match pkg
((('define-public _ expr) _ ...)
(match expr
- ((or ('package _ ('name name) _ ...)
- ('package ('name name) _ ...))
- name)
- (_ #f)))
- (_ #f)))
-
- (define (package-version pkg)
- (match pkg
- ((('define-public _ expr) _ ...)
- (match expr
- ((or ('package _ _ ('version version) _ ...)
- ('package _ ('version version) _ ...))
- version)
- (_ #f)))
- (_ #f)))
+ ((or ('package _ ('name name) ('version version) _ ...)
+ ('package ('name name) ('version version) _ ...))
+ (values name version))
+ (_ (values #f #f))))
+ (_ (values #f #f))))
(define (package>? lst1 lst2)
- (let ((name1 (package-name lst1))
- (name2 (package-name lst2))
- (version1 (package-version lst1))
- (version2 (package-version lst2)))
+ (let-values (((name1 version1) (package-fields lst1))
+ ((name2 version2) (package-fields lst2)))
(and name1 name2 (or (string>? name1 name2)
(and (string=? name1 name2)
version1
@@ -550,7 +539,12 @@ (define* (format-whole-file file order? #:rest rest)
(let* ((lst (call-with-input-file file read-with-comments/sequence
#:guess-encoding #t))
(lst (if order?
- (order-packages lst)
+ (let loop ((lst lst))
+ (match lst
+ (((? blank? blank) rest ...)
+ (cons blank (loop rest)))
+ ((module rest ...)
+ (cons module (order-packages rest)))))
lst)))
(with-atomic-file-output file
(lambda (port)

base-commit: 6dd219387940ba02db02cc81b35cd7437c108287
--
2.47.1
H
H
Herman Rimm wrote on 21 Jan 22:43 +0100
[PATCH v2 2/4] scripts: style: Sort more kinds of package definitions.
(address . 74979@debbugs.gnu.org)
71ca4b6a08f7610dfcb7df41b73b16a0171fb252.1737495587.git.herman@rimm.ee
* guix/scripts/style.scm (order-packages): Match comments before package
S-exp. and its fields. Match in let. Match package/inherit.
* tests/guix-style.sh: Add pkg-baz variable and package/inherit to test.

Change-Id: I48a5976930501c20415b5413966b5294958bc23b
---
guix/scripts/style.scm | 13 +++++++------
tests/guix-style.sh | 10 ++++++++--
2 files changed, 15 insertions(+), 8 deletions(-)

Toggle diff (50 lines)
diff --git a/guix/scripts/style.scm b/guix/scripts/style.scm
index 4b704ddfb7e..6f07f6c3b9e 100644
--- a/guix/scripts/style.scm
+++ b/guix/scripts/style.scm
@@ -503,13 +503,14 @@ (define (order-packages lst)
share a name are placed with versions in descending order."
(define (package-fields pkg)
(match pkg
- ((('define-public _ expr) _ ...)
+ ((('define-public pkg _ ... (or ('let _ expr) expr)) _ ...)
(match expr
- ((or ('package _ ('name name) ('version version) _ ...)
- ('package ('name name) ('version version) _ ...))
- (values name version))
- (_ (values #f #f))))
- (_ (values #f #f))))
+ (((or 'package 'package/inherit) fields ...)
+ (let ((name (and=> (assoc-ref fields 'name) first))
+ (version (and=> (assoc-ref fields 'version) first)))
+ (values name version)))
+ (_ (and (values #f #f)))))
+ (_ (and (values #f #f)))))
(define (package>? lst1 lst2)
(let-values (((name1 version1) (package-fields lst1))
diff --git a/tests/guix-style.sh b/tests/guix-style.sh
index 93331394353..703e148b699 100644
--- a/tests/guix-style.sh
+++ b/tests/guix-style.sh
@@ -65,10 +65,16 @@ cat > "$tmpfile" <<EOF
(name "bar")
(version "2")))
+(define-public pkg-baz
+ (let ()
+ (package
+ (name "baz")
+ (version "2"))))
+
;; The comment below belongs to the foo package.
(define-public pkg
- (package
- (name "bar")
+ (package/inherit pkg-baz
+ (name "baz")
(version "1")))
;; Incomplete package definitions in alphabetical order.
--
2.47.1
H
H
Herman Rimm wrote on 21 Jan 22:43 +0100
[PATCH v2 3/4] scripts: style: Only sort packages with string literal name.
(address . 74979@debbugs.gnu.org)
f18def074591f5b45d8deb210ae17f599d8721e5.1737495587.git.herman@rimm.ee
* guix/scripts/style.scm (order-packages): Only match string literals.

Change-Id: I48a5976930501c20415b5413966b5294958bc23b
---
guix/scripts/style.scm | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)

Toggle diff (25 lines)
diff --git a/guix/scripts/style.scm b/guix/scripts/style.scm
index 6f07f6c3b9e..4801529f7e9 100644
--- a/guix/scripts/style.scm
+++ b/guix/scripts/style.scm
@@ -515,11 +515,13 @@ (define (order-packages lst)
(define (package>? lst1 lst2)
(let-values (((name1 version1) (package-fields lst1))
((name2 version2) (package-fields lst2)))
- (and name1 name2 (or (string>? name1 name2)
- (and (string=? name1 name2)
- version1
- version2
- (version>? version2 version1))))))
+ (and (string? name1)
+ (string? name2)
+ (or (string>? name1 name2)
+ (and (string=? name1 name2)
+ (string? version1)
+ (string? version2)
+ (version>? version2 version1))))))
;; Group define-public with preceding blanks and defines.
(let ((lst (fold2 (lambda (expr tail head)
--
2.47.1
H
H
Herman Rimm wrote on 21 Jan 22:43 +0100
[PATCH v2 4/4] scripts: style: Warn about unmatched package definitions.
(address . 74979@debbugs.gnu.org)
7c2818a626dcece74e293363fd58023543f0df91.1737495587.git.herman@rimm.ee
* guix/scripts/style.scm (order-packages): Warn.

Change-Id: Iddbf979ee9ee5ed1ebada63776a390db024154fa
---
guix/scripts/style.scm | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

Toggle diff (24 lines)
diff --git a/guix/scripts/style.scm b/guix/scripts/style.scm
index 4801529f7e9..2555b3c6108 100644
--- a/guix/scripts/style.scm
+++ b/guix/scripts/style.scm
@@ -508,9 +508,15 @@ (define (order-packages lst)
(((or 'package 'package/inherit) fields ...)
(let ((name (and=> (assoc-ref fields 'name) first))
(version (and=> (assoc-ref fields 'version) first)))
+ (if (and name version)
+ (unless (and (string? name) (string? version))
+ (warning (G_ "non-string name/version for ~a~%") pkg))
+ (warning (G_ "package fields not found for ~a~%") pkg))
(values name version)))
- (_ (and (values #f #f)))))
- (_ (and (values #f #f)))))
+ (_ (warning (G_ "package record missing for ~a~%") pkg)
+ (values #f #f))))
+ (_ (info (G_ "not sorting top-level S-exp.: ~a~%") pkg)
+ (values #f #f))))
(define (package>? lst1 lst2)
(let-values (((name1 version1) (package-fields lst1))
--
2.47.1
?
Your comment

Commenting via the web interface is currently disabled.

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

To respond to this issue using the mumi CLI, first switch to it
mumi current 74979
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