Revert adding loaded specs to Gem::Specification.stubs and Gem::Specification.stubs_for

[deivid-rodriguez]

Description:

In this PR I'm proposing to partially revert #2309. In that PR, Gem::Specification.stubs and Gem::Specification.stubs_for were changed to also include loaded specifications.

Rationale

• The change has caused realworld issues. See for example No DidYouMean constants in Travis CI ruby/did_you_mean#117 and specifically this 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:
    

    rubygems/bundler/spec/support/rubygems_ext.rb

    Lines 109 to 114 in 2bbcdcd

    	gems.each do |name, req|
    	dependency = Gem::Dependency.new(name, req)
    	next unless dependency.matching_specs.empty?
    	Gem::DependencyInstaller.new(:document => false).install(dependency)
    	end

    .
  • 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:
    

    rubygems/bundler/spec/support/builders.rb

    Lines 372 to 383 in 2bbcdcd

    	if rake_path.nil?
    	Spec::Path.base_system_gems.rmtree
    	Spec::Rubygems.setup
    	rake_path = Dir["#{Path.base_system_gems}/**/rake*.gem"].first
    	end
    	if rake_path
    	FileUtils.mkdir_p("#{path}/gems")
    	FileUtils.cp rake_path, "#{path}/gems/"
    	else
    	abort "Your test gems are missing! Run `rm -rf #{tmp}` and try again."
    	end

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.

Investigation of the original issue

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:

  rubygems/lib/rubygems/test_case.rb

  Line 26 in 2bbcdcd

  require 'bundler'

• 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.

• Rubygems base TestCase class requires rubygems/rdoc:
  

  rubygems/lib/rubygems/test_case.rb

  Line 1536 in 2bbcdcd

  require 'rubygems/rdoc'

• And that file ends up calling Gem.finish_resolve:
  

  rubygems/lib/rubygems/rdoc.rb

  Line 13 in 2bbcdcd

  Gem.finish_resolve

• Gem.finish_resolve adds the currently loaded specs to the resolution:
  

  rubygems/lib/rubygems.rb

  Line 235 in 2bbcdcd

  request_set.import Gem.loaded_specs.values.map {|s| Gem::Dependency.new(s.name, s.version) }

• 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.

Conclusions

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

Tasks:

• Describe the problem / feature
• Write tests
• Write code to solve the problem
• Get code review from coworkers / friends

I will abide by the code of conduct.

[deivid-rodriguez]

I ended up keeping the hack to add the local bundler gemspec stub to the list of loaded stubs at the beginning of the base test case file. It's a big hack, but it makes sense to me and I think it might fix some issues in cases where you have the exact same version as the one being tested installed globally.

Plus, the goal of this PR was removing loaded specs from Gem::Specification.stubs and Gem::Specification.stubs_for, so I limited the PR to that.