bug#49827: Error message for missing synopsis in opam importer

[Alice BRENON]

Hello,

I triggered a confusing behaviour from the opam importer trying to
import package iter 1.2.1 today on a Guix System install.

The package iter is missing a "synopsis" field as can be seen on
https://opam.ocaml.org/packages/iter/ , which when I tried

guix import opam iter

yielded the following backtrace:

Backtrace:
   8 (primitive-load "/home/alice/.config/guix/current/bin/g…")
In guix/ui.scm:
   2185:7  7 (run-guix . _)
  2148:10  6 (run-guix-command _ . _)
In guix/scripts/import.scm:
   120:11  5 (guix-import . _)
In guix/scripts/import/opam.scm:
   104:23  4 (guix-import-opam . _)
In guix/utils.scm:
752:8  3 (call-with-temporary-output-file _)
In guix/import/opam.scm:
   337:34  2 (_ _ _)
In srfi/srfi-1.scm:
   460:18  1 (fold # …)
In guix/import/opam.scm:
   193:15  0 (_ _ _)

guix/import/opam.scm:193:15: Throw to key `match-error' with args
`("match" "no matching pattern" string-pat)'.


the final error is raised l.193 of guix/import/opam.scm because
metadata-ref supports various types for a metadata field, but not the
lack of it. As discussed with Maxime Devos on the IRC channel, it would
be helpful to either allow the import of a package with a missing field
(possibly filling it in the output scheme code for the imported package
with some bad value requiring the user to fill it and causing any build
to crash until replaced properly) or at least to handle that missing
field with a more explicit error message than the above backtrace
(something like "Can't import that package because it's missing such or
such field").

Thanks,

Alice BRENON

bug#49827: Error message for missing synopsis in opam importer

[Alice BRENON]

Hello,

Thanks for your answer Sarah. Simon, I don't know if you have been able
to make any progress but I wanted to make sure you had seen the patch
proposal I sent to let the opam importer work from more repositories
than the few initially defined (opam's official and three for coq):

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=49958

Though I had a local "bypass" on the metadata reader to be able to
perform the import I wanted and orginally designed my patch for, I paid
attention not to commit it to keep matters separated. Any insight on the
general form the improved error handling will take ? Please let me
know if I can update my #49958 patch to play along more nicely with
your rework.


Alice

Le Mon, 02 Aug 2021 12:28:20 -0700,
Sarah Morgensen  a écrit :

> Hi,
> 
> Thanks for the report. I'm CC'ing Simon since they have been working
> on improved error handling/reporting for the importers.
> 
> Alice BRENON  writes:
> 
> > Hello,
> >
> > I triggered a confusing behaviour from the opam importer trying to
> > import package iter 1.2.1 today on a Guix System install.
> >
> > The package iter is missing a "synopsis" field as can be seen on
> > https://opam.ocaml.org/packages/iter/ , which when I tried
> >
> > guix import opam iter
> >
> > yielded the following backtrace:
> >
> > Backtrace:
> >8 (primitive-load
> > "/home/alice/.config/guix/current/bin/g…") In guix/ui.scm:
> >2185:7  7 (run-guix . _)
> >   2148:10  6 (run-guix-command _ . _)
> > In guix/scripts/import.scm:
> >120:11  5 (guix-import . _)
> > In guix/scripts/import/opam.scm:
> >104:23  4 (guix-import-opam . _)
> > In guix/utils.scm:
> > 752:8  3 (call-with-temporary-output-file _)
> > In guix/import/opam.scm:
> >337:34  2 (_ _ _)
> > In srfi/srfi-1.scm:
> >460:18  1 (fold # > guix/import/opam.scm…> …) In guix/import/opam.scm:
> >193:15  0 (_ _ _)
> >
> > guix/import/opam.scm:193:15: Throw to key `match-error' with args
> > `("match" "no matching pattern" string-pat)'.
> >
> >
> > the final error is raised l.193 of guix/import/opam.scm because
> > metadata-ref supports various types for a metadata field, but not
> > the lack of it. As discussed with Maxime Devos on the IRC channel,
> > it would be helpful to either allow the import of a package with a
> > missing field (possibly filling it in the output scheme code for
> > the imported package with some bad value requiring the user to fill
> > it and causing any build to crash until replaced properly) or at
> > least to handle that missing field with a more explicit error
> > message than the above backtrace (something like "Can't import that
> > package because it's missing such or such field").  
> 
> IMO, a warning should be emitted, but the package should be buildable
> if at all possible; it's the submitter's responsibility to vet
> imported packages.
> 
> Simon, how's that error handling rework coming? ;)
> 
> >
> > Thanks,
> >
> > Alice BRENON  
> 
> --
> Sarah

bug#49827: Error message for missing synopsis in opam importer

[Alice BRENON]

Hello,

Thanks for your answer !

Le Tue, 17 Aug 2021 09:43:10 +0200,
zimoun  a écrit :

> Hi,
> 
> I am back from holidays. :-)
> 
> …   
> 
> From my understanding, there is 2 issues:  
> 
>  - gentle handler for error
>  - warn for incomplete metadata
> 

Yes, absolutely, because currently understanding the cause of the error
requires to delve into the source to understand what is going on. The
warning part is more optional, but if this pattern matching is modified
to handle that special case of a missing metadata instead of entirely
crashing, I thought it could be useful not to be too permissive either,
and to at least mention that a missing metadata was caught and should
be filled by hand.

This could take the form of a message above the output of the actual
scheme code for the package declaration while the importer is running,
or of an invalid value for that missing field in the generated scheme
output, something like "" or such that would be invalid in
scheme and would make guix build fail when trying to use the output
directly without manually editing it to fill the missing metadata.

> With Jérémy (jeko), we have started to work time to time using
> experimental pair-programming to fix the former.  Currently, each
> importer uses its own error mechanism and obviously incoherence
> between them happens; especially when ’--recursive’.  We are trying
> to unify that.
> 
> Thanks for the report of this use case. :-)

Glad to learn my report could help : )

> 
> 
> Cheers,
> simon

bug#51463: Lack of error message in several guix subcommands

[Alice BRENON]

Hi list,

I was giving guix shell a try today and noticed this annoying lack of
relevant feedback from the tool: when running on a particularly
malformed guix.scm, either by auto-loading or by explicitely passing -f
guix.scm, guix shell returned in error ($? == 1) without printing any
error message, which is a bit unhelpful.

The particular malformed guix.scm simply contains an extra parenthesis
after the package definition. Compare to the case when the package
definition lacks the final parenthesis, which yields a helpful message
like this:

/tmp/bug/guix.scm:25:1: missing closing parenthesis

This is not specific to guix shell because I could then reproduce this
behaviour with other commands like guix environment or guix build.

Find attached the file I've been using to reproduce the bug, which is
essentially the "hello" package example from the manual[1] without the
(define-public …) layer, in order for the top-level expression
contained in the file to be directly a package usable by -f (-l for
guix environment) and not have to put hello on the last line. The
version attached is correct and will allow guix shell -f guix.scm to
enter an environment where hello is installed. Remove a parenthesis,
you should have the above message. Add one, on the contrary, and you
should get nothing but silence.

I'm using guix on Guix System, and pulled this morning:

guix 5cbf9a4
  URL du dépôt : https://git.savannah.gnu.org/git/guix.git
  branche : master
  commit : 5cbf9a48d766191d8f17b2e9d1cf7b7db69b99ea


Regards,

Alice


[1]
https://guix.gnu.org/manual/devel/en/html_node/Defining-Packages.html
(define-module (gnu packages hello)
  #:use-module (guix packages)
  #:use-module (guix download)
  #:use-module (guix build-system gnu)
  #:use-module (guix licenses)
  #:use-module (gnu packages gawk))

(package
  (name "hello")
  (version "2.10")
  (source (origin
(method url-fetch)
(uri (string-append "mirror://gnu/hello/hello-" version
".tar.gz"))
(sha256
 (base32
  "0ssi1wpaf7plaswqqjwigppsg5fyh99vdlb9kzl7c9lng89ndq1i"
  (build-system gnu-build-system)
  (arguments '(#:configure-flags '("--enable-silent-rules")))
  (inputs `(("gawk" ,gawk)))
  (synopsis "Hello, GNU world: An example GNU package")
  (description "Guess what GNU Hello prints!")
  (home-page "https://www.gnu.org/software/hello/";)
  (license gpl3+)))

bug#63205: quodlibet fails to build after python 3.10 update

[Alice BRENON]

This problem seems to have been reported and fixed upstream:

https://github.com/quodlibet/quodlibet/commit/5f55431a28509fd4f4f7b40dc246f3d34fa8549e

Since no newer release has been out, I guess we can backport the commit itself.
I've tried with the attached patch. The operon tests still fail due to some
weird utime magic, a problem also known and discussed at:

https://github.com/quodlibet/quodlibet/pull/4053/commits/06a32b319f065550efe0d2a9ff10ca6bdc32b893

So I guess we could backport that as well. Does anyone have a better fix ?
>From 7e1eb6cd1c15923c7eb64b06afac9f83f6b6cfb3 Mon Sep 17 00:00:00 2001
Message-Id: <7e1eb6cd1c15923c7eb64b06afac9f83f6b6cfb3.1683293409.git.alice.bre...@ens-lyon.fr>
From: Alice BRENON 
Date: Fri, 5 May 2023 15:26:37 +0200
Subject: [PATCH] gnu: quodlibet: Fix glob failure in quodlibet tests.

Reported in #63205.

* gnu/packages/music.scm (quodlibet): Backport patch 5f55431 from
upstream and reactivate passing tests.
---
 gnu/packages/music.scm|  6 +-
 .../patches/quodlibet-fix-invalid-glob.patch  | 89 +++
 2 files changed, 92 insertions(+), 3 deletions(-)
 create mode 100644 gnu/packages/patches/quodlibet-fix-invalid-glob.patch

diff --git a/gnu/packages/music.scm b/gnu/packages/music.scm
index 446580dc52..3a005b2848 100644
--- a/gnu/packages/music.scm
+++ b/gnu/packages/music.scm
@@ -7082,6 +7082,8 @@ (define-public quodlibet
  (url "https://github.com/quodlibet/quodlibet";)
  (commit (string-append "release-" version
(file-name (git-file-name name version))
+   (patches (search-patches
+"quodlibet-fix-invalid-glob.patch"))
(sha256
 (base32 "1i5k93k3bfp7hpcwkbr865mbj9jam3jv2a5k1bazcyp4f5vdrb0v"
 (build-system python-build-system)
@@ -7105,9 +7107,7 @@ (define-public quodlibet
   "--ignore=tests/test_browsers_iradio.py"
   ;; broken upstream
   "--disable-warnings"
-  "--ignore=tests/quality"
-  ;; missing legacy icons in adwaita-icon-theme
-  "--ignore=tests/plugin/test_trayicon.py")
+  "--ignore=tests/quality/test_flake8.py")
   (format #t "test suite not run~%"
   (add-after 'install 'glib-or-gtk-wrap ; ensure icons loaded
 (assoc-ref glib-or-gtk:%standard-phases 'glib-or-gtk-wrap))
diff --git a/gnu/packages/patches/quodlibet-fix-invalid-glob.patch b/gnu/packages/patches/quodlibet-fix-invalid-glob.patch
new file mode 100644
index 00..95f95d8aab
--- /dev/null
+++ b/gnu/packages/patches/quodlibet-fix-invalid-glob.patch
@@ -0,0 +1,89 @@
+From 5f55431a28509fd4f4f7b40dc246f3d34fa8549e Mon Sep 17 00:00:00 2001
+From: Christoph Reiter 
+Date: Sun, 26 Jun 2022 23:14:28 +0200
+Subject: [PATCH] builtin cover: fix handling of invalid glob ranges with
+ Python 3.10.5+ (#4027)
+
+Previously Python would raise if an invalid range was given
+to glob, but with 3.10.5 they fixed it to not match anything.
+https://github.com/python/cpython/issues/89973
+
+Our tests depended on the previous logic and treating the glob pattern
+as a literal file name in that case.
+
+One could argue that this is wrong since a range that doesn't contain anything
+should also not match anything, so wrap glob() to make it not match for all
+Python versions in that case and adjust the tests accordingly.
+
+This should fix the Windows CI, which is currently the only job using 3.10.5
+---
+ quodlibet/util/cover/built_in.py | 22 +++---
+ tests/test_util_cover.py | 12 +++-
+ 2 files changed, 14 insertions(+), 20 deletions(-)
+
+diff --git a/quodlibet/util/cover/built_in.py b/quodlibet/util/cover/built_in.py
+index f2a8791a2..01474c9b6 100644
+--- a/quodlibet/util/cover/built_in.py
 b/quodlibet/util/cover/built_in.py
+@@ -100,6 +100,15 @@ class FilesystemCover(CoverSourcePlugin):
+ base = self.song('~dirname')
+ images = []
+ 
++def safe_glob(*args, **kwargs):
++try:
++return glob.glob(*args, **kwargs)
++except sre_constants.error:
++# https://github.com/python/cpython/issues/89973
++# old glob would fail with invalid ranges, newer one
++# handles it correctly.
++return []
++
+ if config.getboolean("albumart", "force_filename"):
+ score = 100
+ for filename in config.get("albumart", "filename").split(","):
+@@ -107,17 +116,8 @@ class FilesystemCover(CoverSourcePlugin):
+ filename = filename.strip()
+ 
+ escaped_path = os.path.join(glob.escape(base

bug#63205: quodlibet fails to build after python 3.10 update

[Alice BRENON]

The second patch needs an edit because part of it doesn't apply to the
actual source (4.5.0 doesn't contain 427793aa88fb57 which applied the
@flaky markers to the test). But quodlibet builds again on my machine
with this second patch.

Le Fri, 5 May 2023 15:31:42 +0200,
Alice BRENON  a écrit :

> This problem seems to have been reported and fixed upstream:
> 
> https://github.com/quodlibet/quodlibet/commit/5f55431a28509fd4f4f7b40dc246f3d34fa8549e
> 
> Since no newer release has been out, I guess we can backport the
> commit itself. I've tried with the attached patch. The operon tests
> still fail due to some weird utime magic, a problem also known and
> discussed at:
> 
> https://github.com/quodlibet/quodlibet/pull/4053/commits/06a32b319f065550efe0d2a9ff10ca6bdc32b893
> 
> So I guess we could backport that as well. Does anyone have a better
> fix ?

>From 184c009037ac04ed1c8b2892a4e111fe8b5b9540 Mon Sep 17 00:00:00 2001
Message-Id: <184c009037ac04ed1c8b2892a4e111fe8b5b9540.1683294873.git.alice.bre...@ens-lyon.fr>
From: Alice BRENON 
Date: Fri, 5 May 2023 15:26:37 +0200
Subject: [PATCH] gnu: quodlibet: Fix failures in tests.

Reported in #63205.

* gnu/packages/music.scm (quodlibet): Backport patches 5f55431 and a
  part of 06a32b3 from upstream and reactivate passing tests.
---
 gnu/packages/music.scm|  7 +-
 .../patches/quodlibet-fix-invalid-glob.patch  | 89 +++
 .../patches/quodlibet-fix-mtime-tests.patch   | 39 
 3 files changed, 132 insertions(+), 3 deletions(-)
 create mode 100644 gnu/packages/patches/quodlibet-fix-invalid-glob.patch
 create mode 100644 gnu/packages/patches/quodlibet-fix-mtime-tests.patch

diff --git a/gnu/packages/music.scm b/gnu/packages/music.scm
index 446580dc52..05fb25f1b1 100644
--- a/gnu/packages/music.scm
+++ b/gnu/packages/music.scm
@@ -7082,6 +7082,9 @@ (define-public quodlibet
  (url "https://github.com/quodlibet/quodlibet";)
  (commit (string-append "release-" version
(file-name (git-file-name name version))
+   (patches (search-patches
+"quodlibet-fix-invalid-glob.patch"
+"quodlibet-fix-mtime-tests.patch"))
(sha256
 (base32 "1i5k93k3bfp7hpcwkbr865mbj9jam3jv2a5k1bazcyp4f5vdrb0v"
 (build-system python-build-system)
@@ -7105,9 +7108,7 @@ (define-public quodlibet
   "--ignore=tests/test_browsers_iradio.py"
   ;; broken upstream
   "--disable-warnings"
-  "--ignore=tests/quality"
-  ;; missing legacy icons in adwaita-icon-theme
-  "--ignore=tests/plugin/test_trayicon.py")
+  "--ignore=tests/quality/test_flake8.py")
   (format #t "test suite not run~%"
   (add-after 'install 'glib-or-gtk-wrap ; ensure icons loaded
 (assoc-ref glib-or-gtk:%standard-phases 'glib-or-gtk-wrap))
diff --git a/gnu/packages/patches/quodlibet-fix-invalid-glob.patch b/gnu/packages/patches/quodlibet-fix-invalid-glob.patch
new file mode 100644
index 00..95f95d8aab
--- /dev/null
+++ b/gnu/packages/patches/quodlibet-fix-invalid-glob.patch
@@ -0,0 +1,89 @@
+From 5f55431a28509fd4f4f7b40dc246f3d34fa8549e Mon Sep 17 00:00:00 2001
+From: Christoph Reiter 
+Date: Sun, 26 Jun 2022 23:14:28 +0200
+Subject: [PATCH] builtin cover: fix handling of invalid glob ranges with
+ Python 3.10.5+ (#4027)
+
+Previously Python would raise if an invalid range was given
+to glob, but with 3.10.5 they fixed it to not match anything.
+https://github.com/python/cpython/issues/89973
+
+Our tests depended on the previous logic and treating the glob pattern
+as a literal file name in that case.
+
+One could argue that this is wrong since a range that doesn't contain anything
+should also not match anything, so wrap glob() to make it not match for all
+Python versions in that case and adjust the tests accordingly.
+
+This should fix the Windows CI, which is currently the only job using 3.10.5
+---
+ quodlibet/util/cover/built_in.py | 22 +++---
+ tests/test_util_cover.py | 12 +++-
+ 2 files changed, 14 insertions(+), 20 deletions(-)
+
+diff --git a/quodlibet/util/cover/built_in.py b/quodlibet/util/cover/built_in.py
+index f2a8791a2..01474c9b6 100644
+--- a/quodlibet/util/cover/built_in.py
 b/quodlibet/util/cover/built_in.py
+@@ -100,6 +100,15 @@ class FilesystemCover(CoverSourcePlugin):
+ base = self.song('~dirname')
+ images = []
+ 
++def safe_glob(*args, **kwargs):
++try:
++return glob.glob(*args, **kwargs)
++except sre_constants.error:
++#