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

Use Bundler.with_original_env so that BUNDLER_APP_CONFIG is honored #546

Merged
merged 1 commit into from Jun 2, 2018

Conversation

Projects
None yet
@mattbrictson
Contributor

mattbrictson commented Nov 8, 2017

Fixes #545.

Bundler's with_clean_env completely erases all Bundler-related environment variables, even if those variables existed prior to loading Bundler. That means that custom Bundler settings that we legitimately want to pass down to child processes - namely, BUNDLE_APP_CONFIG - are erased, leading to errors when the child process loads bundler/setup.

Note that clean_env (used by with_clean_env) is actually deprecated by Bundler. The method recommended by Bundler going forward, and the one used in this commit, is with_original_env. This clears any variables that were set by Bundler itself, but retains any user-supplied settings that existed prior.

This fixes a bug where spring binstubs would fail if BUNDLE_APP_CONFIG was set to a custom location (i.e. other than the default ./.bundle) and bundle install --path was used. The binstub would not be able to load the bundle because without BUNDLE_APP_CONFIG, it could not know the path where the gems were installed.

with_original_env so BUNDLER_APP_CONFIG is honored
Bundler's `with_clean_env` completely erases all Bundler-related
environment variables, even if those variables existed prior to loading
Bundler. That means that custom Bundler settings that we legitimately
want to pass down to child processes - namely, `BUNDLE_APP_CONFIG` - are
erased, leading to errors when the child process loads `bundler/setup`.

Note that `with_clean_env` is actually deprecated by Bundler. The method
recommended by Bundler going forward, and the one used in this commit,
is `with_original_env`. This clears any variables that were set by
Bundler itself, but retains any user-supplied settings that existed
prior.

This fixes a bug where spring binstubs would fail if `BUNDLE_APP_CONFIG`
was set to a custom location (i.e. other than the default `./.bundle`)
and `bundle install --path` was used. The binstub would not be able to
load the bundle because without `BUNDLE_APP_CONFIG`, it could not know
the path where the gems were installed.
@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Nov 8, 2017

r? @rafaelfranca

(@rails-bot has picked a reviewer for you, use r? to override)

rails-bot commented Nov 8, 2017

r? @rafaelfranca

(@rails-bot has picked a reviewer for you, use r? to override)

@cmwall

This comment has been minimized.

Show comment
Hide comment
@cmwall

cmwall Nov 10, 2017

Our builds were failing at the test suite stage in Circle CI because the rake gem was not being found in any of our sources.

We updated our Gemfile to use this branch for spring and the problem was solved.

Thanks!

The error we received was:

#!/bin/bash -eo pipefail
xvfb-run -a \
./bin/rspec --profile 10 \
--format RspecJunitFormatter \
--out ./test-results/rspec.xml \
--format progress \
$(circleci tests glob "spec/**/*_spec.rb" | circleci tests split --split-by=timings)
Could not find rake-12.2.1 in any of the sources
Run `bundle install` to install missing gems.
Exited with code 1

cmwall commented Nov 10, 2017

Our builds were failing at the test suite stage in Circle CI because the rake gem was not being found in any of our sources.

We updated our Gemfile to use this branch for spring and the problem was solved.

Thanks!

The error we received was:

#!/bin/bash -eo pipefail
xvfb-run -a \
./bin/rspec --profile 10 \
--format RspecJunitFormatter \
--out ./test-results/rspec.xml \
--format progress \
$(circleci tests glob "spec/**/*_spec.rb" | circleci tests split --split-by=timings)
Could not find rake-12.2.1 in any of the sources
Run `bundle install` to install missing gems.
Exited with code 1
@mattbrictson

This comment has been minimized.

Show comment
Hide comment
@mattbrictson

mattbrictson Nov 20, 2017

Contributor

@rafaelfranca in your opinion is this fix worth pursuing? It seems that at least a handful of other people have found that this branch fixes their problems using spring binstubs on CircleCI.

I can spend some more time on this PR to try and fix the Travis failures, but first I want to make sure it has a good chance of being accepted. Thanks!

Contributor

mattbrictson commented Nov 20, 2017

@rafaelfranca in your opinion is this fix worth pursuing? It seems that at least a handful of other people have found that this branch fixes their problems using spring binstubs on CircleCI.

I can spend some more time on this PR to try and fix the Travis failures, but first I want to make sure it has a good chance of being accepted. Thanks!

@anvox

This comment has been minimized.

Show comment
Hide comment
@anvox

anvox Jan 3, 2018

The same problem to me, CircleCI 2.0. I must disable spring on CI :( https://github.com/rails/spring#temporarily-disabling-spring
Under company policy, I dont want to put a dev branch gem to app.

anvox commented Jan 3, 2018

The same problem to me, CircleCI 2.0. I must disable spring on CI :( https://github.com/rails/spring#temporarily-disabling-spring
Under company policy, I dont want to put a dev branch gem to app.

@sponomarev

This comment has been minimized.

Show comment
Hide comment
@sponomarev

sponomarev Jan 19, 2018

@mattbrictson Do you know what introduced that problem? We haven't experienced this problem about a month ago, but now it's a common problem for local and Docker environment

sponomarev commented Jan 19, 2018

@mattbrictson Do you know what introduced that problem? We haven't experienced this problem about a month ago, but now it's a common problem for local and Docker environment

scytacki added a commit to concord-consortium/Data-Analytics-Log-Manager that referenced this pull request Feb 22, 2018

Fix an issue with spring in our docker setup
This code also updates spring and bundler.

The spring issue is caused by a bug in spring, reported here:
rails/spring#545
rails/spring#339
With a suggested fix here:
rails/spring#546
However the fix is failing the spring travis tests.

The work around in this commit manually sets the the ENV variable in ~/.spring.rb
@matiasleidemer

This comment has been minimized.

Show comment
Hide comment
@matiasleidemer

matiasleidemer Feb 26, 2018

I have the same issue in a Docker environment. Updating bundler to 1.16.1causes the error, but after monkeypatching spring to use Bundler.with_original_env it works like a charm. Downgrading bundler to 1.15.4 also fixes the issue.

I'm not sure how this change would impact other projects, however. As a side note, we don't have the BUNDLE_APP_CONFIG env variable.

EDIT: my rubygems version is 2.5.2.2

matiasleidemer commented Feb 26, 2018

I have the same issue in a Docker environment. Updating bundler to 1.16.1causes the error, but after monkeypatching spring to use Bundler.with_original_env it works like a charm. Downgrading bundler to 1.15.4 also fixes the issue.

I'm not sure how this change would impact other projects, however. As a side note, we don't have the BUNDLE_APP_CONFIG env variable.

EDIT: my rubygems version is 2.5.2.2

matiasleidemer added a commit to printbear/spring that referenced this pull request Feb 26, 2018

matiasleidemer added a commit to printbear/spring that referenced this pull request Feb 26, 2018

@rmm5t

This comment has been minimized.

Show comment
Hide comment
@rmm5t

rmm5t Mar 1, 2018

Confirmed that I am experiencing the same underlying problem and that this change fixes it.

What's the likelihood of something like this making it into a new release? I guess I'm not sure I fully understand the benefits of .with_clean_env vs .with_original_env.

rmm5t commented Mar 1, 2018

Confirmed that I am experiencing the same underlying problem and that this change fixes it.

What's the likelihood of something like this making it into a new release? I guess I'm not sure I fully understand the benefits of .with_clean_env vs .with_original_env.

@sponomarev

This comment has been minimized.

Show comment
Hide comment
@sponomarev

sponomarev Mar 1, 2018

with_clean_env was legacy and have a lot of caveats - not all the proper ENV variables are cleaned. It will be deprecated in Bundler 2.0. Recent Bundler major version changed the logic a little bit (to correct it), but, unfortunately, Spring relies on that incorrect behaviour in some limited cases.

As a temporary solution, we set GEM_HOME and BUNDLE_APP_CONFIG to our own location if we want to use custom BUNDLE_PATH.

Not actually related, but could be useful. Our part of Dockerfile

ENV GEM_HOME /bundle
ENV BUNDLE_PATH=$GEM_HOME \
  BUNDLE_APP_CONFIG=$BUNDLE_PATH \
  BUNDLE_BIN=$BUNDLE_PATH/bin
ENV PATH /app/bin:$BUNDLE_BIN:$PATH

sponomarev commented Mar 1, 2018

with_clean_env was legacy and have a lot of caveats - not all the proper ENV variables are cleaned. It will be deprecated in Bundler 2.0. Recent Bundler major version changed the logic a little bit (to correct it), but, unfortunately, Spring relies on that incorrect behaviour in some limited cases.

As a temporary solution, we set GEM_HOME and BUNDLE_APP_CONFIG to our own location if we want to use custom BUNDLE_PATH.

Not actually related, but could be useful. Our part of Dockerfile

ENV GEM_HOME /bundle
ENV BUNDLE_PATH=$GEM_HOME \
  BUNDLE_APP_CONFIG=$BUNDLE_PATH \
  BUNDLE_BIN=$BUNDLE_PATH/bin
ENV PATH /app/bin:$BUNDLE_BIN:$PATH
@rmm5t

This comment has been minimized.

Show comment
Hide comment
@rmm5t

rmm5t Mar 1, 2018

with_clean_env was legacy and have a lot of caveats - not all the proper ENV variables are cleaned. It will be deprecated in Bundler 2.0

Thanks for that explanation. Assuming this is all true, what's holding up this PR from being applied?
Obviously the build failed, but it looks like the build is failing on the master branch for the same reasons.

Where is help needed to move this forward? I have some incentive and willingness to help here where I can.

rmm5t commented Mar 1, 2018

with_clean_env was legacy and have a lot of caveats - not all the proper ENV variables are cleaned. It will be deprecated in Bundler 2.0

Thanks for that explanation. Assuming this is all true, what's holding up this PR from being applied?
Obviously the build failed, but it looks like the build is failing on the master branch for the same reasons.

Where is help needed to move this forward? I have some incentive and willingness to help here where I can.

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Mar 1, 2018

Member

What are the version/dependency implications of this change / when did Bundler introduce with_original_env?

Is it safe to just switch outright, or might there be a need to support both methods?

Member

matthewd commented Mar 1, 2018

What are the version/dependency implications of this change / when did Bundler introduce with_original_env?

Is it safe to just switch outright, or might there be a need to support both methods?

@rmm5t

This comment has been minimized.

Show comment
Hide comment
@rmm5t

rmm5t commented Mar 1, 2018

@sponomarev

This comment has been minimized.

Show comment
Hide comment

sponomarev commented Mar 1, 2018

@rmm5t

This comment has been minimized.

Show comment
Hide comment
@rmm5t

rmm5t Apr 3, 2018

@matthewd Any chance we might see some progress on this issue? Where do you need help to move this forward. I'm willing to help if needed.

rmm5t commented Apr 3, 2018

@matthewd Any chance we might see some progress on this issue? Where do you need help to move this forward. I'm willing to help if needed.

@davidstosik

This comment has been minimized.

Show comment
Hide comment
@davidstosik

davidstosik Apr 19, 2018

Interested in seeing this fixed too! 👍
The issue goes a bit further down: setting BUNDLE_PATH only seems to lead to the same problem.
See this issue I originally created on the Rails project, thinking the problem was located there: rails/rails#32587.

Using @mattbrictson's fix branch for Spring (gem 'spring', github: 'mattbrictson/spring', branch: 'honor-bundle-app-config' in my Gemfile) fixes everything and I can run bundle exec rails test now! 🚀

davidstosik commented Apr 19, 2018

Interested in seeing this fixed too! 👍
The issue goes a bit further down: setting BUNDLE_PATH only seems to lead to the same problem.
See this issue I originally created on the Rails project, thinking the problem was located there: rails/rails#32587.

Using @mattbrictson's fix branch for Spring (gem 'spring', github: 'mattbrictson/spring', branch: 'honor-bundle-app-config' in my Gemfile) fixes everything and I can run bundle exec rails test now! 🚀

davidstosik added a commit to davidstosik/circle_ci_basic_test that referenced this pull request Apr 19, 2018

@pradyumna2905

This comment has been minimized.

Show comment
Hide comment
@pradyumna2905

pradyumna2905 Apr 19, 2018

Facing the same issue in GitLab CI as well.

Since I'm just starting out with my app and don't have a ton of tests, I'm disabling spring till this gets fixed.

DISABLE_SPRING=true bin/rails test

pradyumna2905 commented Apr 19, 2018

Facing the same issue in GitLab CI as well.

Since I'm just starting out with my app and don't have a ton of tests, I'm disabling spring till this gets fixed.

DISABLE_SPRING=true bin/rails test

@ajacksified

This comment has been minimized.

Show comment
Hide comment
@ajacksified

ajacksified Apr 19, 2018

I wasted my morning trying to figure out why rails s worked but rails g didn't; it was this issue. Commenting spring out of the Gemfile immediately solved my issue - we use a virtualenv for projects in a dev vm, and .bundle/config points at vendor/bundle.

Can we merge this one-line change that's been open for half a year? It's especially bad because it's in the default Gemfile for new rails projects, and the error is pretty difficult to track down.

ajacksified commented Apr 19, 2018

I wasted my morning trying to figure out why rails s worked but rails g didn't; it was this issue. Commenting spring out of the Gemfile immediately solved my issue - we use a virtualenv for projects in a dev vm, and .bundle/config points at vendor/bundle.

Can we merge this one-line change that's been open for half a year? It's especially bad because it's in the default Gemfile for new rails projects, and the error is pretty difficult to track down.

@camallen camallen referenced this pull request May 16, 2018

Merged

disable spring in docker compose envs #2766

0 of 4 tasks complete

@matthewd matthewd merged commit eb80f1f into rails:master Jun 2, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@rmm5t

This comment has been minimized.

Show comment
Hide comment
@rmm5t

rmm5t Jun 2, 2018

Thank you, @matthewd!!

Will there be a new versioned release soon, or should we plan to work against the master branch for the near term?

rmm5t commented Jun 2, 2018

Thank you, @matthewd!!

Will there be a new versioned release soon, or should we plan to work against the master branch for the near term?

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Jun 5, 2018

Member

I'd like to get master's CI properly green before we cut a release.

For this change: I'm happy that only Bundler >= 1.0 < 1.1 had with_clean_env and not with_original_env, and that substantially predates when we started using it.

Member

matthewd commented Jun 5, 2018

I'd like to get master's CI properly green before we cut a release.

For this change: I'm happy that only Bundler >= 1.0 < 1.1 had with_clean_env and not with_original_env, and that substantially predates when we started using it.

@mtsmfm

This comment has been minimized.

Show comment
Hide comment
@mtsmfm

mtsmfm Jul 25, 2018

Contributor

@matthewd Can I ask you to cut new release?

master's CI was fixed via #562

Contributor

mtsmfm commented Jul 25, 2018

@matthewd Can I ask you to cut new release?

master's CI was fixed via #562

@welkie

This comment has been minimized.

Show comment
Hide comment
@welkie

welkie Aug 28, 2018

Just wanted to mention that I needed to resort to using the DISABLE_SPRING=true bundle exec rails test command in my CircleCI config for it to work, despite this branch being merged in. Without disabling Spring, it did not find the Rake gem:

#!/bin/bash -eo pipefail
bundle exec rails test
Could not find rake-12.3.1 in any of the sources
Run `bundle install` to install missing gems.
Exited with code 1

welkie commented Aug 28, 2018

Just wanted to mention that I needed to resort to using the DISABLE_SPRING=true bundle exec rails test command in my CircleCI config for it to work, despite this branch being merged in. Without disabling Spring, it did not find the Rake gem:

#!/bin/bash -eo pipefail
bundle exec rails test
Could not find rake-12.3.1 in any of the sources
Run `bundle install` to install missing gems.
Exited with code 1
@davidstosik

This comment has been minimized.

Show comment
Hide comment
@davidstosik

davidstosik Aug 28, 2018

@welkie The fix was merged, but no new version of Spring was released.

I confirmed the fix works for me by using gem 'spring', github: 'rails/spring' in my Gemfile.

Thanks to anyone involved in fixing this! 👏

davidstosik commented Aug 28, 2018

@welkie The fix was merged, but no new version of Spring was released.

I confirmed the fix works for me by using gem 'spring', github: 'rails/spring' in my Gemfile.

Thanks to anyone involved in fixing this! 👏

@welkie

This comment has been minimized.

Show comment
Hide comment
@welkie

welkie Aug 28, 2018

@davidstosik Awesome. I've been out of the loop with Rails for like 2 years but I wanted to update a project's dependencies and throw it on CircleCI. So this threw me off. Just wanted to comment so if anyone else comes here from googling, they know a fix is still needed.

@literally_everyone Ty for the fix. :)

welkie commented Aug 28, 2018

@davidstosik Awesome. I've been out of the loop with Rails for like 2 years but I wanted to update a project's dependencies and throw it on CircleCI. So this threw me off. Just wanted to comment so if anyone else comes here from googling, they know a fix is still needed.

@literally_everyone Ty for the fix. :)

