bug#75065: Upon archive download failure print the original error

bug#75065: Upon archive download failure print the original error

[Konstantin Kharlamov]

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

I was recently helping out a new Emacs user with package installation¹,
and I found an interesting thing: if you put to `package-archives` a
URL without `https` prefix, download will fail. Long story short, the
reason turns out that `package-archives` also supports local paths,
which the URL being considered as. However, Emacs never prints a
message about that, even though such message exists in the code.
Instead it just says that download failed, leaving a user wondering
why.

That happens because (package--download-and-read-archives) ignores the
exception message, and always just prints generic failure message.

This code fixes this, so now the actual failure message will be
correctly shown.

1:
https://emacs.stackexchange.com/questions/82828/is-installing-deadgrep-fron-melpa-still-possible/82829#82829

[-- Attachment #2: 1.patch --]
[-- Type: text/x-patch, Size: 1390 bytes --]

From fb4685238726a79599f6388318916d2962da93ae Mon Sep 17 00:00:00 2001
From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
Date: Tue, 24 Dec 2024 18:16:41 +0300
Subject: [PATCH] Upon archive download failure print the original error

* lisp/emacs-lisp/package.el (package--download-and-read-archives):
upon catching exception, use the exception message as part of the
error to provide more context about the failure.
---
 lisp/emacs-lisp/package.el | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 5f785071ea3..cb81efc71f0 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -1829,10 +1829,10 @@ Populate `package-archive-contents' with the result.
 If optional argument ASYNC is non-nil, perform the downloads
 asynchronously."
   (dolist (archive package-archives)
-    (condition-case-unless-debug nil
+    (condition-case-unless-debug err
         (package--download-one-archive archive "archive-contents" async)
-      (error (message "Failed to download `%s' archive."
-               (car archive))))))
+      (error (message "Failed to download `%s' archive. Error: %S"
+               (car archive) (cdr err))))))
 
 (defvar package-refresh-contents-hook (list #'package--download-and-read-archives)
   "List of functions to call to refresh the package archive.
-- 
2.47.1

bug#75065: Upon archive download failure print the original error

[Eli Zaretskii]

> From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> Date: Tue, 24 Dec 2024 18:25:30 +0300
> 
> I was recently helping out a new Emacs user with package installation¹,
> and I found an interesting thing: if you put to `package-archives` a
> URL without `https` prefix, download will fail. Long story short, the
> reason turns out that `package-archives` also supports local paths,
> which the URL being considered as. However, Emacs never prints a
> message about that, even though such message exists in the code.
> Instead it just says that download failed, leaving a user wondering
> why.
> 
> That happens because (package--download-and-read-archives) ignores the
> exception message, and always just prints generic failure message.
> 
> This code fixes this, so now the actual failure message will be
> correctly shown.
> 
> 1:
> https://emacs.stackexchange.com/questions/82828/is-installing-deadgrep-fron-melpa-still-possible/82829#82829
> 
> 
> From fb4685238726a79599f6388318916d2962da93ae Mon Sep 17 00:00:00 2001
> From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> Date: Tue, 24 Dec 2024 18:16:41 +0300
> Subject: [PATCH] Upon archive download failure print the original error
> 
> * lisp/emacs-lisp/package.el (package--download-and-read-archives):
> upon catching exception, use the exception message as part of the
> error to provide more context about the failure.
> ---
>  lisp/emacs-lisp/package.el | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index 5f785071ea3..cb81efc71f0 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -1829,10 +1829,10 @@ Populate `package-archive-contents' with the result.
>  If optional argument ASYNC is non-nil, perform the downloads
>  asynchronously."
>    (dolist (archive package-archives)
> -    (condition-case-unless-debug nil
> +    (condition-case-unless-debug err
>          (package--download-one-archive archive "archive-contents" async)
> -      (error (message "Failed to download `%s' archive."
> -               (car archive))))))
> +      (error (message "Failed to download `%s' archive. Error: %S"
> +               (car archive) (cdr err))))))
>  
>  (defvar package-refresh-contents-hook (list #'package--download-and-read-archives)
>    "List of functions to call to refresh the package archive.
> -- 
> 2.47.1

Thanks.

Stefan and Philip, is this okay to install?

bug#75065: Upon archive download failure print the original error

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
>> Date: Tue, 24 Dec 2024 18:25:30 +0300
>> 
>> I was recently helping out a new Emacs user with package installation¹,
>> and I found an interesting thing: if you put to `package-archives` a
>> URL without `https` prefix, download will fail. Long story short, the
>> reason turns out that `package-archives` also supports local paths,
>> which the URL being considered as. However, Emacs never prints a
>> message about that, even though such message exists in the code.
>> Instead it just says that download failed, leaving a user wondering
>> why.
>> 
>> That happens because (package--download-and-read-archives) ignores the
>> exception message, and always just prints generic failure message.
>> 
>> This code fixes this, so now the actual failure message will be
>> correctly shown.
>> 
>> 1:
>> https://emacs.stackexchange.com/questions/82828/is-installing-deadgrep-fron-melpa-still-possible/82829#82829
>> 
>> 
>> From fb4685238726a79599f6388318916d2962da93ae Mon Sep 17 00:00:00 2001
>> From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
>> Date: Tue, 24 Dec 2024 18:16:41 +0300
>> Subject: [PATCH] Upon archive download failure print the original error
>> 
>> * lisp/emacs-lisp/package.el (package--download-and-read-archives):
>> upon catching exception, use the exception message as part of the
>> error to provide more context about the failure.
>> ---
>>  lisp/emacs-lisp/package.el | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
>> index 5f785071ea3..cb81efc71f0 100644
>> --- a/lisp/emacs-lisp/package.el
>> +++ b/lisp/emacs-lisp/package.el
>> @@ -1829,10 +1829,10 @@ Populate `package-archive-contents' with the result.
>>  If optional argument ASYNC is non-nil, perform the downloads
>>  asynchronously."
>>    (dolist (archive package-archives)
>> -    (condition-case-unless-debug nil
>> +    (condition-case-unless-debug err
>>          (package--download-one-archive archive "archive-contents" async)
>> -      (error (message "Failed to download `%s' archive."
>> -               (car archive))))))
>> +      (error (message "Failed to download `%s' archive. Error: %S"
>> +               (car archive) (cdr err))))))
>>  
>>  (defvar package-refresh-contents-hook (list #'package--download-and-read-archives)
>>    "List of functions to call to refresh the package archive.
>> -- 
>> 2.47.1
>
> Thanks.
>
> Stefan and Philip, is this okay to install?

It seems harmless, I am just uncertain if we should prefer %S or %s to
format the error message.

bug#75065: Upon archive download failure print the original error

[Konstantin Kharlamov]

On Thu, 2024-12-26 at 18:13 +0000, Philip Kaludercic wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > > From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> > > Date: Tue, 24 Dec 2024 18:25:30 +0300
> > > 
> > > I was recently helping out a new Emacs user with package
> > > installation¹,
> > > and I found an interesting thing: if you put to `package-
> > > archives` a
> > > URL without `https` prefix, download will fail. Long story short,
> > > the
> > > reason turns out that `package-archives` also supports local
> > > paths,
> > > which the URL being considered as. However, Emacs never prints a
> > > message about that, even though such message exists in the code.
> > > Instead it just says that download failed, leaving a user
> > > wondering
> > > why.
> > > 
> > > That happens because (package--download-and-read-archives)
> > > ignores the
> > > exception message, and always just prints generic failure
> > > message.
> > > 
> > > This code fixes this, so now the actual failure message will be
> > > correctly shown.
> > > 
> > > 1:
> > > https://emacs.stackexchange.com/questions/82828/is-installing-deadgrep-fron-melpa-still-possible/82829#82829
> > > 
> > > 
> > > From fb4685238726a79599f6388318916d2962da93ae Mon Sep 17 00:00:00
> > > 2001
> > > From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
> > > Date: Tue, 24 Dec 2024 18:16:41 +0300
> > > Subject: [PATCH] Upon archive download failure print the original
> > > error
> > > 
> > > * lisp/emacs-lisp/package.el (package--download-and-read-
> > > archives):
> > > upon catching exception, use the exception message as part of the
> > > error to provide more context about the failure.
> > > ---
> > >  lisp/emacs-lisp/package.el | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-
> > > lisp/package.el
> > > index 5f785071ea3..cb81efc71f0 100644
> > > --- a/lisp/emacs-lisp/package.el
> > > +++ b/lisp/emacs-lisp/package.el
> > > @@ -1829,10 +1829,10 @@ Populate `package-archive-contents' with
> > > the result.
> > >  If optional argument ASYNC is non-nil, perform the downloads
> > >  asynchronously."
> > >    (dolist (archive package-archives)
> > > -    (condition-case-unless-debug nil
> > > +    (condition-case-unless-debug err
> > >          (package--download-one-archive archive "archive-
> > > contents" async)
> > > -      (error (message "Failed to download `%s' archive."
> > > -               (car archive))))))
> > > +      (error (message "Failed to download `%s' archive. Error:
> > > %S"
> > > +               (car archive) (cdr err))))))
> > >  
> > >  (defvar package-refresh-contents-hook (list #'package--download-
> > > and-read-archives)
> > >    "List of functions to call to refresh the package archive.
> > > -- 
> > > 2.47.1
> > 
> > Thanks.
> > 
> > Stefan and Philip, is this okay to install?
> 
> It seems harmless, I am just uncertain if we should prefer %S or %s
> to
> format the error message.

The reason I used %S is that 2x lines below there's similar
construction with (error …) and it's using %S, so the usage here goes
in line with the other code.

That said, I have no preference, I can change it to %s, even in both
locations if you want.

bug#75065: Upon archive download failure print the original error

[Philip Kaludercic]

Konstantin Kharlamov <Hi-Angel@yandex.ru> writes:

> On Thu, 2024-12-26 at 18:13 +0000, Philip Kaludercic wrote:
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > > From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
>> > > Date: Tue, 24 Dec 2024 18:25:30 +0300
>> > > 
>> > > I was recently helping out a new Emacs user with package
>> > > installation¹,
>> > > and I found an interesting thing: if you put to `package-
>> > > archives` a
>> > > URL without `https` prefix, download will fail. Long story short,
>> > > the
>> > > reason turns out that `package-archives` also supports local
>> > > paths,
>> > > which the URL being considered as. However, Emacs never prints a
>> > > message about that, even though such message exists in the code.
>> > > Instead it just says that download failed, leaving a user
>> > > wondering
>> > > why.
>> > > 
>> > > That happens because (package--download-and-read-archives)
>> > > ignores the
>> > > exception message, and always just prints generic failure
>> > > message.
>> > > 
>> > > This code fixes this, so now the actual failure message will be
>> > > correctly shown.
>> > > 
>> > > 1:
>> > > https://emacs.stackexchange.com/questions/82828/is-installing-deadgrep-fron-melpa-still-possible/82829#82829
>> > > 
>> > > 
>> > > From fb4685238726a79599f6388318916d2962da93ae Mon Sep 17 00:00:00
>> > > 2001
>> > > From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
>> > > Date: Tue, 24 Dec 2024 18:16:41 +0300
>> > > Subject: [PATCH] Upon archive download failure print the original
>> > > error
>> > > 
>> > > * lisp/emacs-lisp/package.el (package--download-and-read-
>> > > archives):
>> > > upon catching exception, use the exception message as part of the
>> > > error to provide more context about the failure.
>> > > ---
>> > >  lisp/emacs-lisp/package.el | 6 +++---
>> > >  1 file changed, 3 insertions(+), 3 deletions(-)
>> > > 
>> > > diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-
>> > > lisp/package.el
>> > > index 5f785071ea3..cb81efc71f0 100644
>> > > --- a/lisp/emacs-lisp/package.el
>> > > +++ b/lisp/emacs-lisp/package.el
>> > > @@ -1829,10 +1829,10 @@ Populate `package-archive-contents' with
>> > > the result.
>> > >  If optional argument ASYNC is non-nil, perform the downloads
>> > >  asynchronously."
>> > >    (dolist (archive package-archives)
>> > > -    (condition-case-unless-debug nil
>> > > +    (condition-case-unless-debug err
>> > >          (package--download-one-archive archive "archive-
>> > > contents" async)
>> > > -      (error (message "Failed to download `%s' archive."
>> > > -               (car archive))))))
>> > > +      (error (message "Failed to download `%s' archive. Error:
>> > > %S"
>> > > +               (car archive) (cdr err))))))
>> > >  
>> > >  (defvar package-refresh-contents-hook (list #'package--download-
>> > > and-read-archives)
>> > >    "List of functions to call to refresh the package archive.
>> > > -- 
>> > > 2.47.1
>> > 
>> > Thanks.
>> > 
>> > Stefan and Philip, is this okay to install?
>> 
>> It seems harmless, I am just uncertain if we should prefer %S or %s
>> to
>> format the error message.
>
> The reason I used %S is that 2x lines below there's similar
> construction with (error …) and it's using %S, so the usage here goes
> in line with the other code.
>
> That said, I have no preference, I can change it to %s, even in both
> locations if you want.

In that case I think %S is preferable, and the patch is fine with me.

bug#75065: Upon archive download failure print the original error

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>>    (dolist (archive package-archives)
>>> -    (condition-case-unless-debug nil
>>> +    (condition-case-unless-debug err
>>>          (package--download-one-archive archive "archive-contents" async)
>>> -      (error (message "Failed to download `%s' archive."
>>> -               (car archive))))))
>>> +      (error (message "Failed to download `%s' archive. Error: %S"
>>> +               (car archive) (cdr err))))))
>>>  
>> Stefan and Philip, is this okay to install?

