bundler/inline should ignore BUNDLE_PATH and install gems to GEM_HOME

[robuye]

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

This addresses the problem described in issue #7131. bundler/inline does not respect BUNDLE_PATH env variable and installs gems to the default location.

What was your diagnosis of the problem?

Bundler.configure is not called at any point in bundler/inline.

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

To call it :)

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

When bundler is invoked via bundle install this method gets called.

---

The included test is failing without this change as expected.

I have also tested it manually using the code from the issue on ruby 2.6 and with bundler 1.17.2 & 2.0.1.

[welcome]

Thanks for opening a pull request and helping make Bundler better! Someone from the Bundler team will take a look at your pull request shortly and leave any feedback. Please make sure that your pull request has tests for any changes or added functionality.

We use Travis CI to test and make sure your change works functionally and uses acceptable conventions, you can review the current progress of Travis CI in the PR status window below.

If you have any questions or concerns that you wish to ask, feel free to leave a comment in this PR or join our #bundler channel on Slack.

For more information about contributing to the Bundler project feel free to review our CONTRIBUTING guide

[robuye]

Hi Bundler team 👋

I see the tests on Travis are failing when BUNDLER_SPEC_SUB_VERSION=3.0.0 is set. Can you advise what can I do about it? I tried to reproduce it locally, but no luck (BUNDLER_SPEC_SUB_VERSION=3.0.0 bin/rspec spec/runtime/inline_spec.rb works for me).

[deivid-rodriguez]

Hei! We should make this easier, but the current way to try that would be BUNDLER_SPEC_SUB_VERSION=3.0.0 bin/rake override_version && bin/rspec spec/runtime/inline_spec.rb, or alternatively to manually edit the bundler version inside lib/bundler/version.rb and then run the specs.

[robuye]

Thank you, I will give it a try and come back, hopefully with fixes!

[robuye]

Hi again! I pushed a commit with a simple fix. It makes the test a bit more fragile as it depends on the implementation of bundler install path now, but it should be very straightforward to understand and fix if needed one day.

I was trying to make it better, but I would always hit a wall when I got to def Bundler.root in bundler/inline. My attempts to work around this always ended up very hacky & complex.

It's been fun & great learning experience to dive into Bundler codebase. If you think this P/R could be improved let me know. Thanks!

[deivid-rodriguez]

Hi @robuye. The approach for the test looks fine to me 👍.

Regarding style, what's normally done for these cases in the code base is to use a common context, and different specs for each version. So something like:

context "when BUNDLE_PATH set" do
  let(:app_dir) do
    dir = bundled_app("inline_with_bundle_path")
    dir.mkpath
    dir
  end

  before do
    Dir.chdir(app_dir) do
      script <<-RUBY, :env => { "BUNDLE_PATH" => "./vendor/inline" }
        gemfile(true) do
          source "file://#{gem_repo1}"
          gem "rack"
        end
      RUBY
    end
  end

  after do
    app_dir.rmtree
  end

  it "installs inline gems to the configured path", :bundler => "2" do
    expect(last_command).to be_success
    expect(app_dir.join("./vendor/inline/gems/rack-1.0.0")).to exist
  end

  it "installs inline gems to the configured path, appending the ruby scope to it", :bundler => "3" do
    expect(last_command).to be_success
    expect(app_dir.join("./vendor/inline/ruby/#{Gem::ConfigMap[:ruby_version]}/gems/rack-1.0.0")).to exist
  end
end

Regarding the fix itself, it also looks fine to me, but I want to ask about the current behavior. Does it actually work, but ignores BUNDLE_PATH and installs to a global location instead, or does it not work at all?

I ask because if it's "working", people might be relying on the bug and successfully sharing BUNDLE_PATH for both their "inline" scripts and their real application bundle, so once fixed, installing to BUNDLE_PATH will mess up the real location of their application bundle. I think we still want to fix this, but I want to know what to expect.

[robuye]

Regarding style, what's normally done for these cases in the code base is to use a common context, and different specs for each version.

👍 makes sense and sounds good, I will rewrite my spec.

Regarding the fix itself, it also looks fine to me, but I want to ask about the current behavior. Does it actually work, but ignores BUNDLE_PATH and installs to a global location instead, or does it not work at all?

It works fine, only ignores BUNDLE_PATH and installs gems to the global location. Given the following code (from the original issue):

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'
  gem 'hello-ruby-gem'
end

When I run BUNDLE_PATH=./vendor/bundle ruby test.rb then hello-ruby-gem is installed into /usr/local/bundle/gems/hello-ruby-gem-0.0.0.

When I run GEM_HOME=/tmp/vendor BUNDLE_PATH=./vendor/bundle ruby test.rb then hello-ruby-gem is installed into /tmp/vendor/gems/hello-ruby-gem-0.0.0.

You're right this may be a breaking change for people who rely on sharing inline & default gems.

