Skip to content

Commit

Permalink
Revert adding loaded specs to Gem::Specification.stubs and `Gem::Sp…
Browse files Browse the repository at this point in the history
…ecification.stubs_for`

The rationale is that:

* The change has caused realworld issues. See for example
ruby/did_you_mean#117 and specifically [this
comment](ruby/did_you_mean#117 (comment))
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. 🎉

```
$ 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.
  • Loading branch information
deivid-rodriguez committed Mar 30, 2020
1 parent bd4b24b commit 5b90223
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 46 deletions.
6 changes: 2 additions & 4 deletions lib/rubygems/specification.rb
Original file line number Diff line number Diff line change
Expand Up @@ -803,7 +803,7 @@ def self.each_spec(dirs) # :nodoc:
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)
Expand Down Expand Up @@ -834,9 +834,7 @@ def self.stubs_for(name)
@@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) }

Expand Down
42 changes: 0 additions & 42 deletions test/rubygems/test_gem_specification.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1137,48 +1137,6 @@ def test_self_remove_spec_removed
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)
Expand Down

0 comments on commit 5b90223

Please sign in to comment.