[bug#53548] [PATCH] Harden beautify-description

[bug#53548] [PATCH] Harden beautify-description

[Alice BRENON]

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

Hi all,

Missing metadata in packages used to break their imports, at least for
opam packages, until this was fixed by Julien in
24aa7b3c21309b63cc6e8e18d6417d2cddccf6c6. When Xinglu improved the
output of descriptions in 155fc235b5e1b41b4665c782365dd2bf11beae9c, it
made the imports break again in the case when the `description` field
is missing and Julien's fix applies, returning #f in `metadata-ref`
instead of crashing. Trouble is: beautify-description expects its
description argument to be a string and will crash if this assumption
isn't met (when calling the `string-prefix?` predicate).

This patch hopes to fix this by prepending a catch-all case in
`beautify-description` to intercept all calls to it without a proper
string as description. In addition, since I had to search for
beautify-description in all the code base I made its import from
opam.scm explicit, to carry on what I think is a good practice which I
would like to see progressively enforced in guix' source code: explicit
imports everywhere, to prevent accidental namespace collisions and to
ease source code browsing by pointing out where everything comes from
in a given source file.

This patch can probably not be pushed as-is and should be improved
first:

- In the generated description, I think it'd be useful to point the
  user to the documentation page helping them to understand what is
  expected in a good description, so I generated a link to it on the
  web-hosted manual. However, this choice is arbitrary: why not an
  info page or the PDF ? Is there an consensus on how to refer to a
  page in the documentation in guix's output ? If not and we stick to
  the web, perhaps the `home-url` and `doc-path` variable could be
  declared outside of (guix import utils), perhaps turned into
  functions and localized to the user's preference instead of the
  built-in english version I used.

- If all-explict use-modules policy is relevant, it should be applied
  to the other import modules that Xinglu edited in 155fc23. If not,
  then my change in guix/import/opam.scm should perhaps not be applied
  at all.

Thanks for your feedback on this proposal !

Best regards,

Alice

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-guix-import-Harden-beautify-description.patch --]
[-- Type: text/x-patch, Size: 3187 bytes --]

From 22f0523ef3599b45af9448bd4f31f7b8f8ce6af2 Mon Sep 17 00:00:00 2001
From: Alice BRENON <alice.brenon@ens-lyon.fr>
Date: Wed, 26 Jan 2022 09:27:12 +0100
Subject: [PATCH] guix: import: Harden beautify-description.

* guix/import/utils.scm (beautify-description): Handle non-string
arguments.
* guix/import/opam.scm: [use-modules] Make imports explicit for module
(guix import utils).
---
 guix/import/opam.scm  |  8 ++++++--
 guix/import/utils.scm | 10 ++++++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/guix/import/opam.scm b/guix/import/opam.scm
index a6f6fe8c9f..f569c921b1 100644
--- a/guix/import/opam.scm
+++ b/guix/import/opam.scm
@@ -3,7 +3,7 @@
 ;;; Copyright © 2020 Martin Becze <mjbecze@riseup.net>
 ;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz>
 ;;; Copyright © 2021 Sarah Morgensen <iskarian@mgsn.dev>
-;;; Copyright © 2021 Alice Brenon <alice.brenon@ens-lyon.fr>
+;;; Copyright © 2021, 2022 Alice Brenon <alice.brenon@ens-lyon.fr>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -42,7 +42,11 @@ (define-module (guix import opam)
   #:use-module ((guix utils) #:select (cache-directory
                                        version>?
                                        call-with-temporary-output-file))
-  #:use-module (guix import utils)
+  #:use-module ((guix import utils) #:select (beautify-description
+                                              guix-hash-url
+                                              recursive-import
+                                              spdx-string->license
+                                              url-fetch))
   #:use-module ((guix licenses) #:prefix license:)
   #:export (opam->guix-package
             opam-recursive-import
