Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

counsel.el: fix xdg-open under Ubuntu for counsel-locate-action-extern #1759

Closed
wants to merge 1 commit into from

Conversation

@yiufung
Copy link

commented Sep 19, 2018

start-process-shell-command doesn't seem to wait for counsel-locate-action-extern to finish under Ubuntu. Gist to reproduce the issue.

Related discussions (#1, #2, #3) suggest to:

  1. Set process-connection-type as nil temporarily
  2. Called with setsid -w to wait for the process to finish.

To avoid interfering with other platforms and applications, I adopted the latter approach which uses lsb_release to check distribution type with fall back as xdg-open. Currently only check for Ubuntu, and to be extended in the future.

Related issues: #863 and #1401.

Cheong Yiu Fung
counsel.el: fix xdg-open under Ubuntu for counsel-locate-action-extern
`start-process-shell-command` doesn't seem to wait for
`counsel-locate-action-extern` to finish under Ubuntu. [Gist to
reproduce the
issue](https://gist.github.com/yiufung/38be4c3a6832334f36f2441550cf0b12).

Related
discussion: ([#1](https://askubuntu.com/questions/646631/emacs-doesnot-work-with-xdg-open),
[#2](https://emacs.stackexchange.com/questions/19344/why-does-xdg-open-not-work-in-eshell),
[#3](http://emacs.1067599.n8.nabble.com/emacs-and-xdg-open-td106892.html))
suggests to:

1. Set `process-connection-type` as nil temporarily
2. Called with `setsid -w` to wait for the process to finish.

To avoid interfering with other platforms and applications, I adopted
the latter approach which uses `lsb_release` to check distribution
type with fall back as `xdg-open`. Currently only check for Ubuntu,
and to be extended in the future.

Related issues: #863 and #1401.

@abo-abo abo-abo closed this in dd66c93 Sep 19, 2018

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Sep 19, 2018

Thanks, I confirm the issue is reproducible and is now fixed.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Sep 19, 2018

To avoid interfering with other platforms and applications

How does:

(let ((process-connection-type nil))
  (start-process-shell-command ...))

interfere with other platforms and applications? Perhaps you were thinking of (setq process-connection-type nil), which should never be done?


I think this issue is also related to #1483 (comment) and #1537.

@yiufung yiufung deleted the yiufung:fix-locate-file-extern-ubuntu branch Sep 19, 2018

basil-conto added a commit to basil-conto/swiper that referenced this pull request Sep 19, 2018
counsel.el: Simplify async xdg-open invocation
(counsel-locate-action-extern): Avoid issues with
start-process-shell-command returning too soon by passing 0 as the
BUFFER argument to call-process-shell-command.

Re: abo-abo#1537, abo-abo#1759
@yiufung

This comment has been minimized.

Copy link
Author

commented Sep 19, 2018

(let ((process-connection-type nil))
(start-process-shell-command ...))

That would have duplicated start-process-shell-command several
times I guess?

(defun ...
  (cl-case system-type
    (darwin (start-process-shell-command 
                    shell-file-name nil (format "%s %s" "open"
                    (shell-quote-argument x))))
    (cygwin (start-process-shell-command 
                   shell-file-name nil (format "%s %s" "cygstart"
                   (shell-quote-argument x))))
    (t (if (ubuntu-check) 
              (let ((process-connection-type nil))
                  start-process-shell-command ....)
             (start-process-shell-command ....)))))

Or maybe there are some other ways to refactor it more nicely. I was trying to
introduce as minimal impact as possible.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Sep 19, 2018

That would have duplicated start-process-shell-command several
times I guess?

No:

(let ((process-connection-type nil))
  (start-process-shell-command shell-file-name nil
                               (format "%s %s"
                                       (cl-case system-type
                                         (darwin "open")
                                         (cygwin "cygstart")
                                         (t "xdg-open"))
                                       (shell-quote-argument x))))

There is nothing intrusive about let-binding process-connection-type to nil. In fact, pipes are better than ptys for the vast majority of asynchronous processes invoked by Emacs libraries such as Counsel.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Sep 19, 2018

(BTW, this is another case where invoking the shell, rather than xdg-open et al. directly, is unnecessary, though calling the process directly would not solve the pipe/pty problem.)

@yiufung

This comment has been minimized.

Copy link
Author

commented Sep 19, 2018

Oh, that's exactly what I was avoiding at the start. I noticed the diffs about pty and pipe in Emacs manual, but I'm uncertain whether not let-binding process-connection-type to nil is a design choice or not. Currently I use Emacs under Linux, but several months ago I met some Windows-specific problem with calling shell commands, so I went the conservative way.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Sep 19, 2018

I'm uncertain whether not let-binding process-connection-type to nil is a design choice or not.

Its default value of t makes sense for interactive commands like M-&, where the user may want to interact with the process via a pty. Libraries that spawn asynchronous processes for effect or output should, for most intents and purposes, use a pipe. That Counsel didn't previously use a pipe was simply an oversight.

I met some Windows-specific problem with calling shell commands

Shell compatibility problems are very common even amongst POSIX OSes, and are not related to the current issue of synchronous vs asynchronous processes and ptys vs pipes. This is why libraries should always prefer direct process invocation via call-process, start-process, etc., rather than call-process-shell-command, shell-command-to-string, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.