I think a workaround for this would be to reset the env inside inline script (ENV['BUNDLE_PATH'] = nil). That worked for me when BUNDLE_BIN would cause bundler/inline to crash back in a day (#5847). It doesn't change the fact that this can be a breaking change for some, but there's a way to "restore" original behavior too.

I didn't test unsetting the ENV but I will confirm that and come back with the answer.

[robuye]

It took longer than I expected, but I think it's all good. I force-pushed the last commit with updates to tests (thanks for doing the hard part!).

There's one significant change I'm introducing to bundler/inline: Bundler v3 installs gems to local directory by default and not the system path. I think this is expected and it does reflect bundle install behavior. It is a change nonetheless.

Adding ENV["BUNDLE_PATH"] = nil in the file works as expected and makes Bundler use system/default install paths so this should be a valid workaround should somebody want to ignore BUNDLE_PATH when used with bundler/inline.

Below is summary of my manual tests.

---

Before my change both bundler v2 and v3 install to system location regardless of BUNDLE_PATH:

/code/tmp/gems/system/gems/rack-1.0.0.

---

With my change and no BUNDLE_PATH Bundler 3 installs to local directory:

/code/tmp/bundled_app/inline_with_bundle_path/.bundle/ruby/2.6.0/gems/rack-1.0.0

Bundler 2 installs to system location as before:

/code/tmp/gems/system/gems/rack-1.0.0

---

With my change and BUNDLE PATH=./vendor/inline Bundler 3 installs into vendor with ruby scope:

/code/tmp/bundled_app/inline_with_bundle_path/vendor/inline/ruby/2.6.0/gems/rack-1.0.0

Bundler 2 installs into vendor without ruby scope:

/code/tmp/bundled_app/inline_with_bundle_path/vendor/inline/gems/rack-1.0.0

[deivid-rodriguez]

@robuye We ended up backporting the new behavior of prepending the ruby scope to bundler 2, so you can actually simplify the tests now (you can rebase to double check it's all good, although our bot will do it anyways, so not needed).

Regarding the change itself, @indirect do you have any thoughts about this? In principle it makes sense to me, and I think it's independent from the fact that once we start installing gems to a local path by default, and autocleaning after bundle install, repos with both a Gemfile and bundle/inline scripts might get messed up, since they will share the same location for gems and conflict with each other.

I wonder if we should provide a different default installation path and environment variable for bundle/inline? Like, BUNDLE_INLINE_PATH, and ./.bundle/inline, or something.

[indirect]

When inline was created it was a deliberate choice to use GEM_HOME rather than BUNDLE_PATH, I think because then 1) all inline scripts can reuse other inline gems, and 2) scripts typically need to be run outside the bundle of the current application.

I might be missing something, but it seems like you would never want to share gems between a bundler/inline script and an application's gems in BUNDLE_PATH. It seems like can use bundler/setup if you want to use BUNDLE_PATH, so it seems like it's specifically bundler/inline that needs different gems than the current directory's application. If inline needs different gems, that seems to imply that inline Gemfiles need to have their own place to put gems, and it needs to be different than BUNDLE_PATH. That said, maybe I'm missing something here. 😄

[deivid-rodriguez]

No, I don't think you're missing anything. The rationale makes sense although I do find the reported bug a bit confusing. The current solution is probably worse than the current situation... :(

Should we introduce the environment variable and default path I suggested in my previous message?

[robuye]

Hey guys, that's very interesting, thanks for taking the time!

Is there any risk in having both application and inline gems installed into the same location? Aside how it'd fit with planned changes I think Bundler handles that perfectly fine and in the end the scripts would work exactly the same regardless if we used GEM_HOME or BUNDLE_PATH.

Respecting BUNDLE_PATH makes it easy to vendor gems, it's consistent and (I think) natural so that appeals the most to me.

Just my 2 cents 😃

[deivid-rodriguez]

The problem I forsee is that bundler 3 automatically cleans up unused gems after bundle install. That means that you will be missing gems all the time when switching between your inline scripts and your regular bundle, and be forced to rerun bundle install all the time. Of course, you can disable the "auto-clean" for those cases, but I think we can aim for a better default?

[robuye]

Yea, that makes sense and it should definitely be factored in.

I wanted to provide a different point of view because I use bundler/inline to run completely isolated scripts so a conflict didn't occur to me.

I think bundler/inline brings writing system scripts in ruby to a new level and when it doesn't perfectly align with the CLI it's okay. Supporting additional option / ENV to bridge the gap feels right, but explicitly ignoring BUNDLE_PATH by bundler/inline feels wrong. I would like to have cake and eat cake if possible please ;)

[indirect]

Another way of looking at this situation: bundle/inline always acts like you used bundle install --system. The reason for that is that an inline script by definition expects to be run inside or outside of other bundled projects. If the inline script paid any attention to the current directory, or settings in the current directory, it wouldn’t be an inline script anymore, it would just be a script that belongs to the current bundled application.