diff --git a/guix/import/utils.scm b/guix/import/utils.scm
index 1c3cfa3e0b..cd716e3dc7 100644
--- a/guix/import/utils.scm
+++ b/guix/import/utils.scm
@@ -10,6 +10,7 @@
 ;;; Copyright © 2021 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;; Copyright © 2021 Sarah Morgensen <iskarian@mgsn.dev>
 ;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz>
+;;; Copyright © 2022 Alice Brenon <alice.brenon@ens-lyon.fr>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -241,6 +242,15 @@ (define* (beautify-description description #:optional (length 80))
 a proper sentence and by using two spaces between sentences, and wrap lines at
 LENGTH characters."
   (let ((cleaned (cond
+                  ((not (string? description))
+                   (let ((home-url "https://guix.gnu.org/")
+                         (doc-path "fr/manual/devel/en/html_node/")
+                         (page
+                           "Synopses-and-Descriptions.html#Synopses-and-Descriptions"))
+                     (string-append
+                       "Please fill in the description of your package before "
+                       "submitting ! See "
+                       home-url doc-path page)))
                   ((string-prefix? "A " description)
                    (string-append "This package provides a"
                                   (substring description 1)))
-- 
2.34.0

[bug#53548] [PATCH] Harden beautify-description

[Ludovic Courtès]

Hi Alice,

Alice BRENON <alice.brenon@ens-lyon.fr> skribis:

> Missing metadata in packages used to break their imports, at least for
> opam packages, until this was fixed by Julien in
> 24aa7b3c21309b63cc6e8e18d6417d2cddccf6c6. When Xinglu improved the
> output of descriptions in 155fc235b5e1b41b4665c782365dd2bf11beae9c, it
> made the imports break again in the case when the `description` field
> is missing and Julien's fix applies, returning #f in `metadata-ref`
> instead of crashing. Trouble is: beautify-description expects its
> description argument to be a string and will crash if this assumption
> isn't met (when calling the `string-prefix?` predicate).

Thanks for explaining!  Some comments:

> From 22f0523ef3599b45af9448bd4f31f7b8f8ce6af2 Mon Sep 17 00:00:00 2001
> From: Alice BRENON <alice.brenon@ens-lyon.fr>
> Date: Wed, 26 Jan 2022 09:27:12 +0100
> Subject: [PATCH] guix: import: Harden beautify-description.
>
> * guix/import/utils.scm (beautify-description): Handle non-string
> arguments.
> * guix/import/opam.scm: [use-modules] Make imports explicit for module
> (guix import utils).

[...]

>    #:use-module ((guix utils) #:select (cache-directory
>                                         version>?
>                                         call-with-temporary-output-file))
> -  #:use-module (guix import utils)
> +  #:use-module ((guix import utils) #:select (beautify-description
> +                                              guix-hash-url
> +                                              recursive-import
> +                                              spdx-string->license
> +                                              url-fetch))

It can’t hurt.

> +                  ((not (string? description))
> +                   (let ((home-url "https://guix.gnu.org/")
> +                         (doc-path "fr/manual/devel/en/html_node/")
> +                         (page
> +                           "Synopses-and-Descriptions.html#Synopses-and-Descriptions"))
> +                     (string-append
> +                       "Please fill in the description of your package before "
> +                       "submitting ! See "
> +                       home-url doc-path page)))

I’d avoid the URL and maybe make the string translatable, like so:

  (G_ "This package lacks a description …
Run \"info '(guix) Synopses and Descriptions'\" for more information.")

… where ‘G_’ comes from (guix i18n).

WDYT?

This looks like a welcome improvement to me.

Thanks!

Ludo’.

[bug#53548] [PATCH] Harden beautify-description

[Alice BRENON]

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

Hi !

Thank you again for the help. I think it looks really better with the
translated reference to the info page. I feared the issues I raised
would yield unending discussions but actually with your edit it's
already pretty neat while remaining very simple. That's inspiring.

For the others interested in this thread, I also made the import of G_
from (guix i18n) explicit because it was being re-exported from (guix
ui), and while I was at it, I made the only used import from (guix ui),
that is fill-paragraph, explicit.

Hope you like this second version better : )

Alice


Le Tue, 01 Feb 2022 00:09:40 +0100,
Ludovic Courtès <ludo@gnu.org> a écrit :

> Hi Alice,
> 
> Alice BRENON <alice.brenon@ens-lyon.fr> skribis:
> 
> > Missing metadata in packages used to break their imports, at least
> > for opam packages, until this was fixed by Julien in
> > 24aa7b3c21309b63cc6e8e18d6417d2cddccf6c6. When Xinglu improved the
> > output of descriptions in 155fc235b5e1b41b4665c782365dd2bf11beae9c,
> > it made the imports break again in the case when the `description`
> > field is missing and Julien's fix applies, returning #f in
> > `metadata-ref` instead of crashing. Trouble is:
> > beautify-description expects its description argument to be a
> > string and will crash if this assumption isn't met (when calling
> > the `string-prefix?` predicate).  
> 
> Thanks for explaining!  Some comments:
> 
> > From 22f0523ef3599b45af9448bd4f31f7b8f8ce6af2 Mon Sep 17 00:00:00
> > 2001 From: Alice BRENON <alice.brenon@ens-lyon.fr>
> > Date: Wed, 26 Jan 2022 09:27:12 +0100
> > Subject: [PATCH] guix: import: Harden beautify-description.
> >
> > * guix/import/utils.scm (beautify-description): Handle non-string
> > arguments.
> > * guix/import/opam.scm: [use-modules] Make imports explicit for
> > module (guix import utils).  
> 
> [...]
> 
> >    #:use-module ((guix utils) #:select (cache-directory  
> >                                         version>?  
> >                                         call-with-temporary-output-file))
> > -  #:use-module (guix import utils)
> > +  #:use-module ((guix import utils) #:select (beautify-description
> > +                                              guix-hash-url
> > +                                              recursive-import
> > +                                              spdx-string->license
> > +                                              url-fetch))  
> 
> It can’t hurt.
> 
> > +                  ((not (string? description))
> > +                   (let ((home-url "https://guix.gnu.org/")
> > +                         (doc-path "fr/manual/devel/en/html_node/")
> > +                         (page
> > +
> > "Synopses-and-Descriptions.html#Synopses-and-Descriptions"))
> > +                     (string-append
> > +                       "Please fill in the description of your
> > package before "
> > +                       "submitting ! See "
> > +                       home-url doc-path page)))  
> 
> I’d avoid the URL and maybe make the string translatable, like so:
> 
>   (G_ "This package lacks a description …
> Run \"info '(guix) Synopses and Descriptions'\" for more
> information.")
> 
> … where ‘G_’ comes from (guix i18n).
> 
> WDYT?
> 
> This looks like a welcome improvement to me.
> 
> Thanks!
> 
> Ludo’.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-guix-import-Harden-beautify-description.patch --]
[-- Type: text/x-patch, Size: 3430 bytes --]

