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

Add initial vim jobs strategy #193

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@edkolev

edkolev commented Oct 24, 2016

This is an initial version of a strategy based on vim 8's jobs. I saw the discussion in #188, hence this PR.

I don't think it's ready yet (I haven't even tested it on files with whitespace in them).

This strategy works only for 'make' actions, but maybe it should also have a check if 'background' is true.

Any feedback is appreciated.

Edit: I haven't thought about windows support at all - the strategy uses dispatch#prepare_make() as well as &shell and &shellcmdflag

@z0rc z0rc referenced this pull request Nov 26, 2016

Closed

Vim 8 background job startegy #153

@z0rc

This comment has been minimized.

z0rc commented Nov 28, 2016

What's the status of this PR? Is it going to be updated? Does it require review?

@tpope

This comment has been minimized.

Owner

tpope commented Apr 2, 2017

My first reaction is that we almost certainly should not be using dispatch#prepare_make(). We can get the exit status from exit_cb and replace the redirect with out_file. I think that just leaves capturing the pid. Is there a clean way to do that? I see it in the string form of the job object. Perhaps we just munge it out of there?

@blueyed

This comment has been minimized.

Contributor

blueyed commented Apr 2, 2017

Just for reference: vim-dispatch-neovim uses Neovim's terminal, and I've been using it for a while (with some modifications)

@tpope

This comment has been minimized.

Owner

tpope commented Apr 3, 2017

I pushed my own attempt. It works but it's really unnerving not to see any output till it finishes.

@shym

This comment has been minimized.

Contributor

shym commented Jun 5, 2017

@tpope I’ve been giving your job branch a try since I would love to see it merged in the main branch.
I noticed a difference with the tmux handler: the command is not interpreted by a shell, so Dispatch echo a; echo b yields a; echo b in job, and a\nb in tmux.

@tpope

This comment has been minimized.

Owner

tpope commented Jun 5, 2017

Just forced pushed a bunch of local changes. Try again.

@shym

This comment has been minimized.

Contributor

shym commented Jun 5, 2017

The progressive output in the quickfix window is great!

  • It would be nice if the height of the window could be configured (I often use a small screen so I set dispatch_quickfix_height). I guess I would even prefer if there was a way to set a different (smaller in my case) height while the compilation happens (since that output is useless until at least an error is found).
  • It would be nice also to have support for background compilations.
  • There is a typo in compiler in get(request, 'complier', '').
@shym

This comment has been minimized.

Contributor

shym commented Jun 6, 2017

I may add that I’d love to be able to set dispatch_quickfix_height and the new dispatch_running_quickfix_height (?) to 0 in special cases when I want this to run without immediate visual feedback (when I run tools like style-checkers in autocmd but want to take into account their report only once in a while).

@tpope tpope force-pushed the tpope:master branch from 682274e to d6927ff Jun 6, 2017

@tpope

This comment has been minimized.

Owner

tpope commented Jun 8, 2017

Before we get into bikeshedding I have some more fundamental concerns. Jobs are really bad for commands that read from stdin or from /dev/tty. That's just 2 examples, but infinite loops and hangs are as bad of examples as you can get. Issues with /dev/tty are probably a lost cause, but maybe we can use channel support to handle the stdin case?

Edit: I did fix it to respect dispatch_quickfix_height. And I think maybe we should just not open the window by default, but I'm keeping that behavior for now because as an experiment I want it to be aggressively in your face.

@shym

This comment has been minimized.

Contributor

shym commented Jun 9, 2017

For /dev/tty vim seems to properly detach jobs from the terminal:
:Dispatch printf 'url=http://whatev.er\n\n' | git credential fill
yields, as hoped:
|| fatal: could not read Username for 'http://whatev.er': No such device or address
which is obviously not ideal but much preferable to a hang as in fugitive#722.

@tpope

This comment has been minimized.

Owner

tpope commented Jun 9, 2017

You're right, I must have confused myself testing on Neovim or something.

@shym

This comment has been minimized.

Contributor

shym commented Jun 10, 2017

For commands that break when stdin is closed, I’m tempted to say that they’re just buggy and should be fixed; but that’s not a very constructive answer. Only some simple workaround comes to mind (such adhoc commands to interact with the command), maybe because I don’t see many use cases for compilers dropping to interactive mode; the only case I know is latex, and so I just tried and seen that it then exits with a fatal error if its stdin is closed (which is a much nicer behaviour than its standard one IMHO).

@tpope

This comment has been minimized.

Owner

tpope commented Jun 12, 2017

I agree, it would be nice to solve but we're down to basically one broken program so it's no longer a deal breaker. Other issues:

  • Sometimes it fails to capture all output, presumably because the exit callback fires while output is still buffered.
  • Incrementally updating the quickfix list won't work if the error format contains multi-line items. We should check the format and do a full re-parse if that's the case (which hopefully won't have performance problems).
@shym

This comment has been minimized.

Contributor

shym commented Jun 14, 2017

Do you have an example for the issue about failing to capture all output, to try a way to address it?
I wrote a silly talktoomuch.c "compiler":

#include <stdio.h>

int main(int argc, char *argv[]) {
    int i;

    for (i = 0; i < 10000; i++)
        printf("but yeah\nbut no\n");

    printf("/tmp/a.c:2:13: warning: something to say\n");

    return 0;
}

Running a Dispatch -compiler=gcc /tmp/talktoomuch lasts horribly long (about 15s on my slow computer), keeping one core fully busy while lines get added to the quickfix list. Dispatch reports a success immediately but the final error gets eventually added to the quickfix list and recognized as such.
:h read-in-close-cb makes me wonder if s:output could detect whether it reached the end of the output to process, so that it could invoke s:exit (and when s:exit is called by vim as the exit callback, it could then abort if there is still some output buffered).

To compare performance, I ran my simple talktoomuch with the tmux adapter. Obviously it performs really fast but every time I got a truncated output (so the actual error is never captured) :-/

Edit: for the record, I reported the issue with tmux there.

@tpope

This comment has been minimized.

Owner

tpope commented Jun 14, 2017

Unfortunately it's only happened when I've been too immersed to investigate. It's always something that fails fast, with the error message getting omitted. :h read-in-close-cb is a good find. Next time it happens I'll rig something up.

@shym

This comment has been minimized.

Contributor

shym commented Jul 1, 2017

I realised that one source of slowness of Dispatch -compiler=gcc /tmp/talktoomuch was an autocommand that fired on every caddexpr. I replaced my autocmd QuickFixCmdPost * with something more fine-grain, to remove caddexpr.
Along with a full re-parse when needed, it might be relevant to trigger an autocmd when the whole thing (the process exited and its output was handled by vim) ends.
I still get the inefficiency from vim handling the output line by line, ever so slowly.

By the way, combining the fact that detecting when everything did really end, the fact that sometimes a full re-parse is needed anyway, I wonder if it might just be more efficient to tee a copy of the output to a file and cgetfile it as soon as the process exits, if it is possible to interrupt the 'callback' from being called (I just tried hacking horrible things like a remove of the callback field in the s:exit function but even if it was horrible it didn’t work ;-).

@gauteh gauteh referenced this pull request Sep 1, 2017

Open

Add support for :terminal #213

@gauteh

This comment has been minimized.

gauteh commented Sep 1, 2017

I'm testing the job branch now and it seems to work fine. Is there a way to Cancel the running job, type Ctrl+C?

@shym

This comment has been minimized.

Contributor

shym commented Sep 1, 2017

@gauteh: you might have a look at #28 for the discussion about such a feature (and / or the abort branch). Indeed, it would be more useful with the job adapter than with most others.

@gauteh

This comment has been minimized.

gauteh commented Sep 4, 2017

@shym: nice. that should do the trick, from what I could quickly glance it does not seem to abort Vim jobs yet?

@tpope

This comment has been minimized.

Owner

tpope commented Sep 5, 2017

Took a quick look at this. The :Abort command essentially just calls kill -HUP <pid>, and it seems to be correctly doing that on the job branch as well. However, unlike other adapters, that use a wrapper script, jobs are invoked with a simple bash -c 'your command', and bash is smart enough to change this exec your command, which means your command receives the HUP directly. Some commands may ignore HUP, causing the abort to appear to fail.

This is annoying. We could add a postfix like ; exit $?, but now we have to worry about alternate shells like fish and cmd.exe.

@gauteh

This comment has been minimized.

gauteh commented Sep 6, 2017

@stevenharman

This comment has been minimized.

stevenharman commented Feb 3, 2018

/me: apologizes in advance for the drive-by comment.

Is adding a Vim 8 Jobs strategy still being considered? As a Vim-in-terminal (usually, MacVim in iTerm, but whatever) user I've long wanted async dispatch w/o the need to also have tmux or the like set up. The fewer moving parts, and all of that. Anyhow... just curious if there's still interest and/or movement on adding it.

Thanks for making so many of the tools we all take for granted. ❤️

@aliou aliou referenced this pull request Apr 25, 2018

Closed

Ack async #24

@tpope

This comment has been minimized.

Owner

tpope commented May 13, 2018

I have decided that the next step will be to merge in the job adapter and make it low precedence, and maybe provide an option to make it high precedence (since adjusting the list manually is a pain). First though, I'd like to add Neovim support. Anyone feel like knocking that out? I'm looking for direct to quickfix list support like the Vim 8 implementation, not :terminal.

@tpope tpope referenced this pull request May 13, 2018

Closed

Add vim 8 support? #922

@tpope

This comment has been minimized.

Owner

tpope commented Sep 20, 2018

I was going to merge the job branch immediately after 1.6 and worry about Neovim later, but ran into a few issues:

  • Sometimes the last few lines would get discarded because the exit event fired while there was still output queued. I've pushed some additional changes to the job branch that seem to resolve this, but it's a confusing belt-and-suspenders mess I don't have a lot of confidence in.
  • If you use :colder and :cnewer to switch between quickfix lists, output can be lost. We need some sort of buffer.
  • :AbortDispatch is not particularly effective at stopping a running task compared to CTRL-C. I think this is because we always signal the top most pid, rather than the foreground task. Maybe job_stop() would be more effective.
@shym

This comment has been minimized.

Contributor

shym commented Oct 8, 2018

About the first issue, I was thinking it might be simpler by hooking on close_cb instead of exit_cb? Of course, we probably want to wait for the two events to have fired, to get the exit status and know that all the output was processed.
At the moment, the job strategy is pretty much unusable for me because my (really sloooow) disk is spinning like crazy while output lines are added one at a time, with half a second between each, even if the job finished long ago. Might it be the current tricks that cause it to be so slow?

About the third issue, I’m not sure I managed to notice the same slowness but maybe kill -HUP -pid might be more efficient to kill them all at once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment