Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

Replace hardcoded gemfile path with methods to discover Gemfile for bundle binstub #6469

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 32 additions & 3 deletions lib/bundler/templates/Executable.bundler
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,39 @@ m = Module.new do
end

def gemfile
gemfile = ENV["BUNDLE_GEMFILE"]
return gemfile if gemfile && !gemfile.empty?
gemfile = find_gemfile
return unless gemfile
File.expand_path(gemfile)
end

def find_gemfile
given = ENV["BUNDLE_GEMFILE"]
return given if given && !given.empty?
find_file(*gemfile_names)
end

File.expand_path("../<%= relative_gemfile_path %>", __FILE__)
def gemfile_names
["Gemfile", "gems.rb"]
end

def find_file(*names)
search_up(*names) do |filename|
return filename if File.file?(filename)
end
end

def search_up(*names)
previous = nil
current = File.expand_path(Dir.pwd)
Copy link
Member

Choose a reason for hiding this comment

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

should this use the binstub directory instead of the current working directory?

Copy link
Author

Choose a reason for hiding this comment

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

It should be the current working directory. If a Gemfile is not found in that current directory it should continue searching upwards to find a Gemfile, similar to SharedHelpers.find_gemfile.

The problem that this PR addresses is that when a bundler binstub is created and then used to install other gem executables, the bundler binstub currently stores the path of the Gemfile used in the last successful bundle install. Trying to run bundle exec when bundling against multiple gemfiles could result in a Gem::Exception or LoadError because bundler may not be using the correct Gemfile path.

From #6162:

...bundle exec foo should always find the Gemfile of the current directory (when BUNDLE_GEMFILE is not set) and then run the foo for that Gemfile.

Copy link
Member

Choose a reason for hiding this comment

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

but this isn't bundle exec foo, this is bin/foo ?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not quite sure I follow...

This particular template, lib/bundler/templates/Executable.bundler, sets up a special bundler binstub that ensures the correct version of bundler is activated (commit). When bundle exec foo is run, this binstub is executed and loads a version of bundler.

The bundler binstub sets ENV['BUNDLE_GEMFILE'], which is then passed along to bundler. At this point, if ENV['BUNDLE_GEMFILE'] is pointing to a gemfile that doesn't include foo, a Gem::Exception or LoadError may be raised even if bin/foo exists.

Not sure if this helps, but it may be easier to follow through this Bundler repro script for bundler issue 6154.

Copy link
Member

Choose a reason for hiding this comment

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

correct, but you quoted me documentation for bundle exec, and how it should find a gemfile. Shouldn't the bundle binstubs binstubs always refer to the Gemfile they were generated with, even if executed from another directory?

Copy link
Author

Choose a reason for hiding this comment

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

I see what you're saying... yes, I agree, except in the case of bundler itself (bin/bundle). The binstub for bundle shouldn't refer to the Gemfile that it was generated with as it runs into the previously mentioned issues.

Copy link
Member

Choose a reason for hiding this comment

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

the only thing is that the other binstubs all use the same logic as the bundle binstubs when it comes to this, iirc

Copy link
Author

Choose a reason for hiding this comment

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

It's okay if the non-bundler binstubs store the path of the gemfile that created them, but the act of using the bundler binstub to load another gem doesn't always work (bin/bundle exec foo).


until !File.directory?(current) || current == previous
names.each do |name|
filename = File.join(current, name)
yield filename
end
previous = current
current = File.expand_path("..", current)
end
end

def lockfile
Expand Down
61 changes: 61 additions & 0 deletions spec/commands/binstubs_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,67 @@
end
end
end

context "when working with multiple gemfiles" do
let(:bundle_binstub) { bundled_app("bin/bundle") }
let(:load_bundle_exe) { "load Gem.bin_path(\"bundler\", \"bundle\")" }
let(:output_bundle_gemfile) { "puts ENV[\"BUNDLE_GEMFILE\"]" }

before do
build_repo2 do
build_gem "print_hello", "1.0" do |s|
s.executables = "print_hello"
s.bindir = "exe"
s.write "exe/print_hello", <<-R
puts "Hello!"
R
end
build_gem "print_goodbye", "1.0" do |s|
s.executables = "print_goodbye"
s.bindir = "exe"
s.write "exe/print_goodbye", <<-R
puts "Goodbye."
R
end
end

bundle! "config --global path #{bundled_app}"
bundle! "config --global bin #{bundled_app("bin")}"

install_gemfile! <<-G
source "file://#{gem_repo2}"
G
bundle! "binstubs bundler"
end

it "uses the correct Gemfile" do
create_file "one/Gemfile", <<-G
source "file://#{gem_repo2}"
gem "print_hello"
G
create_file "two/Gemfile", <<-G
source "file://#{gem_repo2}"
gem "print_goodbye"
G
Dir.chdir("one") { bundle! "install" }
Dir.chdir("two") { bundle! "install" }

binstub = File.read(bundled_app("bin/bundle"))
# Replace load statement with BUNDLE_GEMFILE puts
binstub.gsub!(load_bundle_exe, output_bundle_gemfile)
Copy link
Member

Choose a reason for hiding this comment

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

can we put these in the binstub for the gem, so that way we won't have to mess with the file we're supposed to be testing?

Copy link
Author

Choose a reason for hiding this comment

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

🤔 I don't believe so... I tried a few different ways, but ENV['BUNDLE_GEMFILE'] is nil outside of the bundler binstub. It is temporary assigned within the scope of the binstub execution.

File.open(bundle_binstub, "w") {|f| f.puts binstub }

Dir.chdir("one") do
sys_exec "#{bundled_app("bin/bundle")} exec print_hello"
expect(out).to eq(bundled_app("one/Gemfile").to_s)
end

Dir.chdir("two") do
sys_exec "#{bundled_app("bin/bundle")} exec print_goodbye"
expect(out).to eq(bundled_app("two/Gemfile").to_s)
end
end
end
end

it "installs binstubs from git gems" do
Expand Down