Fix nested bundle exec's when bundler is a default gem #7248
Conversation
In Ruby master/trunk/ruby-head, the bundler lib folder exists at:
and the exe file is located at:
This is true for both ruby-loco and a Travis build downloaded from http://rubies.travis-ci.org/ Hence, the line exe_file = Bundler.rubygems.bin_path("bundler", "bundle", VERSION) unless File.exist?(exe_file) must remain in the logic to find the exe file., but should be used only after the relative check of: exe_file = File.expand_path("../../../exe/bundle", __FILE__) is done... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MSP-Greg This looks good to me! I made one question about the third condition, since seems incorrect to me and maybe fixing that path will also fix CI for you.
That said, the problem you mentioned is real, and the fix seems good to me, so I'm good with it. I also realize that writing a test for this is pretty hard, so I'm happy to merge this without a test.
lib/bundler/shared_helpers.rb
Outdated
# for Ruby core repository | ||
# bundler is a default gem, exe path is separate | ||
exe_file = Bundler.rubygems.bin_path("bundler", "bundle", VERSION) unless File.exist?(exe_file) | ||
# for Ruby core repository testing | ||
exe_file = File.expand_path("../../../../bin/bundle", __FILE__) unless File.exist?(exe_file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this path incorrect though? I look at current's repository structure in ruby-core and it's like
|--- bin/bundle
|--- lib/bundler/shared_helpers.rb
So, I think one level of ../
is incorrect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sort of. It's incorrect from the source folder.
But, we have three folders, source, build, and install. With ruby-loco, since it is now used in so many repos for testing, I patch the test files and run all testing from install. I also do a CLI check from install (I'll add a nested 'bundle exec' test sometime soon). So I can't really check it from ruby-loco, as it's using the install folder, which finds the file with the addition.
Anyway, long story short, I did a similar PR in Ruby, and tests passed. So I don't know if
File.expand_path("../../../../bin/bundle", __FILE__)
is run from the 'build' folder or the 'source' folder.
I left It as is, I could test it in my Ruby fork on Travis & Appveyor? Am I making sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are. Running the tests from the install folder makes sense to me and it helps catching issues like this one, so thanks.
I'll let @hsbt answer the question about ruby-core testing, but the fix for bundler installed as a default gem looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got a build with a patch with debug output running on Travis now... 20 minutes maybe...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking at that, as I ignored the existing code. Using three levels, the code on Travis finds the bin file, and it's in the build folder. So, I pushed that change also. Maybe core testing doesn't test it...
Re testing and Ruby trunk/master, I added the following to shared_helpers.rb in Ruby: TEMP = File.expand_path("../../../bin/bundle", __FILE__)
STDERR.puts("", "shared_helpers.rb find bundle", TEMP, File.exist?(TEMP), "") Output on Travis was:
Hence, @deivid-rodriguez is correct, only three |
Sorry for all the damn testing, but, when I first tried this in Ruby core, I used an assembled path (File.join), and it passed. But, after reviewing the history, I saw that the following was used: Bundler.rubygems.bin_path("bundler", "bundle", VERSION) So, with the 1st & 2nd versions of this PR, I switched to that. But, unlike File.join, bin_path will raise an error if the path isn't found, which happens when running from the core build folder. So, the third version flips the last two 'path searches' so that the bin_path is ran last. I tested locally with ruby-loco with Ruby Core Travis is here, which passed regarding this issue... |
I think you also need to update the expectation in https://github.com/bundler/bundler/blob/36ce7ccf84968d2a7f0eba98c605a5eac8e17e68/spec/bundler/shared_helpers_spec.rb#L405, so that the test doesn't break when run from ruby-core. |
Currently it's failing because of: rubygems/bundler#7248
Currently it's failing because of: rubygems/bundler#7248
Can we please merge this? |
Sorry for the delay, I'm away from home at the moment. @bundlerbot r+ |
7248: Fix nested bundle exec's when bundler is a default gem r=deivid-rodriguez a=MSP-Greg ### What was the end-user problem that led to this PR? The problem was that when bundler is a default gem, nested `bundle exec` commands generate a LoadError. ``` /home/travis/.rvm/rubies/ruby-head/bin/bundle:30:in `load': cannot load such file -- /home/travis/.rvm/rubies/ruby-head/lib/bin/bundle (LoadError) ``` ### What was your diagnosis of the problem? Not accounting for Bundler being installed as a default gem. When it's a default, the lib and exe folders do not share the same root folder. This was the result of a change in e742c3d (#7100). ### Repo Example Using Ruby master/trunk/ruby-head (as of ruby/ruby@0c6c937), from a folder where `bundle exec` can be ran: ``` bundle exec "bundle exec 'ruby -v'" ``` ### What is your fix for the problem, implemented in this PR? Small adjustment to logic for finding the correct exe/bundle file. ### Why did you choose this fix out of the possible options? I chose this fix because it's similar to previous code. Fixes #7244. Co-authored-by: MSP-Greg <msp-greg@users.noreply.github.com>
Thanks so much @MSP-Greg! |
Build succeeded |
Thanks for this! A bunch of my specs which use ruby-head and bundler were broken and I didn't know why! |
You're welcome, and thanks for all your work in core and the 'socketry' gems. We still need it pushed into the bundler copy in core for some repo's CI. Similar to: It's tough to test without a real installation, but I think I can add a test for it in ruby-loco. It's not 'helpful' when ruby-head CI fails in external repos... |
Yeah, it started failing for me and I didn't know what was going on. Maybe can discuss it with core team. |
The ruby 'install' has broken before. Previously, the CLI bin scripts broke. It got fixed quickly, I don't recall where the breakage was or whether it was platform specific. It was quickly fixed. |
Thanks for your work pushing bundler updates to Ruby trunk/master. The current Travis ruby-head build includes the updates, and I checked the repo with the nested bundle commands. Travis was green. JFYI, ruby-loco had a problem with I added another statement that found the file using RbConfig. Probably won't work with prefix/suffix, but I've never tried using them on Windows, as I suspect the *.dll mapping isn't set up to deal with it. Thanks again... |
What was the end-user problem that led to this PR?
The problem was that when bundler is a default gem, nested
bundle exec
commands generate a LoadError.What was your diagnosis of the problem?
Not accounting for Bundler being installed as a default gem. When it's a default, the lib and exe folders do not share the same root folder.
This was the result of a change in e742c3d (#7100).
Repo Example
Using Ruby master/trunk/ruby-head (as of ruby/ruby@0c6c937), from a folder where
bundle exec
can be ran:What is your fix for the problem, implemented in this PR?
Small adjustment to logic for finding the correct exe/bundle file.
Why did you choose this fix out of the possible options?
I chose this fix because it's similar to previous code.
Fixes #7244.