all messages for Guix-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* [bug#33575] [PATCH] guix: lint: Add checker to check if inputs are sorted.
@ 2018-12-02  7:42 Arun Isaac
  2018-12-03 13:31 ` Ludovic Courtès
  2018-12-08  3:51 ` 宋文武
  0 siblings, 2 replies; 14+ messages in thread
From: Arun Isaac @ 2018-12-02  7:42 UTC (permalink / raw)
  To: 33575

* guix/scripts/lint.scm (check-inputs-should-be-sorted): New procedure.
(%checkers): Add it.
---
 guix/scripts/lint.scm | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
index 2314f3b28..37e8a1ec5 100644
--- a/guix/scripts/lint.scm
+++ b/guix/scripts/lint.scm
@@ -8,6 +8,7 @@
 ;;; Copyright © 2017 Alex Kost <alezost@gmail.com>
 ;;; Copyright © 2017 Tobias Geerinckx-Rice <me@tobias.gr>
 ;;; Copyright © 2017 Efraim Flashner <efraim@flashner.co.il>
+;;; Copyright © 2018 Arun Isaac <arunisaac@systemreboot.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -301,6 +302,22 @@ of a package, and INPUT-NAMES, a list of package specifications such as
               (package-input-intersection (package-direct-inputs package)
                                           input-names))))
 
