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))))