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)

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

[Konstantin Kharlamov]

On Fri, 2024-12-27 at 12:01 -0500, Stefan Monnier wrote:
> > `%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!

This is not fair: despite me actively answering on my patch, you took
my changes, just changed one function call to another, and rewritten
the author like if it were you who were writing and debugging the code
and the issue rather than me.