I agree with the idea behind the patch, but printing just `(cdr err)`
doesn't seem right, it should print the whole of `err`.

> It seems harmless, I am just uncertain if we should prefer %S or %s to
> format the error message.

`%s` to print `err` or `(cdr err)` would be wrong, since `%s` is for use
with strings rather than lists.  IOW, IMO, it should be either

    ...%S" ... err)

or

    ...%s" ... (error-message-string err))

where the first is a bit more "debugging/developer" friendly and the second
is a bit more "user" friendly.


        Stefan

bug#75065: Upon archive download failure print the original error

[Konstantin Kharlamov]

On Thu, 2024-12-26 at 14:17 -0500, Stefan Monnier wrote:
> > > >    (dolist (archive package-archives)
> > > > -    (condition-case-unless-debug nil
> > > > +    (condition-case-unless-debug err
> > > >          (package--download-one-archive archive "archive-
> > > > contents" async)
> > > > -      (error (message "Failed to download `%s' archive."
> > > > -               (car archive))))))
> > > > +      (error (message "Failed to download `%s' archive. Error:
> > > > %S"
> > > > +               (car archive) (cdr err))))))
> > > >
> > > Stefan and Philip, is this okay to install?
>
> I agree with the idea behind the patch, but printing just `(cdr err)`
> doesn't seem right, it should print the whole of `err`.

