Initial attempt to extract Vim selection logic

[mudge]

This isn't particularly ready but might be a good point for us to start collaborating on extracting the Vim selection logic into separate classes.

To summarise, I created a Vimrunner::Vim module which has server, client and gui methods to return the appropriate binary for each selection. The different permutations should be covered in spec/vimrunner/vim_spec.rb. The result of this selection is not just a string (e.g. "mvim") but a Vimrunner::VimInstance (for want of a better name) which is either a Vimrunner::HeadlessVim or a Vimrunner::GuiVim: the difference being how they spawn. These VimInstances are then passed around and interrogated by both Client and Server classes.

The reason I created an abstract parent class of both HeadlessVim and GuiVim was to avoid violating the Liskov substitution principle where possible by changing the behaviour of spawn between parent and child.

I'd be keen to get your feedback to see how we can simplify this (particularly the initialisation logic in Server).

[AndrewRadev]

Awesome. I've been pretty busy this week, though, so I'll probably be able to look through it in more detail in the weekend.

[mudge]

No problem. Two things to think about:

• Would things be simpler if we removed the ability to specify a custom vim_path (or perhaps made users pass a HeadlessVim or GuiVim rather than a bare string)?
• Instead of hard-coding the sequence of Vims to try (vim then gvim on Linux, vim then mvim on a Mac) maybe we should have a list of Vims to try and simply go through to determine which is most appropriate? In this way, we don't couple too tightly to the host operating system, we'd just have something like so:

def vims_to_try
  vims = %w( vim gvim )
  vims.insert(1, "mvim") if mac?

  vims
end

[mudge]

One thing to check: there is a lot of logic to determine the best client and the best server separately: couldn't we always use the server as the client as neither --remote-send nor --remote-expr start an instance?

[mudge]

This is a bit rough and I haven't tested it thoroughly but here's another approach: having a single Vimrunner::Command which manages all interaction with the Vim executable (there are probably some other bits to mix in). It also provides a single suitable? query that allows you to determine whether it can be used on the current machine, in this way we could have a Vimrunner.command that would return the first suitable command.

require "pty"

module Vimrunner
  class Command
    attr_reader :path, :pid

    def initialize(path)
      @path = path
    end

    def suitable?
      features = run(path, "--version")

      if gui?
        features.include?("+clientserver")
      else
        features.include?("+clientserver") && features.include?("+xterm_clipboard")
      end
    rescue Errno::ENOENT
      false
    end

    def gui?
      File.basename(path)[0, 1] =~ /g|m/
    end

    def name
      @name ||= "VIMRUNNER#{rand}"
    end

    def server_path
      [path, "--servername", name]
    end

    def start_server(vimrc_path)
      command = server_path + ["--noplugin", "-f", "-u", vimrc_path]
      _r, _w, @pid = PTY.spawn(*command)

      @pid
    end

    def remote_expr(expr)
      command = server_path + ["--remote-expr", expr]
      output = run(*command).strip
      raise InvalidCommandError if output =~ /^Vim:E\d+:/

      output
    end

    def remote_send(keys)
      command = server_path + ["--remote-send", keys]

      run(*command)
    end

    # Sends a TERM signal to the given PID. Returns true if the process was
    # found and killed, false otherwise.
    def kill
      Process.kill("TERM", pid)
      true
    rescue Errno::ESRCH
      false
    end

    alias quit kill

    private

    # Executes a shell command, waits until it's finished, and returns the
    # output
    def run(*command)
      IO.popen(command) { |io| io.read.strip }
    end
  end
end

[AndrewRadev]

Yup, that's something I've been thinking of myself, though with a different name. I want to flesh it out a bit and I'll push later tonight.

[AndrewRadev]

Alright, so I made a few changes here and there, mostly to naming, but to organization as well. Let me know what you think of them. Basically, the current structure looks like this:

• A Driver is the most low-level object that Server and Client use to communicate with a vim instance. This is similar to your idea of Command, as I mentioned earlier. It's initialized with an executable, knows how to run command-line queries with the given vim, knows how to kill processes. Makes Shell obsolete.
• A Server is invoked with some options, and asks System about the most appropriate driver based on those options. It holds the process' pid, starts and kills a server instance
• A Client is instantiated with a Server and gets its driver from it. The driver is used to invoke all --remote calls, so if a server has a working driver, a client would as well.
• System has knowledge of the different features a vim instance needs to have in order to work and decides which driver to dispatch to a server. It's basically a renamed Vim with the client-specific dispatching removed -- you're right about the client logic being (hopefully) unnecessary.

Some thoughts:

• I'm thinking of trying to get rid of System. The choose_driver method could be a private method in Server and the features could be on the driver itself. A list of drivers can be prepared based on the given options and a drivers.find(&:suitable?) would find the appropriate one. I'll have to think about that and try it out first, though.
• The specs for System (formerly Vim) seem a bit too nested now. I decided against moving them around right now to avoid missing something in the process, but I'd like to try doing that tomorrow.
• It would be good if we also threw an exception if the given vim path is unavailable or if no vim could be found at all. This may be a point in favor of keeping System, because we'd be able to structure more logic in there for cases like this.
• I'm getting a weird failing spec in Vimrunner::Client#set activates a boolean setting. Not sure why. I think it's interfering with the server specs, since it works just fine if run on its own. It also works if I use a GUI for testing. I'll try to debug it tomorrow.