+(define (check-inputs-should-be-sorted package)
+  ;; Emit a warning if inputs, native inputs or propagated inputs of PACKAGE
+  ;; are not lexicographically ordered.
+  (define (check-inputs inputs-accessor input-type)
+    (unless (sorted? (map (match-lambda
+                            ((name input) name))
+                          (inputs-accessor package))
+                     string<?)
+      (emit-warning
+       package
+       (format #f (G_ "~a should be in lexicographic order") input-type))))
+
+  (check-inputs package-inputs (G_ "inputs"))
+  (check-inputs package-native-inputs (G_ "native inputs"))
+  (check-inputs package-propagated-inputs (G_ "propagated inputs")))
+
 (define (package-name-regexp package)
   "Return a regexp that matches PACKAGE's name as a word at the beginning of a
 line."
@@ -1032,6 +1049,10 @@ them for PACKAGE."
      (name        'inputs-should-not-be-input)
      (description "Identify inputs that shouldn't be inputs at all")
      (check       check-inputs-should-not-be-an-input-at-all))
+   (lint-checker
+     (name        'inputs-should-be-sorted)
+     (description "Verify that inputs are in lexicographic order")
+     (check       check-inputs-should-be-sorted))
    (lint-checker
      (name        'patch-file-names)
      (description "Validate file names and availability of patches")
-- 
2.19.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [bug#33575] [PATCH] guix: lint: Add checker to check if inputs are sorted.
  2018-12-02  7:42 [bug#33575] [PATCH] guix: lint: Add checker to check if inputs are sorted Arun Isaac
@ 2018-12-03 13:31 ` Ludovic Courtès
  2018-12-04  9:13   ` Tobias Geerinckx-Rice
  2018-12-06  0:42   ` Oleg Pykhalov
  2018-12-08  3:51 ` 宋文武
  1 sibling, 2 replies; 14+ messages in thread
From: Ludovic Courtès @ 2018-12-03 13:31 UTC (permalink / raw)
  To: Arun Isaac; +Cc: 33575

Hello,

Arun Isaac <arunisaac@systemreboot.net> skribis:

> * guix/scripts/lint.scm (check-inputs-should-be-sorted): New procedure.
> (%checkers): Add it.
> ---
>  guix/scripts/lint.scm | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
> index 2314f3b28..37e8a1ec5 100644
> --- a/guix/scripts/lint.scm
> +++ b/guix/scripts/lint.scm
> @@ -8,6 +8,7 @@
>  ;;; Copyright © 2017 Alex Kost <alezost@gmail.com>
>  ;;; Copyright © 2017 Tobias Geerinckx-Rice <me@tobias.gr>
>  ;;; Copyright © 2017 Efraim Flashner <efraim@flashner.co.il>
> +;;; Copyright © 2018 Arun Isaac <arunisaac@systemreboot.net>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -301,6 +302,22 @@ of a package, and INPUT-NAMES, a list of package specifications such as
>                (package-input-intersection (package-direct-inputs package)
>                                            input-names))))
>  
> +(define (check-inputs-should-be-sorted package)
> +  ;; Emit a warning if inputs, native inputs or propagated inputs of PACKAGE
> +  ;; are not lexicographically ordered.

It’s something we rarely do so we’d get warnings for most packages.  As
a side effect, people may pay less attention to what ‘guix lint’ says.

As for the goal itself, I think sorting is a good idea when there are
lots of inputs (things like IceCat), but otherwise I personally don’t
think it matters that much.

What do people think?

Thanks,
Ludo’.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [bug#33575] [PATCH] guix: lint: Add checker to check if inputs are sorted.
  2018-12-03 13:31 ` Ludovic Courtès
@ 2018-12-04  9:13   ` Tobias Geerinckx-Rice
  2018-12-08 13:29     ` Ludovic Courtès
  2018-12-06  0:42   ` Oleg Pykhalov
  1 sibling, 1 reply; 14+ messages in thread
From: Tobias Geerinckx-Rice @ 2018-12-04  9:13 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 33575

Ludo', Arun,

Ludovic Courtès wrote:
>> +  ;; Emit a warning if inputs, native inputs or propagated 
>> inputs
>> of PACKAGE
>> +  ;; are not lexicographically ordered.
>
> It's something we rarely do so we'd get warnings for most
> packages.  As
> a side effect, people may pay less attention to what ‘guix lint’
> says.

Even I agree :-)  There are valid reasons not to sort them.

> As for the goal itself, I think sorting is a good idea when
> there are
> lots of inputs (things like IceCat), but otherwise I personally
> don't
> think it matters that much.

Do we already check for duplication?

I sometimes order inputs for the same reason I sort module 
imports: to catch duplicates.  These are usually harmless and 
produce no errors.

Kind regards,

T G-R

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [bug#33575] [PATCH] guix: lint: Add checker to check if inputs are sorted.
  2018-12-03 13:31 ` Ludovic Courtès
  2018-12-04  9:13   ` Tobias Geerinckx-Rice
@ 2018-12-06  0:42   ` Oleg Pykhalov
  2018-12-06 12:31     ` swedebugia
  1 sibling, 1 reply; 14+ messages in thread
From: Oleg Pykhalov @ 2018-12-06  0:42 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 33575

[-- Attachment #1: Type: text/plain, Size: 379 bytes --]

Hello,

ludo@gnu.org (Ludovic Courtès) writes:

[…]

> It’s something we rarely do so we’d get warnings for most packages.  As
> a side effect, people may pay less attention to what ‘guix lint’ says.

I think this should not stop us from improving a linter and an option
like --misc-checks=sort-input,foo,bar could be used for such cases.

[…]

Oleg.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [bug#33575] [PATCH] guix: lint: Add checker to check if inputs are sorted.
  2018-12-06  0:42   ` Oleg Pykhalov
@ 2018-12-06 12:31     ` swedebugia
  2018-12-07 13:08       ` Maxim Cournoyer
  0 siblings, 1 reply; 14+ messages in thread
From: swedebugia @ 2018-12-06 12:31 UTC (permalink / raw)
  To: Oleg Pykhalov; +Cc: 33575, Guix-patches

On 2018-12-06 01:42, Oleg Pykhalov wrote:
> Hello,
> 
> ludo@gnu.org (Ludovic Courtès) writes:
> 
> […]
> 
>> It’s something we rarely do so we’d get warnings for most packages.  As
>> a side effect, people may pay less attention to what ‘guix lint’ says.
> 
> I think this should not stop us from improving a linter and an option
> like --misc-checks=sort-input,foo,bar could be used for such cases.

I agree with Oleg here. 

If many packages needs inputs to be sorted lets write a guix lint --sort
modelling the updater (that is make the changes in the work tree to be
committed). 

-- 
Cheers 
Swedebugia

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [bug#33575] [PATCH] guix: lint: Add checker to check if inputs are sorted.
  2018-12-06 12:31     ` swedebugia
@ 2018-12-07 13:08       ` Maxim Cournoyer
  0 siblings, 0 replies; 14+ messages in thread
From: Maxim Cournoyer @ 2018-12-07 13:08 UTC (permalink / raw)
  To: swedebugia; +Cc: 33575, Guix-patches

swedebugia@riseup.net writes:

> On 2018-12-06 01:42, Oleg Pykhalov wrote:
>> Hello,
>> 
>> ludo@gnu.org (Ludovic Courtès) writes:
>> 
>> […]
>> 
>>> It’s something we rarely do so we’d get warnings for most packages.  As
>>> a side effect, people may pay less attention to what ‘guix lint’ says.
>> 
>> I think this should not stop us from improving a linter and an option
>> like --misc-checks=sort-input,foo,bar could be used for such cases.
>
> I agree with Oleg here. 
>
> If many packages needs inputs to be sorted lets write a guix lint --sort
> modelling the updater (that is make the changes in the work tree to be
> committed).

+1

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [bug#33575] [PATCH] guix: lint: Add checker to check if inputs are sorted.
  2018-12-02  7:42 [bug#33575] [PATCH] guix: lint: Add checker to check if inputs are sorted Arun Isaac
  2018-12-03 13:31 ` Ludovic Courtès
@ 2018-12-08  3:51 ` 宋文武
  2018-12-08  7:58   ` swedebugia
  2018-12-08 13:34   ` Arun Isaac
  1 sibling, 2 replies; 14+ messages in thread
From: 宋文武 @ 2018-12-08  3:51 UTC (permalink / raw)
  To: Arun Isaac; +Cc: 33575

Arun Isaac <arunisaac@systemreboot.net> writes:

> * guix/scripts/lint.scm (check-inputs-should-be-sorted): New procedure.
> (%checkers): Add it.
> [...]
>  
> +(define (check-inputs-should-be-sorted package)
> +  ;; Emit a warning if inputs, native inputs or propagated inputs of PACKAGE
> +  ;; are not lexicographically ordered.

Hello, consider 'gspell', it has some native-inputs for build and some
for test:

    (native-inputs
     `(("glib" ,glib "bin")
       ("pkg-config" ,pkg-config)
       ("xmllint" ,libxml2)

       ;; For tests.
       ("aspell-dict-en" ,aspell-dict-en)
       ("xorg-server" ,xorg-server)))

Currently I'd seperated them by a comment like this.

If they are sorted, I have to add comment for each test input:

  `(("aspell-dict-en", aspecll-dict-en) ; for test
    ("glib" ,glib "bin")
    ("pkg-config" ,pkg-config)
    ("xmllint" ,libxml2)
    ("xorg-server" ,xorg-server))       ; for test

Which will be a little annoying...

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [bug#33575] [PATCH] guix: lint: Add checker to check if inputs are sorted.
  2018-12-08  3:51 ` 宋文武
@ 2018-12-08  7:58   ` swedebugia
  2018-12-08 13:34   ` Arun Isaac
  1 sibling, 0 replies; 14+ messages in thread
From: swedebugia @ 2018-12-08  7:58 UTC (permalink / raw)
  To: iyzsong; +Cc: 33575, Guix-patches

On 2018-12-08 04:51, iyzsong@member.fsf.org wrote:
> Arun Isaac <arunisaac@systemreboot.net> writes:
> 
>> * guix/scripts/lint.scm (check-inputs-should-be-sorted): New procedure.
>> (%checkers): Add it.
>> [...]
>>
>> +(define (check-inputs-should-be-sorted package)
>> +  ;; Emit a warning if inputs, native inputs or propagated inputs of PACKAGE
>> +  ;; are not lexicographically ordered.
> 
> Hello, consider 'gspell', it has some native-inputs for build and some
> for test:
> 
>     (native-inputs
>      `(("glib" ,glib "bin")
>        ("pkg-config" ,pkg-config)
>        ("xmllint" ,libxml2)
> 
>        ;; For tests.
>        ("aspell-dict-en" ,aspell-dict-en)
>        ("xorg-server" ,xorg-server)))
> 
> Currently I'd seperated them by a comment like this.
> 
> If they are sorted, I have to add comment for each test input:
> 
>   `(("aspell-dict-en", aspecll-dict-en) ; for test
>     ("glib" ,glib "bin")
>     ("pkg-config" ,pkg-config)
>     ("xmllint" ,libxml2)
>     ("xorg-server" ,xorg-server))       ; for test
> 
> Which will be a little annoying...

You convinced me sorting is a bad idea. Thanks for providing a good
argument :)

-- 
Cheers 
Swedebugia

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [bug#33575] [PATCH] guix: lint: Add checker to check if inputs are sorted.
  2018-12-04  9:13   ` Tobias Geerinckx-Rice
@ 2018-12-08 13:29     ` Ludovic Courtès
  0 siblings, 0 replies; 14+ messages in thread
From: Ludovic Courtès @ 2018-12-08 13:29 UTC (permalink / raw)
  To: Tobias Geerinckx-Rice; +Cc: 33575

Hello,

Tobias Geerinckx-Rice <somebody@not-sent-or-endorsed-by.tobias.gr>
skribis:

> Ludovic Courtès wrote:

[...]

>> As for the goal itself, I think sorting is a good idea when
>> there are
>> lots of inputs (things like IceCat), but otherwise I personally
>> don't
>> think it matters that much.
>
> Do we already check for duplication?

No but it would be a worthy check!

Ludo’.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [bug#33575] [PATCH] guix: lint: Add checker to check if inputs are sorted.
  2018-12-08  3:51 ` 宋文武
  2018-12-08  7:58   ` swedebugia
@ 2018-12-08 13:34   ` Arun Isaac
  2018-12-09 22:49     ` Maxim Cournoyer
  2018-12-10 11:19     ` 宋文武
  1 sibling, 2 replies; 14+ messages in thread
From: Arun Isaac @ 2018-12-08 13:34 UTC (permalink / raw)
  To: 宋文武; +Cc: 33575


> If they are sorted, I have to add comment for each test input:
>
>   `(("aspell-dict-en", aspecll-dict-en) ; for test
>     ("glib" ,glib "bin")
>     ("pkg-config" ,pkg-config)
>     ("xmllint" ,libxml2)
>     ("xorg-server" ,xorg-server))       ; for test
>
> Which will be a little annoying...

I too find this convincing. It's not a good idea to enforce sorted
inputs all the time. If there is sufficient consensus, we can close this
bug report.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [bug#33575] [PATCH] guix: lint: Add checker to check if inputs are sorted.
  2018-12-08 13:34   ` Arun Isaac
@ 2018-12-09 22:49     ` Maxim Cournoyer
  2018-12-10 11:45       ` 宋文武
  2018-12-10 11:19     ` 宋文武
  1 sibling, 1 reply; 14+ messages in thread
From: Maxim Cournoyer @ 2018-12-09 22:49 UTC (permalink / raw)
  To: Arun Isaac; +Cc: 33575, 宋文武

Hi,

Arun Isaac <arunisaac@systemreboot.net> writes:

>> If they are sorted, I have to add comment for each test input:
>>
>>   `(("aspell-dict-en", aspecll-dict-en) ; for test
>>     ("glib" ,glib "bin")
>>     ("pkg-config" ,pkg-config)
>>     ("xmllint" ,libxml2)
>>     ("xorg-server" ,xorg-server))       ; for test
>>
>> Which will be a little annoying...
>
> I too find this convincing. It's not a good idea to enforce sorted
> inputs all the time. If there is sufficient consensus, we can close this
> bug report.

Maybe our test inputs should have their own field? This would make their
raison d'être explicit and remove the need of using comments.

Maxim

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [bug#33575] [PATCH] guix: lint: Add checker to check if inputs are sorted.
  2018-12-08 13:34   ` Arun Isaac
  2018-12-09 22:49     ` Maxim Cournoyer
@ 2018-12-10 11:19     ` 宋文武
  2018-12-18 20:36       ` bug#33575: " Arun Isaac
  1 sibling, 1 reply; 14+ messages in thread
From: 宋文武 @ 2018-12-10 11:19 UTC (permalink / raw)
  To: Arun Isaac; +Cc: 33575

Arun Isaac <arunisaac@systemreboot.net> writes:

>> If they are sorted, I have to add comment for each test input:
>>
>>   `(("aspell-dict-en", aspecll-dict-en) ; for test
>>     ("glib" ,glib "bin")
>>     ("pkg-config" ,pkg-config)
>>     ("xmllint" ,libxml2)
>>     ("xorg-server" ,xorg-server))       ; for test
>>
>> Which will be a little annoying...
>
> I too find this convincing. It's not a good idea to enforce sorted
> inputs all the time. If there is sufficient consensus, we can close this
> bug report.

Yes, I think so.  Thank you!

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [bug#33575] [PATCH] guix: lint: Add checker to check if inputs are sorted.
  2018-12-09 22:49     ` Maxim Cournoyer
@ 2018-12-10 11:45       ` 宋文武
  0 siblings, 0 replies; 14+ messages in thread
From: 宋文武 @ 2018-12-10 11:45 UTC (permalink / raw)
  To: Maxim Cournoyer; +Cc: 33575

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> Hi,
>
> Arun Isaac <arunisaac@systemreboot.net> writes:
>
>>> If they are sorted, I have to add comment for each test input:
>>>
>>>   `(("aspell-dict-en", aspecll-dict-en) ; for test
>>>     ("glib" ,glib "bin")
>>>     ("pkg-config" ,pkg-config)
>>>     ("xmllint" ,libxml2)
>>>     ("xorg-server" ,xorg-server))       ; for test
>>>
>>> Which will be a little annoying...
>>
>> I too find this convincing. It's not a good idea to enforce sorted
>> inputs all the time. If there is sufficient consensus, we can close this
>> bug report.
>
> Maybe our test inputs should have their own field? This would make their
> raison d'être explicit and remove the need of using comments.

Yeah, something like:

  (package
    ...
    (inputs ...)
  (test:inputs ...)
  (test:native-inputs ...))

If we plan to support build packages with tests disabled, this would be
the way to go.  And due to how build works in guix, if tests are
disabled, it would be considered as a different derivation/package, so
the main use case may be:

  - I disable substitute servers to build all packages from sources
    locally.
  - I want to disable tests for some packages as they are too slow...

I don't have this use case now, and seperate package inputs will be a
big change, so I think the current way is totally ok.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#33575: [PATCH] guix: lint: Add checker to check if inputs are sorted.
  2018-12-10 11:19     ` 宋文武
@ 2018-12-18 20:36       ` Arun Isaac
  0 siblings, 0 replies; 14+ messages in thread
From: Arun Isaac @ 2018-12-18 20:36 UTC (permalink / raw)
  To: 33575-done


I'm closing this bug report since I believe we have reached a consensus
that this patch is unnecessary. Thank you all for your thoughts!

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2018-12-18 20:38 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-12-02  7:42 [bug#33575] [PATCH] guix: lint: Add checker to check if inputs are sorted Arun Isaac
2018-12-03 13:31 ` Ludovic Courtès
2018-12-04  9:13   ` Tobias Geerinckx-Rice
2018-12-08 13:29     ` Ludovic Courtès
2018-12-06  0:42   ` Oleg Pykhalov
2018-12-06 12:31     ` swedebugia
2018-12-07 13:08       ` Maxim Cournoyer
2018-12-08  3:51 ` 宋文武
2018-12-08  7:58   ` swedebugia
2018-12-08 13:34   ` Arun Isaac
2018-12-09 22:49     ` Maxim Cournoyer
2018-12-10 11:45       ` 宋文武
2018-12-10 11:19     ` 宋文武
2018-12-18 20:36       ` bug#33575: " Arun Isaac

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/guix.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.