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

Add native job support for neovim. #234

Merged
merged 5 commits into from
Aug 6, 2020
Merged

Conversation

okkays
Copy link
Contributor

@okkays okkays commented Aug 2, 2020

Hello!

I think I've managed to get support for neovim's jobcontrol (#125) working (I've tested it manually with vim-coverage, which wasn't working for me and caused me to try and solve this in the first place).

However, in order to get system-vimjob.vroom to succeed, I need to pass vroom --shell-delay .25 (by default vim's delay is 0.25 and neovim's is 0.0). Here's an example failure:

└─ $ vroom --neovim vroom/system-vimjob.vroom
vroom/system-vimjob.vroom
FAILED on line 45: Multiple failures:
Expected system call not received.

Got no chance to inject response:
 EXPECT:        echo hi (regex mode)

Failed command on line 45:
:let g:invocation = AsyncWait(g:syscall.CallAsync('Callback', 0), 2)

Queued system controls are:
 EXPECT:        echo hi (regex mode)

No system calls received. Perhaps your --shell is broken?

Last few commands (most recent last) were:
:if has('nvim')<CR>  set cmdheight=30<CR>endif<CR><CR>
:set nocompatible<CR>
:let g:maktabadir = fnamemodify($VROOMFILE, ':p:h:h')<CR>
:let g:bootstrapfile = g:maktabadir . '/bootstrap.vim'<CR>
:execute 'source' g:bootstrapfile<CR>
:call maktaba#system#SetUsableShellRegex('\v<shell\.vroomfaker$')<CR>
:if !has('job') && !has('nvim')<CR>  echomsg maktaba#error#MissingFeature(      'Must have +job support to run system-vimjob.vroom examples')<CR>endif<CR>
:function AsyncWait(invocation, delay) abort<CR>  let l:deadline = localtime() + a:delay " wait for at most N seconds<CR>  while !a:invocation.finished && localtime() < l:deadline<CR>    sleep 100m<CR>  endwhile<CR>  call maktaba#ensure#IsTrue(a:invocation.finished, 'Async callback was expected to complete within %d seconds', a:delay)<CR>  return a:invocation<CR>endfunction<CR>
:let g:syscall = maktaba#syscall#Create(['echo', 'hi'])<CR>
:function Callback(result) abort<CR>  let g:callback_stdout = a:result.stdout<CR>endfunction<CR>
:let g:invocation = AsyncWait(g:syscall.CallAsync('Callback', 0), 2)<CR>

Ran 1 test in vroom/system-vimjob.vroom. 0 passing, 0 errored, 1 failed.

Again, this works fine:

└─ $ vroom --neovim vroom/system-vimjob.vroom --shell-delay .25
vroom/system-vimjob.vroom
Ran 1 test in vroom/system-vimjob.vroom. 1 passing, 0 errored, 0 failed.

Any ideas on whether this is an actual problem in my use of jobcontrol, or if the async delay is just necessary for nvim in this case? I've looked into it for a bit, but this is my first adventure in maktaba/vroom.

Turns out it's pretty similar to vim - all we need to do is call
jobstart, send stdin to the channel that produces, and then close the
channel.
@google-cla
Copy link

google-cla bot commented Aug 2, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Aug 2, 2020
@okkays
Copy link
Contributor Author

okkays commented Aug 2, 2020

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels Aug 2, 2020
@dbarnett
Copy link
Contributor

dbarnett commented Aug 3, 2020

Sure, I can look into why.

For starters you might hack the AsyncAwait helper in the test to change the loop from

  |  while !a:invocation.finished && localtime() < l:deadline<CR>
  |    sleep 100m<CR>
  |  endwhile<CR>

to

  |  let l:finished_log = []<CR>
  |  while !a:invocation.finished && localtime() < l:deadline<CR>
  |    call add(l:finished_log, a:invocation.finished)<CR>
  |    sleep 100m<CR>
  |  endwhile<CR>
  |  echomsg l:finished_log<CR>

and see what that spits out.

It would be strange if at_exit was getting called before the syscall process actually finished writing its files... maybe a slight bug with neovim jobcontrol?

@okkays
Copy link
Contributor Author

okkays commented Aug 4, 2020

finished_log contains a single 0 at that point - it looks like it's finishing within 100ms (though not before getting to that loop).

The jobcontrol docs actually explicitly mention this kind of bug when using callbacks, but suggests channel-buffered as a workaround (which I'm using).

If I look at self.stdout in maktaba#syscall#neovim#HandleJobExit, it actually contains hi - so it looks like nvim is doing what it's supposed to and populating stdout on time. That would make me think that jobstart isn't using shell.vroomfaker, but since it works fine with the delay, maybe it's a bug in how we're using vroom to capture neovim syscalls? Something there would also explain why it works fine in practice, too.

@dbarnett
Copy link
Contributor

dbarnett commented Aug 5, 2020

K, have you tried adding a delay control in the vroom file to just the one line to see if that takes care of failures? As long as we have the tests passing consistently and a note explaining what's going on, I'm fine with proceeding now that you've done some initial investigation.

Do consider either renaming system-vimjob.vroom or splitting into vim/neovim variants. The "vimjob" was referring specifically to +vimjob.

okkays added 3 commits August 5, 2020 10:58
1. Rename to system-job, to avoid vim-specific reference to +vimjob
2. Add delays to async calls in the tests.  For some reason, vroom doesn't
   pick up stdout from nvim in this case without a small delay.
3. Remove the skip from travis.yml for nvim for system-vimjob
The travis config still uses vim7, and +job requires vim8
@okkays
Copy link
Contributor Author

okkays commented Aug 5, 2020

Went ahead and added some delays. I'm really liking vroom's style and syntax, nice job on that 👍

I've renamed the test to system-job.vroom (:help job will take users to the right place in both vim and nvim).

I chose to rename instead of split because of a note in the issue (#125):

Note this might complicate testing since the actual system calls would be different between vim and neovim, but a single vroom file would have to test them both.

which (barring the delays) makes sense to me, because both applications should have the same API, and this way there won't need to be special handling to skip each split file for one or the other (at least, whenever the tests are updated to use vim8). Plus, changes to that API only need to be tested in one place.

lmk if there's anything else to tweak

vroom/system-job.vroom Outdated Show resolved Hide resolved
@dbarnett
Copy link
Contributor

dbarnett commented Aug 6, 2020

Went ahead and added some delays. I'm really liking vroom's style and syntax, nice job on that 👍

That's credit to @Soares who pushed for literate testing and built the initial implementation. =)

The echomsg delays were just acting as extensions to the CallAsync delay.
Copy link
Contributor

@dbarnett dbarnett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Merging the changes.

@okkays if you could go ahead and report a bug for the mysterious delays, I do suspect we could hit on some fix we could get into the implementation so that we don't have a proliferation of weird workarounds in all the tests affected by it.

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

Successfully merging this pull request may close these issues.

2 participants