Merge pull request #4618 from jrafanie/use_set_for_orderless_array_comparison

[WIP] Properly detect path spec changes to avoid needless re-resolve

4 files changed, 160 insertions, 9 deletions

diff --git a/lib/bundler/definition.rb b/lib/bundler/definition.rb
index d4af6b70e7..efc249be91 100644
--- a/lib/bundler/definition.rb
+++ b/lib/bundler/definition.rb
@@ -216,9 +216,11 @@ module Bundler
       @resolve ||= begin
         last_resolve = converge_locked_specs
         if Bundler.settings[:frozen] || (!@unlocking && nothing_changed?)
+          Bundler.ui.debug("Found no changes, using resolution from the lockfile")
           last_resolve
         else
           # Run a resolve against the locally available gems
+          Bundler.ui.debug("Found changes from the lockfile, re-resolving dependencies")
           last_resolve.merge Resolver.resolve(expanded_dependencies, index, source_requirements, last_resolve, ruby_version)
         end
       end
@@ -480,14 +482,21 @@ module Bundler
         end
       end
 
-      !locked || unlocking || dependencies_for_source_changed?(locked) || source.specs != locked.specs
+      !locked || unlocking || dependencies_for_source_changed?(source) || specs_for_source_changed?(source)
     end
 
     def dependencies_for_source_changed?(source)
       deps_for_source = @dependencies.select {|s| s.source == source }
       locked_deps_for_source = @locked_deps.select {|s| s.source == source }
 
-      deps_for_source != locked_deps_for_source
+      Set.new(deps_for_source) != Set.new(locked_deps_for_source)
+    end
+
+    def specs_for_source_changed?(source)
+      locked_index = Index.new
+      locked_index.use(@locked_specs.select {|s| source.can_lock?(s) })
+
+      source.specs != locked_index
     end
 
     # Get all locals and override their matching sources.
diff --git a/lib/bundler/index.rb b/lib/bundler/index.rb
index c28a120990..f0ee411df8 100644
--- a/lib/bundler/index.rb
+++ b/lib/bundler/index.rb
@@ -134,10 +134,16 @@ module Bundler
     def ==(other)
       all? do |spec|
         other_spec = other[spec].first
-        other_spec && (spec.dependencies & other_spec.dependencies).empty? && spec.source == other_spec.source
+        other_spec && dependencies_eql?(spec, other_spec) && spec.source == other_spec.source
       end
     end
 
+    def dependencies_eql?(spec, other_spec)
+      deps       = spec.dependencies.select {|d| d.type != :development }
+      other_deps = other_spec.dependencies.select {|d| d.type != :development }
+      Set.new(deps) == Set.new(other_deps)
+    end
+
     def add_source(index)
       raise ArgumentError, "Source must be an index, not #{index.class}" unless index.is_a?(Index)
       @sources << index
diff --git a/spec/bundler/definition_spec.rb b/spec/bundler/definition_spec.rb
index 443a2c9703..a8ee3080f1 100644
--- a/spec/bundler/definition_spec.rb
+++ b/spec/bundler/definition_spec.rb
@@ -3,13 +3,12 @@ require "spec_helper"
 require "bundler/definition"
 
 describe Bundler::Definition do
-  before do
-    allow(Bundler).to receive(:settings) { Bundler::Settings.new(".") }
-    allow(Bundler).to receive(:default_gemfile) { Pathname.new("Gemfile") }
-    allow(Bundler).to receive(:ui) { double("UI", :info => "") }
-  end
-
   describe "#lock" do
+    before do
+      allow(Bundler).to receive(:settings) { Bundler::Settings.new(".") }
+      allow(Bundler).to receive(:default_gemfile) { Pathname.new("Gemfile") }
+      allow(Bundler).to receive(:ui) { double("UI", :info => "", :debug => "") }
+    end
     context "when it's not possible to write to the file" do
       subject { Bundler::Definition.new(nil, [], Bundler::SourceList.new, []) }
 
@@ -31,4 +30,107 @@ describe Bundler::Definition do
       end
     end
   end