From cfb57a40a90bfc31d1b846f3e981469285f1bd7b Mon Sep 17 00:00:00 2001
From: Alice BRENON <alice.brenon@ens-lyon.fr>
Date: Wed, 26 Jan 2022 09:27:12 +0100
Subject: [PATCH] guix: import: Harden beautify-description.

* guix/import/utils.scm (beautify-description): Handle non-string
arguments.
[use-modules]: Explicitly import G_ from (guix i18n) and make (guix ui)
import explicit.
* guix/import/opam.scm: [use-modules] Make imports explicit for module
(guix import utils).
---
 guix/import/opam.scm  | 8 ++++++--
 guix/import/utils.scm | 7 ++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/guix/import/opam.scm b/guix/import/opam.scm
index a6f6fe8c9f..f569c921b1 100644
--- a/guix/import/opam.scm
+++ b/guix/import/opam.scm
@@ -3,7 +3,7 @@
 ;;; Copyright © 2020 Martin Becze <mjbecze@riseup.net>
 ;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz>
 ;;; Copyright © 2021 Sarah Morgensen <iskarian@mgsn.dev>
-;;; Copyright © 2021 Alice Brenon <alice.brenon@ens-lyon.fr>
+;;; Copyright © 2021, 2022 Alice Brenon <alice.brenon@ens-lyon.fr>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -42,7 +42,11 @@ (define-module (guix import opam)
   #:use-module ((guix utils) #:select (cache-directory
                                        version>?
                                        call-with-temporary-output-file))
