Merge pull request #8489 from rubygems/deivid-rodriguez/fix-conservativeness-when-lockfile-has-incorrect-deps

Fix dependency locking when Bundler finds incorrect lockfile dependencies

Author: deivid-rodriguez

bundler/lib/bundler/definition.rb

@@ -58,17 +58,28 @@ def self.build(gemfile, lockfile, unlock)
     # @param ruby_version [Bundler::RubyVersion, nil] Requested Ruby Version
     # @param optional_groups [Array(String)] A list of optional groups
     def initialize(lockfile, dependencies, sources, unlock, ruby_version = nil, optional_groups = [], gemfiles = [])
-      if [true, false].include?(unlock)
+      unlock ||= {}
+
+      if unlock == true
+        @unlocking_all = true
         @unlocking_bundler = false
         @unlocking = unlock
+        @sources_to_unlock = []
+        @unlocking_ruby = false
+        @explicit_unlocks = []
+        conservative = false
       else
+        @unlocking_all = false
         @unlocking_bundler = unlock.delete(:bundler)
         @unlocking = unlock.any? {|_k, v| !Array(v).empty? }
+        @sources_to_unlock = unlock.delete(:sources) || []
+        @unlocking_ruby = unlock.delete(:ruby)
+        @explicit_unlocks = unlock.delete(:gems) || []
+        conservative = unlock.delete(:conservative)
       end
 
       @dependencies    = dependencies
       @sources         = sources
-      @unlock          = unlock
       @optional_groups = optional_groups
       @prefer_local    = false
       @specs           = nil
