bug#65649: [PATCH] package-vc: Continue installing package when documentation build fails

bug#65649: [PATCH] package-vc: Continue installing package when documentation build fails

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

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

In light of the recent fix regarding building documentation from
org-mode files, I think it makes sense to ensure that installation
continues even without documentation.

Joseph



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Continue-installing-package-when-documentation-build.patch --]
[-- Type: text/x-diff, Size: 3691 bytes --]

From 83c62d80b5199cee911e30c1f3bb03c5a585b689 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Wed, 30 Aug 2023 23:24:16 -0700
Subject: [PATCH] Continue installing package when documentation building fails

* lisp/emacs-lisp/package-vc.el (package-vc--build-documentation):
Demote errors to messages.
---
 lisp/emacs-lisp/package-vc.el | 61 ++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 30 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 747fe696204..bc1200c583f 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -409,36 +409,37 @@ prepared."
   "Build documentation for package PKG-DESC from documentation source in FILE.
 FILE can be an Org file, indicated by its \".org\" extension,
 otherwise it's assumed to be an Info file."
-  (let* ((pkg-name (package-desc-name pkg-desc))
-         (default-directory (package-desc-dir pkg-desc))
-         (docs-directory (file-name-directory (expand-file-name file)))
-         (output (expand-file-name (format "%s.info" pkg-name)))
-         clean-up)
-    (when (string-match-p "\\.org\\'" file)
-      (require 'ox)
-      (require 'ox-texinfo)
-      (with-temp-buffer
-        (insert-file-contents file)
-        (setq file (make-temp-file "ox-texinfo-"))
-        (let ((default-directory docs-directory))
-          (org-export-to-file 'texinfo file))
-        (setq clean-up t)))
-    (with-current-buffer (get-buffer-create " *package-vc doc*")
-      (erase-buffer)
-      (cond
-       ((/= 0 (call-process "makeinfo" nil t nil
-                            "-I" docs-directory
-                            "--no-split" file
-                            "-o" output))
-        (message "Failed to build manual %s, see buffer %S"
-                 file (buffer-name)))
-       ((/= 0 (call-process "install-info" nil t nil
-                            output (expand-file-name "dir")))
-        (message "Failed to install manual %s, see buffer %S"
-                 output (buffer-name)))
-       ((kill-buffer))))
-    (when clean-up
-      (delete-file file))))
+  (with-demoted-errors "package-vc: Could not build documentation: %s"
+    (let* ((pkg-name (package-desc-name pkg-desc))
+           (default-directory (package-desc-dir pkg-desc))
+           (docs-directory (file-name-directory (expand-file-name file)))
+           (output (expand-file-name (format "%s.info" pkg-name)))
+           clean-up)
+      (when (string-match-p "\\.org\\'" file)
+        (require 'ox)
+        (require 'ox-texinfo)
+        (with-temp-buffer
+          (insert-file-contents file)
+          (setq file (make-temp-file "ox-texinfo-"))
+          (let ((default-directory docs-directory))
+            (org-export-to-file 'texinfo file))
+          (setq clean-up t)))
+      (with-current-buffer (get-buffer-create " *package-vc doc*")
+        (erase-buffer)
+        (cond
+         ((/= 0 (call-process "makeinfo" nil t nil
+                              "-I" docs-directory
+                              "--no-split" file
+                              "-o" output))
+          (message "Failed to build manual %s, see buffer %S"
+                   file (buffer-name)))
+         ((/= 0 (call-process "install-info" nil t nil
+                              output (expand-file-name "dir")))
+          (message "Failed to install manual %s, see buffer %S"
+                   output (buffer-name)))
+         ((kill-buffer))))
+      (when clean-up
+        (delete-file file)))))
 
 (defun package-vc-install-dependencies (deps)
   "Install missing dependencies according to DEPS.
-- 
2.41.0

bug#65649: [PATCH] package-vc: Continue installing package when documentation build fails

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

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

Oops, the format string should be %S, not %s.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Continue-installing-package-when-documentation-build.patch --]
[-- Type: text/x-diff, Size: 3691 bytes --]

From 83c62d80b5199cee911e30c1f3bb03c5a585b689 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Wed, 30 Aug 2023 23:24:16 -0700
Subject: [PATCH] Continue installing package when documentation building fails

* lisp/emacs-lisp/package-vc.el (package-vc--build-documentation):
Demote errors to messages.
---
 lisp/emacs-lisp/package-vc.el | 61 ++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 30 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 747fe696204..bc1200c583f 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -409,36 +409,37 @@ prepared."
   "Build documentation for package PKG-DESC from documentation source in FILE.
 FILE can be an Org file, indicated by its \".org\" extension,
 otherwise it's assumed to be an Info file."
-  (let* ((pkg-name (package-desc-name pkg-desc))
-         (default-directory (package-desc-dir pkg-desc))
-         (docs-directory (file-name-directory (expand-file-name file)))
-         (output (expand-file-name (format "%s.info" pkg-name)))
-         clean-up)
-    (when (string-match-p "\\.org\\'" file)
-      (require 'ox)
-      (require 'ox-texinfo)
-      (with-temp-buffer
-        (insert-file-contents file)
-        (setq file (make-temp-file "ox-texinfo-"))
-        (let ((default-directory docs-directory))
-          (org-export-to-file 'texinfo file))
-        (setq clean-up t)))
-    (with-current-buffer (get-buffer-create " *package-vc doc*")
-      (erase-buffer)
-      (cond
-       ((/= 0 (call-process "makeinfo" nil t nil
-                            "-I" docs-directory
-                            "--no-split" file
-                            "-o" output))
-        (message "Failed to build manual %s, see buffer %S"
-                 file (buffer-name)))
-       ((/= 0 (call-process "install-info" nil t nil
-                            output (expand-file-name "dir")))
-        (message "Failed to install manual %s, see buffer %S"
-                 output (buffer-name)))
-       ((kill-buffer))))
-    (when clean-up
-      (delete-file file))))
+  (with-demoted-errors "package-vc: Could not build documentation: %S"
+    (let* ((pkg-name (package-desc-name pkg-desc))
+           (default-directory (package-desc-dir pkg-desc))
+           (docs-directory (file-name-directory (expand-file-name file)))
+           (output (expand-file-name (format "%s.info" pkg-name)))
+           clean-up)
+      (when (string-match-p "\\.org\\'" file)
+        (require 'ox)
+        (require 'ox-texinfo)
+        (with-temp-buffer
+          (insert-file-contents file)
+          (setq file (make-temp-file "ox-texinfo-"))
+          (let ((default-directory docs-directory))
+            (org-export-to-file 'texinfo file))
+          (setq clean-up t)))
+      (with-current-buffer (get-buffer-create " *package-vc doc*")
+        (erase-buffer)
+        (cond
+         ((/= 0 (call-process "makeinfo" nil t nil
+                              "-I" docs-directory
+                              "--no-split" file
+                              "-o" output))
+          (message "Failed to build manual %s, see buffer %S"
+                   file (buffer-name)))
+         ((/= 0 (call-process "install-info" nil t nil
+                              output (expand-file-name "dir")))
+          (message "Failed to install manual %s, see buffer %S"
+                   output (buffer-name)))
+         ((kill-buffer))))
+      (when clean-up
+        (delete-file file)))))
 
 (defun package-vc-install-dependencies (deps)
   "Install missing dependencies according to DEPS.
-- 
2.41.0

bug#65649: [PATCH] package-vc: Continue installing package when documentation build fails

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

It would be nice to report more information about the package with failed
documentation, but AFAICT with-demoted-errors does not allow its FORMAT
string to be a symbol whose value is a string.

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Oops, the format string should be %S, not %s.
>
> [2. text/x-diff; 0001-Continue-installing-package-when-documentation-build.patch]
> From 83c62d80b5199cee911e30c1f3bb03c5a585b689 Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Wed, 30 Aug 2023 23:24:16 -0700
> Subject: [PATCH] Continue installing package when documentation building fails
>
> * lisp/emacs-lisp/package-vc.el (package-vc--build-documentation):
> Demote errors to messages.
> ---
>  lisp/emacs-lisp/package-vc.el | 61 ++++++++++++++++++-----------------
>  1 file changed, 31 insertions(+), 30 deletions(-)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index 747fe696204..bc1200c583f 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -409,36 +409,37 @@ prepared."
>    "Build documentation for package PKG-DESC from documentation source in FILE.
>  FILE can be an Org file, indicated by its \".org\" extension,
>  otherwise it's assumed to be an Info file."
> -  (let* ((pkg-name (package-desc-name pkg-desc))
> -         (default-directory (package-desc-dir pkg-desc))
> -         (docs-directory (file-name-directory (expand-file-name file)))
> -         (output (expand-file-name (format "%s.info" pkg-name)))
> -         clean-up)
> -    (when (string-match-p "\\.org\\'" file)
> -      (require 'ox)
> -      (require 'ox-texinfo)
> -      (with-temp-buffer
> -        (insert-file-contents file)
> -        (setq file (make-temp-file "ox-texinfo-"))
> -        (let ((default-directory docs-directory))
> -          (org-export-to-file 'texinfo file))
> -        (setq clean-up t)))
> -    (with-current-buffer (get-buffer-create " *package-vc doc*")
> -      (erase-buffer)
> -      (cond
> -       ((/= 0 (call-process "makeinfo" nil t nil
> -                            "-I" docs-directory
> -                            "--no-split" file
> -                            "-o" output))
> -        (message "Failed to build manual %s, see buffer %S"
> -                 file (buffer-name)))
> -       ((/= 0 (call-process "install-info" nil t nil
> -                            output (expand-file-name "dir")))
> -        (message "Failed to install manual %s, see buffer %S"
> -                 output (buffer-name)))
> -       ((kill-buffer))))
> -    (when clean-up
> -      (delete-file file))))
> +  (with-demoted-errors "package-vc: Could not build documentation: %S"
> +    (let* ((pkg-name (package-desc-name pkg-desc))
> +           (default-directory (package-desc-dir pkg-desc))
> +           (docs-directory (file-name-directory (expand-file-name file)))
> +           (output (expand-file-name (format "%s.info" pkg-name)))
> +           clean-up)
> +      (when (string-match-p "\\.org\\'" file)
> +        (require 'ox)
> +        (require 'ox-texinfo)
> +        (with-temp-buffer
> +          (insert-file-contents file)
> +          (setq file (make-temp-file "ox-texinfo-"))
> +          (let ((default-directory docs-directory))
> +            (org-export-to-file 'texinfo file))
> +          (setq clean-up t)))
> +      (with-current-buffer (get-buffer-create " *package-vc doc*")
> +        (erase-buffer)
> +        (cond
> +         ((/= 0 (call-process "makeinfo" nil t nil
> +                              "-I" docs-directory
> +                              "--no-split" file
> +                              "-o" output))
> +          (message "Failed to build manual %s, see buffer %S"
> +                   file (buffer-name)))
> +         ((/= 0 (call-process "install-info" nil t nil
> +                              output (expand-file-name "dir")))
> +          (message "Failed to install manual %s, see buffer %S"
> +                   output (buffer-name)))
> +         ((kill-buffer))))
> +      (when clean-up
> +        (delete-file file)))))
>
>  (defun package-vc-install-dependencies (deps)
>    "Install missing dependencies according to DEPS.

bug#65649: [PATCH] package-vc: Continue installing package when documentation build fails

[Eli Zaretskii]

> Cc: Philip Kaludercic <philipk@posteo.net>
> Date: Wed, 30 Aug 2023 23:26:35 -0700
> From:  Joseph Turner via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> In light of the recent fix regarding building documentation from
> org-mode files, I think it makes sense to ensure that installation
> continues even without documentation.

I don't think we should do this silently.  The user should be asked
to confirm, at the very least.

bug#65649: [PATCH] package-vc: Continue installing package when documentation build fails

[Philip Kaludercic]

Btw, watch out when reporting a bug, you shouldn't CC people directly,
but add a X-Debbugs-CC header
(https://debbugs.gnu.org/Reporting.html#xcc), as otherwise I'd be
creating a new bug report when responding to your message.

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> It would be nice to report more information about the package with failed
> documentation, but AFAICT with-demoted-errors does not allow its FORMAT
> string to be a symbol whose value is a string.

What are the main errors we are concerned with?  I am a bit concerned
that `with-demoted-errors' is a too coarse approach and might be applied
to widely in your current patch.  It might be better to use a classical
condition-case and handle the right errors in the right place, or if
practicable try to detect if an error would occur before doing anything.

Also, the buffer *package-vc doc* should stay persistent and would
include error messages, that could also be used here.

> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> Oops, the format string should be %S, not %s.
>>
>> [2. text/x-diff; 0001-Continue-installing-package-when-documentation-build.patch]
>> From 83c62d80b5199cee911e30c1f3bb03c5a585b689 Mon Sep 17 00:00:00 2001
>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Date: Wed, 30 Aug 2023 23:24:16 -0700
>> Subject: [PATCH] Continue installing package when documentation building fails
>>
>> * lisp/emacs-lisp/package-vc.el (package-vc--build-documentation):
>> Demote errors to messages.
>> ---
>>  lisp/emacs-lisp/package-vc.el | 61 ++++++++++++++++++-----------------
>>  1 file changed, 31 insertions(+), 30 deletions(-)
>>
>> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
>> index 747fe696204..bc1200c583f 100644
>> --- a/lisp/emacs-lisp/package-vc.el
>> +++ b/lisp/emacs-lisp/package-vc.el
>> @@ -409,36 +409,37 @@ prepared."
>>    "Build documentation for package PKG-DESC from documentation source in FILE.
>>  FILE can be an Org file, indicated by its \".org\" extension,
>>  otherwise it's assumed to be an Info file."
>> -  (let* ((pkg-name (package-desc-name pkg-desc))
>> -         (default-directory (package-desc-dir pkg-desc))
>> -         (docs-directory (file-name-directory (expand-file-name file)))
>> -         (output (expand-file-name (format "%s.info" pkg-name)))
>> -         clean-up)
>> -    (when (string-match-p "\\.org\\'" file)
>> -      (require 'ox)
>> -      (require 'ox-texinfo)
>> -      (with-temp-buffer
>> -        (insert-file-contents file)
>> -        (setq file (make-temp-file "ox-texinfo-"))
>> -        (let ((default-directory docs-directory))
>> -          (org-export-to-file 'texinfo file))
>> -        (setq clean-up t)))
>> -    (with-current-buffer (get-buffer-create " *package-vc doc*")
>> -      (erase-buffer)
>> -      (cond
>> -       ((/= 0 (call-process "makeinfo" nil t nil
>> -                            "-I" docs-directory
>> -                            "--no-split" file
>> -                            "-o" output))
>> -        (message "Failed to build manual %s, see buffer %S"
>> -                 file (buffer-name)))
>> -       ((/= 0 (call-process "install-info" nil t nil
>> -                            output (expand-file-name "dir")))
>> -        (message "Failed to install manual %s, see buffer %S"
>> -                 output (buffer-name)))
>> -       ((kill-buffer))))
>> -    (when clean-up
>> -      (delete-file file))))
>> +  (with-demoted-errors "package-vc: Could not build documentation: %S"
>> +    (let* ((pkg-name (package-desc-name pkg-desc))
>> +           (default-directory (package-desc-dir pkg-desc))
>> +           (docs-directory (file-name-directory (expand-file-name file)))
>> +           (output (expand-file-name (format "%s.info" pkg-name)))
>> +           clean-up)
>> +      (when (string-match-p "\\.org\\'" file)
>> +        (require 'ox)
>> +        (require 'ox-texinfo)
>> +        (with-temp-buffer
>> +          (insert-file-contents file)
>> +          (setq file (make-temp-file "ox-texinfo-"))
>> +          (let ((default-directory docs-directory))
>> +            (org-export-to-file 'texinfo file))
>> +          (setq clean-up t)))
>> +      (with-current-buffer (get-buffer-create " *package-vc doc*")
>> +        (erase-buffer)
>> +        (cond
>> +         ((/= 0 (call-process "makeinfo" nil t nil
>> +                              "-I" docs-directory
>> +                              "--no-split" file
>> +                              "-o" output))
>> +          (message "Failed to build manual %s, see buffer %S"
>> +                   file (buffer-name)))
>> +         ((/= 0 (call-process "install-info" nil t nil
>> +                              output (expand-file-name "dir")))
>> +          (message "Failed to install manual %s, see buffer %S"
>> +                   output (buffer-name)))
>> +         ((kill-buffer))))
>> +      (when clean-up
>> +        (delete-file file)))))
>>
>>  (defun package-vc-install-dependencies (deps)
>>    "Install missing dependencies according to DEPS.

bug#65649: [PATCH] package-vc: Continue installing package when documentation build fails

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

Philip Kaludercic <philipk@posteo.net> writes:

> Btw, watch out when reporting a bug, you shouldn't CC people directly,
> but add a X-Debbugs-CC header
> (https://debbugs.gnu.org/Reporting.html#xcc), as otherwise I'd be
> creating a new bug report when responding to your message.

Good to know! Thank you! Is it okay to CC people when responding to a
bug report email (e.g. if I CC'd Eli in this email)?

> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> It would be nice to report more information about the package with failed
>> documentation, but AFAICT with-demoted-errors does not allow its FORMAT
>> string to be a symbol whose value is a string.
>
> What are the main errors we are concerned with?  I am a bit concerned
> that `with-demoted-errors' is a too coarse approach and might be applied
> to widely in your current patch.  It might be better to use a classical
> condition-case and handle the right errors in the right place, or if
> practicable try to detect if an error would occur before doing anything.

I don't know what kinds of errors to expect, which is why I went with
the coarse approach.  Under what circumstances would we want to stop
package installation entirely when documentation can't be built?

> Also, the buffer *package-vc doc* should stay persistent and would
> include error messages, that could also be used here.

Like this?

(with-demoted-errors "package-vc: Could not build documentation. See *package-vc doc* for details" ... )

bug#65649: [PATCH] package-vc: Continue installing package when documentation build fails

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

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Also, the buffer *package-vc doc* should stay persistent and would
>> include error messages, that could also be used here.
>
> Like this?
>
> (with-demoted-errors "package-vc: Could not build documentation. See *package-vc doc* for details" ... )

Actually, that would not have handled the error related to relative org
#+include statements, since that error happened above this line:

(with-current-buffer (get-buffer-create " *package-vc doc*")

bug#65649: [PATCH] package-vc: Continue installing package when documentation build fails

[Philip Kaludercic]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Btw, watch out when reporting a bug, you shouldn't CC people directly,
>> but add a X-Debbugs-CC header
>> (https://debbugs.gnu.org/Reporting.html#xcc), as otherwise I'd be
>> creating a new bug report when responding to your message.
>
> Good to know! Thank you! Is it okay to CC people when responding to a
> bug report email (e.g. if I CC'd Eli in this email)?

Yes, as soon as one is sending a message to [bugnumber]@debbugs.gnu.org,
everything is fine.  The issue if you Cc me directly, is that if I don't
watch out, I'll send my response to bug-gnu-emacs@gnu.org, and thus
create a new bug.

>> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>>
>>> It would be nice to report more information about the package with failed
>>> documentation, but AFAICT with-demoted-errors does not allow its FORMAT
>>> string to be a symbol whose value is a string.
>>
>> What are the main errors we are concerned with?  I am a bit concerned
>> that `with-demoted-errors' is a too coarse approach and might be applied
>> to widely in your current patch.  It might be better to use a classical
>> condition-case and handle the right errors in the right place, or if
>> practicable try to detect if an error would occur before doing anything.
>
> I don't know what kinds of errors to expect, which is why I went with
> the coarse approach.  Under what circumstances would we want to stop
> package installation entirely when documentation can't be built?

I am not sure we want that behaviour at all actually.  Just because
there is a typo in the documentation, doesn't mean the package is
unusable.  The user should be able to install the package, be notified
about the error -- if the have the time, they can fix it and send the
maintainer a patch resolving the issue for everyone.  Likewise, if the
user updates a package, it wouldn't make sense to ignore everything or
worse still revert the update due to a small mistake in the
documentation file.

>> Also, the buffer *package-vc doc* should stay persistent and would
>> include error messages, that could also be used here.
>
> Like this?
>
> (with-demoted-errors "package-vc: Could not build documentation. See *package-vc doc* for details" ... )

Yes, that is what I had in mind.

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Joseph Turner <joseph@breatheoutbreathe.in> writes:
>
>> Philip Kaludercic <philipk@posteo.net> writes:
>>
>>> Also, the buffer *package-vc doc* should stay persistent and would
>>> include error messages, that could also be used here.
>>
>> Like this?
>>
>> (with-demoted-errors "package-vc: Could not build documentation. See *package-vc doc* for details" ... )
>
> Actually, that would not have handled the error related to relative org
> #+include statements, since that error happened above this line:
>
> (with-current-buffer (get-buffer-create " *package-vc doc*")

There is no reason why we cannot already create and use the buffer
earlier, to log org-related bugs.  One has to be careful when emptying
the buffer, but it might make sense to have a separate buffer for each
package, especially when updating multiple packages at once...

bug#65649: [PATCH] package-vc: Continue installing package when documentation build fails

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

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


Philip Kaludercic <philipk@posteo.net> writes:

> Yes, as soon as one is sending a message to [bugnumber]@debbugs.gnu.org,
> everything is fine.  The issue if you Cc me directly, is that if I don't
> watch out, I'll send my response to bug-gnu-emacs@gnu.org, and thus
> create a new bug.

Thanks, that's clear now.

> I am not sure we want that behaviour at all actually.  Just because
> there is a typo in the documentation, doesn't mean the package is
> unusable.  The user should be able to install the package, be notified
> about the error -- if the have the time, they can fix it and send the
> maintainer a patch resolving the issue for everyone.  Likewise, if the
> user updates a package, it wouldn't make sense to ignore everything or
> worse still revert the update due to a small mistake in the
> documentation file.
>
> ...
>
> There is no reason why we cannot already create and use the buffer
> earlier, to log org-related bugs.  One has to be careful when emptying
> the buffer, but it might make sense to have a separate buffer for each
> package, especially when updating multiple packages at once...

Please see attached patches.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Include-package-name-in-package-vc-documentation-log.patch --]
[-- Type: text/x-diff, Size: 1031 bytes --]

From aa356f561ab7861f463d3024f574fc71d45cb00b Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Wed, 30 Aug 2023 23:24:16 -0700
Subject: [PATCH 1/2] Include package name in package-vc documentation log
 buffer name

* lisp/emacs-lisp/package-vc.el (package-vc--build-documentation):
---
 lisp/emacs-lisp/package-vc.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index 747fe696204..ea8d9ecf488 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -423,7 +423,7 @@ otherwise it's assumed to be an Info file."
         (let ((default-directory docs-directory))
           (org-export-to-file 'texinfo file))
         (setq clean-up t)))
-    (with-current-buffer (get-buffer-create " *package-vc doc*")
+    (with-current-buffer (get-buffer-create (format " *package-vc doc: %s*" pkg-name))
       (erase-buffer)
       (cond
        ((/= 0 (call-process "makeinfo" nil t nil
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Log-org-export-errors-to-package-vc-doc-buffer.patch --]
[-- Type: text/x-diff, Size: 3467 bytes --]

From 010dabfbba8ebeb7f7193482ae2ffc7ec5b694e3 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Fri, 1 Sep 2023 16:22:45 -0700
Subject: [PATCH 2/2] Log org export errors to package-vc doc buffer

* lisp/emacs-lisp/package-vc.el (package-vc--build-documentation):
Wrap the org-export logic in condition-case, allowing package
installation to continue while preserving error messages.
---
 lisp/emacs-lisp/package-vc.el | 52 +++++++++++++++++++----------------
 1 file changed, 29 insertions(+), 23 deletions(-)

diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
index ea8d9ecf488..a8393cb7e75 100644
--- a/lisp/emacs-lisp/package-vc.el
+++ b/lisp/emacs-lisp/package-vc.el
@@ -413,30 +413,36 @@ otherwise it's assumed to be an Info file."
          (default-directory (package-desc-dir pkg-desc))
          (docs-directory (file-name-directory (expand-file-name file)))
          (output (expand-file-name (format "%s.info" pkg-name)))
+         (log-buffer (get-buffer-create (format " *package-vc doc: %s*" pkg-name)))
          clean-up)
-    (when (string-match-p "\\.org\\'" file)
-      (require 'ox)
-      (require 'ox-texinfo)
-      (with-temp-buffer
-        (insert-file-contents file)
-        (setq file (make-temp-file "ox-texinfo-"))
-        (let ((default-directory docs-directory))
-          (org-export-to-file 'texinfo file))
-        (setq clean-up t)))
-    (with-current-buffer (get-buffer-create (format " *package-vc doc: %s*" pkg-name))
-      (erase-buffer)
-      (cond
-       ((/= 0 (call-process "makeinfo" nil t nil
-                            "-I" docs-directory
-                            "--no-split" file
-                            "-o" output))
-        (message "Failed to build manual %s, see buffer %S"
-                 file (buffer-name)))
-       ((/= 0 (call-process "install-info" nil t nil
-                            output (expand-file-name "dir")))
-        (message "Failed to install manual %s, see buffer %S"
-                 output (buffer-name)))
-       ((kill-buffer))))
+    (with-current-buffer log-buffer
+      (erase-buffer))
+    (condition-case err
+        (progn
+          (when (string-match-p "\\.org\\'" file)
+            (require 'ox)
+            (require 'ox-texinfo)
+            (with-temp-buffer
+              (insert-file-contents file)
+              (setq file (make-temp-file "ox-texinfo-"))
+              (let ((default-directory docs-directory))
+                (org-export-to-file 'texinfo file))
+              (setq clean-up t)))
+          (cond
+           ((/= 0 (call-process "makeinfo" nil log-buffer nil
+                                "-I" docs-directory
+                                "--no-split" file
+                                "-o" output))
+            (message "Failed to build manual %s, see buffer %S"
+                     file (buffer-name)))
+           ((/= 0 (call-process "install-info" nil log-buffer nil
+                                output (expand-file-name "dir")))
+            (message "Failed to install manual %s, see buffer %S"
+                     output (buffer-name)))
+           ((kill-buffer log-buffer))))
+      (error (with-current-buffer log-buffer
+               (insert (error-message-string err)))
+             (message "Failed to export org manual for %s, see buffer %S" pkg-name log-buffer)))
     (when clean-up
       (delete-file file))))
 
-- 
2.41.0

bug#65649: [PATCH] package-vc: Continue installing package when documentation build fails

[Philip Kaludercic]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> Yes, as soon as one is sending a message to [bugnumber]@debbugs.gnu.org,
>> everything is fine.  The issue if you Cc me directly, is that if I don't
>> watch out, I'll send my response to bug-gnu-emacs@gnu.org, and thus
>> create a new bug.
>
> Thanks, that's clear now.
>
>> I am not sure we want that behaviour at all actually.  Just because
>> there is a typo in the documentation, doesn't mean the package is
>> unusable.  The user should be able to install the package, be notified
>> about the error -- if the have the time, they can fix it and send the
>> maintainer a patch resolving the issue for everyone.  Likewise, if the
>> user updates a package, it wouldn't make sense to ignore everything or
>> worse still revert the update due to a small mistake in the
>> documentation file.
>>
>> ...
>>
>> There is no reason why we cannot already create and use the buffer
>> earlier, to log org-related bugs.  One has to be careful when emptying
>> the buffer, but it might make sense to have a separate buffer for each
>> package, especially when updating multiple packages at once...
>
> Please see attached patches.
>
>>From aa356f561ab7861f463d3024f574fc71d45cb00b Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Wed, 30 Aug 2023 23:24:16 -0700
> Subject: [PATCH 1/2] Include package name in package-vc documentation log
>  buffer name
>
> * lisp/emacs-lisp/package-vc.el (package-vc--build-documentation):
> ---
>  lisp/emacs-lisp/package-vc.el | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index 747fe696204..ea8d9ecf488 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -423,7 +423,7 @@ otherwise it's assumed to be an Info file."
>          (let ((default-directory docs-directory))
>            (org-export-to-file 'texinfo file))
>          (setq clean-up t)))
> -    (with-current-buffer (get-buffer-create " *package-vc doc*")
> +    (with-current-buffer (get-buffer-create (format " *package-vc doc: %s*" pkg-name))
>        (erase-buffer)
>        (cond
>         ((/= 0 (call-process "makeinfo" nil t nil
> -- 
> 2.41.0

This looks good, thanks!

>
>>From 010dabfbba8ebeb7f7193482ae2ffc7ec5b694e3 Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Fri, 1 Sep 2023 16:22:45 -0700
> Subject: [PATCH 2/2] Log org export errors to package-vc doc buffer
>
> * lisp/emacs-lisp/package-vc.el (package-vc--build-documentation):
> Wrap the org-export logic in condition-case, allowing package
> installation to continue while preserving error messages.
> ---
>  lisp/emacs-lisp/package-vc.el | 52 +++++++++++++++++++----------------
>  1 file changed, 29 insertions(+), 23 deletions(-)
>
> diff --git a/lisp/emacs-lisp/package-vc.el b/lisp/emacs-lisp/package-vc.el
> index ea8d9ecf488..a8393cb7e75 100644
> --- a/lisp/emacs-lisp/package-vc.el
> +++ b/lisp/emacs-lisp/package-vc.el
> @@ -413,30 +413,36 @@ otherwise it's assumed to be an Info file."
>           (default-directory (package-desc-dir pkg-desc))
>           (docs-directory (file-name-directory (expand-file-name file)))
>           (output (expand-file-name (format "%s.info" pkg-name)))
> +         (log-buffer (get-buffer-create (format " *package-vc doc: %s*" pkg-name)))
>           clean-up)
> -    (when (string-match-p "\\.org\\'" file)
> -      (require 'ox)
> -      (require 'ox-texinfo)
> -      (with-temp-buffer
> -        (insert-file-contents file)
> -        (setq file (make-temp-file "ox-texinfo-"))
> -        (let ((default-directory docs-directory))
> -          (org-export-to-file 'texinfo file))
> -        (setq clean-up t)))
> -    (with-current-buffer (get-buffer-create (format " *package-vc doc: %s*" pkg-name))
> -      (erase-buffer)
> -      (cond
> -       ((/= 0 (call-process "makeinfo" nil t nil
> -                            "-I" docs-directory
> -                            "--no-split" file
> -                            "-o" output))
> -        (message "Failed to build manual %s, see buffer %S"
> -                 file (buffer-name)))
> -       ((/= 0 (call-process "install-info" nil t nil
> -                            output (expand-file-name "dir")))
> -        (message "Failed to install manual %s, see buffer %S"
> -                 output (buffer-name)))
> -       ((kill-buffer))))
> +    (with-current-buffer log-buffer
> +      (erase-buffer))
> +    (condition-case err
> +        (progn
> +          (when (string-match-p "\\.org\\'" file)
> +            (require 'ox)
> +            (require 'ox-texinfo)
> +            (with-temp-buffer
> +              (insert-file-contents file)
> +              (setq file (make-temp-file "ox-texinfo-"))
> +              (let ((default-directory docs-directory))
> +                (org-export-to-file 'texinfo file))
> +              (setq clean-up t)))
> +          (cond
> +           ((/= 0 (call-process "makeinfo" nil log-buffer nil
> +                                "-I" docs-directory
> +                                "--no-split" file
> +                                "-o" output))
> +            (message "Failed to build manual %s, see buffer %S"
> +                     file (buffer-name)))
> +           ((/= 0 (call-process "install-info" nil log-buffer nil
> +                                output (expand-file-name "dir")))
> +            (message "Failed to install manual %s, see buffer %S"
> +                     output (buffer-name)))
> +           ((kill-buffer log-buffer))))
> +      (error (with-current-buffer log-buffer
> +               (insert (error-message-string err)))
> +             (message "Failed to export org manual for %s, see buffer %S" pkg-name log-buffer)))

I think it would be better to wrap only the org code in the
`condition-case' body, ideally with a more specific error type (if that
doesn't exist, that is something we could mention to the Org
maintainers).

>      (when clean-up
>        (delete-file file))))

bug#65649: [PATCH] package-vc: Continue installing package when documentation build fails

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

Philip Kaludercic <philipk@posteo.net> writes:

> I think it would be better to wrap only the org code in the
> `condition-case' body, ideally with a more specific error type (if that
> doesn't exist, that is something we could mention to the Org
> maintainers).

The reason I wrapped both the org-export and makeinfo logic in
condition-case is so that the makeinfo logic does not run when the
org-export logic fails. Is there a better way?

We could let-bind a flag like failed-org-export-p, then set it to t in
the condition case handler, then only run makeinfo when
failed-org-export-p is nil. Seems a bit ugly.

The only define-error in org-mode that's relevant to org-export is not
general enough:

(define-error 'org-link-broken "Unable to resolve link; aborting")

bug#65649: [PATCH] package-vc: Continue installing package when documentation build fails

[Philip Kaludercic]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> I think it would be better to wrap only the org code in the
>> `condition-case' body, ideally with a more specific error type (if that
>> doesn't exist, that is something we could mention to the Org
>> maintainers).
>
> The reason I wrapped both the org-export and makeinfo logic in
> condition-case is so that the makeinfo logic does not run when the
> org-export logic fails. Is there a better way?
>
> We could let-bind a flag like failed-org-export-p, then set it to t in
> the condition case handler, then only run makeinfo when
> failed-org-export-p is nil. Seems a bit ugly.

Oh right, never-mind then, I guess this is the simplest solution.

> The only define-error in org-mode that's relevant to org-export is not
> general enough:
>
> (define-error 'org-link-broken "Unable to resolve link; aborting")

Then this should be fine.  I'll apply the changes the master.  Thanks.