summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDavid Rodríguez <deivid.rodriguez@riseup.net>2020-03-04 21:10:31 +0100
committerHiroshi SHIBATA <hsbt@ruby-lang.org>2020-05-08 14:13:29 +0900
commit18ac783ea6ad00b664d784661643bcbabc5e48eb (patch)
treeaf932b290254d18dc44609cb98f0da597f51a312
parent46462200afef55fd21b72ad1ff745739b085a793 (diff)
[rubygems/rubygems] Revert adding loaded specs to `Gem::Specification.stubs` and `Gem::Specification.stubs_for`
The rationale is that: * The change has caused realworld issues. See for example https://github.com/ruby/did_you_mean/issues/117 and specifically [this comment](https://github.com/ruby/did_you_mean/issues/117#issuecomment-482733159) for a great explanation of the issue it caused for `did_you_mean`. * The change also causes problems for our development workflows. For example, because of it, our `bundler` specs cannot currently be run with `bin/rake` and we have to use `bin/rspec` or `bin/parallel_spec` directly. The explanation for this is: - Our specs install test dependencies to `tmp` before running specs. - `rake` is one of these test dependencies. - Before installing each test dependency, we check whether it has matching installed specs: https://github.com/rubygems/rubygems/blob/2bbcdcde08b90d4ef03da8fb1f7a8a3313e13bb8/bundler/spec/support/rubygems_ext.rb#L109-L114. - Normally, if `rake` has not yet been installed to `tmp`, this check fails and `rake` is installed, but since the loaded specs are now added to `Gem::Specification.stubs` and `rake`'s specification _is_ loaded because we're running through `bin/rake`, the check incorrectly assumes that `rake` is already installed to `tmp` and skips installation. - At a later point the specs check whether `rake` is actually installed and fail if it's not: https://github.com/rubygems/rubygems/blob/2bbcdcde08b90d4ef03da8fb1f7a8a3313e13bb8/bundler/spec/support/builders.rb#L372-L383 Essentially, both of the issues are the same. If at runtime we change the location of gems, we'll _want_ to not consider loaded specifications when dealing with the new gem location, because the loaded specifications have not been loaded from there. Loaded specifications is something different from installed stub specifications and those should not be mixed. The PR still seemed to have fixed an issue, so I did my archaeology job and investigated the original issue to double check if reverting is ok. The logs for the original error can be found here: https://ci.appveyor.com/project/rubygems/rubygems/build/1172/job/ogubyucpljcv22ux. So I installed ruby 2.4.4, checked out the commit reference before the offending PR, and the exact error reproduced. :tada: ``` $ rake test /home/deivid/Code/rubygems/lib/rubygems/resolver.rb:231:in `search_for': Unable to resolve dependency: user requested 'bundler (= 1.16.2)' (Gem::UnsatisfiableDependencyError) from /home/deivid/Code/rubygems/lib/rubygems/resolver.rb:283:in `block in sort_dependencies' from /home/deivid/Code/rubygems/lib/rubygems/resolver.rb:277:in `each' from /home/deivid/Code/rubygems/lib/rubygems/resolver.rb:277:in `sort_by' from /home/deivid/Code/rubygems/lib/rubygems/resolver.rb:277:in `with_index' from /home/deivid/Code/rubygems/lib/rubygems/resolver.rb:277:in `sort_dependencies' from /home/deivid/Code/rubygems/lib/rubygems/resolver/molinillo/lib/molinillo/delegates/specification_provider.rb:52:in `block in sort_dependencies' from /home/deivid/Code/rubygems/lib/rubygems/resolver/molinillo/lib/molinillo/delegates/specification_provider.rb:69:in `with_no_such_dependency_error_handling' from /home/deivid/Code/rubygems/lib/rubygems/resolver/molinillo/lib/molinillo/delegates/specification_provider.rb:51:in `sort_dependencies' from /home/deivid/Code/rubygems/lib/rubygems/resolver/molinillo/lib/molinillo/resolution.rb:165:in `initial_state' from /home/deivid/Code/rubygems/lib/rubygems/resolver/molinillo/lib/molinillo/resolution.rb:106:in `start_resolution' from /home/deivid/Code/rubygems/lib/rubygems/resolver/molinillo/lib/molinillo/resolution.rb:64:in `resolve' from /home/deivid/Code/rubygems/lib/rubygems/resolver/molinillo/lib/molinillo/resolver.rb:42:in `resolve' from /home/deivid/Code/rubygems/lib/rubygems/resolver.rb:188:in `resolve' from /home/deivid/Code/rubygems/lib/rubygems/request_set.rb:396:in `resolve' from /home/deivid/Code/rubygems/lib/rubygems/request_set.rb:408:in `resolve_current' from /home/deivid/Code/rubygems/lib/rubygems.rb:243:in `finish_resolve' from /home/deivid/Code/rubygems/lib/rubygems/rdoc.rb:13:in `<top (required)>' from /home/deivid/Code/rubygems/lib/rubygems/core_ext/kernel_require.rb:54:in `require' from /home/deivid/Code/rubygems/lib/rubygems/core_ext/kernel_require.rb:54:in `require' from /home/deivid/Code/rubygems/lib/rubygems/test_case.rb:1563:in `<top (required)>' from /home/deivid/Code/rubygems/test/rubygems/test_bundled_ca.rb:2:in `require' from /home/deivid/Code/rubygems/test/rubygems/test_bundled_ca.rb:2:in `<top (required)>' from /home/deivid/.rbenv/versions/2.4.4/lib/ruby/gems/2.4.0/gems/rake-12.0.0/lib/rake/rake_test_loader.rb:15:in `require' from /home/deivid/.rbenv/versions/2.4.4/lib/ruby/gems/2.4.0/gems/rake-12.0.0/lib/rake/rake_test_loader.rb:15:in `block in <main>' from /home/deivid/.rbenv/versions/2.4.4/lib/ruby/gems/2.4.0/gems/rake-12.0.0/lib/rake/rake_test_loader.rb:4:in `select' from /home/deivid/.rbenv/versions/2.4.4/lib/ruby/gems/2.4.0/gems/rake-12.0.0/lib/rake/rake_test_loader.rb:4:in `<main>' rake aborted! Command failed with status (1) Tasks: TOP => test ``` Now the explanation of the error: * Rubygems base `TestCase` class requires `bundler` because some tests use `bundler`: https://github.com/rubygems/rubygems/blob/2bbcdcde08b90d4ef03da8fb1f7a8a3313e13bb8/lib/rubygems/test_case.rb#L26 * That `require` (our custom rubygems require) would activate the default bundler spec (1.16.1 for ruby 2.4.4) but then overwrite it with a 1.16.2 version (the locally provided bundler those days) due to [this old hack](https://github.com/rubygems/bundler/blob/9f7bf0ac3ab8d995e3a274cec3c292a5203f4534/lib/bundler/version.rb#L7-L23). * Rubygems base `TestCase` class requires `rubygems/rdoc`: https://github.com/rubygems/rubygems/blob/2bbcdcde08b90d4ef03da8fb1f7a8a3313e13bb8/lib/rubygems/test_case.rb#L1536 * And that file ends up calling `Gem.finish_resolve`: https://github.com/rubygems/rubygems/blob/2bbcdcde08b90d4ef03da8fb1f7a8a3313e13bb8/lib/rubygems/rdoc.rb#L13 * `Gem.finish_resolve` adds the currently loaded specs to the resolution: https://github.com/rubygems/rubygems/blob/2bbcdcde08b90d4ef03da8fb1f7a8a3313e13bb8/lib/rubygems.rb#L235 * That means it would try to resolve bundler 1.16.2, but no specification for that version was installed since the default was 1.16.1. That explains why upgrading to rubygems 2.7.7 fixed the issue, since it provided bundler 1.16.2 by default so there was not bundler version discrepancy. After understanding the error, I conclude that: * Only this part of the original patch was actually needed to resolve the error, not any of the changes in `Gem::Specification.stubs` and `Gem::Specification.stubs_for`: ```diff diff --git a/lib/rubygems/test_case.rb b/lib/rubygems/test_case.rb index f1cd3d274c..92c848e870 100644 --- a/lib/rubygems/test_case.rb +++ b/lib/rubygems/test_case.rb @@ -13,6 +13,15 @@ else require 'rubygems' end +# If bundler gemspec exists, add to stubs +bundler_gemspec = File.expand_path("../../../bundler/bundler.gemspec", __FILE__) +if File.exist?(bundler_gemspec) + Gem::Specification.dirs.unshift File.dirname(bundler_gemspec) + Gem::Specification.class_variable_set :@@stubs, nil + Gem::Specification.stubs + Gem::Specification.dirs.shift +end + begin gem 'minitest' rescue Gem::LoadError ``` So, I propose to revert adding loaded specification to `Gem::Specification.stubs` and `Gem::Specification.stubs_for` because I think it's safe, it fixes the issues caused by their addition, and it simplifies `Gem::Specification` code, which is already complicated enough. https://github.com/rubygems/rubygems/commit/5269cd617c
Notes
Notes: Merged: https://github.com/ruby/ruby/pull/3092
-rw-r--r--lib/rubygems/specification.rb6
-rw-r--r--test/rubygems/test_gem_specification.rb42
2 files changed, 2 insertions, 46 deletions
diff --git a/lib/rubygems/specification.rb b/lib/rubygems/specification.rb
index 4f2b64e652..ba90974657 100644
--- a/lib/rubygems/specification.rb
+++ b/lib/rubygems/specification.rb
@@ -801,7 +801,7 @@ class Gem::Specification < Gem::BasicSpecification
def self.stubs
@@stubs ||= begin
pattern = "*.gemspec"
- stubs = Gem.loaded_specs.values + installed_stubs(dirs, pattern) + default_stubs(pattern)
+ stubs = installed_stubs(dirs, pattern) + default_stubs(pattern)
stubs = stubs.uniq { |stub| stub.full_name }
_resort!(stubs)
@@ -832,9 +832,7 @@ class Gem::Specification < Gem::BasicSpecification
@@stubs_by_name[name]
else
pattern = "#{name}-*.gemspec"
- stubs = Gem.loaded_specs.values +
- installed_stubs(dirs, pattern).select { |s| Gem::Platform.match s.platform } +
- default_stubs(pattern)
+ stubs = installed_stubs(dirs, pattern).select { |s| Gem::Platform.match s.platform } + default_stubs(pattern)
stubs = stubs.uniq { |stub| stub.full_name }.group_by(&:name)
stubs.each_value { |v| _resort!(v) }
diff --git a/test/rubygems/test_gem_specification.rb b/test/rubygems/test_gem_specification.rb
index afcdc0dab3..7b45f0a865 100644
--- a/test/rubygems/test_gem_specification.rb
+++ b/test/rubygems/test_gem_specification.rb
@@ -1137,48 +1137,6 @@ dependencies: []
refute_includes Gem::Specification.stubs.map { |s| s.full_name }, 'a-1'
end
- def test_self_stubs
- Gem.loaded_specs.clear
- Gem::Specification.class_variable_set(:@@stubs, nil)
-
- dir_standard_specs = File.join Gem.dir, 'specifications'
- dir_default_specs = Gem.default_specifications_dir
-
- # Create gemspecs in three locations used in stubs
- loaded_spec = Gem::Specification.new 'a', '3'
- Gem.loaded_specs['a'] = loaded_spec
- save_gemspec 'a', '2', dir_default_specs
- save_gemspec 'a', '1', dir_standard_specs
-
- full_names = ['a-3', 'a-2', 'a-1']
- assert_equal full_names, Gem::Specification.stubs.map { |s| s.full_name }
-
- Gem.loaded_specs.delete 'a'
- Gem::Specification.class_variable_set(:@@stubs, nil)
- end
-
- def test_self_stubs_for
- Gem.loaded_specs.clear
- Gem::Specification.class_variable_set(:@@stubs, nil)
-
- dir_standard_specs = File.join Gem.dir, 'specifications'
- dir_default_specs = Gem.default_specifications_dir
-
- # Create gemspecs in three locations used in stubs
- loaded_spec = Gem::Specification.new 'a', '3'
- Gem.loaded_specs['a'] = loaded_spec
- save_gemspec('a-2', '2', dir_default_specs) { |s| s.name = 'a' }
- save_gemspec('a-1', '1', dir_standard_specs) { |s| s.name = 'a' }
-
- full_names = ['a-3', 'a-2', 'a-1']
-
- assert_equal full_names, Gem::Specification.stubs_for('a').map { |s| s.full_name }
- assert_equal 1, Gem::Specification.class_variable_get(:@@stubs_by_name).length
-
- Gem.loaded_specs.delete 'a'
- Gem::Specification.class_variable_set(:@@stubs, nil)
- end
-
def test_self_stubs_for_lazy_loading
Gem.loaded_specs.clear
Gem::Specification.class_variable_set(:@@stubs, nil)