@@ -97,17 +108,15 @@ def initialize(lockfile, dependencies, sources, unlock, ruby_version = nil, opti
         @originally_locked_specs = SpecSet.new(@locked_gems.specs)
         @locked_checksums = @locked_gems.checksums
 
-        if unlock != true
-          @locked_specs   = @originally_locked_specs
-          @locked_sources = @locked_gems.sources
-        else
-          @unlock         = {}
+        if @unlocking_all
           @locked_specs   = SpecSet.new([])
           @locked_sources = []
+        else
+          @locked_specs   = @originally_locked_specs
+          @locked_sources = @locked_gems.sources
         end
       else
-        @unlock         = {}
-        @locked_gems    = nil
+        @locked_gems = nil
         @locked_platforms = []
         @most_specific_locked_platform = nil
         @platforms      = []
@@ -131,21 +140,18 @@ def initialize(lockfile, dependencies, sources, unlock, ruby_version = nil, opti
         @sources.merged_gem_lockfile_sections!(locked_gem_sources.first)
       end
 
-      @sources_to_unlock = @unlock.delete(:sources) || []
-      @unlock[:ruby] ||= if @ruby_version && locked_ruby_version_object
+      @unlocking_ruby ||= if @ruby_version && locked_ruby_version_object
         @ruby_version.diff(locked_ruby_version_object)
       end
-      @unlocking ||= @unlock[:ruby] ||= (!@locked_ruby_version ^ !@ruby_version)
+      @unlocking ||= @unlocking_ruby ||= (!@locked_ruby_version ^ !@ruby_version)
 
       @current_platform_missing = add_current_platform unless Bundler.frozen_bundle?
 
       converge_path_sources_to_gemspec_sources
       @path_changes = converge_paths
       @source_changes = converge_sources
 
-      @explicit_unlocks = @unlock.delete(:gems) || []
-
-      if @unlock[:conservative]
+      if conservative
         @gems_to_unlock = @explicit_unlocks.any? ? @explicit_unlocks : @dependencies.map(&:name)
       else
         eager_unlock = @explicit_unlocks.map {|name| Dependency.new(name, ">= 0") }
@@ -373,7 +379,7 @@ def lock(file_or_preserve_unknown_sections = false, preserve_unknown_sections_or
 
     def locked_ruby_version
       return unless ruby_version
-      if @unlock[:ruby] || !@locked_ruby_version
+      if @unlocking_ruby || !@locked_ruby_version
         Bundler::RubyVersion.system
       else
         @locked_ruby_version
@@ -434,7 +440,7 @@ def ensure_equivalent_gemfile_and_lockfile(explicit_flag = false)
         changed << "* #{name} from `#{lockfile_source_name}` to `#{gemfile_source_name}`"
       end
 
-      reason = nothing_changed? ? "some dependencies were deleted from your gemfile" : change_reason
+      reason = resolve_needed? ? change_reason : "some dependencies were deleted from your gemfile"
       msg = String.new
       msg << "#{reason.capitalize.strip}, but the lockfile can't be updated because frozen mode is set"
       msg << "\n\nYou have added to the Gemfile:\n" << added.join("\n") if added.any?
@@ -450,7 +456,7 @@ def ensure_equivalent_gemfile_and_lockfile(explicit_flag = false)
                "freeze by running `#{suggested_command}`." if suggested_command
       end
 
-      raise ProductionError, msg if added.any? || deleted.any? || changed.any? || !nothing_changed?
+      raise ProductionError, msg if added.any? || deleted.any? || changed.any? || resolve_needed?
     end
 
     def validate_runtime!
@@ -619,7 +625,7 @@ def resolution_packages
       @resolution_packages ||= begin
         last_resolve = converge_locked_specs
         remove_invalid_platforms!
-        packages = Resolver::Base.new(source_requirements, expanded_dependencies, last_resolve, @platforms, locked_specs: @originally_locked_specs, unlock: @gems_to_unlock, prerelease: gem_version_promoter.pre?, prefer_local: @prefer_local)
+        packages = Resolver::Base.new(source_requirements, expanded_dependencies, last_resolve, @platforms, locked_specs: @originally_locked_specs, unlock: @unlocking_all || @gems_to_unlock, prerelease: gem_version_promoter.pre?, prefer_local: @prefer_local, new_platforms: @new_platforms)
         packages = additional_base_requirements_to_prevent_downgrades(packages, last_resolve)
         packages = additional_base_requirements_to_force_updates(packages)
         packages
@@ -762,6 +768,7 @@ def add_current_platform
       @most_specific_non_local_locked_ruby_platform = find_most_specific_locked_ruby_platform
       return if @most_specific_non_local_locked_ruby_platform
 
+      @new_platforms << local_platform
       @platforms << local_platform
       true
     end
@@ -783,7 +790,7 @@ def change_reason
         unlock_reason = if unlock_targets
           "#{unlock_targets.first}: (#{unlock_targets.last.join(", ")})"
         else
-          @unlock[:ruby] ? "ruby" : ""
+          @unlocking_ruby ? "ruby" : ""
         end
 
         return "bundler is unlocking #{unlock_reason}"
@@ -853,8 +860,6 @@ def converge_locals
     end
 
     def check_lockfile
-      @missing_lockfile_dep = nil
-
       @locked_spec_with_invalid_deps = nil
       @locked_spec_with_missing_deps = nil
 
@@ -872,10 +877,6 @@ def check_lockfile
         @locked_specs.delete(missing)
 
         @locked_spec_with_missing_deps = missing.first.name
-      elsif !@dependency_changes
-        @missing_lockfile_dep = current_dependencies.find do |d|
-          @locked_specs[d.name].empty? && d.name != "bundler"
-        end&.name
       end
 
       if invalid.any?
@@ -936,21 +937,30 @@ def converge_sources
     end
 
     def converge_dependencies
+      @missing_lockfile_dep = nil
       changes = false
 
-      @dependencies.each do |dep|
+      current_dependencies.each do |dep|
         if dep.source
           dep.source = sources.get(dep.source)
         end
 
-        unless locked_dep = @locked_deps[dep.name]
-          changes = true
-          next
+        name = dep.name
+
+        dep_changed = @locked_deps[name].nil?
+
+        unless name == "bundler"
+          locked_specs = @originally_locked_specs[name]
+
+          if locked_specs.any? && !dep.matches_spec?(locked_specs.first)
+            @gems_to_unlock << name
+            dep_changed = true
+          elsif locked_specs.empty? && dep_changed == false
+            @missing_lockfile_dep = name
+          end
         end
 
-        # We already know the name matches from the hash lookup
-        # so we only need to check the requirement now
-        changes ||= dep.requirement != locked_dep.requirement
+        changes ||= dep_changed
       end
 
       changes
@@ -1015,11 +1025,6 @@ def converge_specs(specs)
           end
         end
 
-        if dep.nil? && requested_dep = requested_dependencies.find {|d| name == d.name }
-          @gems_to_unlock << name
-          deps << requested_dep
-        end
-
         converged << s
       end
 

bundler/lib/bundler/gem_version_promoter.rb

@@ -132,8 +132,6 @@ def segments_do_not_match?(a, b, level)
     # Specific version moves can't always reliably be done during sorting
     # as not all elements are compared against each other.
     def post_sort(result, unlock, locked_version)
-      # default :major behavior in Bundler does not do this
-      return result if major?
       if unlock || locked_version.nil?
         result
       else

bundler/lib/bundler/resolver/package.rb

@@ -15,7 +15,7 @@ class Resolver
     class Package
       attr_reader :name, :platforms, :dependency, :locked_version
 
-      def initialize(name, platforms, locked_specs:, unlock:, prerelease: false, prefer_local: false, dependency: nil)
+      def initialize(name, platforms, locked_specs:, unlock:, prerelease: false, prefer_local: false, dependency: nil, new_platforms: [])
         @name = name
         @platforms = platforms
         @locked_version = locked_specs.version_for(name)
@@ -24,10 +24,14 @@ def initialize(name, platforms, locked_specs:, unlock:, prerelease: false, prefe
         @top_level = !dependency.nil?
         @prerelease = @dependency.prerelease? || @locked_version&.prerelease? || prerelease ? :consider_first : :ignore
         @prefer_local = prefer_local
+        @new_platforms = new_platforms
       end
 
       def platform_specs(specs)
-        platforms.map {|platform| GemHelpers.select_best_platform_match(specs, platform, prefer_locked: !unlock?) }
+        platforms.map do |platform|
+          prefer_locked = @new_platforms.include?(platform) ? false : !unlock?
+          GemHelpers.select_best_platform_match(specs, platform, prefer_locked: prefer_locked)
+        end
       end
 
       def to_s
@@ -55,7 +59,7 @@ def hash
       end
 
       def unlock?
-        @unlock.empty? || @unlock.include?(name)
+        @unlock == true || @unlock.include?(name)
       end
 
       def ignores_prereleases?

bundler/spec/bundler/gem_version_promoter_spec.rb

@@ -20,13 +20,13 @@ def build_candidates(versions)
       end
     end
 
-    def build_package(name, version, locked = [])
-      Bundler::Resolver::Package.new(name, [], locked_specs: Bundler::SpecSet.new(build_spec(name, version)), unlock: locked)
+    def build_package(name, version, unlock)
+      Bundler::Resolver::Package.new(name, [], locked_specs: Bundler::SpecSet.new(build_spec(name, version)), unlock: unlock)
     end
 
-    def sorted_versions(candidates:, current:, name: "foo", locked: [])
+    def sorted_versions(candidates:, current:, unlock: true)
       gvp.sort_versions(
-        build_package(name, current, locked),
+        build_package("foo", current, unlock),
         build_candidates(candidates)
       ).flatten.map(&:version).map(&:to_s)
     end
@@ -127,7 +127,7 @@ def sorted_versions(candidates:, current:, name: "foo", locked: [])
       before { gvp.level = :minor }
 
       it "keeps the current version first" do
-        versions = sorted_versions(candidates: %w[0.2.0 0.3.0 0.3.1 0.9.0 1.0.0 2.1.0 2.0.1], current: "0.3.0", locked: ["bar"])
+        versions = sorted_versions(candidates: %w[0.2.0 0.3.0 0.3.1 0.9.0 1.0.0 2.1.0 2.0.1], current: "0.3.0", unlock: [])
         expect(versions.first).to eq("0.3.0")
       end
     end

bundler/spec/commands/lock_spec.rb

@@ -645,13 +645,13 @@
     it "single gem updates dependent gem to minor" do
       bundle "lock --update foo --patch"
 
-      expect(the_bundle.locked_gems.specs.map(&:full_name)).to eq(%w[foo-1.4.5 bar-2.1.1 qux-1.0.0].sort)
+      expect(the_bundle.locked_specs).to eq(%w[foo-1.4.5 bar-2.1.1 qux-1.0.0].sort)
     end
 
     it "minor preferred with strict" do
       bundle "lock --update --minor --strict"
 
-      expect(the_bundle.locked_gems.specs.map(&:full_name)).to eq(%w[foo-1.5.0 bar-2.1.1 qux-1.1.0].sort)
+      expect(the_bundle.locked_specs).to eq(%w[foo-1.5.0 bar-2.1.1 qux-1.1.0].sort)
     end
 
     it "shows proper error when Gemfile changes forbid patch upgrades, and --patch --strict is given" do
@@ -674,25 +674,25 @@
       it "defaults to major" do
         bundle "lock --update --pre"
 
-        expect(the_bundle.locked_gems.specs.map(&:full_name)).to eq(%w[foo-2.0.0.pre bar-4.0.0.pre qux-2.0.0].sort)
+        expect(the_bundle.locked_specs).to eq(%w[foo-2.0.0.pre bar-4.0.0.pre qux-2.0.0].sort)
       end
 
       it "patch preferred" do
         bundle "lock --update --patch --pre"
 
-        expect(the_bundle.locked_gems.specs.map(&:full_name)).to eq(%w[foo-1.4.5 bar-2.1.2.pre qux-1.0.1].sort)
+        expect(the_bundle.locked_specs).to eq(%w[foo-1.4.5 bar-2.1.2.pre qux-1.0.1].sort)
       end
 
       it "minor preferred" do
         bundle "lock --update --minor --pre"
 
-        expect(the_bundle.locked_gems.specs.map(&:full_name)).to eq(%w[foo-1.5.1 bar-3.1.0.pre qux-1.1.0].sort)
+        expect(the_bundle.locked_specs).to eq(%w[foo-1.5.1 bar-3.1.0.pre qux-1.1.0].sort)
       end
 
       it "major preferred" do
         bundle "lock --update --major --pre"
 
-        expect(the_bundle.locked_gems.specs.map(&:full_name)).to eq(%w[foo-2.0.0.pre bar-4.0.0.pre qux-2.0.0].sort)
+        expect(the_bundle.locked_specs).to eq(%w[foo-2.0.0.pre bar-4.0.0.pre qux-2.0.0].sort)
       end
     end
   end
@@ -734,7 +734,7 @@
     it "adds the latest version of the new dependency" do
       bundle "lock --minor --update sequel"
 
-      expect(the_bundle.locked_gems.specs.map(&:full_name)).to eq(%w[sequel-5.72.0 bigdecimal-99.1.4].sort)
+      expect(the_bundle.locked_specs).to eq(%w[sequel-5.72.0 bigdecimal-99.1.4].sort)
     end
   end
 

bundler/spec/install/gemfile/gemspec_spec.rb

@@ -420,6 +420,7 @@
 
         simulate_new_machine
         simulate_platform("jruby") { bundle "install" }
+        expect(lockfile).to include("platform_specific (1.0-java)")
         simulate_platform("x64-mingw32") { bundle "install" }
       end
 

bundler/spec/lock/lockfile_spec.rb

@@ -1875,6 +1875,62 @@
     L
   end
 
+  it "automatically fixes the lockfile when it has incorrect deps, keeping the locked version" do
+    build_repo4 do
+      build_gem "net-smtp", "0.5.0" do |s|
+        s.add_dependency "net-protocol"
+      end
+
+      build_gem "net-smtp", "0.5.1" do |s|
+        s.add_dependency "net-protocol"
+      end
+
+      build_gem "net-protocol", "0.2.2"
+    end
+
+    gemfile <<~G
+      source "#{file_uri_for(gem_repo4)}"
+      gem "net-smtp"
+    G
+
+    lockfile <<~L
+      GEM
+        remote: #{file_uri_for(gem_repo4)}/
+        specs:
+          net-protocol (0.2.2)
+          net-smtp (0.5.0)
+
+      PLATFORMS
+        #{lockfile_platforms}
+
+      DEPENDENCIES
+        net-smtp
+
+      BUNDLED WITH
+         #{Bundler::VERSION}
+    L
+
+    bundle "install"
+
+    expect(lockfile).to eq <<~L
+      GEM
+        remote: #{file_uri_for(gem_repo4)}/
+        specs:
+          net-protocol (0.2.2)
+          net-smtp (0.5.0)
+            net-protocol
+
+      PLATFORMS
+        #{lockfile_platforms}
+
+      DEPENDENCIES
+        net-smtp
+
+      BUNDLED WITH
+         #{Bundler::VERSION}
+    L
+  end
+
   shared_examples_for "a lockfile missing dependent specs" do
     it "auto-heals" do
       build_repo4 do