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

  • Done
  • quality assurance status badge
Details
4 participants
  • Julien Lepiller
  • Ludovic Courtès
  • Xinglu Chen
  • zimoun
Owner
unassigned
Submitted by
Julien Lepiller
Severity
normal
J
J
Julien Lepiller wrote on 8 Oct 2021 05:03
(address . guix-patches@gnu.org)
20211008050310.407d6b23@tachikoma.lepiller.eu
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(-)

Toggle diff (36 lines)
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
X
X
Xinglu Chen wrote on 9 Oct 2021 11:39
87y272lepg.fsf@yoctocell.xyz
On Fri, Oct 08 2021, Julien Lepiller wrote:

Toggle quote (41 lines)
> 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?
-----BEGIN PGP SIGNATURE-----

iQJJBAEBCAAzFiEEAVhh4yyK5+SEykIzrPUJmaL7XHkFAmFhY0sVHHB1YmxpY0B5
b2N0b2NlbGwueHl6AAoJEKz1CZmi+1x556cP/2YgvaXx7HRVhnHBGhlST4urdQNM
x4qcJn5KPXUF+S2UFjwRrRspaLLLvIZcyM2wyeoWBtqkeWC5QaJyZdnYXM1iKrEz
Dt9Y3GJ31RmPkn0Ssv2B+Qq+AhHp3t0KjEmBaheqdT8PE9Pnr9FQCdpLicMEd3Gf
7F6+k06cLjZZqDABofq+NWFA359XLFZyEEBkRyanqIvcRFFAJ68VZlToqPbK9U2F
hnkPuFyea3ki4LrdqN61sy512aapkCQ2pmBeWe1Mxkk7itCXmVevj4LzlgzwEjvn
0NiWz3XrEJLWBKosYl53vR8fvBw5iWHn87mbAzVSey8028ACXalZWvgdWajeVL45
9KzPdmD3jzI+Ba2h/IOIidhGphBUTnGz1w6OlpchGpE3pTK692xgpSDebX1DFBq1
MCxsizihpikJ2vBfMt5UzHXjOBpazebqsvrM5UElfL1IWLO76BG3DOaKVCa9TWus
YwbVK8CjpzawFv6KVwohWF37Efw1rdGOdD0vD0r2UtdYC4/ho5Hpm9uvVbUgDZSU
G2qwwntBi6jHWKjAebZbL12rCP3S37iUiPCsCGiwQ4XePNG71ksHJ/TI+pw7Jzlf
CW5SYxSn2dmA/TghbuEBCcYdsXdqOkM97lu6cBxLjZ4jAkfZDCx6Nvl2FS8dcZjT
L0FLfozTXLK1/VOp
=M5U1
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 14 Oct 2021 15:01
Re: bug#51091: [PATCH] guix: opam: Do not fail when refreshing.
(name . Xinglu Chen)(address . public@yoctocell.xyz)
871r4nrc9t.fsf_-_@gnu.org
Hi,

Xinglu Chen <public@yoctocell.xyz> skribis:

Toggle quote (2 lines)
> On Fri, Oct 08 2021, Julien Lepiller wrote:

[...]

Toggle quote (35 lines)
>> 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’.
J
J
Julien Lepiller wrote on 16 Oct 2021 04:10
Re: bug#51091: [PATCH v2] guix: opam: Do not fail when refreshing.
(name . Ludovic Courtès)(address . ludo@gnu.org)
20211016041009.5b4ddff0@tachikoma.lepiller.eu
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.
L
L
Ludovic Courtès wrote on 18 Oct 2021 10:39
(name . Julien Lepiller)(address . julien@lepiller.eu)
87sfwybuc7.fsf@gnu.org
Hi!

Julien Lepiller <julien@lepiller.eu> skribis:

Toggle quote (4 lines)
> 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.

