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

Add flag to generate generic binstubs #6524

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@eanlain

eanlain commented May 8, 2018

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

Issues #6154 and #6162, in which Bundler v1.16.1 appeared to be looking for the wrong Gemfile when using Docker. Further investigation showed that this issue was not just limited to the use of Docker (see non-Docker repro of 6154).

What was your diagnosis of the problem?

The diagnosis was that as of 1.16 Bundler started generating a bin/bundle and calling it from other binstubs. This change led to two problems (taken from conversation in #6469):

  1. Docker (and other users) used to be able to treat bundler binstubs as if they were rubygems binstubs, not tied to any application, and usable on any ruby where that gem was installed. This change made that no longer true, and tied each binstub a to a specific application and Gemfile.
  2. Previously, bundle exec CMD would find the Gemfile based on the CWD, and even if Bundler exec’ed a binstub from the PATH that was tied to a different application, that was fine, because binstubs were fully generic. It sounds like the bin/bundle binstub no longer does that, and instead when you run bin/bundle exec rspec, you get bin/rspec, which is tied to a specific application and Gemfile.

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

My fix was to add a flag to the binstubs command to generate application-locked binstubs and to make that flag the default for Bundler >= 2.

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

I chose this fix because of the conversation from #6469.

@segiddins

This comment has been minimized.

Member

segiddins commented May 8, 2018

Can you please add a test for the change in behavior as well, in addition to the contents of the generated file?

@eanlain

This comment has been minimized.

eanlain commented May 11, 2018

Added some additional tests.

@eanlain

This comment has been minimized.

eanlain commented May 17, 2018

@segiddins any other changes needed?

@@ -317,6 +317,8 @@ def info(gem_name)
"Binstub destination directory (default bin)"
method_option "shebang", :type => :string, :banner =>
"Specify a different shebang executable name than the default (usually 'ruby')"
method_option "locked", :type => :boolean, :default => Bundler.bundler_major_version >= 2, :banner =>

This comment has been minimized.

@segiddins

segiddins May 25, 2018

Member

can this use a feature flag instead of directly checking the version?

This comment has been minimized.

@eanlain

eanlain May 25, 2018

👍 I added a commit to use a feature flag instead of directly checking the version

@@ -119,6 +119,8 @@ def generate_bundler_executable_stubs(spec, options = {})
relative_gemfile_path = relative_gemfile_path
ruby_command = Thor::Util.ruby_command
ruby_command = ruby_command
application_locked = options[:locked] || Bundler.settings[:application_locked]

This comment has been minimized.

@segiddins

segiddins May 28, 2018

Member

Bundler.feature_flag.application_locked?

This comment has been minimized.

@eanlain

eanlain May 28, 2018

The use of Bundler.settings[:application_locked] is just returning whether the :application_locked setting is set for this run of the installer as opposed to returning the state of the feature. For example, if using Bundler >= 2 and using --locked=false with the binstub command.

@segiddins segiddins requested a review from indirect May 28, 2018

@colby-swandale colby-swandale added this to the 1.17.0 milestone May 29, 2018

@indirect

indirect requested changes Jul 1, 2018 edited

As some background information, the binstubs command was created specifically to create binstubs that are locked to a single application.

If you want binstubs that are not locked to the given application, you can currently use the binstubs that are already generated for every Bundle, at $BUNDLE_PATH/ruby/${RUBY_VERSION}/bin/.

I think I would be okay a flag that caused Bundler to generate additional global binstubs on demand, if you need that for some reason.

Is there something stopping you from using those binstubs that already exist?

@@ -40,6 +40,8 @@ def run
Bundler.settings.temporary(:path => (Bundler.settings[:path] || Bundler.root)) do
installer.generate_standalone_bundler_executable_stubs(spec)
end
elsif gem_name == "bundler" && options[:locked] == false
next Bundler.ui.warn("Sorry, Bundler can only be run via RubyGems.")

This comment has been minimized.

@indirect

indirect Jul 1, 2018

Member

I'm not positive, but I think this is a change from our current behavior? For example, we work around the Rails binstub not fully loading Bundler by telling users to overwrite it with bundle binstubs bundler --force. It seems like this will stop that from working?

@@ -29,6 +29,7 @@ def self.settings_method(name, key, &default)
settings_flag(:allow_bundler_dependency_conflicts) { bundler_2_mode? }
settings_flag(:allow_offline_install) { bundler_2_mode? }
settings_flag(:application_locked) { bundler_2_mode? }

This comment has been minimized.

@segiddins

segiddins Jul 3, 2018

Member

nit -- application_locked_binstubs ?

@indirect

This comment has been minimized.

Member

indirect commented Jul 6, 2018

From a conversation with @eanlain:

In Ye Olden Times™, anytime you installed gems (with rubygems) you would get rubygems binstubs
when bundler layered on top of that, you still got rubygems binstubs, but then you had to start using bundle exec if you wanted to run the command using the bundled gems. so rake was the rubygems binstub, and bundle exec rake was the rubygems binstub but with env vars that forced the bundled version of rake to load.

then we had the idea to add bundle-specific binstubs, so you could run bin/rake, and it would NOT be the rubygems binstub, and would instead be a binstub that loaded the bundled version of rake and then ran rake.

it seems like at some point, the docker folks realized that bin/rake could be used to load any rake, and they started treating bundler binstubs as if they were rubygems binstubs… but no one on the bundler team even realized that was possible, because we had all come from this situation where you used the rubygems binstubs if you needed any version of rake, and you used the bundler binstubs if you needed just the exact version of rake that was specified in the bundle.

then we started getting bug reports about binstubs, where sometimes they would NOT load the version of rake specified in the application bundle, because they were being called from a CWD that contained a different gemfile. for example, cd app2; ../app1/bin/rake would give you the rake from app2, which was bad… because you were calling the app1 binstub specifically to get the app1 version of rake.

that’s when we “fixed the bug” by adding a hardcoded gemfile path to bin/bundle and requiring bin/bundle from all the other binstubs—and that’s when docker broke… because it was depending on behavior that the bundler team considered a bug (and also didn’t even really know was possible, tbh).

@eanlain:

just looking at my company’s usecase with docker, we have dockerfiles where we set the bundle path and bin (via bundle config --global path "$GEM_HOME" and bundle config --global bin "$GEM_HOME/bin"), and when we bundle install the bundle binstubs are generated in the bundle bin path

wow, okay... I now see what you are saying. I am kind of astonished and impressed and horrified. So that configuration will actually cause bundler to create the rubygems binstubs and then overwrite them with bundler-application-specific binstubs.

if you want your binstubs to only work with one application, it’s kind of genius. Someone clearly came up with a clever plan for docker images that only have one application in them, to stop needing bundle exec. Of course, if you do that, you have made it completely impossible for yourself to run any command without bundle exec, which is why bundler doesn’t do that 🙂.

So if you want binstubs that you can use outside your bundled application, you should unset both of those configs and run bundle install --system instead. Bundler will install gems to GEM_HOME, and install rubygems binstubs to GEM_HOME/bin. Then you can add $GEM_HOME/bin to your PATH, and you will be set. 👍🏻 (And, as usual, you will have to use application binstubs or run bundle exec NAME to run commands inside your application bundle.)

@indirect

This comment has been minimized.

Member

indirect commented Jul 6, 2018

After the discussion, we have tentatively decided to close this PR and resolve the issues discussed via Bundler and Docker configuration instead. Hopefully @eanlain can follow up with what he eventually settles on in his Dockerfile, and we can use that as the basis for a future website page about using Bundler with Docker.

@eanlain

This comment has been minimized.

eanlain commented Jul 20, 2018

The Dockerfiles that I ran into issues with were explicitly setting GEM_HOME, BUNDLE_BIN, BUNDLE_PATH, and then adding the BUNDLE_BIN to the PATH, similar to the the following snippet:

ENV GEM_HOME="/usr/local/bundle"
ENV BUNDLE_PATH="$GEM_HOME"
ENV BUNDLE_BIN="$GEM_HOME/bin"
ENV PATH="$BUNDLE_BIN:$PATH"

The Dockerfiles would then run bundle install in one or more applications. This behavior resulted in Bundler creating rubygem binstubs at $BUNDLE_PATH/bin then overwriting those generic binstubs with the new, as of 1.16.0, application-locked bundler binstubs (because BUNDLE_BIN was set).

The application-locked bundler binstubs were "locked" to the gemfile that was present when running bundle install. In the instances where multiple applications (multiple gemfiles) were used within the same Docker container this causes a number of issues including:

  • Parsing Gemfile issues:

    [!] There was an error parsing `Gemfile`: No such file or directory @ rb_sysopen - /Gemfile. Bundler cannot continue.
    
  • LoadErrors:

    LoadError: cannot load such file -- active_record/railtie
    
  • Issues loading commands:

    bundler: failed to load command: rspec (/usr/local/bundle/bin/rspec)
    

The fix was to unset BUNDLE_PATH, BUNDLE_BIN, and change the PATH to use $GEM_HOME/bin which resulted in the original generic rubygems binstubs being created in $GEM_HOME/bin. This still allows for the direct usage of gem commands like rake IF there's only one version of the gem installed. If more than one version of a gem is installed or if you want to use the application/gemfile specified version of a gem, bundle exec <name> accomplishes the task.

ENV GEM_HOME="/usr/local/bundle"
ENV PATH $GEM_HOME/bin:$GEM_HOME/gems/bin:$PATH

It looks like similar changes were made in the docker-library/ruby dockerfiles, see:

@indirect

This comment has been minimized.

Member

indirect commented Jul 20, 2018

Awesome, thanks for the writeup!

bundlerbot added a commit to bundler/bundler-site that referenced this pull request Aug 18, 2018

Auto merge of #395 - bundler:docker-guide, r=indirect
Created "how to use Bundler with Docker guide"

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

closes #386

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

The problem was...

Per #386:

> Based on bundler/bundler#6524, it looks like the Docker default images for Ruby have cleverly engineered a situation where only one bundle and one application can exist inside the docker container.

> For users who want to have more than one Gemfile, or simply install gems via RubyGems and use them as system gems, this situation is confusing, and has historically led to a lot of bug reports and even pull request against Bundler.

> Instead, we want to document how Bundler handles binstubs, document how the default Docker Ruby image handles binstubs, and then document how the user can configure Bundler within Docker to support whatever setup it is that they want.

...there

### What was your diagnosis of the problem?

My diagnosis was...

...to create a guide based on the fix @eanlain outlined [in this comment](bundler/bundler#6524 (comment))

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

My fix...

...to create a guide based on the fix one of our users outlined [here](bundler/bundler#6524 (comment))

cc @indirect

@colby-swandale colby-swandale removed this from the 1.17.0 milestone Sep 20, 2018

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