[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 <gouravkhunger18@gmail.com>
2024-08-19 21:53:13 +02:00 · 2024-08-19 21:53:13 +02:00 · 2569413b1c
--- 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)
--- 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

--- 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)
--- 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
--- 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