mbox series

[bug#58231,0/2] Checking the 'license' field of packages

Message ID 20221001161911.8116-1-ludo@gnu.org
Headers
Series Checking the 'license' field of packages |

Message

Ludovic Courtès Oct. 1, 2022, 4:19 p.m. UTC 
Hello Guix!

We already have ‘guix lint -c license’ but that hasn’t prevented
us from occasionally having invalid licenses committed.  This patch
adds add a ‘sanitize’ option to the ‘license’ field of <package> to
detect invalid licenses early on, as zimoun suggested at:

  https://lists.gnu.org/archive/html/guix-devel/2022-09/msg00067.html

The funny part of this patch is that the license validation expands
to #t in the majority of cases, meaning that it comes without
run-time overhead (there is macro-expansion overhead, but I didn’t
measure it).  Kinda like static type checking, except that we can
only tell when a value is definitely a valid license and cannot
conclude in other cases.

Feedback welcome!

Thanks,
Ludo’.

Ludovic Courtès (2):
  licenses: Let 'license?' expand to #t in trivial cases.
  packages: Raise an exception for invalid 'license' values.

 guix/licenses.scm  | 58 +++++++++++++++++++++++++++++++++++++++-------
 guix/packages.scm  | 40 +++++++++++++++++++++++++++++++-
 tests/packages.scm |  7 ++++++
 3 files changed, 95 insertions(+), 10 deletions(-)


base-commit: d9b7982ba58fdea0934b60a81f507440a56c82ee

Comments

pelzflorian (Florian Pelz) Oct. 8, 2022, 11:44 a.m. UTC | #1 
Thank you!  This is good.  Just to let you know though; when reverting
e61c581805b2338ea8feaac86617c5d7bff41c41 and guix pull’ing, there is
no error location.  Doesn’t matter to me though.

Computing Guix derivation for 'x86_64-linux'... -Backtrace:
In ice-9/boot-9.scm:
   222:29 19 (map1 _)
   222:29 18 (map1 _)
   222:29 17 (map1 _)
   222:29 16 (map1 _)
   222:29 15 (map1 _)
   222:29 14 (map1 _)
   222:29 13 (map1 _)
   222:17 12 (map1 (((gnu packages qt)) ((gnu packages sqlite)) ((gnu packages web)) ((gnu packages xml)) ((gnu ?)) ?))
  3326:17 11 (resolve-interface (gnu packages qt) #:select _ #:hide _ #:prefix _ #:renamer _ #:version _)
In ice-9/threads.scm:
    390:8 10 (_ _)
In ice-9/boot-9.scm:
  3252:13  9 (_)
In ice-9/threads.scm:
    390:8  8 (_ _)
In ice-9/boot-9.scm:
  3536:20  7 (_)
   2835:4  6 (save-module-excursion _)
  3556:26  5 (_)
In unknown file:
           4 (primitive-load-path "gnu/packages/qt" #<procedure 7fded2ddefc0 at ice-9/boot-9.scm:3543:37 ()>)
In ice-9/eval.scm:
    619:8  3 (_ #f)
   626:19  2 (_ #<directory (gnu packages qt) 7fded3df5780>)
   293:34  1 (_ #(#(#(#(#(#(#(#(#(#(#<directory (gnu packages qt) 7fded3df5780> "qtshad?") #) #) #) #) #) #) #) #) #))
    619:8  0 (_ #(#(#(#(#(#(#(#(#(#(#<directory (gnu packages qt) 7fded3df5780> "qtshad?") #) #) #) #) #) #) #) #) #))

ice-9/eval.scm:619:8: ERROR:
  1. &error-location: #f
  2. &package-license-error:
      package: #f
      license: "https://www.qt.io/"
  3. &formatted-message:
      format: "~s: invalid package license~%\n"
      arguments: ("https://www.qt.io/")
guix pull: Fehler: You found a bug: the program '/gnu/store/vbk7jc9w4808cn7697bc3h24g2kl78wx-compute-guix-derivation'
failed to compute the derivation for Guix (version: "4b2ae06e49fc7bd41856b02604b5f277a6df06ce"; system: "x86_64-linux";
host version: "0dec41f329c37a4293a2a8326f1fe7d9318ec455"; pull-version: 1).
Please report the COMPLETE output above by email to <bug-guix@gnu.org>.
Ludovic Courtès Oct. 10, 2022, 7:36 a.m. UTC | #2 
Hi,

"pelzflorian (Florian Pelz)" <pelzflorian@pelzflorian.de> skribis:

> Thank you!  This is good.  Just to let you know though; when reverting
> e61c581805b2338ea8feaac86617c5d7bff41c41 and guix pull’ing, there is
> no error location.  Doesn’t matter to me though.
>
> Computing Guix derivation for 'x86_64-linux'... -Backtrace:

[...]

>   1. &error-location: #f
>   2. &package-license-error:
>       package: #f
>       license: "https://www.qt.io/"
>   3. &formatted-message:
>       format: "~s: invalid package license~%\n"
>       arguments: ("https://www.qt.io/")

It may be that there’s no location info because during the “Computing
Guix derivation” step, things are being evaluated.

However, most likely the invalid license wouldn’t have been committed in
the first place because the committer would have been unable to test the
package.  (There is location info when running Guix from one’s
checkout.)

Thanks for checking!

Ludo’.
Ludovic Courtès Oct. 10, 2022, 7:51 a.m. UTC | #3 
Ludovic Courtès <ludo@gnu.org> skribis:

> The funny part of this patch is that the license validation expands
> to #t in the majority of cases, meaning that it comes without
> run-time overhead (there is macro-expansion overhead, but I didn’t
> measure it).

A quick measurement of the time taken by the “Computing Guix derivation”
step¹ (most of which is macro expansion) suggests it’s going from ~1m34s
to ~1m44s on my laptop—so roughly a 10% slowdown on this stage.

I guess that’s the price to pay.

That phase has always been very slow though, mostly due to psyntax, and
we need to do something about it anyway.

Ludo’.

¹ I ran ‘time guix time-machine --branch=wip-license-checks --url=$PWD’
  and similar for the commit before those changes.
Ludovic Courtès Oct. 10, 2022, 9:20 a.m. UTC | #4 
Hi,

Ludovic Courtès <ludo@gnu.org> skribis:

>   licenses: Let 'license?' expand to #t in trivial cases.
>   packages: Raise an exception for invalid 'license' values.

I pushed these two patches as b6bc4c109b807c646e99ec40360e681122d85b2c
(see <https://issues.guix.gnu.org/58231> for context).

Now we all need to run “make clean-go && make”!

Thank you for your understanding.  :-)

Ludo’.