From 2569413b1cb9765d0e31fc8dcf0e674d428bb4e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Mon, 19 Aug 2024 21:53:13 +0200 Subject: [PATCH] [rubygems/rubygems] Fix `--prefer-local` flag The original implementation of this flag was too naive and all it did was restricting gems to locally installed versions if there are any local versions installed. However, it should be much smarter. For example: * It should fallback to remote versions if locally installed version don't satisfy the requirements. * It should pick locally installed versions even for subdependencies not yet discovered. This commit fixes both issues by using a smarter approach similar to how we resolve prereleases: * First resolve optimistically using only locally installed gems. * If any conflicts are found, scan those conflicts, allow remote versions for the specific gems that run into conflicts, and re-resolve. https://github.com/rubygems/rubygems/commit/607a3bf479 Co-authored-by: Gourav Khunger --- lib/bundler/definition.rb | 16 +--------- lib/bundler/resolver.rb | 38 ++++++++++++++++++----- lib/bundler/resolver/base.rb | 6 ++++ lib/bundler/resolver/package.rb | 11 ++++++- spec/bundler/commands/install_spec.rb | 44 ++++++++++++++++++++++++++- 5 files changed, 90 insertions(+), 25 deletions(-) diff --git a/lib/bundler/definition.rb b/lib/bundler/definition.rb index d2cab437a0..5471a72fdd 100644 --- a/lib/bundler/definition.rb +++ b/lib/bundler/definition.rb @@ -571,7 +571,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?) + 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 = additional_base_requirements_to_prevent_downgrades(packages, last_resolve) packages = additional_base_requirements_to_force_updates(packages) packages @@ -655,19 +655,6 @@ module Bundler sources.non_global_rubygems_sources.all?(&:dependency_api_available?) && !sources.aggregate_global_source? end - def pin_locally_available_names(source_requirements) - source_requirements.each_with_object({}) do |(name, original_source), new_source_requirements| - local_source = original_source.dup - local_source.local_only! - - new_source_requirements[name] = if local_source.specs.search(name).any? - local_source - else - original_source - end - end - end - def current_platform_locked? @platforms.any? do |bundle_platform| MatchPlatform.platforms_match?(bundle_platform, local_platform) @@ -983,7 +970,6 @@ module Bundler # look for that gemspec (or its dependencies) source_requirements = if precompute_source_requirements_for_indirect_dependencies? all_requirements = source_map.all_requirements - all_requirements = pin_locally_available_names(all_requirements) if @prefer_local { default: default_source }.merge(all_requirements) else { default: Source::RubygemsAggregate.new(sources, source_map) }.merge(source_map.direct_requirements) diff --git a/lib/bundler/resolver.rb b/lib/bundler/resolver.rb index de324fbcf3..a38b6974f8 100644 --- a/lib/bundler/resolver.rb +++ b/lib/bundler/resolver.rb @@ -84,9 +84,9 @@ module Bundler rescue PubGrub::SolveFailure => e incompatibility = e.incompatibility - names_to_unlock, names_to_allow_prereleases_for, extended_explanation = find_names_to_relax(incompatibility) + names_to_unlock, names_to_allow_prereleases_for, names_to_allow_remote_specs_for, extended_explanation = find_names_to_relax(incompatibility) - names_to_relax = names_to_unlock + names_to_allow_prereleases_for + names_to_relax = names_to_unlock + names_to_allow_prereleases_for + names_to_allow_remote_specs_for if names_to_relax.any? if names_to_unlock.any? @@ -101,6 +101,12 @@ module Bundler @base.include_prereleases(names_to_allow_prereleases_for) end + if names_to_allow_remote_specs_for.any? + Bundler.ui.debug "Found conflicts with local versions of #{names_to_allow_remote_specs_for.join(", ")}. Will retry considering remote versions...", true + + @base.include_remote_specs(names_to_allow_remote_specs_for) + end + root, logger = setup_solver Bundler.ui.debug "Retrying resolution...", true @@ -120,6 +126,7 @@ module Bundler def find_names_to_relax(incompatibility) names_to_unlock = [] names_to_allow_prereleases_for = [] + names_to_allow_remote_specs_for = [] extended_explanation = nil while incompatibility.conflict? @@ -134,6 +141,8 @@ module Bundler names_to_unlock << name elsif package.ignores_prereleases? && @all_specs[name].any? {|s| s.version.prerelease? } names_to_allow_prereleases_for << name + elsif package.prefer_local? && @all_specs[name].any? {|s| !s.is_a?(StubSpecification) } + names_to_allow_remote_specs_for << name end no_versions_incompat = [cause.incompatibility, cause.satisfier].find {|incompat| incompat.cause.is_a?(PubGrub::Incompatibility::NoVersions) } @@ -143,7 +152,7 @@ module Bundler end end - [names_to_unlock.uniq, names_to_allow_prereleases_for.uniq, extended_explanation] + [names_to_unlock.uniq, names_to_allow_prereleases_for.uniq, names_to_allow_remote_specs_for.uniq, extended_explanation] end def parse_dependency(package, dependency) @@ -244,7 +253,7 @@ module Bundler def all_versions_for(package) name = package.name - results = (@base[name] + filter_prereleases(@all_specs[name], package)).uniq {|spec| [spec.version.hash, spec.platform] } + results = (@base[name] + filter_specs(@all_specs[name], package)).uniq {|spec| [spec.version.hash, spec.platform] } if name == "bundler" && !bundler_pinned_to_current_version? bundler_spec = Gem.loaded_specs["bundler"] @@ -368,12 +377,22 @@ module Bundler end end + def filter_specs(specs, package) + filter_remote_specs(filter_prereleases(specs, package), package) + end + def filter_prereleases(specs, package) return specs unless package.ignores_prereleases? && specs.size > 1 specs.reject {|s| s.version.prerelease? } end + def filter_remote_specs(specs, package) + return specs unless package.prefer_local? + + specs.select {|s| s.is_a?(StubSpecification) } + end + # Ignore versions that depend on themselves incorrectly def filter_invalid_self_dependencies(specs, name) specs.reject do |s| @@ -405,10 +424,13 @@ module Bundler dep_range = dep_constraint.range versions = select_sorted_versions(dep_package, dep_range) - if versions.empty? && dep_package.ignores_prereleases? - @all_versions.delete(dep_package) - @sorted_versions.delete(dep_package) - dep_package.consider_prereleases! + if versions.empty? + if dep_package.ignores_prereleases? || dep_package.prefer_local? + @all_versions.delete(dep_package) + @sorted_versions.delete(dep_package) + end + dep_package.consider_prereleases! if dep_package.ignores_prereleases? + dep_package.consider_remote_versions! if dep_package.prefer_local? versions = select_sorted_versions(dep_package, dep_range) end diff --git a/lib/bundler/resolver/base.rb b/lib/bundler/resolver/base.rb index b0c5c58047..3f2436672a 100644 --- a/lib/bundler/resolver/base.rb +++ b/lib/bundler/resolver/base.rb @@ -72,6 +72,12 @@ module Bundler end end + def include_remote_specs(names) + names.each do |name| + get_package(name).consider_remote_versions! + end + end + private def indirect_pins(names) diff --git a/lib/bundler/resolver/package.rb b/lib/bundler/resolver/package.rb index 16c43d05af..5aecc12d05 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, dependency: nil) + def initialize(name, platforms, locked_specs:, unlock:, prerelease: false, prefer_local: false, dependency: nil) @name = name @platforms = platforms @locked_version = locked_specs[name].first&.version @@ -23,6 +23,7 @@ module Bundler @dependency = dependency || Dependency.new(name, @locked_version) @top_level = !dependency.nil? @prerelease = @dependency.prerelease? || @locked_version&.prerelease? || prerelease ? :consider_first : :ignore + @prefer_local = prefer_local end def platform_specs(specs) @@ -69,6 +70,14 @@ module Bundler @prerelease = :consider_last end + def prefer_local? + @prefer_local + end + + def consider_remote_versions! + @prefer_local = false + end + def force_ruby_platform? @dependency.force_ruby_platform end diff --git a/spec/bundler/commands/install_spec.rb b/spec/bundler/commands/install_spec.rb index 09f920052a..c89ed0c870 100644 --- a/spec/bundler/commands/install_spec.rb +++ b/spec/bundler/commands/install_spec.rb @@ -1362,12 +1362,20 @@ RSpec.describe "bundle install with gem sources" do build_gem "foo", "1.0.1" build_gem "foo", "1.0.0" build_gem "bar", "1.0.0" + + build_gem "a", "1.0.0" do |s| + s.add_dependency "foo", "~> 1.0.0" + end + + build_gem "b", "1.0.0" do |s| + s.add_dependency "foo", "~> 1.0.1" + end end system_gems "foo-1.0.0", path: default_bundle_path, gem_repo: gem_repo4 end - it "fetches remote sources only when not available locally" do + it "fetches remote sources when not available locally" do install_gemfile <<-G, "prefer-local": true, verbose: true source "https://gem.repo4" @@ -1378,6 +1386,40 @@ RSpec.describe "bundle install with gem sources" do expect(out).to include("Using foo 1.0.0").and include("Fetching bar 1.0.0").and include("Installing bar 1.0.0") expect(last_command).to be_success end + + it "fetches remote sources when local version does not match requirements" do + install_gemfile <<-G, "prefer-local": true, verbose: true + source "https://gem.repo4" + + gem "foo", "1.0.1" + gem "bar" + G + + expect(out).to include("Fetching foo 1.0.1").and include("Installing foo 1.0.1").and include("Fetching bar 1.0.0").and include("Installing bar 1.0.0") + expect(last_command).to be_success + end + + it "uses the locally available version for sub-dependencies when possible" do + install_gemfile <<-G, "prefer-local": true, verbose: true + source "https://gem.repo4" + + gem "a" + G + + expect(out).to include("Using foo 1.0.0").and include("Fetching a 1.0.0").and include("Installing a 1.0.0") + expect(last_command).to be_success + end + + it "fetches remote sources for sub-dependencies when the locally available version does not satisfy the requirement" do + install_gemfile <<-G, "prefer-local": true, verbose: true + source "https://gem.repo4" + + gem "b" + G + + expect(out).to include("Fetching foo 1.0.1").and include("Installing foo 1.0.1").and include("Fetching b 1.0.0").and include("Installing b 1.0.0") + expect(last_command).to be_success + end end context "with a symlinked configured as bundle path and a gem with symlinks" do