Toggle quote (20 lines)
> 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’.
J
J
Julien Lepiller wrote on 19 Nov 2021 02:16
Re: bug#51091: [PATCH v3] guix: opam: Do not fail when refreshing.
(name . Ludovic Courtès)(address . ludo@gnu.org)
20211119021646.03e85542@tachikoma.lepiller.eu
Le Mon, 18 Oct 2021 10:39:04 +0200,
Ludovic Courtès <ludo@gnu.org> a écrit :

Toggle quote (36 lines)
> 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?
Z
Z
zimoun wrote on 19 Nov 2021 02:43
Re: [bug#51091] [PATCH v3] guix: opam: Do not fail when refreshing.
(name . Julien Lepiller)(address . julien@lepiller.eu)
CAJ3okZ0xWkyjdobSWsfmJwDR71j+K4Hp6wFHubPifrOaseW5Pg@mail.gmail.com
Hi,

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

Toggle quote (5 lines)
> 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
L
L
Ludovic Courtès wrote on 19 Nov 2021 10:16
Re: bug#51091: [PATCH v3] guix: opam: Do not fail when refreshing.
(name . Julien Lepiller)(address . julien@lepiller.eu)
87czmwo68f.fsf@gnu.org
Hi,

Julien Lepiller <julien@lepiller.eu> skribis:

Toggle quote (5 lines)
> 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?

Toggle quote (20 lines)
> 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:

Toggle quote (2 lines)
> 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’.
Z
Z
zimoun wrote on 19 Nov 2021 10:40
Re: [bug#51091] [PATCH v3] guix: opam: Do not fail when refreshing.
(name . Ludovic Courtès)(address . ludo@gnu.org)
CAJ3okZ2k6XtZzb1hVFMBG5LpY8iS7rLXnkoDSUynAQAccH7mrg@mail.gmail.com
Hi Ludo and Julien,

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

Toggle quote (3 lines)
> 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. :-)


Toggle quote (2 lines)
> Julien: your call!

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



Cheers,
simon
J
J
Julien Lepiller wrote on 19 Nov 2021 12:19
Re: bug#51091: [PATCH v3] guix: opam: Do not fail when refreshing.
(name . Ludovic Courtès)(address . ludo@gnu.org)
83620617-04DC-472D-80B8-300044832A37@lepiller.eu
Le 19 novembre 2021 04:16:32 GMT-05:00, "Ludovic Courtès" <ludo@gnu.org> a écrit :
Toggle quote (12 lines)
>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.

Toggle quote (32 lines)
>
>> 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.

Toggle quote (3 lines)
>
>Thanks,
>Ludo’.
Z
Z
zimoun wrote on 19 Nov 2021 12:30
Re: [bug#51091] [PATCH v3] guix: opam: Do not fail when refreshing.
(name . Julien Lepiller)(address . julien@lepiller.eu)
CAJ3okZ0OMY3PgoJ3DdiyQNFo3ows8M42CisrkcFZVO77+-jh3g@mail.gmail.com
Hi Julien,

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

Toggle quote (15 lines)
> >> 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. :-)




Toggle quote (2 lines)
> 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
J
J
Julien Lepiller wrote on 19 Nov 2021 20:49
(address . 51091-close@debbugs.gnu.org)
C9F1C28B-4DAA-47A0-B183-FB754C8CDDC6@lepiller.eu
Forgot to send to the address that actually closes the issue ^^"



-------- Courriel d’origine --------
De : Julien Lepiller <julien@lepiller.eu>
Envoyé : 19 novembre 2021 13:13:08 GMT-05:00
À : zimoun <zimon.toutoune@gmail.com>
Cc : "Ludovic Courtès" <ludo@gnu.org>, Xinglu Chen <public@yoctocell.xyz>
Objet : Re: [bug#51091] [PATCH v3] guix: opam: Do not fail when refreshing.

So, since I pushed Zimoun's patch, I'm closing this one. I received some help on IRC this morning so I should be able to come up with something more generic for all importers at some point. Zimoun if you want to work on that, I'd be happy to collaborate :)

Le 19 novembre 2021 06:30:20 GMT-05:00, zimoun <zimon.toutoune@gmail.com> a écrit :
Toggle quote (38 lines)
>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
Attachment: file
?