The `car` seems to just contain word error. Here's how both compare:

• current patch with `(cdr err)`:
    Failed to download ‘melpa’ archive. Error: ("Location melpa.org/packages/ is not a url nor an absolute file name")

• suggested change with `err`:
    Failed to download ‘melpa’ archive. Error: (error "Location melpa.org/packages/ is not a url nor an absolute file name")

I can of course remove the word `Error` in the second case. My question
then is: will `(car err)` always be the word "error"? Or may there be
another content?

> > It seems harmless, I am just uncertain if we should prefer %S or %s
> > to
> > format the error message.
>
> `%s` to print `err` or `(cdr err)` would be wrong, since `%s` is for
> use
> with strings rather than lists.  IOW, IMO, it should be either
>
>     ...%S" ... err)
>
> or
>
>     ...%s" ... (error-message-string err))
>
> where the first is a bit more "debugging/developer" friendly and the
> second
> is a bit more "user" friendly.

In my tests there seems to be no difference in the output between %s
and %S. I would presume doing `(message "%s" '(a b))` would result in
error as the param isn't a string, but it works.

bug#75065: Upon archive download failure print the original error

[Stefan Kangas]

Konstantin Kharlamov <Hi-Angel@yandex.ru> writes:

> On Thu, 2024-12-26 at 14:17 -0500, Stefan Monnier wrote:
>> > > >    (dolist (archive package-archives)
>> > > > -    (condition-case-unless-debug nil
>> > > > +    (condition-case-unless-debug err
>> > > >          (package--download-one-archive archive "archive-
>> > > > contents" async)
>> > > > -      (error (message "Failed to download `%s' archive."
>> > > > -               (car archive))))))
>> > > > +      (error (message "Failed to download `%s' archive. Error:
>> > > > %S"
>> > > > +               (car archive) (cdr err))))))
>> > > >
>> > > Stefan and Philip, is this okay to install?
>>
>> I agree with the idea behind the patch, but printing just `(cdr err)`
>> doesn't seem right, it should print the whole of `err`.
>
> The `car` seems to just contain word error. Here's how both compare:
>
> • current patch with `(cdr err)`:
>     Failed to download ‘melpa’ archive. Error: ("Location melpa.org/packages/ is not a url nor an absolute file name")
>
> • suggested change with `err`:
>     Failed to download ‘melpa’ archive. Error: (error "Location melpa.org/packages/ is not a url nor an absolute file name")

