Re: bug#44824: [PATCH] org.el: Avoid xdg-open silent failure
On 19/03/2021 10:50, Kyle Meyer wrote: Maxim Nikulin writes: A few comments in addition to Eli's advice to drop the (eq system-type 'gnu/linux) condition... Feel free to commit the change suggested in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=44824#82 instead of this patch. +(defun org--error-process-sentinel (proc event) + "Show a message if process failed (exited with non-zero code +or killed by a signal. Pass the function as :SENTINEL argument Please rework the first sentence so that it fits on the first line, though I'd be in favor dropping the function and using a lambda in the make-process call. My impression is that org-open-file function is already too long and complex. Another reason to use standalone function is that I am unsure if elisp compiler and interpreter are smart enough to reuse single instance of lambda. I was afraid that every opened file caused creation of new sentinel possibly with a closure containing chain of stack frames. On the other hand even in worst case memory footprint is negligible in comparison to any GUI viewer. + (unless (string-match "finished" event) There's no need for substring matching, right? So it could be (equal event "finished\n") I was surprised by final "\n" that is not always suitable and I was in doubts concerning its stability. I would prefer something like (starts-with event "finished") Certainly match-data is not necessary, so even match-string-p is better. (when (and (memq (process-status proc) '(exit signal)) (/= (process-exit-status proc) 0)) Thank you, I was too lazy to implement such kind of check myself. Certainly this variant is better. I hope, I have addressed other your comments in the updated patch. commit 5eca7764d94dd46b9f9a7792d1b786a3f03b20b6 Author: Max Nikulin Date: Wed Feb 17 16:35:58 2021 + org.el: Avoid xdg-open silent failure * lisp/org.el (org-open-file): Use 'pipe :connection-type instead of 'pty to prevent killing of background process on handler exit. (Bug#44824) Problem happens only in some desktop environments where configured through `org-file-apps' or mailcap handlers launches actual viewer (as defined in .desktop files and obtained from mimeapps.list) in background. E.g. xdg-open invokes "gio open" or kde-open5 for Gnome or KDE accordingly and these handlers launches e.g. eog or okular in background. As soon as main process exits, temporary terminal session created by `start-process-shell-command' is terminated. As a result background processes receive SIGHUP. Previously command were executed with no buffer, so the change does not affect "needsterminal" and "copiousoutput" mailcap features, they are not supported as earlier. If handler main process fails then show a message with exit reason. Output (including error messages) is ignored as before. Gtk application tends to report significant amount of failed asserts hardly informative for majority of users. diff --git a/lisp/org.el b/lisp/org.el index 4db2dbe04..c58708a5f 100644 --- a/lisp/org.el +++ b/lisp/org.el @@ -8768,7 +8768,21 @@ If the file does not exist, throw an error." (save-window-excursion (message "Running %s...done" cmd) - (start-process-shell-command cmd nil cmd) +;; Handlers such as "gio open" and kde-open5 start viewer in background +;; and exit immediately. Avoid `start-process' since it assumes +;; :connection-type 'pty and kills children processes with SIGHUP +;; when temporary terminal session is finished. +(make-process + :name "org-open-file" :connection-type 'pipe :noquery t + :buffer nil ; use "*Messages*" for debugging + :sentinel (lambda (proc event) + (when (and (memq (process-status proc) '(exit signal)) +(/= (process-exit-status proc) 0)) + (message +"Command %s: %s." +(mapconcat #'identity (process-command proc) " ") +(substring event 0 -1 + :command (list shell-file-name shell-command-switch cmd)) (and (boundp 'org-wait) (numberp org-wait) (sit-for org-wait ((or (stringp cmd) (eq cmd 'emacs))
Re: bug#44824: [PATCH] org.el: Avoid xdg-open silent failure
On 19/02/2021 21:54, Eli Zaretskii wrote: From: Maxim Nikulin Date: Fri, 19 Feb 2021 19:29:49 +0700 On Windows Emacs always uses pipes, because we don't have PTYs there. And there's no xdg-open on MS-Windows anyway, so it's a moot point. Should I consider your response as a suggestion to remove the `if' related to `system-type'? Yes, that 'if' isn't necessary. Let's wait for comments comments from org developers. I do not mind to remove the `if' but they could ask for other changes. If I remember correctly, on windows it is possible to communicate with a process through stdin and stdout only if the application is compiled as a *console* one. That's true. But in this case we don't really want to communicate with the sub-process, do we? We just want to invoke it and let it run. So the fact that there's no way of communicating with the sub-process is not important here, as the pipes will not be used. We just need to specify pipes because that works around the problem with xdg-open. The reason to switch to pipe process here is to avoid side effects of *terminal session*. PTY is redundant here. Pipe process is the only alternative available in emacs. Currently there is no data exchange with subprocess. Reading of stderr and stdout from the handler and its descendants could be useful to realize a cause of a problem when it happens. At least on linux it is enough to specify some buffer instead of `nil' or add a :filter function. The reason why I did not make such change is that Gtk applications tends to report significant number of failed asserts. I do not like to pollute the "*Messages*" buffer. I am in doubts if a dedicated buffer should be created for such purpose. Reading of stdout could be useful in mailcap handlers e.g. to display text from PDF file when only text terminal is available (no X11 or wayland). Currently emacs ignores "needsterminal" and "copiousoutput" flags in mailcap. Example of an entry for PDF: application/pdf; pdftotext -layout %s -; test=test -z "$DISPLAY"; copiousoutput Likely only a small part of users could benefit from such feature. "start file.pdf" executed in cmd.exe launches an application that does not block command prompt. In this sense it similar to background processes launched by kde-open5 or "gio open". However I am unaware if there is something similar to process groups on windows that leads to termination of all group members when leader process finishes. Things are fairly similar on Windows. But is this really relevant to the issue at hand? There's no xdg-open on Windows, so whatever problems you had with xdg-open will never happen on Windows. the proposed patch fixes the problem only on systems where org.el invokes the PDF viewer via xdg-open. Right? 1. It is not a problem of namely xdg-open. It is (mostly reasonable but with some caveats however) specific of DE-specific handlers as "gio open" and kde-open5. They exit just after spawning of a process with actual handler. (Just like "start file.pdf" on windows.) I am afraid that I could break something on windows, so I would prefer that somebody will test the changes on non-linux systems. 2. It is not specific to PDF files, the problem could happen with any format if wrapper handler such as "gio open" is invoked instead of e.g. direct execution of eog.
Re: bug#44824: [PATCH] org.el: Avoid xdg-open silent failure
On 18/02/2021 21:48, Eli Zaretskii wrote: From: Maxim Nikulin Date: Thu, 18 Feb 2021 19:56:03 +0700 I could not estimate effect of such change on windows, so pipe process is used only on linux. I am unsure concerning mac however. On Windows Emacs always uses pipes, because we don't have PTYs there. And there's no xdg-open on MS-Windows anyway, so it's a moot point. Should I consider your response as a suggestion to remove the `if' related to `system-type'? If I remember correctly, on windows it is possible to communicate with a process through stdin and stdout only if the application is compiled as a *console* one. That is why it is unclear for me, which component handles console or GUI distinction when process is specified as pipe in lisp code, and whether all arguments of `make-process', that I have used, are suitable on windows. "start file.pdf" executed in cmd.exe launches an application that does not block command prompt. In this sense it similar to background processes launched by kde-open5 or "gio open". However I am unaware if there is something similar to process groups on windows that leads to termination of all group members when leader process finishes.