Feel free to change things around and let me know what you think. I'll work on the points above some more tomorrow.

[mudge]

I think we could get rid of the separate GUI and Headless if we just start both gvim and vim with PTY.spawn: you're right that we don't need a PTY to start a GUI instance but it could clean up the need for several classes. Doing this means we could also ditch the gui? flag and all calling start(:gui => true) would do is affect the choosing process (so it will ignore vim).

I'll try to put together a proof of concept today.

[AndrewRadev]

True, but I'm not sure if the PTY would work. I used to think so, but there was some kind of an issue at some point when the gui version was being started with PTY. Do give it a shot, though, it may have been something else.

[mudge]

Can you please test the following for me on Linux?

require 'pty'
r, w, pid = PTY.spawn('vim', '--servername', 'FOO')
r.close_read
w.close_write #=> the Process quits at this point
Process.kill('QUIT', pid) #=> returns 1

[AndrewRadev]

I don't know if it works as intended for vim -- it does return 1, so I assume it's all right. For gvim, it either doesn't start at all, or, if I sleep 1 to give it a chance to start, it doesn't die.

Personally, I don't mind the subclassing much. It may make sense in the future as well, maybe allowing for some more differences between a gvim and a headless vim. Might be a good idea to have a separate class for mvim as well, though it doesn't seem necessary for now. The problem is simply the dispatching logic. I'll try to clean it right now, I'll probably be able to push something soonish.

[AndrewRadev]

I've attempted to simplify the choice logic. This fails the specs badly, because I haven't adapted System for it. I'll work on re-spec-ing it in a bit. It's possible I've missed something, though, so let me know if you notice anything.

This should also allow us to do:

if not driver.suitable?
  raise "Sorry, couldn't find a suitable driver for this system"
end

...in the server. The idea is that if it's reached the server, there's nothing more we can do (could be a missing vim/gvim executable, or a non-existent custom path).

[AndrewRadev]

Nah, this won't work at all. On a mac, this needs to always start mvim with a gui, but it won't. Man, this is complicated :).

[mudge]

Haha, well I've got a branch that changes the choose_driver logic to something like so:

def vims_to_try(force_gui = false)
  vims = []
  vims << Driver.new("vim") unless force_gui
  vims << Driver.new("mvim") if mac?
  vims << Driver.new("gvim")

  vims
end

def choose_driver(vim_path, force_gui)
  vims = vims_to_try(force_gui)
  vims.unshift(Driver.new(vim_path)) if vim_path

  vim = vims.find { |vim| vim.suitable? }
  raise NoSuitableVimFound, "no client-server capable Vim has been found." unless vim

  vim
end

Note that my suitable? logic is different to yours though: it won't check for xterm_clipboard if the executable is a GUI.

But I'm still trying to figure out the "right way" to deal with PTY. I've finally installed Ubuntu in VirtualBox so I can play around with a real xterm_clipboard supporting Vim and think this is how we can deal with it:

require 'pty'
r, w, pid = PTY.spawn('vim', '-f', '--servername', 'FOO') # => or gvim

# Do whatever we want with the server.

# this will kill the process on a Mac but just render it "defunct" on Linux
r.close_read
w.close_write 

# this will get rid of the process on Linux and do nothing on the Mac and
# is what using PTY.spawn with a block will ensure, 
# c.f. https://github.com/ruby/ruby/blob/trunk/ext/pty/pty.c#L627
Process.detach(pid) 

[AndrewRadev]

Hmm, well, it seems a bit fragile to me. If you can get it to work, then I guess there's no harm in it, but at this point, I'm thinking this might be the simplest working approach:

if File.basename(executable) =~ /^(g|m)/
  pid = Kernel.spawn(...)
else
  _in, _out, pid = PTY.spawn(...)
end

def gui?
  File.basename(executable) =~ /^(g|m)/
end

[mudge]

Just to keep you updated, I've been experimenting with an (almost) one file, miniature Vimrunner at https://gist.github.com/f8a4a88bbda60fbf5cba (the almost is because it requires your vimrc) and the spawning seems to be working well on both Mac and Linux. (Note that it doesn't aim to be fully feature complete but is to test the separation of concerns.)

It allows you to use Vimrunner in one of two main ways:

require 'vimrunner'
Vimrunner.start do |client|
  client.type "ihello"
  client.normal ":w test.txt"
end

client = Vimrunner.start
client.type "ihello"
client.normal ":w test2.txt"
client.close

I'm planning to now try and TDD this into the existing Vimrunner and match the existing interface, etc. Have a look and see what you think regarding the way the different vim executables are dealt with and how responsibility is split between Platform, Server and Client.

