diff options
author | Bundlerbot <bot@bundler.io> | 2018-09-26 09:01:50 +0000 |
---|---|---|
committer | Colby Swandale <me@colby.fyi> | 2018-10-05 13:48:27 +1000 |
commit | 3336aba9c946d9711d6716f7e73eb743c1d01f3d (patch) | |
tree | 5dbf8c116cd5c33c6e4e65a0a3d46aa8757d9123 | |
parent | ef12e08f7c21348b66d57e4f9c7f9c5ea0f88e45 (diff) | |
download | bundler-3336aba9c946d9711d6716f7e73eb743c1d01f3d.tar.gz |
Merge #6708
6708: Fix only_update_to_newer_versions regression r=greysteil a=theflow
This is my attempt to fix #6529
### What was the end-user problem that led to this PR?
Running `bundle update` with `BUNDLE_ONLY_UPDATE_TO_NEWER_VERSIONS: "true"` resulted in a gem getting downgraded to a really old version in a certain edge case. Ironically it wouldn't get downgraded when `BUNDLE_ONLY_UPDATE_TO_NEWER_VERSIONS` was set to false.
### What was your diagnosis of the problem?
My diagnosis was that https://github.com/bundler/bundler/commit/47256d20cb05ebc724ee67173094682153b6b4aa tried to solve the problem of still allowing manual downgrades in the Gemfile while `only_update_to_newer_versions` is true. But introduced a regression that prevented the `additional_base_requirements_for_resolve` method to work as intended:
This is the relevant change from that commit that tries to avoid adding the `>=` requirement if the requirement in the Gemfile is different than the requirement in the lockfile (as far as I understand it):
```ruby
next requirements if @locked_deps[name] != dependencies_by_name[name]
```
I identified two problems
1. `dependencies_by_name[name]` returns an array of `Bundler::Dependency`, where as
`@locked_deps[name]` just returns a single `Bundler::Dependency`. Comparing the two will always be false.
1. `@locked_deps` is always empty in case of `bundle update`. See: https://github.com/bundler/bundler/blob/3d9e6167a7df9ca89a030dfe95c7cdff293e74a9/lib/bundler/definition.rb#L95
### What is your fix for the problem, implemented in this PR?
My fixes:
1. Make sure `dependencies_by_name` is a hash with `Bundler::Dependency` as values
1. Fetch the `@locked_gems.dependencies` again instead of using `@locked_deps`
1. The existing test worked for me with and without the `only_update_to_newer_versions` set to true, I replaced it with a reproduction of the edge case I was investigating (this is as minimal as I could make it)
1. I've added a test for the manual downgrading case.
### Why did you choose this fix out of the possible options?
This is the only way I could make these cases work. It's possible there are other edge cases I don't understand.
Co-authored-by: Florian Munz <surf@theflow.de>
(cherry picked from commit 8501b1e3608579acf53a4978b62c0d8891d23005)
-rw-r--r-- | lib/bundler/definition.rb | 4 | ||||
-rw-r--r-- | spec/commands/update_spec.rb | 56 |
2 files changed, 47 insertions, 13 deletions
diff --git a/lib/bundler/definition.rb b/lib/bundler/definition.rb index 89ae1ce926..1585fc3125 100644 --- a/lib/bundler/definition.rb +++ b/lib/bundler/definition.rb @@ -975,10 +975,10 @@ module Bundler def additional_base_requirements_for_resolve return [] unless @locked_gems && Bundler.feature_flag.only_update_to_newer_versions? - dependencies_by_name = dependencies.group_by(&:name) + dependencies_by_name = dependencies.inject({}) {|memo, dep| memo.update(dep.name => dep) } @locked_gems.specs.reduce({}) do |requirements, locked_spec| name = locked_spec.name - next requirements if @locked_deps[name] != dependencies_by_name[name] + next requirements if @locked_gems.dependencies[name] != dependencies_by_name[name] dep = Gem::Dependency.new(name, ">= #{locked_spec.version}") requirements[name] = DepProxy.new(dep, locked_spec.platform) requirements diff --git a/spec/commands/update_spec.rb b/spec/commands/update_spec.rb index 675e8574f1..e8f5275118 100644 --- a/spec/commands/update_spec.rb +++ b/spec/commands/update_spec.rb @@ -121,33 +121,67 @@ RSpec.describe "bundle update" do before do bundle! "config only_update_to_newer_versions true" end + it "does not go to an older version" do build_repo4 do - build_gem "a" do |s| - s.add_dependency "b" - s.add_dependency "c" + build_gem "tilt", "2.0.8" + build_gem "slim", "3.0.9" do |s| + s.add_dependency "tilt", [">= 1.3.3", "< 2.1"] + end + build_gem "slim_lint", "0.16.1" do |s| + s.add_dependency "slim", [">= 3.0", "< 5.0"] + end + build_gem "slim-rails", "0.2.1" do |s| + s.add_dependency "slim", ">= 0.9.2" + end + build_gem "slim-rails", "3.1.3" do |s| + s.add_dependency "slim", "~> 3.0" end - build_gem "b" - build_gem "c" - build_gem "c", "2.0" end install_gemfile! <<-G source "file:#{gem_repo4}" - gem "a" + gem "slim-rails" + gem "slim_lint" G - expect(the_bundle).to include_gems("a 1.0", "b 1.0", "c 2.0") + expect(the_bundle).to include_gems("slim 3.0.9", "slim-rails 3.1.3", "slim_lint 0.16.1") update_repo4 do - build_gem "b", "2.0" do |s| - s.add_dependency "c", "< 2" + build_gem "slim", "4.0.0" do |s| + s.add_dependency "tilt", [">= 2.0.6", "< 2.1"] end end bundle! "update", :all => bundle_update_requires_all? - expect(the_bundle).to include_gems("a 1.0", "b 1.0", "c 2.0") + expect(the_bundle).to include_gems("slim 3.0.9", "slim-rails 3.1.3", "slim_lint 0.16.1") + end + + it "should still downgrade if forced by the Gemfile" do + build_repo4 do + build_gem "a" + build_gem "b", "1.0" + build_gem "b", "2.0" + end + + install_gemfile! <<-G + source "file:#{gem_repo4}" + gem "a" + gem "b" + G + + expect(the_bundle).to include_gems("a 1.0", "b 2.0") + + gemfile <<-G + source "file://#{gem_repo4}" + gem "a" + gem "b", "1.0" + G + + bundle! "update b" + + expect(the_bundle).to include_gems("a 1.0", "b 1.0") end end end |