From c77354157f7a0af8de5f16dc1b4f352f4bc1402f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Wed, 12 Feb 2025 15:45:24 +0100 Subject: [PATCH] [rubygems/rubygems] Fix locked gems being upgraded when locked dependencies are incorrect Resolver had internal logic to prioritize locked versions when sorting versions, however part of it was not being actually hit because of how unlocking worked in the resolver: a package was allow to be unlocked when that was explicit requested or when the list of unlocks was empty. That did not make a lot of sense and other cases were working because the explicit list of unlocks was getting "artificially filled". Now we consider a package unlocked when explicitly requested (`bundle update `), or when everything is being unlocked (`bundle install` with no lockfile or `bundle update`). This makes things simpler and gets the edge case added as a test case working as expected. https://github.com/rubygems/rubygems/commit/b8e55087f0 --- lib/bundler/definition.rb | 9 +-- lib/bundler/gem_version_promoter.rb | 2 - lib/bundler/resolver/package.rb | 10 +++- .../bundler/gem_version_promoter_spec.rb | 10 ++-- spec/bundler/lock/lockfile_spec.rb | 56 +++++++++++++++++++ spec/bundler/resolver/basic_spec.rb | 20 +++---- spec/bundler/support/indexes.rb | 1 - 7 files changed, 81 insertions(+), 27 deletions(-) diff --git a/lib/bundler/definition.rb b/lib/bundler/definition.rb index ec9c50c2d8..3bb849d98f 100644 --- a/lib/bundler/definition.rb +++ b/lib/bundler/definition.rb @@ -625,7 +625,7 @@ module Bundler @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 @@ -768,6 +768,7 @@ module Bundler @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 @@ -952,6 +953,7 @@ module 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 @@ -1023,11 +1025,6 @@ module Bundler 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 diff --git a/lib/bundler/gem_version_promoter.rb b/lib/bundler/gem_version_promoter.rb index ecc65b4956..d64dbacfdb 100644 --- a/lib/bundler/gem_version_promoter.rb +++ b/lib/bundler/gem_version_promoter.rb @@ -132,8 +132,6 @@ module Bundler # 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 diff --git a/lib/bundler/resolver/package.rb b/lib/bundler/resolver/package.rb index 7bdcd7e9eb..0e86a4f84d 100644 --- a/lib/bundler/resolver/package.rb +++ b/lib/bundler/resolver/package.rb @@ -15,7 +15,7 @@ module Bundler 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 @@ module Bundler @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 @@ module Bundler end def unlock? - @unlock.empty? || @unlock.include?(name) + @unlock == true || @unlock.include?(name) end def ignores_prereleases? diff --git a/spec/bundler/bundler/gem_version_promoter_spec.rb b/spec/bundler/bundler/gem_version_promoter_spec.rb index 917daba95d..0e1b7c9cc8 100644 --- a/spec/bundler/bundler/gem_version_promoter_spec.rb +++ b/spec/bundler/bundler/gem_version_promoter_spec.rb @@ -20,13 +20,13 @@ RSpec.describe Bundler::GemVersionPromoter do 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 @@ RSpec.describe Bundler::GemVersionPromoter do 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 diff --git a/spec/bundler/lock/lockfile_spec.rb b/spec/bundler/lock/lockfile_spec.rb index ce7d7fb131..00ec05a8a1 100644 --- a/spec/bundler/lock/lockfile_spec.rb +++ b/spec/bundler/lock/lockfile_spec.rb @@ -1875,6 +1875,62 @@ RSpec.describe "the lockfile format" do 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 diff --git a/spec/bundler/resolver/basic_spec.rb b/spec/bundler/resolver/basic_spec.rb index 0d3eadbaf8..852c710415 100644 --- a/spec/bundler/resolver/basic_spec.rb +++ b/spec/bundler/resolver/basic_spec.rb @@ -211,12 +211,12 @@ RSpec.describe "Resolving" do it "resolves all gems to latest patch" do # strict is not set, so bar goes up a minor version due to dependency from foo 1.4.5 - should_conservative_resolve_and_include :patch, [], %w[foo-1.4.5 bar-2.1.1] + should_conservative_resolve_and_include :patch, true, %w[foo-1.4.5 bar-2.1.1] end it "resolves all gems to latest patch strict" do # strict is set, so foo can only go up to 1.4.4 to avoid bar going up a minor version, and bar can go up to 2.0.5 - should_conservative_resolve_and_include [:patch, :strict], [], %w[foo-1.4.4 bar-2.0.5] + should_conservative_resolve_and_include [:patch, :strict], true, %w[foo-1.4.4 bar-2.0.5] end it "resolves foo only to latest patch - same dependency case" do @@ -256,20 +256,20 @@ RSpec.describe "Resolving" do it "resolves all gems to latest minor" do # strict is not set, so bar goes up a major version due to dependency from foo 1.4.5 - should_conservative_resolve_and_include :minor, [], %w[foo-1.5.1 bar-3.0.0] + should_conservative_resolve_and_include :minor, true, %w[foo-1.5.1 bar-3.0.0] end it "resolves all gems to latest minor strict" do # strict is set, so foo can only go up to 1.5.0 to avoid bar going up a major version - should_conservative_resolve_and_include [:minor, :strict], [], %w[foo-1.5.0 bar-2.1.1] + should_conservative_resolve_and_include [:minor, :strict], true, %w[foo-1.5.0 bar-2.1.1] end it "resolves all gems to latest major" do - should_conservative_resolve_and_include :major, [], %w[foo-2.0.0 bar-3.0.0] + should_conservative_resolve_and_include :major, true, %w[foo-2.0.0 bar-3.0.0] end it "resolves all gems to latest major strict" do - should_conservative_resolve_and_include [:major, :strict], [], %w[foo-2.0.0 bar-3.0.0] + should_conservative_resolve_and_include [:major, :strict], true, %w[foo-2.0.0 bar-3.0.0] end # Why would this happen in real life? If bar 2.2 has a bug that the author of foo wants to bypass @@ -292,21 +292,21 @@ RSpec.describe "Resolving" do end it "could revert to a previous version level patch" do - should_conservative_resolve_and_include :patch, [], %w[foo-1.4.4 bar-2.1.1] + should_conservative_resolve_and_include :patch, true, %w[foo-1.4.4 bar-2.1.1] end it "cannot revert to a previous version in strict mode level patch" do # fall back to the locked resolution since strict means we can't regress either version - should_conservative_resolve_and_include [:patch, :strict], [], %w[foo-1.4.3 bar-2.2.3] + should_conservative_resolve_and_include [:patch, :strict], true, %w[foo-1.4.3 bar-2.2.3] end it "could revert to a previous version level minor" do - should_conservative_resolve_and_include :minor, [], %w[foo-1.5.0 bar-2.0.5] + should_conservative_resolve_and_include :minor, true, %w[foo-1.5.0 bar-2.0.5] end it "cannot revert to a previous version in strict mode level minor" do # fall back to the locked resolution since strict means we can't regress either version - should_conservative_resolve_and_include [:minor, :strict], [], %w[foo-1.4.3 bar-2.2.3] + should_conservative_resolve_and_include [:minor, :strict], true, %w[foo-1.4.3 bar-2.2.3] end end end diff --git a/spec/bundler/support/indexes.rb b/spec/bundler/support/indexes.rb index 7960bae59f..2d592808f0 100644 --- a/spec/bundler/support/indexes.rb +++ b/spec/bundler/support/indexes.rb @@ -66,7 +66,6 @@ module Spec end def should_conservative_resolve_and_include(opts, unlock, specs) - # empty unlock means unlock all opts = Array(opts) search = Bundler::GemVersionPromoter.new.tap do |s| s.level = opts.first