bug#30063: 26.0.90; Silent fail with `rst-compile-pdf-preview'

bug#30063: 26.0.90; Silent fail with `rst-compile-pdf-preview'

[Simen Heggestøyl]

If either of the `xpdf' or `rst2pdf' programs are missing,
`rst-compile-pdf-preview' fails without leaving any message. It would be
useful if the command informed the user what's stopping it from working.

-- Simen

bug#30063: 26.0.90; Silent fail with `rst-compile-pdf-preview'

[Simen Heggestøyl]

[-- Attachment #1.1: Type: text/plain, Size: 55 bytes --]

How about something like the attached patch?

-- Simen

[-- Attachment #1.2: Type: text/html, Size: 116 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Warn-about-missing-executables-in-RST-PDF-preview.patch --]
[-- Type: text/x-patch, Size: 1538 bytes --]

From 11bb53e567588539c92a520db321a5a9b746df58 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Simen=20Heggest=C3=B8yl?= <simenheg@gmail.com>
Date: Sat, 27 Jan 2018 12:18:40 +0100
Subject: [PATCH] Warn about missing executables in RST PDF preview

* lisp/textmodes/rst.el (rst-compile-pdf-preview): Warn about missing
executables when attempting to compile and preview an RST file as PDF.
(Bug#30218)
---
 lisp/textmodes/rst.el | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lisp/textmodes/rst.el b/lisp/textmodes/rst.el
index c93e4e474c..a16d5bca3a 100644
--- a/lisp/textmodes/rst.el
+++ b/lisp/textmodes/rst.el
@@ -4382,10 +4382,15 @@ rst-compile-pdf-preview
   "Convert the document to a PDF file and launch a preview program."
   (interactive)
   (let* ((tmp-filename (make-temp-file "rst_el" nil ".pdf"))
+         (pdf-compile-program (cadr (assq 'pdf rst-compile-toolsets)))
 	 (command (format "%s %s %s && %s %s ; rm %s"
-			  (cadr (assq 'pdf rst-compile-toolsets))
+			  pdf-compile-program
 			  buffer-file-name tmp-filename
 			  rst-pdf-program tmp-filename tmp-filename)))
+    (unless (executable-find pdf-compile-program)
+      (error "Cannot find executable `%s'" pdf-compile-program))
+    (unless (executable-find rst-pdf-program)
+      (error "Cannot find executable `%s'" rst-pdf-program))
     (start-process-shell-command "rst-pdf-preview" nil command)
     ;; Note: you could also use (compile command) to view the compilation
     ;; output.
-- 
2.15.1

bug#30063: 26.0.90; Silent fail with `rst-compile-pdf-preview'

[Noam Postavsky]

Simen Heggestøyl <simenheg@gmail.com> writes:

>  	 (command (format "%s %s %s && %s %s ; rm %s"
> +			  pdf-compile-program
>  			  buffer-file-name tmp-filename
>  			  rst-pdf-program tmp-filename tmp-filename)))
> +    (unless (executable-find pdf-compile-program)
> +      (error "Cannot find executable `%s'" pdf-compile-program))
> +    (unless (executable-find rst-pdf-program)
> +      (error "Cannot find executable `%s'" rst-pdf-program))

It's possible to have PATH and exec-path desynchronized, such that the
above code could throw an error even though the
start-process-shell-command call later would succeed.  Maybe we should
should just consider that a misconfiguration on the user's part though.

bug#30063: 26.0.90; Silent fail with `rst-compile-pdf-preview'

[Glenn Morris]

Noam Postavsky wrote:

> Simen Heggestøyl <simenheg@gmail.com> writes:
>
>>  	 (command (format "%s %s %s && %s %s ; rm %s"
>> +			  pdf-compile-program
>>  			  buffer-file-name tmp-filename
>>  			  rst-pdf-program tmp-filename tmp-filename)))
>> +    (unless (executable-find pdf-compile-program)
>> +      (error "Cannot find executable `%s'" pdf-compile-program))
>> +    (unless (executable-find rst-pdf-program)
>> +      (error "Cannot find executable `%s'" rst-pdf-program))
>
> It's possible to have PATH and exec-path desynchronized, such that the
> above code could throw an error even though the
> start-process-shell-command call later would succeed.  Maybe we should
> should just consider that a misconfiguration on the user's part though.

Yes, I think PATH != exec-path is a user error.

BTW what happens with the above if the program is present, but fails for
some reason? Is nothing still shown to the user in that case?

Also, does it actually need to go through the shell?

bug#30063: 26.0.90; Silent fail with `rst-compile-pdf-preview'

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Sun, 28 Jan 2018 13:11:34 -0500
> Cc: stefan@merten-home.de, 30063@debbugs.gnu.org
> 
> Simen Heggestøyl <simenheg@gmail.com> writes:
> 
> >  	 (command (format "%s %s %s && %s %s ; rm %s"
> > +			  pdf-compile-program
> >  			  buffer-file-name tmp-filename
> >  			  rst-pdf-program tmp-filename tmp-filename)))
> > +    (unless (executable-find pdf-compile-program)
> > +      (error "Cannot find executable `%s'" pdf-compile-program))
> > +    (unless (executable-find rst-pdf-program)
> > +      (error "Cannot find executable `%s'" rst-pdf-program))
> 
> It's possible to have PATH and exec-path desynchronized, such that the
> above code could throw an error even though the
> start-process-shell-command call later would succeed.  Maybe we should
> should just consider that a misconfiguration on the user's part though.

Yes, we could do that.  But can we signal an error only when the
command fails?  That should minimize false negatives.

Also please note that the shell command as written is unportable: the
";" part will not work on MS-Windows, we need to use "&" instead.  But
that's a separate issue.

bug#30063: 26.0.90; Silent fail with `rst-compile-pdf-preview'

[Noam Postavsky]

On Tue, Jan 30, 2018 at 8:49 AM, Eli Zaretskii <eliz@gnu.org> wrote:

> Yes, we could do that.  But can we signal an error only when the
> command fails?  That should minimize false negatives.

I think it's a bit tricky due to being asynchronous, though it should be doable.

> Also please note that the shell command as written is unportable: the
> ";" part will not work on MS-Windows, we need to use "&" instead.  But
> that's a separate issue.

"&" would do the wrong thing for a POSIX sh compatible shell, so the
only way to fix that would be to avoid the shell entirely like Glenn
hinted at.

bug#30063: 26.0.90; Silent fail with `rst-compile-pdf-preview'

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Tue, 30 Jan 2018 09:30:30 -0500
> Cc: Simen Heggestøyl <simenheg@gmail.com>, 
> 	stefan@merten-home.de, 30063@debbugs.gnu.org
> 
> > Also please note that the shell command as written is unportable: the
> > ";" part will not work on MS-Windows, we need to use "&" instead.  But
> > that's a separate issue.
> 
> "&" would do the wrong thing for a POSIX sh compatible shell

I meant to leave ";" on Posix, and use "&" on Windows.

bug#30063: 26.0.90; Silent fail with `rst-compile-pdf-preview'

[Lars Ingebrigtsen]

Simen Heggestøyl <simenheg@gmail.com> writes:

> * lisp/textmodes/rst.el (rst-compile-pdf-preview): Warn about missing
> executables when attempting to compile and preview an RST file as PDF.
> (Bug#30218)

[...]

> +    (unless (executable-find pdf-compile-program)
> +      (error "Cannot find executable `%s'" pdf-compile-program))
> +    (unless (executable-find rst-pdf-program)
> +      (error "Cannot find executable `%s'" rst-pdf-program))

Others pointed out that it might be nice to report error messages back
from the pdf commands, and that's true, but I think this patch makes
sense, too, because it gives good, early feedback on a likely problem,
so I think it should be applied.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#30063: 26.0.90; Silent fail with `rst-compile-pdf-preview'

[Stefan Merten]

Hi all!

Sorry for not looking into this for so long. This whole part of
`rst.el' needs a couple of improvements. I did not write this code and
frankly did not care much about it - besides adding some FIXMEs.

4 days ago Lars Ingebrigtsen wrote:
> Simen Heggestøyl <simenheg@gmail.com> writes:
> 
>> * lisp/textmodes/rst.el (rst-compile-pdf-preview): Warn about missing
>> executables when attempting to compile and preview an RST file as PDF.
>> (Bug#30218)
> 
> [...]
> 
>> +    (unless (executable-find pdf-compile-program)
>> +      (error "Cannot find executable `%s'" pdf-compile-program))
>> +    (unless (executable-find rst-pdf-program)
>> +      (error "Cannot find executable `%s'" rst-pdf-program))
> 
> Others pointed out that it might be nice to report error messages back
> from the pdf commands, and that's true, but I think this patch makes
> sense, too, because it gives good, early feedback on a likely problem,
> so I think it should be applied.

Well, IMHO this patch would be less than optimal. There is already

      (defcustom rst-compile-toolsets ...)

There is an `executable-find' already for the default values. Although
it only takes an alternative if it doesn't find the first guess. This
is also less than optimal :-( .

`rst-pdf-program' on the other hand is not yet integrated in
`rst-compile-toolsets'. This is a necessary improvement.


A good solution IMHO would be to have a reasonable list of defaults
for (all) the executables in `rst-compile-toolsets', then check this
list of defaults for executability and then set the default
customization accordingly. If none of the defaults is found the
customization for the respective symbol should be set to nil and the
error should appear when the executable is to be used.

[...browsing through the code...]

Ok, a sensible solution would need quite an effort. So it's probably
best to apply this patch now and when at any point in the future I
come up with a more general solution it will do something equivalent
or rather something better.


						Grüße

						Stefan

bug#30063: 26.0.90; Silent fail with `rst-compile-pdf-preview'

[Lars Ingebrigtsen]

Stefan Merten <stefan@merten-home.de> writes:

> A good solution IMHO would be to have a reasonable list of defaults
> for (all) the executables in `rst-compile-toolsets', then check this
> list of defaults for executability and then set the default
> customization accordingly. If none of the defaults is found the
> customization for the respective symbol should be set to nil and the
> error should appear when the executable is to be used.

Yeah, that sounds like a good thing.

> [...browsing through the code...]
>
> Ok, a sensible solution would need quite an effort. So it's probably
> best to apply this patch now and when at any point in the future I
> come up with a more general solution it will do something equivalent
> or rather something better.

OK, Simen -- I think you should just apply the patch and close the bug
report, and we can hope that further improvements arrive in the future.
:-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#30063: 26.0.90; Silent fail with `rst-compile-pdf-preview'

[Simen Heggestøyl]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> OK, Simen -- I think you should just apply the patch and close the bug
> report, and we can hope that further improvements arrive in the future.
> :-)

Alright, applied!

-- Simen