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

Add check for BUNDLE_GEMFILE in cli #6331

Closed

Conversation

agrim123
Copy link
Contributor

@agrim123 agrim123 commented Mar 7, 2018

Thanks so much for the contribution!
To make reviewing this PR a bit easier, please fill out answers to the following questions.

What was the end-user problem that led to this PR?

The problem was BUNDLE_GEMFILE is not used when .bundle/config specifies an alternate Gemfile.

What was your diagnosis of the problem?

My diagnosis was bundler was copying the config variables and aways using them.

What is your fix for the problem, implemented in this PR?

My fix was to check if the user has provided BUNDLE_GEMFILE option, then give priority to this env variable.

Why did you choose this fix out of the possible options?

I chose this fix because checking and setting the gemfile variable earlier in the process is the best way to give precedence to env variable over config variable.

Fixes #6270

@colby-swandale
Copy link
Member

colby-swandale commented Mar 7, 2018

This needs a test to verify the changes work as expected.

@agrim123
Copy link
Contributor Author

agrim123 commented Mar 7, 2018

Sure. Where does the test reside for this? 😅
This seems to be the target file https://github.com/bundler/bundler/blob/master/spec/bundler/cli_spec.rb?

@agrim123 agrim123 force-pushed the bundle_install_gemfile_option branch from c858364 to a4e06c6 Compare March 7, 2018 06:25
@@ -34,8 +34,14 @@ def initialize(*args)
super

custom_gemfile = options[:gemfile] || Bundler.settings[:gemfile]

if ENV["BUNDLE_GEMFILE"] && !ENV["BUNDLE_GEMFILE"].empty?
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to set custom_gemfile with ENV["BUNDLE_GEMFILE"] if we're going to skip using it using unless ENV["BUNDLE_GEMFILE"]

Copy link
Contributor Author

@agrim123 agrim123 Mar 9, 2018

Choose a reason for hiding this comment

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

Then we will have to move Bundle.reset_paths! outside because we want to run it in any case?

@@ -51,6 +51,22 @@
end
end

context "when ENV['BUNDLE_GEMFILE'] is specified" do
before { create_file "Gemfile.dev" }
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to create_file, gemfile will create it for us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@colby-swandale
Copy link
Member

The test in this PR will continue to pass without the fix. We need to make sure we have a failing test firstly.

@agrim123
Copy link
Contributor Author

agrim123 commented Mar 9, 2018

The current flow is

$ bundle install  # Uses gemfile specified in the config file
$ BUNDLE_GEMFILE=Gemfile bundle install  # Uses this specified file
$ BUNDLE_GEMFILE=  bundle install  # Searches for default gemfiles (Gemfile, gems.rb)

Is this our desired operation?

@agrim123 agrim123 force-pushed the bundle_install_gemfile_option branch from 14b90e7 to 851e9b3 Compare March 9, 2018 06:39
@agrim123
Copy link
Contributor Author

agrim123 commented Mar 9, 2018

I tried something like this

before { Bundler.settings.set_local :gemfile, "Gemfile.dev" }

before the context block but still it is looking for Gemfile not Gemfile.dev in the first test. 😢

@agrim123
Copy link
Contributor Author

agrim123 commented Mar 9, 2018

@colby-swandale
Observed something weird!

The test that I wrote first was

it "uses the value specified in the config file" do
  bundle :install

  expect(the_bundle).to include_gems "rack 1.0.0"
end

was failing because when Bundle.setup() is called then it does not respect config variables, which resulted in test failures.

$ /home/user/.rvm/rubies/ruby-2.4.1/bin/ruby -I/home/user/projects/bundler/lib -e  \
  <<EOS
  require 'rubygems' ; require 'bundler' ; Bundler.setup()
  require 'rack.rb'; puts RACK
EOS
/home/user/projects/bundler/lib/bundler/shared_helpers.rb:34:in `default_gemfile': Could not locate Gemfile (Bundler::GemfileNotFound)
	from /home/user/projects/bundler/lib/bundler.rb:328:in `default_gemfile'
	from /home/user/projects/bundler/lib/bundler.rb:135:in `definition'
	from /home/user/projects/bundler/lib/bundler.rb:101:in `setup'
	from -e:1:in `<main>'
# $? => 1

But on modifying it to

it "uses the value specified in the config file" do
  bundle :install
  bundle :list

  expect(out).to include "rack (1.0.0)"
end

works. This happens because bundle :list calls the cli which picks up the config variable.

While trying to fix the problem, I found this find_gemfile function, which does not look for Bundler.settings.

So, I tried something like this (a way to give respect to config var)

def find_gemfile(order_matters = false)
  given = ENV["BUNDLE_GEMFILE"]
  return given if given && !given.empty?

  # Check for gemfile config variable
  from_config = Bundler.settings[:gemfile]
  return from_config if from_config && !from_config.empty?
  names = gemfile_names
  names.reverse! if order_matters && Bundler.feature_flag.prefer_gems_rb?
  find_file(*names)
end

But, this resulted in Stack level too deep (SystemStackError) error.

An error occurred while loading ./spec/bundler/cli_spec.rb.
Failure/Error: gemfile = find_gemfile

SystemStackError:
  stack level too deep
# ./lib/bundler.rb:232:in `root'
# ./lib/bundler.rb:244:in `app_config_path'
# ./lib/bundler.rb:271:in `settings'
# ./lib/bundler.rb:232:in `root'
# ./lib/bundler.rb:244:in `app_config_path'
# ./lib/bundler.rb:271:in `settings'
# ./lib/bundler.rb:232:in `root'
# ./lib/bundler.rb:244:in `app_config_path'
# ./lib/bundler.rb:271:in `settings'
# ./lib/bundler.rb:232:in `root'
# ./lib/bundler.rb:244:in `app_config_path'
# ./lib/bundler.rb:271:in `settings'
# ./lib/bundler.rb:232:in `root'
# ./lib/bundler.rb:244:in `app_config_path'
# ./lib/bundler.rb:271:in `settings'
# ./lib/bundler.rb:232:in `root'
# ./lib/bundler.rb:244:in `app_config_path'
# ./lib/bundler.rb:271:in `settings'
# ./lib/bundler.rb:232:in `root'
# ./lib/bundler.rb:244:in `app_config_path'
# ./lib/bundler.rb:271:in `settings'
# ./lib/bundler.rb:232:in `root'
# ./lib/bundler.rb:244:in `app_config_path'
# ./lib/bundler.rb:271:in `settings'
# ./lib/bundler.rb:232:in `root'
# ./lib/bundler.rb:244:in `app_config_path'
# ./lib/bundler.rb:271:in `settings'
# ./lib/bundler.rb:232:in `root'
# ./lib/bundler.rb:244:in `app_config_path'
...

app_config_path seems to be calling root which calls root in shared_helpers that has call to that same function find_gemile. Thus resulting in infinite recursion.

This seems like a bug!

Any suggestions?

@agrim123 agrim123 force-pushed the bundle_install_gemfile_option branch from 8a671ac to 06c9136 Compare March 9, 2018 11:36
@colby-swandale
Copy link
Member

colby-swandale commented Mar 9, 2018

Apologies, I've screwed up here. Looking into this issue further, the issue reported at #6270 is actually intended behavior of Bundler (to my surprise). The local configuration takes precedent over ENV and merging this PR would be technically breaking functionality, which we don't want to do. Apologies for not looking into this sooner and figuring out this isn't actually a bug.

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.

BUNDLE_GEMFILE does not override .bundle/config path to Gemfile
2 participants