+
+  describe "detects changes" do
+    it "for a path gem with changes" do
+      build_lib "foo", "1.0", :path => lib_path("foo")
+
+      install_gemfile <<-G
+        source "file://#{gem_repo1}"
+        gem "foo", :path => "#{lib_path("foo")}"
+      G
+
+      build_lib "foo", "1.0", :path => lib_path("foo") do |s|
+        s.add_dependency "rack", "1.0"
+      end
+
+      bundle :install, :env => { "DEBUG" => 1 }
+
+      expect(out).to match(/re-resolving dependencies/)
+      lockfile_should_be <<-G
+        PATH
+          remote: #{lib_path("foo")}
+          specs:
+            foo (1.0)
+              rack (= 1.0)
+
+        GEM
+          remote: file:#{gem_repo1}/
+          specs:
+            rack (1.0.0)
+
+        PLATFORMS
+          ruby
+
+        DEPENDENCIES
+          foo!
+
+        BUNDLED WITH
+           #{Bundler::VERSION}
+      G
+    end
+
+    it "for a path gem with deps and no changes" do
+      build_lib "foo", "1.0", :path => lib_path("foo") do |s|
+        s.add_dependency "rack", "1.0"
+        s.add_development_dependency "net-ssh", "1.0"
+      end
+
+      install_gemfile <<-G
+        source "file://#{gem_repo1}"
+        gem "foo", :path => "#{lib_path("foo")}"
+      G
+
+      bundle :check, :env => { "DEBUG" => 1 }
+
+      expect(out).to match(/using resolution from the lockfile/)
+      lockfile_should_be <<-G
+        PATH
+          remote: #{lib_path("foo")}
+          specs:
+            foo (1.0)
+              rack (= 1.0)
+
+        GEM
+          remote: file:#{gem_repo1}/
+          specs:
+            rack (1.0.0)
+
+        PLATFORMS
+          ruby
+
+        DEPENDENCIES
+          foo!
+
+        BUNDLED WITH
+           #{Bundler::VERSION}
+      G
+    end
+
+    it "for a rubygems gem" do
+      install_gemfile <<-G
+        source "file://#{gem_repo1}"
+        gem "foo"
+      G
+
+      bundle :check, :env => { "DEBUG" => 1 }
+
+      expect(out).to match(/using resolution from the lockfile/)
+      lockfile_should_be <<-G
+        GEM
+          remote: file:#{gem_repo1}/
+          specs:
+            foo (1.0)
+
+        PLATFORMS
+          ruby
+
+        DEPENDENCIES
+          foo
+
+        BUNDLED WITH
+           #{Bundler::VERSION}
+      G
+    end
+  end
 end
diff --git a/spec/install/gemfile/path_spec.rb b/spec/install/gemfile/path_spec.rb
index 64919370be..39b0aed788 100644
--- a/spec/install/gemfile/path_spec.rb
+++ b/spec/install/gemfile/path_spec.rb
@@ -363,6 +363,40 @@ describe "bundle install with explicit source paths" do
     expect(exitstatus).to eq(0) if exitstatus
   end
 
+  context "existing lockfile" do
+    it "rubygems gems don't re-resolve without changes" do
+      install_gemfile <<-G
+        source "file://#{gem_repo1}"
+        gem 'rack-obama', '1.0'
+        gem 'net-ssh', '1.0'
+      G
+
+      bundle :check, :env => { "DEBUG" => 1 }
+      expect(out).to match(/using resolution from the lockfile/)
+      should_be_installed "rack-obama 1.0", "net-ssh 1.0"
+    end
+
+    it "source path gems w/deps don't re-resolve without changes" do
+      build_lib "rack-obama", "1.0", :path => lib_path("omg") do |s|
+        s.add_dependency "yard"
+      end
+
+      build_lib "net-ssh", "1.0", :path => lib_path("omg") do |s|
+        s.add_dependency "yard"
+      end
+
+      install_gemfile <<-G
+        source "file://#{gem_repo1}"
+        gem 'rack-obama', :path => "#{lib_path("omg")}"
+        gem 'net-ssh', :path => "#{lib_path("omg")}"
+      G
+
+      bundle :check, :env => { "DEBUG" => 1 }
+      expect(out).to match(/using resolution from the lockfile/)
+      should_be_installed "rack-obama 1.0", "net-ssh 1.0"
+    end
+  end
+
   it "installs executable stubs" do
     build_lib "foo" do |s|
       s.executables = ["foo"]