Make sure to require "rubygems" explicitly

[nobu]

What was the end-user or developer problem that led to this PR?

Requiring just bundler with --disable=gems, rubygems/deprecate.rb fails because Gem is not defined yet.

$ ruby --disable=gems -rbundler -ep
/opt/local/lib/ruby/3.3.0+0/rubygems/deprecate.rb:72:in `<top (required)>': uninitialized constant Gem (NameError)
	from /opt/local/lib/ruby/3.3.0+0/rubygems/specification.rb:10:in `require_relative'
	from /opt/local/lib/ruby/3.3.0+0/rubygems/specification.rb:10:in `<top (required)>'
	from /opt/local/lib/ruby/3.3.0+0/bundler/rubygems_ext.rb:5:in `require'
	from /opt/local/lib/ruby/3.3.0+0/bundler/rubygems_ext.rb:5:in `<top (required)>'
	from /opt/local/lib/ruby/3.3.0+0/bundler.rb:10:in `require_relative'
	from /opt/local/lib/ruby/3.3.0+0/bundler.rb:10:in `<top (required)>'
	from -e:in `require'

This happened in a CI too.
https://github.com/ruby/ruby/actions/runs/6793669817/job/18468881547?pr=8867#step:8:1049

What is your fix for the problem, implemented in this PR?

Make sure to require "rubygems" explicitly in bundler/lib/bundler/rubygems_ext.rb.
This is also done in bundler/lib/bundler/rubygems_integration.rb, but bundler/lib/bundler.rb loads this file before it.

Make sure the following tasks are checked

• Describe the problem / feature
• Write tests for features and bug fixes
• Write code to solve the problem
• Make sure you follow the current code style and write meaningful commit messages without tags

[martinemde]

Is the defined check faster than the library loaded check? I've not often seen a require guarded like this.

[nobu]

Probably no significant difference.

[simi]

I don't follow, AFAIK --disable=gems is not meant for public usage and if you explicitly disable gems this kind of problem is expected. Bundler doesn't work without RubyGems. 🤔

[hsbt]

This is same as

• Make sure to require "rubygems" explicitly bundler#7505
• Do require "rubygems" only when needed bundler#7520

Basically I agreed policy for "Bundler doesn't work without RubyGems". But this case happened irb with ruby core test suite.

[simi]

And is it known what have changed this is needed?

[nobu]

It is the test for ruby itself.
We often need to test features without overridden by rubygems.
And, this failure is from IRB, a standard library, but it recently uses RBS which requires bundler explicitly.
Frankly I'm not sure where this inversion should be resolved, but bundler/lib/bundler/rubygems_integration.rb already considers it, so copied it to this file.

[hsbt]

@simi I handle this. I consider to fix rbs or our test-suite side.

[simi]

@simi I handle this. I consider to fix rbs or our test-suite side.

Would be great, IMHO adding "random" requires in RubyGems is not really addressing the root problem. Anyway I'm not against merging this if needed.

[deivid-rodriguez]

I don't think this is harmful, and it allows users to easily override a RUBYOPT=--disable-gems env by just requiring bundler. For example, when they have no easy way to modify the environment. I would move the current require from bundler/lib/bundler/rubygems_integration.rb to here, since this is the first place where bundler uses stuff from rubygems.

[simi]

I don't think this is harmful, and it allows users to easily override a RUBYOPT=--disable-gems env by just requiring bundler. For example, when they have no easy way to modify the environment.

I would expect the opposite. If RUBYOPT=--disable-gems is set, require of rubygems should fail hard. Using bundler to bypass this seems unreasonable to me. Why would you run anything in such an environment since it is meant for Ruby core devs only per my understanding?

[deivid-rodriguez]

Why would you run anything in such an environment since it is meant for Ruby core devs only per my understanding?

It does happen. Ruby core has warned that --disable-gems is only for ruby core devs for a couple years, but --disable-gems has been used for many more years.

[deivid-rodriguez]

I'm not strongly attached to this patch, but RubyGems is a dependency of Bundler, so I don't see any harm in making sure it's available before using it.

[deivid-rodriguez]

If RUBYOPT=--disable-gems is set, require of rubygems should fail hard.

Require of bundler, you mean? Or are you suggesting we also disallow to require rubygems? In my opinion, that's going too far. It feels like disallowing changing the $LOAD_PATH if you used -I, or changing the current dir if you used -C.

[simi]

I mean there are already various files assuming RubyGems is loaded like here

rubygems/bundler/lib/bundler/fetcher/index.rb

Line 10 in d79c609

rescue Gem::RemoteFetcher::FetchError => e

. Would it be possible to load RubyGems in bundler entrypoint like bundler.rb to prevent need of loading that individually at other places?

[deivid-rodriguez]

I think that's fine too, but here also makes sense to me since it's the place where we monkey patch RubyGems, so we'll need this file to be loaded as early as possible, and always, anyways.

[martinemde]

I don't think we should guard it with defined?, but standard practice is to require libs in the file where you need them. It seems harmless to require rubygems there, since that's where we'd require it if it wasn't already loaded.

Keeping requires localized means libs that don't have other dependencies might be loadable separately.

[deivid-rodriguez]

Yeah, I agree in general but this case is too special 😅. I initially added the require without the guard at rubygems/bundler#7505, and it caused circular require issues, that's why we added the guard at rubygems/bundler#7520.

[simi]

That's a long story written in here :) @deivid-rodriguez feel free to merge if you think this is the way to go. But it would be great to find out a way to test this. Is ruby-core test on RubyGems side failing as well?

[martinemde]

Funny. I didn't expect the circular require problem. Hehe

[deivid-rodriguez]

I rebased this and added a test.

[hsbt]

@deivid-rodriguez Thanks!

@pocke make load bundler lazily at ruby/rbs#1612 . and I and @mame, @nobu try to fix test-suite of ruby-core. But It's not completely.

It's good to merge this.