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

Fix for bundler path issue in Cucumber Rake task #386 #388

Closed
wants to merge 4 commits into from

Conversation

y-higuchi
Copy link

This is a fix for #386.

It changes the method to obtain the bundle command name from
Gem.default_exec_format % 'bundle' to File.basename( Gem.bin_path('bundler', 'bundle') ) when building the command line in Cucumber Rake task.

@@ -92,7 +92,7 @@ def gem_available_new_rubygems?(gemname)

def cmd
if use_bundler
bundle_cmd = Gem.default_exec_format % 'bundle'
bundle_cmd = File.basename( Gem.bin_path('bundler', 'bundle') )
Copy link
Member

Choose a reason for hiding this comment

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

Could you please extract this code into separate method. To remove code duplication and make this code more readable.

Copy link
Author

Choose a reason for hiding this comment

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

I've extracted the duplicate as ForkedCucumberRunner#bundle_cmd method. d08dc89

@os97673
Copy link
Member

os97673 commented Mar 6, 2013

It looks like jruby is also affected by the problem, and the fix works for it too.

@y-higuchi
Copy link
Author

I've been checking the rubygems source code, and found out that this Gem.bin_path approach will not work on an environment, where bundle command has suffixes. (original issue #324 situation)


Gem.bin_path approach problem detail

Gem.bin_path 'bundler', 'bundle' is implemented by simply appending the second argument to the bin directory of the bundler gem package, thus File.basename( Gem.bin_path 'bundler', 'bundle' ) will always result in 'bundle'.
definition of Gem.bin_path
definition of spec.bin_file called inside Gem.bin_path


As an alternative approach, I found an interesting comment in Bundler Issue about speeding up bundle exec.
According to the bundler developer's comment, bundle exec foo is equivalent to ruby -rbundler/setup foo.
This approach only requires the path to the bundler gem resides, and eliminates the need to determine the bundler command name.

I have added an commit for this alternative approach.
I've tested on Ubuntu environment, where ruby1.8 is suffixed and bundler is not suffixed.
I haven't been able to test this on a Windows environment yet, since I won't have access to them until sometime next week.

@os97673
Copy link
Member

os97673 commented Mar 7, 2013

Hm, I've re-read #324 and I'm not sure it fix a real problem :( At least when I've built ruby with --program-prefix and --program-suffix and than installed bundler, I still have bundle binary, not bundle.
So, I'd say we just need to back out the changes for #324.
@mattwynne @y-higuchi what do you think about this?

@y-higuchi
Copy link
Author

Reverting #324 will fix the problem I'm facing, so that will be fine for me.
One thing I'm concerned is that, the person who reported #324 seems to be one of the top contributers of rubygems so there may be some circumstances, that we're not aware of, where bundler installed from gems will have suffixes.

@os97673
Copy link
Member

os97673 commented Mar 8, 2013

I agree there is a possibility to break something (again) but since #324 was integrated into 1.2.2 and it breaks more common situations (such as jruby and mri installed using apt-get on ubuntu) I think it is safer to simply revert the changes and allow the contributor or someone else to re-fix the problem with possible prefix/suffix of bundler's binary.

@os97673 os97673 closed this in e3e0d51 Mar 8, 2013
@os97673
Copy link
Member

os97673 commented Mar 8, 2013

@y-higuchi thank you very match for your deep evaluation of the problem it helps us a lot.
And sorry that we cause such a headache with 1.2.2, I've just released 1.2.3 hope it will be a pain relief for you :)

@y-higuchi
Copy link
Author

@os97673 No problem. It's nothing compared to all the benefit this project has provided me.
I've confirmed that everything now works on my project environment using 1.2.3.
Thank you for the quick release.

os97673 added a commit that referenced this pull request Mar 10, 2013
@lock
Copy link

lock bot commented Oct 25, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants