-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Bundler 2 support #850
Bundler 2 support #850
Conversation
9ade75b
to
141c176
Compare
82303e4
to
edd9334
Compare
## Problem 1 Bundler 2.x cannot be used with a Gemfile.lock that specifies bundler 1.x: ``` BUNDLED WITH 1.16.4 ``` If you try, then Bundler 2.x will look for an installed bundler 1.x version and try to use it. If no such version exists on the system then an error is thrown: ``` can't find gem bundler (>= 0.a) with executable bundle (Gem::GemNotFoundException) ``` - rubygems/rubygems#2426 - rubygems/rubygems#2515 ## Problem 2 Likewise a version of bundler 1.x cannot be used on a project where a `BUNDLED WITH` specifies 2.x: ``` $ bundle -v 1.17.3 $ cat Gemfile.lock | grep -A 1 BUNDLED WITH BUNDLED WITH 2.0.1 $ bundle Traceback (most recent call last): 2: from /Users/rschneeman/.gem/ruby/2.6.0/bin/bundle:23:in `<main>' 1: from /Users/rschneeman/.rubies/ruby-2.6.0/lib/ruby/2.6.0/rubygems.rb:302:in `activate_bin_path' /Users/rschneeman/.rubies/ruby-2.6.0/lib/ruby/2.6.0/rubygems.rb:283:in `find_spec_for_exe': Could not find 'bundler' (2.0.0) required by your /private/tmp/default_ruby/Gemfile.lock. (Gem::GemNotFoundException) To update to the latest version installed on your system, run `bundle update --bundler`. To install the missing version, run `gem install bundler:2.0.0` ``` This is due to a bug in Rubygems 3.x rubygems/rubygems#2592 ## Proposed Solution This PR implements a solution where we have a different "blessed" version of bundler for each major version. The way this is implemented is to read in a Gemfile.lock and to read in the `BUNDLED WITH` value. Then the major version is pulled out and converted into a "blessed" version. This allows for multiple major versions of bundler to be used on Heroku without sacrificing stability.
edd9334
to
a382e1d
Compare
Usually this would take a LONG time as the test suite when run serially takes about an hour. The other thing that is tough about this, is it only fails on CI and not locally. On a hunch I guessed the failing behavior came only from `spec/helpers` which is all unit tested and therefore pretty fast. This allowed me to actually find the failing order. Via `$ heroku ci:debug` First I ran: ``` $ bundle exec rspec spec/helpers --bisect --seed 1 ``` The `--seed` argument is used because otherwise the order is randomized on every run. It doesn't matter what value is used as long as it's consistent. This produce a "minimal failing example" of running: ``` $ rspec './spec/helpers/fetcher_spec.rb[1:1]' './spec/helpers/rails_runner_spec.rb[1:3,1:5]' './spec/helpers/rake_runner_spec.rb[1:1,1:2]' --seed 1 ``` Sure enough this command fails 100% of the time. I knew the culprit had to be mutating something global. Previously I've been focused on files on disk but considering every example is now run inside of it's own temp directory then it is likely something else. The other culprit could be an environment variable. This lead me to look into the bundler wrapper which sure enoug mutates the env var `BUNDLE_GEMFILE`.
a382e1d
to
80f3d88
Compare
msg << "\n" | ||
msg << "```\n" | ||
msg << "BUNDLED WITH\n" | ||
msg << " #{version_hash["1"]}\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why index 1 instead of 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the major version. So this will be "1.15.2"
|
||
if LanguagePack::Ruby::BUNDLER_VERSION == "1.7.12" | ||
if version == "1.7.12" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we extract this into a constant somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on this code I don't think it's possible to ever actually trigger this case. Since we hard code a version and we're not using "1.7.12" it won't ever fire.
Let me know if you agree and I can remove it.
|
||
def major_bundler_version | ||
# https://rubular.com/r/uuRpai9IheL68d | ||
bundler_version_match = @gemfile_lock_path.read.match(/^BUNDLED WITH$(\r?\n) (?<major>\d*)\.\d*\.\d*/m) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be +
and not *
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, scraping BUNDLED_WITH
seems awful :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be
+
Great call.
Also, scraping
BUNDLED_WITH
seems awful :(
Agreed, though It's the least awful thing I could come up with :(
if bundler_version_match | ||
bundler_version_match[:major] | ||
else | ||
"1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we actually fail instead of defaulting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People can still deploy without having a BUNDLED WITH
in their Gemfile.lock. For example in some of our hatchet test apps https://github.com/sharpstone/default_ruby/blob/master/Gemfile.lock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good if scary. I'm not sure what other ramifications we need to take into account for bundler 2 "support".
Since we control the version, this code will never execute unless we regress to a REALLY old version. I'm deleting it.
Detect bundler version
Problem 1
Bundler 2.x cannot be used with a Gemfile.lock that specifies bundler 1.x:
If you try, then Bundler 2.x will look for an installed bundler 1.x version and try to use it. If no such version exists on the system then an error is thrown:
Problem 2
Likewise a version of bundler 1.x cannot be used on a project where a
BUNDLED WITH
specifies 2.x:This is due to a bug in Rubygems 3.x rubygems/rubygems#2592
Proposed Solution
This PR implements a solution where we have a different "blessed" version of bundler for each major version.
The way this is implemented is to read in a Gemfile.lock and to read in the
BUNDLED WITH
value. Then the major version is pulled out and converted into a "blessed" version.This allows for multiple major versions of bundler to be used on Heroku without sacrificing stability.