[AndrewRadev]

Looks good. I like Platform better than System :). Extracting methods like remote_send and remote_expr is also a pretty good idea. That way, there's no point in the client having an executable at all and the interface looks like it belongs well in the server. A separate Driver becomes unnecessary, too.

I'm a bit worried about the spawning logic, mainly because I don't understand it (what's with the detaches and closing, and why the difference between Linux and Mac), but if it works, it works. I'll just have to sit down these days and try to figure it out.

[mudge]

Regarding the spawning logic, here's how I currently understand it:

Using PTY.spawn is ideal for "interfacing with highly interactive subprocesses" but, as we don't use input and output on the vim subprocess at all, we need to clean up its input and output streams ourselves.

In my experiments, a PTY.spawned mvim will not quit while it still has open handlers (which makes some sense): instead, we need to close both its r and w streams in order to signal that we're done with it (and the PTY allocated).

As for Process.detach and the different behaviour on Mac and Linux: see the documentation of detach for full information but the gist is as follows:

Some operating systems retain the status of terminated child processes until the parent collects that status (normally using some variant of wait(). If the parent never collects this status, the child stays around as a zombie process. Process::detach prevents this by setting up a separate Ruby thread whose sole job is to reap the status of the process pid when it terminates. Use detach only when you do not intent to explicitly wait for the child to terminate.

On a Mac, calling close on both input and output will kill the process immediately but on Linux it will exhibit the "zombie" process behaviour above. We could use Process.wait instead but then we block until the process exits and actually we don't care as long as it finally goes. The thing that actually guided me to this method is the source of using PTY.spawn with a block which boils down to:

static VALUE
pty_detach_process(struct pty_info *info)
{
    rb_detach_process(info->child_pid);
    return Qnil;
}

[AndrewRadev]

Sounds reasonable. It's a bit weird to me that you don't have to explicitly kill the process, but with a pseudo-terminal, I guess not having input or output would leave it with nothing to do except kill itself :). Thanks for the explanation.

[mudge]

So I've just pushed a big commit that tries to (mostly) match the old interface, restore the Client and Server specs where applicable, etc.

I've tested it on both Mac OS X and Ubuntu 12.04 and it seems to work pretty well. We still need to cover the Server and Vimrunner module itself with some tests and documentation regarding the new block interface.

I added in Vimrunner.start_gvim for explicitly starting a GUI vim (though we could put this back to start_gui_vim for compatibility) and you should be able to start a custom server by simply doing:

Vimrunner::Server.new("my custom vim path").start

I also started to conform to TomDoc but wanted to push up what I had so far so we could review it together.

[travisbot]

This pull request passes (merged 9a8c8453 into f31f1345).

[travisbot]

This pull request passes (merged dffc074c into f31f1345).

[AndrewRadev]

Looks great. I added simplecov to the project to catch some areas we may not have specced. Apart from the server spawning, there's just a few small spots left.

I'll try to finish the specs and work on writing documentation this evening.

[mudge]

One thing before we cut a new release: it might be a good idea to use something like Semantic Versioning regarding API changes so that people can use:

gem 'vimrunner', '~> 0.1.0'

Or similar in their Gemfiles. I was personally bitten by this moving from 0.0.2 to 0.0.4 as I had specified ~> 0.0.2.

[AndrewRadev]

SemVer specifies that versions before 1.0 are not subject to the rules, so that you can experiment wth the API a lot before coming to the decision to maintain backwards compatibility. Otherwise, you'd never be able to remove any public API without releasing 1.0.

As for the dependency problem, I'd say for any gem under 1.0 it's reasonable to do a hard requirement instead:

gem 'vimrunner', '0.0.4'

[mudge]

Ah, that's fair enough; I did wonder how it applied to sub 1.0.0.

[travisbot]

This pull request passes (merged 8d764a74 into f31f1345).

[AndrewRadev]

I've written some docs by following the TomDoc spec, but it's my first time with it, so do correct me if I've messed up somewhere. I'm not sure if we should also document the private methods, it seems a bit of a waste at this point in time.

I've yet to write the missing specs and maybe a few tweaks that I had in mind. I should also update the README. I'll probably have to leave those for the weekend.

[travisbot]

This pull request passes (merged 7ddf1661 into f31f1345).

[mudge]

I've added specs to cover the missing parts of the code and a little more documentation where needed but we still need to revise the README, version and gemspec.

[travisbot]

This pull request passes (merged 8bb3d663 into f31f1345).

[travisbot]

This pull request passes (merged ab298ee0 into f31f1345).

[AndrewRadev]

Thanks. Sorry I wasn't able to rewrite the README myself, turned out to be a busy weekend.

I'm going to merge this into master, bump the version to 0.1.0 and release a new gem. The minor version bump is kinda arbitrary, but in my Vim plugins, I use it to show some new functionality or API change, and I keep patch versions for bug fixes and refactoring. Seems like a reasonable way to go until a 1.0 release.