tpope / vim-bundler

bundler.vim: Lightweight support for Ruby's Bundler
https://www.vim.org/scripts/script.php?script_id=4280
404 stars 29 forks source link

Fix handling for ruby version-specific gem paths #34

Closed a-warner closed 10 years ago

a-warner commented 10 years ago

It seems that, depending on what version of ruby you have, bundler will use subtly different paths when gems are bundled locally using BUNDLE_PATH.

Let's say your gems are bundled in /path/to/project/vendor/bundle because you set BUNDLE_PATH to ./vendor/bundle in .bundle/config, and you have BUNDLE_DISABLED_SHARED_GEMS set. If you're on ruby 1.8.x, your bundled gems will be in /path/to/project/vendor/bundle/ruby/1.8/. On 1.9.x, your gems will be in /path/to/project/vendor/bundle/ruby/1.9.1/.

This commit handles the two different special cases for ruby 1.8 and ruby 1.9.

a-warner commented 10 years ago

@tpope given your deeper familiarity with vimscript/bundler.vim/bundler, I'm confident that you have a better idea for how to handle these cases. It does seem like this info should be available in other places, but this specific thing was really annoying me so I had to at least fix it for myself.

Let me know if you want me to change anything! (or if I'm an idiot and there is a better way to figure out the bundle paths)

tpope commented 10 years ago

What was the incorrectly detected ABI version that you're overriding?

a-warner commented 10 years ago

so as far as I can tell the regex wasn't even matching anything for my 1.8 or 1.9 ruby...I'm pretty bad with vimscript but I did manage to dump some debugging stuff into a file and find that it simply wasn't matching at all.

Here is one example for a $GEM_PATH that I was testing with:

$ cat .bundle/config 
--- 
BUNDLE_WITHOUT: production
BUNDLE_PATH: ./vendor/bundle
BUNDLE_DISABLE_SHARED_GEMS: "1"
$ echo $GEM_PATH
/Users/Andrew/.rvm/gems/ruby-1.9.3-p286@rails3-test:/Users/Andrew/.rvm/gems/ruby-1.9.3-p286@global

The regex was previously: '[0-9.]\+$', and it should have been trying to match /Users/Andrew/.rvm/gems/ruby-1.9.3-p286@rails3-test and extract "1.9.3" (I think?)

Anyway, that regex wouldn't match that because of of the $...

In any case, my gems live in ./vendor/bundle/ruby/1.9.1/ after bundling with BUNDLE_PATH, so I'm not sure how the regex would extract 1.9.1 from /Users/Andrew/.rvm/gems/ruby-1.9.3-p286@rails3-test, but it's entirely possible that something else is going on / I'm missing something...

tpope commented 10 years ago

It's trying to match the default load path rather than the gem path, actually. Here's mine with rbenv:

$ ruby -e 'puts $:'
/home/tpope/.rbenv/versions/2.0.0-p247/lib/ruby/site_ruby/2.0.0
/home/tpope/.rbenv/versions/2.0.0-p247/lib/ruby/site_ruby/2.0.0/x86_64-linux
/home/tpope/.rbenv/versions/2.0.0-p247/lib/ruby/site_ruby
/home/tpope/.rbenv/versions/2.0.0-p247/lib/ruby/vendor_ruby/2.0.0
/home/tpope/.rbenv/versions/2.0.0-p247/lib/ruby/vendor_ruby/2.0.0/x86_64-linux
/home/tpope/.rbenv/versions/2.0.0-p247/lib/ruby/vendor_ruby
/home/tpope/.rbenv/versions/2.0.0-p247/lib/ruby/2.0.0
/home/tpope/.rbenv/versions/2.0.0-p247/lib/ruby/2.0.0/x86_64-linux

See those 2.0.0's in there? That's what we're targeting. Pretty sure I tried it on RVM too but I don't have an install handy to check. What does yours have?

a-warner commented 10 years ago

@tpope I agree that's a much better way to do it, but it looks like it goes after GEM_PATH if it's set?

https://github.com/a-warner/vim-bundler/blob/master/plugin/bundler.vim#L275-L278

let gem_paths = []
if exists('$GEM_PATH')
  let gem_paths = split($GEM_PATH, has('win32') ? ';' : ':')
else

(vs load path which I agree is wayyy better)

so maybe the right fix is something like:

let load_paths = split(system(prefix.'ruby -rubygems -e "print $:.join(%(;))"'), ';')
let abi_version = matchstr(get(load_paths, 0, '1.9.1'), '[0-9.]\+$')

or is that somehow what was already happening, and I'm in some weirdo state?

(updated to embed the code)

a-warner commented 10 years ago

for reference, my $: is:

$ ruby -e "puts $:"
/Users/Andrew/.rvm/rubies/ruby-1.9.3-p286/lib/ruby/site_ruby/1.9.1
/Users/Andrew/.rvm/rubies/ruby-1.9.3-p286/lib/ruby/site_ruby/1.9.1/x86_64-darwin12.2.0
/Users/Andrew/.rvm/rubies/ruby-1.9.3-p286/lib/ruby/site_ruby
/Users/Andrew/.rvm/rubies/ruby-1.9.3-p286/lib/ruby/vendor_ruby/1.9.1
/Users/Andrew/.rvm/rubies/ruby-1.9.3-p286/lib/ruby/vendor_ruby/1.9.1/x86_64-darwin12.2.0
/Users/Andrew/.rvm/rubies/ruby-1.9.3-p286/lib/ruby/vendor_ruby
/Users/Andrew/.rvm/rubies/ruby-1.9.3-p286/lib/ruby/1.9.1
/Users/Andrew/.rvm/rubies/ruby-1.9.3-p286/lib/ruby/1.9.1/x86_64-darwin12.2.0
tpope commented 10 years ago

Oh, look, you're right. I was thinking of some earlier rolled back behavior.

My rbenv gem path does have 2.0.0 in it. But I'm guessing if you check Gem.path on rvm (skipping the $GEM_PATH check), it's lacking the ABI version as well. If so, I think we should probably just find a way to ask Ruby for the ABI version directly.

a-warner commented 10 years ago

@tpope that's correct – what's wrong with ruby -e "print $:.first.split('/').last"? (doesn't ask ruby directly, but some googling around didn't immediately reveal a good solution for that)

tpope commented 10 years ago

It fails if the $RUBYLIB environment variable is set. One alternative would be ruby -rrbconfig -e "print RbConfig::CONFIG['ruby_version']".

Another solution would be to bring back the g:ruby_version_paths shenanigans, but use it only to determine the ABI version. Advantage of this approach is it should get the right path for the project even if the current directory is wrong. Disadvantage is that in the case of RVM, you need rvm.vim installed (although it will work without it if the current directory is correct).

a-warner commented 10 years ago

Off the top of my head ruby -rrbconfig -e "print RbConfig::CONFIG['ruby_version']" seems the simplest (especially given the rvm edge cases).

Is there any case (that you know about) where that won't work? The only case I can think of is something like: open vim, :cd into a project directory. (which doesn't seem to work in the rvm case but I think will work for rbenv?)

