[bug#51091] [PATCH] guix: opam: Do not fail when refreshing.

[bug#51091] [PATCH] guix: opam: Do not fail when refreshing.

[Julien Lepiller]

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

Hi Guix!

the attached patch prevents early failures in "guix refresh -t opam".
It will now simply continue when it encounters a package that is not in
the opam repository.

[-- Attachment #2: 0001-guix-opam-Do-not-fail-when-refreshing.patch --]
[-- Type: text/x-patch, Size: 2069 bytes --]

From f6260b762dd78772e0d90d96dd92d22346a09007 Mon Sep 17 00:00:00 2001
From: Julien Lepiller <julien@lepiller.eu>
Date: Fri, 8 Oct 2021 04:58:27 +0200
Subject: [PATCH] guix: opam: Do not fail when refreshing.

Because we throw an error when a package is not in the opam repository,
the updater would crash when encountering a package that is not in opam
but uses the ocaml-build-system, such as opam itself.  This catches the
error and continues without updating said package, and lets us update
the rest of the packages.

* guix/import/opam.scm (latest-release): Catch errors and do not crash.
---
 guix/import/opam.scm | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/guix/import/opam.scm b/guix/import/opam.scm
index fe13d29f03..8ff1a3ae63 100644
--- a/guix/import/opam.scm
+++ b/guix/import/opam.scm
@@ -409,16 +409,19 @@ package in OPAM."
 
 (define (latest-release package)
   "Return an <upstream-source> for the latest release of PACKAGE."
-  (and-let* ((opam-name (guix-package->opam-name package))
-             (opam-file (opam-fetch opam-name))
-             (version (assoc-ref opam-file "version"))
-             (opam-content (assoc-ref opam-file "metadata"))
-             (url-dict (metadata-ref opam-content "url"))
-             (source-url (metadata-ref url-dict "src")))
-    (upstream-source
-      (package (package-name package))
-      (version version)
-      (urls (list source-url)))))
+  (catch #t
+    (lambda _
+      (and-let* ((opam-name (guix-package->opam-name package))
+                 (opam-file (opam-fetch opam-name))
+                 (version (assoc-ref opam-file "version"))
+                 (opam-content (assoc-ref opam-file "metadata"))
+                 (url-dict (metadata-ref opam-content "url"))
+                 (source-url (metadata-ref url-dict "src")))
+        (upstream-source
+          (package (package-name package))
+          (version version)
+          (urls (list source-url)))))
+    (const #f)))
 
 (define %opam-updater
   (upstream-updater
-- 
2.33.0

[bug#51091] [PATCH] guix: opam: Do not fail when refreshing.

[Xinglu Chen]

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

On Fri, Oct 08 2021, Julien Lepiller wrote:

> Hi Guix!
>
> the attached patch prevents early failures in "guix refresh -t opam".
> It will now simply continue when it encounters a package that is not in
> the opam repository.
> From f6260b762dd78772e0d90d96dd92d22346a09007 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien@lepiller.eu>
> Date: Fri, 8 Oct 2021 04:58:27 +0200
> Subject: [PATCH] guix: opam: Do not fail when refreshing.
>
> Because we throw an error when a package is not in the opam repository,
> the updater would crash when encountering a package that is not in opam
> but uses the ocaml-build-system, such as opam itself.  This catches the
> error and continues without updating said package, and lets us update
> the rest of the packages.
>
> * guix/import/opam.scm (latest-release): Catch errors and do not crash.
> ---
>  guix/import/opam.scm | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/guix/import/opam.scm b/guix/import/opam.scm
> index fe13d29f03..8ff1a3ae63 100644
> --- a/guix/import/opam.scm
> +++ b/guix/import/opam.scm
> @@ -409,16 +409,19 @@ package in OPAM."
>  
>  (define (latest-release package)
>    "Return an <upstream-source> for the latest release of PACKAGE."
> -  (and-let* ((opam-name (guix-package->opam-name package))
> -             (opam-file (opam-fetch opam-name))
> -             (version (assoc-ref opam-file "version"))
> -             (opam-content (assoc-ref opam-file "metadata"))
> -             (url-dict (metadata-ref opam-content "url"))
> -             (source-url (metadata-ref url-dict "src")))
> -    (upstream-source
> -      (package (package-name package))
> -      (version version)
> -      (urls (list source-url)))))
> +  (catch #t

Using (catch #t ...) is generally not a good idea.  Maybe ‘opam-fetch’ should
raise a ‘opam-fetch’ condition, and then we would only catch those
conditions?

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

[bug#51091] [PATCH] guix: opam: Do not fail when refreshing.

[Ludovic Courtès]

Hi,

Xinglu Chen <public@yoctocell.xyz> skribis:

> On Fri, Oct 08 2021, Julien Lepiller wrote:

[...]

>> Because we throw an error when a package is not in the opam repository,
>> the updater would crash when encountering a package that is not in opam
>> but uses the ocaml-build-system, such as opam itself.  This catches the
>> error and continues without updating said package, and lets us update
>> the rest of the packages.
>>
>> * guix/import/opam.scm (latest-release): Catch errors and do not crash.
>> ---
>>  guix/import/opam.scm | 23 +++++++++++++----------
>>  1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/guix/import/opam.scm b/guix/import/opam.scm
>> index fe13d29f03..8ff1a3ae63 100644
>> --- a/guix/import/opam.scm
>> +++ b/guix/import/opam.scm
>> @@ -409,16 +409,19 @@ package in OPAM."
>>  
>>  (define (latest-release package)
>>    "Return an <upstream-source> for the latest release of PACKAGE."
>> -  (and-let* ((opam-name (guix-package->opam-name package))
>> -             (opam-file (opam-fetch opam-name))
>> -             (version (assoc-ref opam-file "version"))
>> -             (opam-content (assoc-ref opam-file "metadata"))
>> -             (url-dict (metadata-ref opam-content "url"))
>> -             (source-url (metadata-ref url-dict "src")))
>> -    (upstream-source
>> -      (package (package-name package))
>> -      (version version)
>> -      (urls (list source-url)))))
>> +  (catch #t
>
> Using (catch #t ...) is generally not a good idea.  Maybe ‘opam-fetch’ should
> raise a ‘opam-fetch’ condition, and then we would only catch those
> conditions?

Agreed, it’s best to catch the most specific exception, around the most
narrow bits of code, so unexpected errors that denote bugs are properly
reported.

Nitpick: in the commit log, the subject line should start with
“import: opam:”.  :-)

Thanks,
Ludo’.

[bug#51091] [PATCH v2] guix: opam: Do not fail when refreshing.

[Julien Lepiller]

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

Thanks for the reviews! I've managed to use conditions, which was not
easy because I didn't notice there was already a "condition" defined,
and that lead to weird error messages. Attached is the updated patch.

[-- Attachment #2: 0001-import-opam-Do-not-fail-when-refreshing.patch --]
[-- Type: text/x-patch, Size: 7923 bytes --]

From 0cadac6c3dabea8b986cd59d97c84beaf7a33325 Mon Sep 17 00:00:00 2001
Message-Id: <0cadac6c3dabea8b986cd59d97c84beaf7a33325.1634350078.git.julien@lepiller.eu>
From: Julien Lepiller <julien@lepiller.eu>
Date: Fri, 8 Oct 2021 04:58:27 +0200
Subject: [PATCH] import: opam: Do not fail when refreshing.

Because we throw an error when a package is not in the opam repository,
the updater would crash when encountering a package that is not in opam
but uses the ocaml-build-system, such as opam itself.  This catches the
error and continues without updating said package, and lets us update
the rest of the packages.

* guix/scripts/import/opam.scm (guix-import-opam): Catch not-found
condition and leave.
* guix/import/opam.scm (&opam-not-found-error): New condition type.
(opam-fetch): Raise condition instead of leaving.
(latest-release): Catch not-found condition and warn.
(conditional): Rename from `condition'.
* tests/opam.scm (parse-conditions): Change accordingly.
---
 guix/import/opam.scm         | 45 +++++++++++++++++++++++++-----------
 guix/scripts/import/opam.scm | 31 ++++++++++++++-----------
 tests/opam.scm               |  2 +-
 3 files changed, 49 insertions(+), 29 deletions(-)

diff --git a/guix/import/opam.scm b/guix/import/opam.scm
index fe13d29f03..cd746f6dc6 100644
--- a/guix/import/opam.scm
+++ b/guix/import/opam.scm
@@ -30,6 +30,8 @@
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-2)
   #:use-module ((srfi srfi-26) #:select (cut))
+  #:use-module (srfi srfi-34)
+  #:use-module (srfi srfi-35)
   #:use-module ((web uri) #:select (string->uri uri->string))
   #:use-module ((guix build utils) #:select (dump-port find-files mkdir-p))
   #:use-module (guix build-system)
@@ -47,12 +49,16 @@
             opam-recursive-import
             %opam-updater
 
+            &opam-not-found-error
+            opam-not-found-error?
+            opam-not-found-error-package
+
             ;; The following patterns are exported for testing purposes.
             string-pat
             multiline-string
             list-pat
             dict
-            condition))
+            conditional))
 
 ;; Define a PEG parser for the opam format
 (define-peg-pattern comment none (and "#" (* COMMCHR) "\n"))
@@ -85,7 +91,7 @@
                     (and (or conditional-value ground-value) (* SP) (ignore "&") (* SP)
                          (or group-pat conditional-value ground-value)))
 (define-peg-pattern ground-value body (and (or multiline-string string-pat choice-pat list-pat var) (* SP)))
-(define-peg-pattern conditional-value all (and ground-value (* SP) condition))
+(define-peg-pattern conditional-value all (and ground-value (* SP) conditional))
 (define-peg-pattern string-pat all (and QUOTE (* STRCHR) QUOTE))
 (define-peg-pattern list-pat all (and (ignore "[") (* SP) (* (and value (* SP))) (ignore "]")))
 (define-peg-pattern var all (+ (or (range #\a #\z) "-")))
@@ -96,7 +102,7 @@
                          QUOTE QUOTE QUOTE))
 (define-peg-pattern dict all (and (ignore "{") (* SP) records (* SP) (ignore "}")))
 
-(define-peg-pattern condition body (and (ignore "{") condition-form (ignore "}")))
+(define-peg-pattern conditional body (and (ignore "{") condition-form (ignore "}")))
 
 (define-peg-pattern condition-form body
                     (and
@@ -310,6 +316,10 @@ path to the repository."
      (list dependency (list 'unquote (string->symbol dependency))))
    (ocaml-names->guix-names lst)))
 
+(define-condition-type &opam-not-found-error &error
+  opam-not-found-error?
+  (package opam-not-found-error-package))
+
 (define* (opam-fetch name #:optional (repositories-specs '("opam")))
   (or (fold (lambda (repository others)
               (match (find-latest-version name repository)
@@ -318,7 +328,7 @@ path to the repository."
                 (_ others)))
             #f
             (filter-map get-opam-repository repositories-specs))
-      (leave (G_ "package '~a' not found~%") name)))
+      (raise (condition (&opam-not-found-error (package name))))))
 
 (define* (opam->guix-package name #:key (repo '()) version)
   "Import OPAM package NAME from REPOSITORIES (a list of names, URLs or local
@@ -409,16 +419,23 @@ package in OPAM."
 
 (define (latest-release package)
   "Return an <upstream-source> for the latest release of PACKAGE."
-  (and-let* ((opam-name (guix-package->opam-name package))
-             (opam-file (opam-fetch opam-name))
-             (version (assoc-ref opam-file "version"))
-             (opam-content (assoc-ref opam-file "metadata"))
-             (url-dict (metadata-ref opam-content "url"))
-             (source-url (metadata-ref url-dict "src")))
-    (upstream-source
-      (package (package-name package))
-      (version version)
-      (urls (list source-url)))))
+  (catch #t
+    (lambda _
+      (and-let* ((opam-name (guix-package->opam-name package))
+                 (opam-file (guard* (c ((opam-not-found-error? c)
+                                        (warning (G_ "package '~a' not found~%")
+                                                 (opam-not-found-error-package c))
+                                        #f))
+                              (opam-fetch opam-name)))
+                 (version (assoc-ref opam-file "version"))
+                 (opam-content (assoc-ref opam-file "metadata"))
+                 (url-dict (metadata-ref opam-content "url"))
+                 (source-url (metadata-ref url-dict "src")))
+        (upstream-source
+          (package (package-name package))
+          (version version)
+          (urls (list source-url)))))
+    (const #f)))
 
 (define %opam-updater
   (upstream-updater
diff --git a/guix/scripts/import/opam.scm b/guix/scripts/import/opam.scm
index 834ac34cb0..043d05d717 100644
--- a/guix/scripts/import/opam.scm
+++ b/guix/scripts/import/opam.scm
@@ -93,20 +93,23 @@ Import and convert the opam package for PACKAGE-NAME.\n"))
                            (reverse opts))))
     (match args
       ((package-name)
-       (if (assoc-ref opts 'recursive)
-           ;; Recursive import
-           (map (match-lambda
-                  ((and ('package ('name name) . rest) pkg)
-                   `(define-public ,(string->symbol name)
-                      ,pkg))
-                  (_ #f))
-                (opam-recursive-import package-name #:repo repo))
-           ;; Single import
-           (let ((sexp (opam->guix-package package-name #:repo repo)))
-             (unless sexp
-               (leave (G_ "failed to download meta-data for package '~a'~%")
-                      package-name))
-             sexp)))
+       (guard* (c ((opam-not-found-error? c)
+                  (leave (G_ "package '~a' not found~%")
+                         (opam-not-found-error-package c))))
+         (if (assoc-ref opts 'recursive)
+             ;; Recursive import
+             (map (match-lambda
+                    ((and ('package ('name name) . rest) pkg)
+                     `(define-public ,(string->symbol name)
+                        ,pkg))
+                    (_ #f))
+                  (opam-recursive-import package-name #:repo repo))
+             ;; Single import
+             (let ((sexp (opam->guix-package package-name #:repo repo)))
+               (unless sexp
+                 (leave (G_ "failed to download meta-data for package '~a'~%")
+                        package-name))
+               sexp))))
       (()
        (leave (G_ "too few arguments~%")))
       ((many ...)
diff --git a/tests/opam.scm b/tests/opam.scm
index 31b4ea41ff..96b748bcd9 100644
--- a/tests/opam.scm
+++ b/tests/opam.scm
@@ -171,7 +171,7 @@ url {
     ("{a: \"b\"\nc: \"d\"}" . (dict (record "a" (string-pat "b")) (record "c" (string-pat "d"))))))
 
 (test-opam-syntax
-  "parse-conditions" condition
+  "parse-conditions" conditional
   '(("" . #f)
     ("{}" . #f)
     ("{build}" . (condition-var "build"))
-- 
2.33.0

[bug#51091] [PATCH v2] guix: opam: Do not fail when refreshing.

[Ludovic Courtès]

Hi!

Julien Lepiller <julien@lepiller.eu> skribis:

> Thanks for the reviews! I've managed to use conditions, which was not
> easy because I didn't notice there was already a "condition" defined,
> and that lead to weird error messages. Attached is the updated patch.

Oh, I see.

> From 0cadac6c3dabea8b986cd59d97c84beaf7a33325 Mon Sep 17 00:00:00 2001
> Message-Id: <0cadac6c3dabea8b986cd59d97c84beaf7a33325.1634350078.git.julien@lepiller.eu>
> From: Julien Lepiller <julien@lepiller.eu>
> Date: Fri, 8 Oct 2021 04:58:27 +0200
> Subject: [PATCH] import: opam: Do not fail when refreshing.
>
> Because we throw an error when a package is not in the opam repository,
> the updater would crash when encountering a package that is not in opam
> but uses the ocaml-build-system, such as opam itself.  This catches the
> error and continues without updating said package, and lets us update
> the rest of the packages.
>
> * guix/scripts/import/opam.scm (guix-import-opam): Catch not-found
> condition and leave.
> * guix/import/opam.scm (&opam-not-found-error): New condition type.
> (opam-fetch): Raise condition instead of leaving.
> (latest-release): Catch not-found condition and warn.
> (conditional): Rename from `condition'.
> * tests/opam.scm (parse-conditions): Change accordingly.

LGTM, thanks!

Ludo’.

[bug#51091] [PATCH v3] guix: opam: Do not fail when refreshing.

[Julien Lepiller]

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

Le Mon, 18 Oct 2021 10:39:04 +0200,
Ludovic Courtès <ludo@gnu.org> a écrit :

> Hi!
> 
> Julien Lepiller <julien@lepiller.eu> skribis:
> 
> > Thanks for the reviews! I've managed to use conditions, which was
> > not easy because I didn't notice there was already a "condition"
> > defined, and that lead to weird error messages. Attached is the
> > updated patch.  
> 
> Oh, I see.
> 
> > From 0cadac6c3dabea8b986cd59d97c84beaf7a33325 Mon Sep 17 00:00:00
> > 2001 Message-Id:
> > <0cadac6c3dabea8b986cd59d97c84beaf7a33325.1634350078.git.julien@lepiller.eu>
> > From: Julien Lepiller <julien@lepiller.eu> Date: Fri, 8 Oct 2021
> > 04:58:27 +0200 Subject: [PATCH] import: opam: Do not fail when
> > refreshing.
> >
> > Because we throw an error when a package is not in the opam
> > repository, the updater would crash when encountering a package
> > that is not in opam but uses the ocaml-build-system, such as opam
> > itself.  This catches the error and continues without updating said
> > package, and lets us update the rest of the packages.
> >
> > * guix/scripts/import/opam.scm (guix-import-opam): Catch not-found
> > condition and leave.
> > * guix/import/opam.scm (&opam-not-found-error): New condition type.
> > (opam-fetch): Raise condition instead of leaving.
> > (latest-release): Catch not-found condition and warn.
> > (conditional): Rename from `condition'.
> > * tests/opam.scm (parse-conditions): Change accordingly.  
> 
> LGTM, thanks!
> 
> Ludo’.

I forgot to remove the catch #t around the whole body of the function.
I noticed that guard* was raising &non-continuable so I tried to fix it
by using raise-continuable from (ice-9 exceptions). Is this the correct
solution?

[-- Attachment #2: 0001-import-opam-Do-not-fail-when-refreshing.patch --]
[-- Type: text/x-patch, Size: 7502 bytes --]

From a60e2ca645f4f8a6da72111d047f8cbb41ebe3e6 Mon Sep 17 00:00:00 2001
Message-Id: <a60e2ca645f4f8a6da72111d047f8cbb41ebe3e6.1637284512.git.julien@lepiller.eu>
From: Julien Lepiller <julien@lepiller.eu>
Date: Fri, 8 Oct 2021 04:58:27 +0200
Subject: [PATCH] import: opam: Do not fail when refreshing.

Because we throw an error when a package is not in the opam repository,
the updater would crash when encountering a package that is not in opam
but uses the ocaml-build-system, such as opam itself.  This catches the
error and continues without updating said package, and lets us update
the rest of the packages.

* guix/scripts/import/opam.scm (guix-import-opam): Catch not-found
condition and leave.
* guix/import/opam.scm (&opam-not-found-error): New condition type.
(opam-fetch): Raise condition instead of leaving.
(latest-release): Catch not-found condition and warn.
(conditional): Rename from `condition'.
* tests/opam.scm (parse-conditions): Change accordingly.
---
 guix/import/opam.scm         | 23 ++++++++++++++++++-----
 guix/scripts/import/opam.scm | 31 +++++++++++++++++--------------
 tests/opam.scm               |  2 +-
 3 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/guix/import/opam.scm b/guix/import/opam.scm
index fe13d29f03..aeeab541c1 100644
--- a/guix/import/opam.scm
+++ b/guix/import/opam.scm
@@ -20,6 +20,7 @@
 ;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
 
 (define-module (guix import opam)
+  #:use-module (ice-9 exceptions)
   #:use-module (ice-9 ftw)
   #:use-module (ice-9 match)
   #:use-module (ice-9 peg)
@@ -30,6 +31,8 @@ (define-module (guix import opam)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-2)
   #:use-module ((srfi srfi-26) #:select (cut))
+  #:use-module (srfi srfi-34)
+  #:use-module (srfi srfi-35)
   #:use-module ((web uri) #:select (string->uri uri->string))
   #:use-module ((guix build utils) #:select (dump-port find-files mkdir-p))
   #:use-module (guix build-system)
@@ -47,12 +50,16 @@ (define-module (guix import opam)
             opam-recursive-import
             %opam-updater
 
+            &opam-not-found-error
+            opam-not-found-error?
+            opam-not-found-error-package
+
             ;; The following patterns are exported for testing purposes.
             string-pat
             multiline-string
             list-pat
             dict
-            condition))
+            conditional))
 
 ;; Define a PEG parser for the opam format
 (define-peg-pattern comment none (and "#" (* COMMCHR) "\n"))
@@ -85,7 +92,7 @@ (define-peg-pattern group-pat all
                     (and (or conditional-value ground-value) (* SP) (ignore "&") (* SP)
                          (or group-pat conditional-value ground-value)))
 (define-peg-pattern ground-value body (and (or multiline-string string-pat choice-pat list-pat var) (* SP)))
-(define-peg-pattern conditional-value all (and ground-value (* SP) condition))
+(define-peg-pattern conditional-value all (and ground-value (* SP) conditional))
 (define-peg-pattern string-pat all (and QUOTE (* STRCHR) QUOTE))
 (define-peg-pattern list-pat all (and (ignore "[") (* SP) (* (and value (* SP))) (ignore "]")))
 (define-peg-pattern var all (+ (or (range #\a #\z) "-")))
@@ -96,7 +103,7 @@ (define-peg-pattern multiline-string all
                          QUOTE QUOTE QUOTE))
 (define-peg-pattern dict all (and (ignore "{") (* SP) records (* SP) (ignore "}")))
 
-(define-peg-pattern condition body (and (ignore "{") condition-form (ignore "}")))
+(define-peg-pattern conditional body (and (ignore "{") condition-form (ignore "}")))
 
 (define-peg-pattern condition-form body
                     (and
@@ -310,6 +317,10 @@ (define (dependency-list->inputs lst)
      (list dependency (list 'unquote (string->symbol dependency))))
    (ocaml-names->guix-names lst)))
 
+(define-condition-type &opam-not-found-error &error
+  opam-not-found-error?
+  (package opam-not-found-error-package))
+
 (define* (opam-fetch name #:optional (repositories-specs '("opam")))
   (or (fold (lambda (repository others)
               (match (find-latest-version name repository)
@@ -318,7 +329,7 @@ (define* (opam-fetch name #:optional (repositories-specs '("opam")))
                 (_ others)))
             #f
             (filter-map get-opam-repository repositories-specs))
-      (leave (G_ "package '~a' not found~%") name)))
+      (raise-continuable (condition (&opam-not-found-error (package name))))))
 
 (define* (opam->guix-package name #:key (repo '()) version)
   "Import OPAM package NAME from REPOSITORIES (a list of names, URLs or local
@@ -410,7 +421,9 @@ (define (opam-package? package)
 (define (latest-release package)
   "Return an <upstream-source> for the latest release of PACKAGE."
   (and-let* ((opam-name (guix-package->opam-name package))
-             (opam-file (opam-fetch opam-name))
+             (opam-file (guard* (c ((opam-not-found-error? c)
+                                    #f))
+                          (opam-fetch opam-name)))
              (version (assoc-ref opam-file "version"))
              (opam-content (assoc-ref opam-file "metadata"))
              (url-dict (metadata-ref opam-content "url"))
diff --git a/guix/scripts/import/opam.scm b/guix/scripts/import/opam.scm
index 834ac34cb0..043d05d717 100644
--- a/guix/scripts/import/opam.scm
+++ b/guix/scripts/import/opam.scm
@@ -93,20 +93,23 @@ (define (guix-import-opam . args)
                            (reverse opts))))
     (match args
       ((package-name)
-       (if (assoc-ref opts 'recursive)
-           ;; Recursive import
-           (map (match-lambda
-                  ((and ('package ('name name) . rest) pkg)
-                   `(define-public ,(string->symbol name)
-                      ,pkg))
-                  (_ #f))
-                (opam-recursive-import package-name #:repo repo))
-           ;; Single import
-           (let ((sexp (opam->guix-package package-name #:repo repo)))
-             (unless sexp
-               (leave (G_ "failed to download meta-data for package '~a'~%")
-                      package-name))
-             sexp)))
+       (guard* (c ((opam-not-found-error? c)
+                  (leave (G_ "package '~a' not found~%")
+                         (opam-not-found-error-package c))))
+         (if (assoc-ref opts 'recursive)
+             ;; Recursive import
+             (map (match-lambda
+                    ((and ('package ('name name) . rest) pkg)
+                     `(define-public ,(string->symbol name)
+                        ,pkg))
+                    (_ #f))
+                  (opam-recursive-import package-name #:repo repo))
+             ;; Single import
+             (let ((sexp (opam->guix-package package-name #:repo repo)))
+               (unless sexp
+                 (leave (G_ "failed to download meta-data for package '~a'~%")
+                        package-name))
+               sexp))))
       (()
        (leave (G_ "too few arguments~%")))
       ((many ...)
diff --git a/tests/opam.scm b/tests/opam.scm
index 31b4ea41ff..96b748bcd9 100644
--- a/tests/opam.scm
+++ b/tests/opam.scm
@@ -171,7 +171,7 @@ (define (test-opam-syntax name pattern test-cases)
     ("{a: \"b\"\nc: \"d\"}" . (dict (record "a" (string-pat "b")) (record "c" (string-pat "d"))))))
 
 (test-opam-syntax
-  "parse-conditions" condition
+  "parse-conditions" conditional
   '(("" . #f)
     ("{}" . #f)
     ("{build}" . (condition-var "build"))
-- 
2.33.1

[bug#51091] [PATCH v3] guix: opam: Do not fail when refreshing.

[zimoun]

Hi,

On Fri, 19 Nov 2021 at 02:17, Julien Lepiller <julien@lepiller.eu> wrote:

> I forgot to remove the catch #t around the whole body of the function.
> I noticed that guard* was raising &non-continuable so I tried to fix it
> by using raise-continuable from (ice-9 exceptions). Is this the correct
> solution?

I think <http://issues.guix.gnu.org/51888> is simpler and fix the same thing.


Cheers,
simon

[bug#51091] [PATCH v3] guix: opam: Do not fail when refreshing.

[Ludovic Courtès]

Hi,

Julien Lepiller <julien@lepiller.eu> skribis:

> I forgot to remove the catch #t around the whole body of the function.
> I noticed that guard* was raising &non-continuable so I tried to fix it
> by using raise-continuable from (ice-9 exceptions). Is this the correct
> solution?

I suppose, though I’m not sure why it needs to be continuable: you could
just catch the exception and move on to the next package?

> From a60e2ca645f4f8a6da72111d047f8cbb41ebe3e6 Mon Sep 17 00:00:00 2001
> Message-Id: <a60e2ca645f4f8a6da72111d047f8cbb41ebe3e6.1637284512.git.julien@lepiller.eu>
> From: Julien Lepiller <julien@lepiller.eu>
> Date: Fri, 8 Oct 2021 04:58:27 +0200
> Subject: [PATCH] import: opam: Do not fail when refreshing.
>
> Because we throw an error when a package is not in the opam repository,
> the updater would crash when encountering a package that is not in opam
> but uses the ocaml-build-system, such as opam itself.  This catches the
> error and continues without updating said package, and lets us update
> the rest of the packages.
>
> * guix/scripts/import/opam.scm (guix-import-opam): Catch not-found
> condition and leave.
> * guix/import/opam.scm (&opam-not-found-error): New condition type.
> (opam-fetch): Raise condition instead of leaving.
> (latest-release): Catch not-found condition and warn.
> (conditional): Rename from `condition'.
> * tests/opam.scm (parse-conditions): Change accordingly.

[...]

zimoun <zimon.toutoune@gmail.com> skribis:

> I think <http://issues.guix.gnu.org/51888> is simpler and fix the same thing.

I’m fine either way.  I think there’s value longer-term in having
structured exceptions in importers, though.

Julien: your call!

Thanks,
Ludo’.

[bug#51091] [PATCH v3] guix: opam: Do not fail when refreshing.

[zimoun]

Hi Ludo and Julien,

On Fri, 19 Nov 2021 at 10:17, Ludovic Courtès <ludo@gnu.org> wrote:

> I’m fine either way.  I think there’s value longer-term in having
> structured exceptions in importers, though.

Yes, I agree.  For instance, as discussed after this old quick fix
[1].  Then I did some pair programming with jeko to have a v2.  The
idea is to have a common exception mechanism for all the importers.
Instead of case per case and scattered implementation through various
importers (proposed patch for opam, another example
guix/import/elpa.scm using &message etc.).  All the importers should
share the same interface for handling exceptions and errors.  Well, it
has never seen the light because time flies so fast and because at one
point, one needs to sit down and unknot all; personally, I have been a
bit lazy for completing the task. :-)

1: <http://issues.guix.gnu.org/issue/45984#9>

> Julien: your call!

I agree, as mentioned [2]. :-)

2: <http://issues.guix.gnu.org/issue/51888#3>


Cheers,
simon

[bug#51091] [PATCH v3] guix: opam: Do not fail when refreshing.

[Julien Lepiller]

Le 19 novembre 2021 04:16:32 GMT-05:00, "Ludovic Courtès" <ludo@gnu.org> a écrit :
>Hi,
>
>Julien Lepiller <julien@lepiller.eu> skribis:
>
>> I forgot to remove the catch #t around the whole body of the function.
>> I noticed that guard* was raising &non-continuable so I tried to fix it
>> by using raise-continuable from (ice-9 exceptions). Is this the correct
>> solution?
>
>I suppose, though I’m not sure why it needs to be continuable: you could
>just catch the exception and move on to the next package?

I don't understand how to catch the exception though, unless you mean wrap everything with catch #t, which kinda defeats the purpose of having a condition in the first pjace. guard* raises &non-continuable unless the condition is continuable, or I'm missing something in the way I use it. I have no idea what a continuable exception is, so let me just push the other patch.

(guard* (c ((opam-error? c) #f)))
  (raise (condition (&opam-error …))))

Doesn't return #f as I expect, but raises &non-continuable.

>
>> From a60e2ca645f4f8a6da72111d047f8cbb41ebe3e6 Mon Sep 17 00:00:00 2001
>> Message-Id: <a60e2ca645f4f8a6da72111d047f8cbb41ebe3e6.1637284512.git.julien@lepiller.eu>
>> From: Julien Lepiller <julien@lepiller.eu>
>> Date: Fri, 8 Oct 2021 04:58:27 +0200
>> Subject: [PATCH] import: opam: Do not fail when refreshing.
>>
>> Because we throw an error when a package is not in the opam repository,
>> the updater would crash when encountering a package that is not in opam
>> but uses the ocaml-build-system, such as opam itself.  This catches the
>> error and continues without updating said package, and lets us update
>> the rest of the packages.
>>
>> * guix/scripts/import/opam.scm (guix-import-opam): Catch not-found
>> condition and leave.
>> * guix/import/opam.scm (&opam-not-found-error): New condition type.
>> (opam-fetch): Raise condition instead of leaving.
>> (latest-release): Catch not-found condition and warn.
>> (conditional): Rename from `condition'.
>> * tests/opam.scm (parse-conditions): Change accordingly.
>
>[...]
>
>zimoun <zimon.toutoune@gmail.com> skribis:
>
>> I think <http://issues.guix.gnu.org/51888> is simpler and fix the same thing.
>
>I’m fine either way.  I think there’s value longer-term in having
>structured exceptions in importers, though.
>
>Julien: your call!

Hopefully someone smarter than me can figure it out. I'll push the other patch, although I don't like the double warning in the updater.

>
>Thanks,
>Ludo’.

[bug#51091] [PATCH v3] guix: opam: Do not fail when refreshing.

[zimoun]

Hi Julien,

On Fri, 19 Nov 2021 at 12:21, Julien Lepiller <julien@lepiller.eu> wrote:

> >> I forgot to remove the catch #t around the whole body of the function.
> >> I noticed that guard* was raising &non-continuable so I tried to fix it
> >> by using raise-continuable from (ice-9 exceptions). Is this the correct
> >> solution?
> >
> >I suppose, though I’m not sure why it needs to be continuable: you could
> >just catch the exception and move on to the next package?
>
> I don't understand how to catch the exception though, unless you mean wrap everything with catch #t, which kinda defeats the purpose of having a condition in the first pjace. guard* raises &non-continuable unless the condition is continuable, or I'm missing something in the way I use it. I have no idea what a continuable exception is, so let me just push the other patch.
>
> (guard* (c ((opam-error? c) #f)))
>   (raise (condition (&opam-error …))))
>
> Doesn't return #f as I expect, but raises &non-continuable.

I sympathize and I had / is still having hard time with similar use
cases.  That's one of the reasons (among my laziness :-)) that [1] is
not fixed yet. :-)

1: <1: <http://issues.guix.gnu.org/issue/45984>



> Hopefully someone smarter than me can figure it out. I'll push the other patch, although I don't like the double warning in the updater.

I agree.  And move all G_ strings to guix/scripts/ is a good idea, IMHO.
Well, I do not know. :-)

(I secretly hoped that you would be the smarter than me person fixing
the recursive importers. ;-))


Cheers,
simon