@ypresto

This comment has been minimized.

Show comment
Hide comment
@ypresto

ypresto Sep 18, 2018

Contributor

FYI:
binstub (bin/spring) won't work with gem 'spring', github: 'rails/spring'.
It assumes spring gem to be installed on standard gem path (vendor/bundle/ruby/2.5.0/gems) , not bundler's git repo directory (vendor/bundle/ruby/2.5.0/bundler/gems).

Part of binstub (gem is rubygems' one):

if spring
  Gem.use_paths Gem.dir, Bundler.bundle_path.to_s, *Gem.path
  gem 'spring', spring.version
  require 'spring/binstub'
end

Workaround not to stop using bin/spring:

Replace contents of bin/spring with below snippet.

#!/bin/bash
exec bundle exec spring "$@"
Contributor

ypresto commented Sep 18, 2018

FYI:
binstub (bin/spring) won't work with gem 'spring', github: 'rails/spring'.
It assumes spring gem to be installed on standard gem path (vendor/bundle/ruby/2.5.0/gems) , not bundler's git repo directory (vendor/bundle/ruby/2.5.0/bundler/gems).

Part of binstub (gem is rubygems' one):

if spring
  Gem.use_paths Gem.dir, Bundler.bundle_path.to_s, *Gem.path
  gem 'spring', spring.version
  require 'spring/binstub'
end

Workaround not to stop using bin/spring:

Replace contents of bin/spring with below snippet.

#!/bin/bash
exec bundle exec spring "$@"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment