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

Parallel installer for Neovim #103

Merged
merged 13 commits into from
Oct 10, 2014
Merged

Parallel installer for Neovim #103

merged 13 commits into from
Oct 10, 2014

Conversation

junegunn
Copy link
Owner

@junegunn junegunn commented Oct 9, 2014

This pull request implements parallel installer using Neovim's job control.

(/cc @justinmk)

Caveats

Asynchronous

Due to the nature (or limitation?) of job control, this version of parallel installer is asynchronous. (Polling doesn't work since it blocks main thread.) You can freely move your cursor, edit other buffers while installation is ongoing. Which is cool and all but it's inconsistent with the current behavior, and it's not anymore possible to write something like the following:

# The installer is asynchronous, so this will exit immediately
nvim +PlugUpdate +qa

Being asynchronous also means that the user can close the plug window or switch tabs anytime during the installation. If the window is closed, we have to make sure that the running jobs are correctly terminated. Currently I'm calling jobstop() function to abort tasks, but I've noticed that the git processes are not immediately killed.

So we definitely need more tests but unfortunately I don't think it's possible to write test cases for asynchronous behavior using Vader.vim.

Unable to detect errors

As far as I know, the current API of job control does not expose the exit status of the process.

EDIT: Implemented with a workaround

Task timeout not implemented

I didn't bother to implement task timeout.

Windows support

I don't think jobstart(name, 'sh', ['-c', args]) would run on Windows. 😒
Does Neovim support Windows?

- Abort running jobs when plug windows is reset
- Multi-line error report
- Retain window view
@junegunn
Copy link
Owner Author

junegunn commented Oct 9, 2014

nvim

@justinmk
Copy link
Contributor

justinmk commented Oct 9, 2014

🌋 🎅 🌋 !!!111

I'm going to test the hell out of this :)

it's not anymore possible to write something like nvim +PlugUpdate +qa

This is a great point. We will definitely consider this use case.

we have to make sure that the running jobs are correctly terminated. Currently I'm calling jobstop() function to abort tasks, but I've noticed that the git processes are not immediately killed.

Will also add a Neovim issue for this, or if you want to, please feel free.

the current API of job control does not expose the exit status of the process

This is planned.

Does Neovim support Windows

Not yet, we need to (1) finish transitioning all OS-specfic code to libuv, and (2) migrate the integration tests.

@justinmk
Copy link
Contributor

justinmk commented Oct 9, 2014

Just gave it a whirl, updated 62 plugins in 11 seconds. cc @tarruda

@junegunn
Copy link
Owner Author

junegunn commented Oct 9, 2014

@justinmk Thanks for checking out.

While asynchronous installer makes things a bit complex, like making sure that you're on the right buffer whenever you try to update the screen, I think I like the responsiveness of it. Well, job control is not a good fit for synchronous fork/join model anyway.

As for the case of nvim +PlugUpdate +qa, it can be counter-intuitive, but users will immediately realize that it doesn't work as intended and I don't think it's going to be a show-stopper. I can instead add an option to the commands for the purpose. e.g. nvim +'PlugUpdate --quit'

Maybe this is also planned, but I wish I could pass the name of the jobs directly to jobstop() and jobsend() functions so I don't have to maintain a silly inverted index just to remember the ids.

I wonder when would be the right time to really merge this to master. Is the job control API stable? I'd like to know if it's going to be backward-compatible.

@justinmk
Copy link
Contributor

justinmk commented Oct 9, 2014

Maybe this is also planned, but I wish I could pass the name of the jobs directly to jobstop() and jobsend() functions

I remember thinking that too, we should definitely add that.

Is the job control API stable? I'd like to know if it's going to be backward-compatible.

There might be a few breaking changes but I wouldn't let that hold it back from master. Neovim users expect things to break, and I will be a daily user of this plugin so I'll be happy to send a PR (any potential breaking changes to job control are not likely to require any major logic changes--just some arity or syntax tweaks).

edit:

asynchronous installer makes things a bit complex, like making sure that you're on the right buffer whenever you try to update the screen,

This is a great example of why buffers need to be easier to work with. neovim/neovim#901

@tarruda
Copy link

tarruda commented Oct 10, 2014

Time to refactor my vimrc to use this, great work!

@tarruda
Copy link

tarruda commented Oct 10, 2014

BTW, it will probably be easy to add a jobwait(jobids...) function to wait for multiple jobs and return a list of exit status codes, such a function could be used to fix compatibility with the current implementation.(I need to investigate though)

@junegunn
Copy link
Owner Author

@justinmk

Neovim users expect things to break, and I will be a daily user of this plugin so I'll be happy to send a PR

Thanks. The only missing feature that seemed relevant was error detection, but I managed to find a simple, albeit naive, workaround for it.

@tarruda Thanks!

@junegunn
Copy link
Owner Author

@tarruda Will JobActivity handlers run while the execution is blocked on jobwait()? I need them to run in order to update the buffer on the fly. If they do, I can easily make the installer synchronous only on has('vim_starting') and that would be great.

@tarruda
Copy link

tarruda commented Oct 10, 2014

@tarruda Will JobActivity handlers run while the execution is blocked on jobwait()? I need them to run in order to update the buffer on the fly. If they do, I can easily make the installer synchronous only on has('vim_starting') and that would be great.

Yes, callbacks will run normally and you will be able to trigger window redraws(Just user interaction would be blocked).

Though as I said, I still need to think about the jobwait implementation. My first impression is that it would be easy to do it, especially because I have already added some support for running the event loop "focusing" in a set of event sources.

@junegunn
Copy link
Owner Author

@tarruda I see, that would be great.

Maybe this should be filed as an issue on Neovim repo, but one thing I noticed while testing this async installer is that multi-key command sequences are interrupted by JobActivity event. For example if I run this code,

call jobstart('loop', 'bash', ['-c', 'while [ 1 ]; do echo .; sleep 0.01; done'])

even without any handler attached, I can't execute multi-key commands such as gg, diw, etc as the key sequence is interrupted in between. This is very noticeable with vim-plug installer.

junegunn added a commit that referenced this pull request Oct 10, 2014
Parallel installer for Neovim
@junegunn junegunn merged commit a45c383 into master Oct 10, 2014
@junegunn junegunn deleted the neuevim branch October 10, 2014 14:11
@tarruda
Copy link

tarruda commented Oct 12, 2014

I can't execute multi-key commands such as gg, diw, etc as the key sequence is interrupted in between. This is very noticeable with vim-plug installer.

This is probably because Nvim uses a special key(K_EVENT) code to notify the main loop of asynchronous events, this might be getting in the way of multi-key commands. I will look for a fix

@justinmk
Copy link
Contributor

I don't think jobstart(name, 'sh', ['-c', args]) would run on Windows.

@junegunn Probably you already know, but normally sh -c ... should not be needed for neovim jobs. Instead just call jobstart(name, [arg1, arg2, ...]).

The only missing feature that seemed relevant was error detection, but I managed to find a simple, albeit naive, workaround for it.

There's movement on this here: neovim/neovim#1844

@junegunn
Copy link
Owner Author

normally sh -c ... should not be needed for neovim jobs

@justinmk I see but the argument to jobstart can be quite lengthy (chained commands), so it was easier for me to just use sh -c.

Thanks for the update!

@benekastah
Copy link

Instead of passing sh or bash when I need to run a program with args as a string, I've used &shell. That's one step in the direction of windows support, but I have no idea if windows shells can accept a program as an argument with -c like bash and related shells can. It's an idea, anyway.

@starcraftman
Copy link
Contributor

@benekastah In general, I assume by Windows support you mean cmd.exe the default shell. In that case, no such animal. To start on Windows they use the very logical / instead of - to signal flags. Secondly, I believe by default you can pass cmd.exe an arbitrarily long string but no flag in front.

You can see the flags at > http://www.microsoft.com/resources/documentation/windows/xp/all/proddocs/en-us/cmd.mspx?mfr=true

On a related note, I may help with testing Windows support soonish, but for now working on my python fork.

EDIT: Further related note, if your looking for cmd.exe support you might wanna browse the Vundle src as it worked fairly well on Windows. Including suppressing shell popups I believe.

@justinmk
Copy link
Contributor

Passing the arguments as a list to jobstart avoids shell escaping problems, and will avoid cmd.exe completely on windows.

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

Successfully merging this pull request may close these issues.

None yet

5 participants