From 6a69807576b911dd9d5a81befe08a7c3bb870b85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Rodr=C3=ADguez?= Date: Wed, 17 Nov 2021 16:45:50 +0100 Subject: [PATCH] [rubygems/rubygems] Completely drop base parameter from index This parameter was coupling the concept of lockfile with the index. I don't think it's necessary. Also I believe it's causing some flaky test failures, which might leak into realworld issues. They are like this: ```` Invoking `/opt/hostedtoolcache/Ruby/3.0.4/x64/bin/ruby -I/home/runner/work/rubygems/rubygems/bundler/spec -r/home/runner/work/rubygems/rubygems/bundler/spec/support/artifice/fail.rb -r/home/runner/work/rubygems/rubygems/bundler/spec/support/hax.rb /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/bin/bundle lock` failed with output: ---------------------------------------------------------------------- --- ERROR REPORT TEMPLATE ------------------------------------------------------- ``` NoMethodError: undefined method `identifier' for #, @authors=["no one"], @bindir="bin", @cert_chain=[], @dependencies=[], @executables=[], @extensions=[], @extra_rdoc_files=[], @files=[], @licenses=[], @metadata={}, @platform="ruby", @rdoc_options=[], @require_paths=["lib"], @required_ruby_version=#=", #]]>, @required_rubygems_version=#=", #]]>, @requirements=[], @rubygems_version="3.2.33", @specification_version=4, @test_files=[], @new_platform="ruby", @full_name="win32-process-0.8.3", @has_rdoc=true, @license=["MIT"] win32-process-0.8.3> /home/runner/work/rubygems/rubygems/bundler/tmp/rubygems/lib/rubygems/specification.rb:2116:in `method_missing' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/remote_specification.rb:115:in `method_missing' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/lazy_specification.rb:34:in `eql?' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/index.rb:189:in `eql?' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/index.rb:189:in `search_by_dependency' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/index.rb:96:in `local_search' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/index.rb:64:in `unsorted_search' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/index.rb:60:in `search' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/resolver.rb:179:in `results_for' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/resolver.rb:113:in `search_for' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/resolver.rb:216:in `block in sort_dependencies' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/resolver.rb:207:in `each' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/resolver.rb:207:in `sort_by' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/resolver.rb:207:in `sort_dependencies' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/vendor/molinillo/lib/molinillo/delegates/specification_provider.rb:60:in `block in sort_dependencies' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/vendor/molinillo/lib/molinillo/delegates/specification_provider.rb:77:in `with_no_such_dependency_error_handling' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/vendor/molinillo/lib/molinillo/delegates/specification_provider.rb:59:in `sort_dependencies' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/vendor/molinillo/lib/molinillo/resolution.rb:754:in `push_state_for_requirements' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/vendor/molinillo/lib/molinillo/resolution.rb:744:in `require_nested_dependencies_for' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/vendor/molinillo/lib/molinillo/resolution.rb:727:in `activate_new_spec' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/vendor/molinillo/lib/molinillo/resolution.rb:684:in `attempt_to_activate' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/vendor/molinillo/lib/molinillo/resolution.rb:254:in `process_topmost_state' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/vendor/molinillo/lib/molinillo/resolution.rb:182:in `resolve' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/vendor/molinillo/lib/molinillo/resolver.rb:43:in `resolve' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/resolver.rb:50:in `start' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/resolver.rb:24:in `resolve' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/definition.rb:480:in `reresolve' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/definition.rb:283:in `resolve' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/definition.rb:181:in `resolve_remotely!' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/cli/lock.rb:53:in `run' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/cli.rb:674:in `lock' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/vendor/thor/lib/thor/invocation.rb:127:in `invoke_command' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/vendor/thor/lib/thor.rb:392:in `dispatch' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/cli.rb:31:in `dispatch' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/vendor/thor/lib/thor/base.rb:485:in `start' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/cli.rb:25:in `start' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/exe/bundle:48:in `block in ' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/lib/bundler/friendly_errors.rb:120:in `with_friendly_errors' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/gems/bundler-2.4.0.dev/exe/bundle:36:in `' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/bin/bundle:23:in `load' /home/runner/work/rubygems/rubygems/bundler/tmp/1/gems/system/bin/bundle:23:in `
' ``` ```` I think the issue is that now we eagerly materialize some base specifications before resolving in order to give better errors if user specified an incorrect source in the Gemfile. This means that the key for the index hash will have heterogeneous specification objects (some LazySpecification, some real Specification), and `LazySpecification#eql?` is incompatible with that. By dropping the base parameter from the index, we should no longer have these heterogenous objects as hash keys. https://github.com/rubygems/rubygems/commit/dc179d41c3 --- lib/bundler/index.rb | 22 ++++++++++------------ lib/bundler/resolver.rb | 6 +++--- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/lib/bundler/index.rb b/lib/bundler/index.rb index 5bc24fc0b2..00c7a9e00d 100644 --- a/lib/bundler/index.rb +++ b/lib/bundler/index.rb @@ -56,17 +56,17 @@ module Bundler # Search this index's specs, and any source indexes that this index knows # about, returning all of the results. - def search(query, base = nil) - sort_specs(unsorted_search(query, base)) + def search(query) + sort_specs(unsorted_search(query)) end - def unsorted_search(query, base) - results = local_search(query, base) + def unsorted_search(query) + results = local_search(query) seen = results.map(&:full_name).uniq unless @sources.empty? @sources.each do |source| - source.unsorted_search(query, base).each do |spec| + source.unsorted_search(query).each do |spec| next if seen.include?(spec.full_name) seen << spec.full_name @@ -89,12 +89,12 @@ module Bundler self.class.sort_specs(specs) end - def local_search(query, base = nil) + def local_search(query) case query when Gem::Specification, RemoteSpecification, LazySpecification, EndpointSpecification then search_by_spec(query) when String then specs_by_name(query) - when Gem::Dependency then search_by_dependency(query, base) - when DepProxy then search_by_dependency(query.dep, base) + when Gem::Dependency then search_by_dependency(query) + when DepProxy then search_by_dependency(query.dep) else raise "You can't search for a #{query.inspect}." end @@ -185,11 +185,9 @@ module Bundler @specs[name].values end - def search_by_dependency(dependency, base = nil) - @cache[base || false] ||= {} - @cache[base || false][dependency] ||= begin + def search_by_dependency(dependency) + @cache[dependency] ||= begin specs = specs_by_name(dependency.name) - specs += base if base found = specs.select do |spec| next true if spec.source.is_a?(Source::Gemspec) dependency.matches_spec?(spec) diff --git a/lib/bundler/resolver.rb b/lib/bundler/resolver.rb index 33f232e184..40bc247b32 100644 --- a/lib/bundler/resolver.rb +++ b/lib/bundler/resolver.rb @@ -110,7 +110,7 @@ module Bundler dependency = dependency_proxy.dep name = dependency.name @search_for[dependency_proxy] ||= begin - results = results_for(dependency, @base[name]) + results = results_for(dependency) + @base[name].select {|spec| requirement_satisfied_by?(dependency, nil, spec) } if vertex = @base_dg.vertex_named(name) locked_requirement = vertex.payload.requirement @@ -175,8 +175,8 @@ module Bundler @source_requirements[name] || @source_requirements[:default] end - def results_for(dependency, base) - index_for(dependency).search(dependency, base) + def results_for(dependency) + index_for(dependency).search(dependency) end def name_for(dependency)