The "Error:" part is redundant, so I think it could be shortened to
something like:

    Failed to download ‘melpa’ archive: Location melpa.org/packages/ is
    not a url nor an absolute file name

bug#75065: Upon archive download failure print the original error

[Konstantin Kharlamov]

On Thu, 2024-12-26 at 20:32 +0000, Stefan Kangas wrote:
> Konstantin Kharlamov <Hi-Angel@yandex.ru> writes:
> 
> > On Thu, 2024-12-26 at 14:17 -0500, Stefan Monnier wrote:
> > > > > >    (dolist (archive package-archives)
> > > > > > -    (condition-case-unless-debug nil
> > > > > > +    (condition-case-unless-debug err
> > > > > >          (package--download-one-archive archive "archive-
> > > > > > contents" async)
> > > > > > -      (error (message "Failed to download `%s' archive."
> > > > > > -               (car archive))))))
> > > > > > +      (error (message "Failed to download `%s' archive.
> > > > > > Error:
> > > > > > %S"
> > > > > > +               (car archive) (cdr err))))))
> > > > > > 
> > > > > Stefan and Philip, is this okay to install?
> > > 
> > > I agree with the idea behind the patch, but printing just `(cdr
> > > err)`
> > > doesn't seem right, it should print the whole of `err`.
> > 
> > The `car` seems to just contain word error. Here's how both
> > compare:
> > 
> > • current patch with `(cdr err)`:
> >     Failed to download ‘melpa’ archive. Error: ("Location
> > melpa.org/packages/ is not a url nor an absolute file name")
> > 
> > • suggested change with `err`:
> >     Failed to download ‘melpa’ archive. Error: (error "Location
> > melpa.org/packages/ is not a url nor an absolute file name")
> 
> The "Error:" part is redundant, so I think it could be shortened to
> something like:
> 
>     Failed to download ‘melpa’ archive: Location melpa.org/packages/
> is
>     not a url nor an absolute file name

Please note that in the above examples everything after the word
`Error: ` is taken from the exception message, which includes the
parentheses and (if I follow Stefan's advice) the downcased word
"error".

The reason in my patch I used `Error:` is because I have no control
over the text that will follow, so I need to make sure it's clear that
what follows is an error message. Hence my question to Stefan above: if
the downcase word `error` will always be there, then removing `Error:`
and replacing `(cdr err)` with `err` will works just as well.

bug#75065: Upon archive download failure print the original error

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> The `car` seems to just contain word error. Here's how both compare:
>
> • current patch with `(cdr err)`:
>     Failed to download ‘melpa’ archive. Error: ("Location melpa.org/packages/ is not a url nor an absolute file name")
>
> • suggested change with `err`:
>     Failed to download ‘melpa’ archive. Error: (error "Location melpa.org/packages/ is not a url nor an absolute file name")
>
> I can of course remove the word `Error` in the second case.
> My question then is: will `(car err)` always be the word "error"? Or
> may there be another content?

The shape and content of `err` depends on the actual error that caused
the download to fail.  `(car err)` contains the error "type", which can
be `error` but can also be more specific such as
`wrong-number-of-arguments`, `file-error`, ...

>> > It seems harmless, I am just uncertain if we should prefer %S or %s
>> > to format the error message.
>>
>> `%s` to print `err` or `(cdr err)` would be wrong, since `%s` is for
>> use with strings rather than lists.  IOW, IMO, it should be either
>>
>>     ...%S" ... err)
>>
>> or
>>
>>     ...%s" ... (error-message-string err))
>>
>> where the first is a bit more "debugging/developer" friendly and the
>> second is a bit more "user" friendly.
>
> In my tests there seems to be no difference in the output between %s
> and %S.

Try (format "%S" '(a "b")) vs (format "%s" '(a "b"))

> I would presume doing `(message "%s" '(a b))` would result in
> error as the param isn't a string, but it works.

I agree that in an ideal world it should signal an error, but here
we are.


        Stefan

bug#75065: Upon archive download failure print the original error

[Konstantin Kharlamov]

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

On Thu, 2024-12-26 at 15:40 -0500, Stefan Monnier wrote:
> > The `car` seems to just contain word error. Here's how both
> > compare:
> > 
> > • current patch with `(cdr err)`:
> >     Failed to download ‘melpa’ archive. Error: ("Location
> > melpa.org/packages/ is not a url nor an absolute file name")
> > 
> > • suggested change with `err`:
> >     Failed to download ‘melpa’ archive. Error: (error "Location
> > melpa.org/packages/ is not a url nor an absolute file name")
> > 
> > I can of course remove the word `Error` in the second case.
> > My question then is: will `(car err)` always be the word "error"?
> > Or
> > may there be another content?
> 
> The shape and content of `err` depends on the actual error that
> caused
> the download to fail.  `(car err)` contains the error "type", which
> can
> be `error` but can also be more specific such as
> `wrong-number-of-arguments`, `file-error`, ...

Thank you, I see!

Please check the attached patch. With this change the error of
accessing https-less address is shown as follows:

	Failed to download ‘melpa’ archive: (error "Location melpa.org/packages/ is not a url nor an absolute file name")

[-- Attachment #2: 1.patch --]
[-- Type: text/x-patch, Size: 1377 bytes --]

From cea8962c423522e6afcc7abce4eb30280fe1027c Mon Sep 17 00:00:00 2001
From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
Date: Tue, 24 Dec 2024 18:16:41 +0300
Subject: [PATCH] Upon archive download failure print the original error

* lisp/emacs-lisp/package.el (package--download-and-read-archives):
upon catching exception, use the exception message as part of the
error to provide more context about the failure.
---
 lisp/emacs-lisp/package.el | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 5f785071ea3..81f0ac0162f 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -1829,10 +1829,10 @@ Populate `package-archive-contents' with the result.
 If optional argument ASYNC is non-nil, perform the downloads
 asynchronously."
   (dolist (archive package-archives)
-    (condition-case-unless-debug nil
+    (condition-case-unless-debug err
         (package--download-one-archive archive "archive-contents" async)
-      (error (message "Failed to download `%s' archive."
-               (car archive))))))
+      (error (message "Failed to download `%s' archive: %S"
+               (car archive) err)))))
 
 (defvar package-refresh-contents-hook (list #'package--download-and-read-archives)
   "List of functions to call to refresh the package archive.
-- 
2.47.1

bug#75065: Upon archive download failure print the original error

[Konstantin Kharlamov]

Oh, actually, let me fix the other place as well then…

On Thu, 2024-12-26 at 23:51 +0300, Konstantin Kharlamov wrote:
> On Thu, 2024-12-26 at 15:40 -0500, Stefan Monnier wrote:
> > > The `car` seems to just contain word error. Here's how both
> > > compare:
> > > 
> > > • current patch with `(cdr err)`:
> > >     Failed to download ‘melpa’ archive. Error: ("Location
> > > melpa.org/packages/ is not a url nor an absolute file name")
> > > 
> > > • suggested change with `err`:
> > >     Failed to download ‘melpa’ archive. Error: (error "Location
> > > melpa.org/packages/ is not a url nor an absolute file name")
> > > 
> > > I can of course remove the word `Error` in the second case.
> > > My question then is: will `(car err)` always be the word "error"?
> > > Or
> > > may there be another content?
> > 
> > The shape and content of `err` depends on the actual error that
> > caused
> > the download to fail.  `(car err)` contains the error "type", which
> > can
> > be `error` but can also be more specific such as
> > `wrong-number-of-arguments`, `file-error`, ...
> 
> Thank you, I see!
> 
> Please check the attached patch. With this change the error of
> accessing https-less address is shown as follows:
> 
> 	Failed to download ‘melpa’ archive: (error "Location
> melpa.org/packages/ is not a url nor an absolute file name")

bug#75065: Upon archive download failure print the original error

[Konstantin Kharlamov]

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

On Thu, 2024-12-26 at 23:53 +0300, Konstantin Kharlamov wrote:
> Oh, actually, let me fix the other place as well then…
> 
> On Thu, 2024-12-26 at 23:51 +0300, Konstantin Kharlamov wrote:
> > On Thu, 2024-12-26 at 15:40 -0500, Stefan Monnier wrote:
> > > > The `car` seems to just contain word error. Here's how both
> > > > compare:
> > > > 
> > > > • current patch with `(cdr err)`:
> > > >     Failed to download ‘melpa’ archive. Error: ("Location
> > > > melpa.org/packages/ is not a url nor an absolute file name")
> > > > 
> > > > • suggested change with `err`:
> > > >     Failed to download ‘melpa’ archive. Error: (error "Location
> > > > melpa.org/packages/ is not a url nor an absolute file name")
> > > > 
> > > > I can of course remove the word `Error` in the second case.
> > > > My question then is: will `(car err)` always be the word
> > > > "error"?
> > > > Or
> > > > may there be another content?
> > > 
> > > The shape and content of `err` depends on the actual error that
> > > caused
> > > the download to fail.  `(car err)` contains the error "type",
> > > which
> > > can
> > > be `error` but can also be more specific such as
> > > `wrong-number-of-arguments`, `file-error`, ...
> > 
> > Thank you, I see!
> > 
> > Please check the attached patch. With this change the error of
> > accessing https-less address is shown as follows:
> > 
> > 	Failed to download ‘melpa’ archive: (error "Location
> > melpa.org/packages/ is not a url nor an absolute file name")
> 

Done, please review 😊

[-- Attachment #2: 1.patch --]
[-- Type: text/x-patch, Size: 2574 bytes --]

From 642c934fb0cb1650ffd5b13127774d73c8a1af35 Mon Sep 17 00:00:00 2001
From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
Date: Tue, 24 Dec 2024 18:16:41 +0300
Subject: [PATCH 1/2] Upon archive download failure print the original error
 (Bug#75065)

* lisp/emacs-lisp/package.el (package--download-and-read-archives):
upon catching exception, use the exception message as part of the
error to provide more context about the failure.
---
 lisp/emacs-lisp/package.el | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 5f785071ea3..81f0ac0162f 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -1829,10 +1829,10 @@ Populate `package-archive-contents' with the result.
 If optional argument ASYNC is non-nil, perform the downloads
 asynchronously."
   (dolist (archive package-archives)
-    (condition-case-unless-debug nil
+    (condition-case-unless-debug err
         (package--download-one-archive archive "archive-contents" async)
-      (error (message "Failed to download `%s' archive."
-               (car archive))))))
+      (error (message "Failed to download `%s' archive: %S"
+               (car archive) err)))))
 
 (defvar package-refresh-contents-hook (list #'package--download-and-read-archives)
   "List of functions to call to refresh the package archive.
-- 
2.47.1


From cf8f2ca0915743027160dc516eea4fd565b62ffa Mon Sep 17 00:00:00 2001
From: Konstantin Kharlamov <Hi-Angel@yandex.ru>
Date: Thu, 26 Dec 2024 23:54:25 +0300
Subject: [PATCH 2/2] Upon keyring import fail print the full error (Bug#75065)

As discussed at Bug#75065, the `(car error)' contains the exception
type and it's better to print the full error content.

* lisp/emacs-lisp/package.el (package-refresh-contents): upon keyring
import failure print the full content.
---
 lisp/emacs-lisp/package.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 81f0ac0162f..e24db4567e1 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -1856,7 +1856,7 @@ downloads in the background."
     (when (and (package-check-signature) (file-exists-p default-keyring))
       (condition-case-unless-debug error
           (package-import-keyring default-keyring)
-        (error (message "Cannot import default keyring: %S" (cdr error))))))
+        (error (message "Cannot import default keyring: %S" error)))))
   (run-hook-with-args 'package-refresh-contents-hook async))
 
 \f
-- 
2.47.1

bug#75065: Upon archive download failure print the original error

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Philip Kaludercic <philipk@posteo.net>,  Eli Zaretskii <eliz@gnu.org>,
>   75065@debbugs.gnu.org
> Date: Thu, 26 Dec 2024 15:40:21 -0500
> 
> > The `car` seems to just contain word error. Here's how both compare:
> >
> > • current patch with `(cdr err)`:
> >     Failed to download ‘melpa’ archive. Error: ("Location melpa.org/packages/ is not a url nor an absolute file name")
> >
> > • suggested change with `err`:
> >     Failed to download ‘melpa’ archive. Error: (error "Location melpa.org/packages/ is not a url nor an absolute file name")
> >
> > I can of course remove the word `Error` in the second case.
> > My question then is: will `(car err)` always be the word "error"? Or
> > may there be another content?
> 
> The shape and content of `err` depends on the actual error that caused
> the download to fail.  `(car err)` contains the error "type", which can
> be `error` but can also be more specific such as
> `wrong-number-of-arguments`, `file-error`, ...

Yes, the car part is not redundant, it can include useful information
that we should not lose.

bug#75065: Upon archive download failure print the original error

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

> `%s` to print `err` or `(cdr err)` would be wrong, since `%s` is for use
> with strings rather than lists.  IOW, IMO, it should be either
>
>     ...%S" ... err)
>
> or
>
>     ...%s" ... (error-message-string err))
>
> where the first is a bit more "debugging/developer" friendly and the second
> is a bit more "user" friendly.

Revising the code of `package.el` I see that those errors&messages are
meant to be exposed to the user, so they should preferably use
`error-message-string`.

So I installed the patch below into `master`.
Thank you!
Closing,


        Stefan

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

commit a99e1cc745047977b0b7cbd25fe9df66e9c86a39
Author: Stefan Monnier <monnier@iro.umontreal.ca>
Date:   Fri Dec 27 11:58:30 2024 -0500

    (package--download-and-read-archives): Fix bug#75065
    
    * lisp/emacs-lisp/package.el (package--download-and-read-archives):
    Include the error info in the error message.
    Suggested by Konstantin Kharlamov <Hi-Angel@yandex.ru>.
    (package-refresh-contents, package-menu--perform-transaction):
    Use `error-message-string`.

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 5f785071ea3..bff45d57b07 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -1829,10 +1829,11 @@ package--download-and-read-archives
 If optional argument ASYNC is non-nil, perform the downloads
 asynchronously."
   (dolist (archive package-archives)
-    (condition-case-unless-debug nil
+    (condition-case-unless-debug err
         (package--download-one-archive archive "archive-contents" async)
-      (error (message "Failed to download `%s' archive."
-               (car archive))))))
+      (error (message "Failed to download `%s' archive: %s"
+                      (car archive)
+                      (error-message-string err))))))
 
 (defvar package-refresh-contents-hook (list #'package--download-and-read-archives)
   "List of functions to call to refresh the package archive.
@@ -1856,7 +1857,8 @@ package-refresh-contents
     (when (and (package-check-signature) (file-exists-p default-keyring))
       (condition-case-unless-debug error
           (package-import-keyring default-keyring)
-        (error (message "Cannot import default keyring: %S" (cdr error))))))
+        (error (message "Cannot import default keyring: %s"
+                        (error-message-string error))))))
   (run-hook-with-args 'package-refresh-contents-hook async))
 
 \f
@@ -3989,8 +3991,9 @@ package-menu--perform-transaction
               (package-delete elt nil 'nosave))
           (error
            (push (package-desc-full-name elt) errors)
-           (message "Error trying to delete `%s': %S"
-                    (package-desc-full-name elt) err)))))
+           (message "Error trying to delete `%s': %s"
+                    (package-desc-full-name elt)
+                    (error-message-string err))))))
     errors))
 
 (defun package--update-selected-packages (add remove)