Fix RubyGemsBundlerInstaller.shebang for gems installing from git. #19

Merged
merged 1 commit into from May 15, 2012

3 participants

@whitequark

No description provided.

@mpapis mpapis was assigned May 14, 2012
@mpapis
Ruby enVironment Manager member

@whitequark I think it is already fixed with version 1.0.0 ... which was released just few minutes ago, please test again

@whitequark

Nope, it's still broken. Looks like you need a 1.0.1 ;)

@mpapis
Ruby enVironment Manager member

that is weird, I thought inst.respond_to?('options') ? inst.options[:env_shebang] :inst.instance_variable_get(:@env_shebang) solves this problem ... can you rebase your fix to current codebase ?

@whitequark

Updated

@mpapis
Ruby enVironment Manager member

what is the difference between those two:

inst.respond_to?('options') && inst.options

i thought the first checks existence of options already, not trying to reject it - just trying to find out why.

@whitequark

Here, inst responds to method options, but a call to inst.options returns nil. Thus, while inst.options is valid code, any method call on inst.options (including inst.options[:whatever], which desugars to inst.options.[](:whatever), you can try the latter in irb) would fail.

@mpapis mpapis commented on the diff May 15, 2012
lib/rubygems-bundler/rubygems_bundler_installer.rb
@@ -24,7 +24,11 @@ def self.bundler_generate_bin(inst)
def self.shebang(inst, bin_file_name)
# options were defined first in 1.5, we want to support back to 1.3.7
- ruby_name = Gem::ConfigMap[:ruby_install_name] if inst.respond_to?('options') ? inst.options[:env_shebang] :inst.instance_variable_get(:@env_shebang)
+ options_supported = inst.respond_to?('options') && inst.options
+ if (options_supported && inst.options[:env_shebang]) || inst.instance_variable_get(:@env_shebang)
@mpapis
Ruby enVironment Manager member
mpapis added a note May 15, 2012

I think this line should be:

if inst.respond_to?('options') ? inst.options && inst.options[:env_shebang] : inst.instance_variable_get(:@env_shebang)

it prevents reading @env_shebang in cases like:

options == nil
options == {}
options == {:env_shebang=>nil}

it has no difference as long as there is no @env_shebang in rubygems ... but can we do assume that?

I've no idea. I don't really understand what inst is, anyway.

@mpapis
Ruby enVironment Manager member
mpapis added a note May 15, 2012

inst is an instance of https://github.com/rubygems/rubygems/blob/1.8/lib/rubygems/installer.rb#L27,
it gets options here https://github.com/rubygems/rubygems/blob/1.8/lib/rubygems/installer.rb#L94
and they are parsed here https://github.com/rubygems/rubygems/blob/1.8/lib/rubygems/installer.rb#L412

comparing process_options implementations:
https://github.com/rubygems/rubygems/blob/master/lib/rubygems/installer.rb#L550
https://github.com/rubygems/rubygems/blob/1.8/lib/rubygems/installer.rb#L412 (1.5- 1.8 are similar)
https://github.com/rubygems/rubygems/blob/1.4/lib/rubygems/installer.rb#L96 (1.3-1.4 does it in initialize)

@options + attr_reader :options are not available bellow 1.5
@env_shebang is available in every version ... this code could be simplified to just:

ruby_name = Gem::ConfigMap[:ruby_install_name] if inst.instance_variable_get(:@env_shebang)

unless there is something that prevents us from doing this ... but I do not think so.

even the implementation would change in master this code is here to backport it to older versions of rubygems, it would not be required when in rubygems 2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mpapis mpapis merged commit 1bc8f1a into rvm:master May 15, 2012
@mpapis mpapis added a commit that referenced this pull request May 15, 2012
@mpapis mpapis simplify the env_shebang check, update #19 ed592d7
@mpapis
Ruby enVironment Manager member

@whitequark could you have a try with my fix?

@whitequark

Yeah, current master works just fine, thank you.

@kucaahbe

👍 had the same issue, works just fine with current master

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