a-warner commented 10 years ago

@tpope I'm down to make the RbConfig change if you think that's the best option – thoughts? I'm not really familiar with how the ruby_version_paths stuff worked, but I'm down to mess around with that as well if you point me towards some old code

tpope commented 10 years ago

Submitted this yesterday but it didn't take:

Yeah the cd issue (which also includes editing a file not in your current directory.)

The advantage of using g:ruby_version_paths is that it fixes the cd issue and handles a lot of the heavy lifting, including the try/cd boilerplate and caching (execs aren't free). Disadvantage is a bit more work with filter(), plus you need a fallback if it's not available as it's a recent vim-ruby addition. I'd recommend this approach but I'll entertain the rbconfig version as well, so long as you wrap it in the try/cd construct.

a-warner commented 10 years ago

@tpope sorry I let this sit so long – I'll try to have something this weekend. In the meantime, can you explain what you mean by try / cd? Are you saying, trying RbConfig and then attempting to cd up until getting to a "root" project directory and then trying again?

tpope commented 10 years ago

I mean like https://github.com/vim-ruby/vim-ruby/blob/2597860f4ee117d6d762d6caa98e73e89e41769b/ftplugin/ruby.vim#L82-91.

a-warner commented 10 years ago

ok @tpope is this what you had in mind? take a look and let me know. I tested it on rvm but I don't have an rbenv install handy – if you like the approach it's probably worth double checking that I didn't break rbenv support before merging. I can check monday with an rbenv machine but if you want to merge before then I'm down to :fire:

Another somewhat related issue that I'll probably submit as a separate pull is that I found that bufkill.vim prints an annoying message when doing something like:

This only happens with locally bundled gems, BUNDLE_DISABLE_SHARED_GEMS set, and the BUNDLE_PATH ends with a /. I "fixed" it with this diff – should I file a separate pull or not?

diff --git a/plugin/bundler.vim b/plugin/bundler.vim
index f8a2922..2463480 100644
--- a/plugin/bundler.vim
+++ b/plugin/bundler.vim
@@ -293,7 +293,7 @@ function! s:project_paths(...) dict abort
     for config in [expand('~/.bundle/config'), self.path('.bundle/config')]
       if filereadable(config)
         let body = join(readfile(config), "\n")
-        let bundle_path = matchstr(body, "\\C\\<BUNDLE_PATH: '\\=\\zs[^\n']*")
+        let bundle_path = substitute(matchstr(body, "\\C\\<BUNDLE_PATH: '\\=\\zs[^\n']*"), '/$', '', '')
         if !empty(bundle_path)
           if body =~# '\C\<BUNDLE_DISABLE_SHARED_GEMS:'
             let gem_paths = [self.path(bundle_path, 'ruby', abi_version)]
tpope commented 10 years ago

I can put it through its paces Monday on my own rbenv install.

Yeah, go ahead and make a separate request for the other thing. No problem merging it in.

a-warner commented 10 years ago

Great! The other diff is over at https://github.com/tpope/vim-bundler/pull/35

a-warner commented 10 years ago

ok so I verified that this one is fine with rbenv