-  #:use-module (guix import utils)
+  #:use-module ((guix import utils) #:select (beautify-description
+                                              guix-hash-url
+                                              recursive-import
+                                              spdx-string->license
+                                              url-fetch))
   #:use-module ((guix licenses) #:prefix license:)
   #:export (opam->guix-package
             opam-recursive-import
diff --git a/guix/import/utils.scm b/guix/import/utils.scm
index 1c3cfa3e0b..934b224bec 100644
--- a/guix/import/utils.scm
+++ b/guix/import/utils.scm
@@ -10,6 +10,7 @@
 ;;; Copyright © 2021 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;; Copyright © 2021 Sarah Morgensen <iskarian@mgsn.dev>
 ;;; Copyright © 2021 Xinglu Chen <public@yoctocell.xyz>
+;;; Copyright © 2022 Alice Brenon <alice.brenon@ens-lyon.fr>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -37,10 +38,11 @@ (define-module (guix import utils)
   #:use-module (guix discovery)
   #:use-module (guix build-system)
   #:use-module (guix gexp)
+  #:use-module ((guix i18n) #:select (G_))
   #:use-module (guix store)
   #:use-module (guix download)
   #:use-module (guix sets)
-  #:use-module (guix ui)
+  #:use-module ((guix ui) #:select (fill-paragraph))
   #:use-module (gnu packages)
   #:use-module (ice-9 match)
   #:use-module (ice-9 rdelim)
@@ -241,6 +243,9 @@ (define* (beautify-description description #:optional (length 80))
 a proper sentence and by using two spaces between sentences, and wrap lines at
 LENGTH characters."
   (let ((cleaned (cond
+                  ((not (string? description))
+                   (G_ "This package lacks a description.  Run \"info '(guix)
+                       Synopses and Descriptions'\" for more information."))
                   ((string-prefix? "A " description)
                    (string-append "This package provides a"
                                   (substring description 1)))
-- 
2.34.0

bug#53548: [PATCH] Harden beautify-description

[Ludovic Courtès]

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

Hi!

Alice BRENON <alice.brenon@ens-lyon.fr> skribis:

> From cfb57a40a90bfc31d1b846f3e981469285f1bd7b Mon Sep 17 00:00:00 2001
> From: Alice BRENON <alice.brenon@ens-lyon.fr>
> Date: Wed, 26 Jan 2022 09:27:12 +0100
> Subject: [PATCH] guix: import: Harden beautify-description.
>
> * guix/import/utils.scm (beautify-description): Handle non-string
> arguments.
> [use-modules]: Explicitly import G_ from (guix i18n) and make (guix ui)
> import explicit.
> * guix/import/opam.scm: [use-modules] Make imports explicit for module
> (guix import utils).

Applied.

I removed extra space from the message, as shown below.

Thanks!

Ludo’.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 802 bytes --]

diff --git a/guix/import/utils.scm b/guix/import/utils.scm
index 934b224bec..9cadbb3d5f 100644
--- a/guix/import/utils.scm
+++ b/guix/import/utils.scm
@@ -244,8 +244,8 @@ (define* (beautify-description description #:optional (length 80))
 LENGTH characters."
   (let ((cleaned (cond
                   ((not (string? description))
-                   (G_ "This package lacks a description.  Run \"info '(guix)
-                       Synopses and Descriptions'\" for more information."))
+                   (G_ "This package lacks a description.  Run \
+\"info '(guix) Synopses and Descriptions'\" for more information."))
                   ((string-prefix? "A " description)
                    (string-append "This package provides a"
                                   (substring description 1)))