That said, if you want to set the location for inline gems with an env var... you can do that today by setting GEM_HOME, either from the command line or inside your script before you require bundler/inline. Does that let you do what you want to do?

[robuye]

Hey guys, apologies for very long wait.

@indirect I hear you and see the motivation behind current behaviour. I'm not using bundler/inline in this fashion myself, but I'm sure there are people who do (I acknowledge it).

I opened this P/R because I saw an issue on Github and figured that would be a great way to learn more about bundler/inline and show appreciation for your work 😄

I didn't intend to introduce a big change here and based on the original issue I assumed it's a bug. Would you guys like me to rebase this P/R, undo the changes and add a test to ensure BUNDLE_PATH is always ignored? I think that would indicate it's a design choice and should protect future us from regressions.

Otherwise there's an idea from @deivid-rodriguez to introduce BUNDLE_INLINE_PATH and that feels much less intrusive than playing with GEM_HOME. I think it's a good one and it should work well, but if we don't anticipate running into bundle install autoclean issues by design, additional env var to maintain long term may be less appealing.

I'm familiar with the available workarounds, but I'm not original author of the issue. Granted we're mostly settled on not making this change it's probably good idea to check in with @lastk.

Let me know how you'd like to proceed and thanks very much for all the time you guys invested into this P/R!

[lastk]

Hi all, sorry for my late reply, my motivation to open the ticket was because I thought it was a bug. I just wanted to run some scripts to test a rubygem and be able to edit the gem( that's why I wanted to set the folder where those gems were installed ) and I thought the easiest way was just getting one file with everything without worrying about creating a Gemfile. I suppose now I can close the ticket and maybe just use GEM_HOME instead.
Thank you very much @robuye

[indirect]

@robuye @lastk thanks so much for following up on this <3

I think it's a good idea to write a test to verify that BUNDLE_PATH gets ignored by bundle/inline.

For changing the destination, I think I personally would also be ok with a BUNDLE_INLINE_PATH env var that can overrule GEM_HOME, if someone really needs to install their inline gems somewhere else. Maybe we can wait to see if anyone asks for that before we add it?

[robuye]

I pushed one more commit. Given it's just one spec in the context and the required setup is much easier I decided to inline it to avoid excessive code. Hopefully you will find it easier to read this way, but if the before & let style is preferred let me know. Otherwise I will just rebase everything into one.

@lastk that's what I thought too. I use bundler/inlne in an environment where I can only pass a single file for execution so Gemfile is a no-no, but bundler/inline was a game changer because I'm no longer limited to stdlib. It's fun & refreshing to write self contained programs 😄

@indirect I agree, there's no need to be adding an option before someone needs it. Both me and @lastk would be able to use proposed workarounds should we need it.

This has been very fruitful discussion, thank you everyone for participating.

[deivid-rodriguez]

@robuye The spec looks good to me, I think you can squash everything yeah.

[robuye]

Hey guys, I updated the branch last night. I think I will leave it in your hands now. Thanks again for your time! 👋

[deivid-rodriguez]

Merging this since it's exactly what @indirect requested :). Thanks so much @robuye ❤️

@bundlerbot r+

[ghost]

Build succeeded

• continuous-integration/travis-ci/push

[Nowaker]

I think it's a good idea to write a test to verify that BUNDLE_PATH gets ignored by bundle/inline.

Whaaaat. The title of this PR is bundler/inline should install gems to BUNDLE_PATH while the outcome is the opposite. :( @deivid-rodriguez @indirect Can you guys edit the title for clarity?

bundle/inline always acts like you used bundle install --system. The reason for that is that an inline script by definition expects to be run inside or outside of other bundled projects.

That's a very strong assumption. Respecting BUNDLE_PATH or at least BUNDLE_INLINE_PATH would be a good addition. Currently, the way to work it around is something like this:

Gem.paths = {'GEM_HOME' => Dir.pwd + '/.gemnew'}
require 'bundler/inline'
gemfile do
  # ...
end

# your code

This is how I tricked coderpad.io to make it possible to use dependencies from Rubygems there.

However, this also breaks some of Coderpad's own code that runs right after user code since require is now bundler-owned. (I'll keep poking around on how to unbundlerize the runtime...)

[robuye]

Title updated, good catch @Nowaker, thanks! 👍

However, this also breaks some of Coderpad's own code that runs right after user code since require is now bundler-owned. (I'll keep poking around on how to unbundlerize the runtime...)

Does it work if you restore GEM_HOME to its original value after the user code is done? You may also play with GEM_PATH so it includes both user & Coderpad's paths. I think GEM_HOME is used for installing and GEM_PATH for loading so that could help, although I didn't ever need to tinker with it.

[Nowaker]

@robuye I tried restoring Gem.paths to original values but no bueno. With Bundler loaded you're not allowed to require things from gems other than the ones specified in gemfile. The goal is to restore Gem.paths as well as unbundlerize the require method so it works in a "normal" way.