rubygems icon indicating copy to clipboard operation
rubygems copied to clipboard

Bundler removes require decorations made by other gems

Open fxn opened this issue 2 years ago • 7 comments

I am not sure how to address this in a generic way. Let me open this issue at least to start a conversation about it.

Bundler hard-resets Kernel#require this way:

def reverse_rubygems_kernel_mixin
  # Disable rubygems' gem activation system
  kernel = (class << ::Kernel; self; end)
  [kernel, ::Kernel].each do |k|
    if k.private_method_defined?(:gem_original_require)
      redefine_method(k, :require, k.instance_method(:gem_original_require))
    end
  end
end

Therefore, if you have other decorations below RubyGems, they are just erased. Who decorates Kernel#require? Zeitwerk and Bootsnap do, for example (here, and here).

For instance, that means that if a Ruby program loads Zeitwerk first, and then Bundler, Zeitwerk doesn't work. This is a synthetic, minimal reproduction of the issue:

require 'tmpdir'

require 'zeitwerk'

require 'bundler/inline'
gemfile do
  source 'https://rubygems.org'
  gem 'zeitwerk'
end

Dir.mktmpdir do |dir|
  Dir.chdir(dir)

  Dir.mkdir('lib')
  File.write('lib/foo.rb', 'Foo = 1')

  loader = Zeitwerk::Loader.new
  loader.push_dir('lib')
  loader.on_load("Foo") { p :CALLED }
  loader.setup

  Foo
end

If you run that as is, the on_load callback is not called. If you comment out line 3, it is called (as expected).

The problem is not only the callback, the purpose of that script is to show you something concrete that we can run, Zeitwerk is broken in more fundamental ways without its Kernel#require decoration in place.

And in turn, this is not only about Zeitwerk, but about how Bundler could be better coordinated with other gems decorating Kernel#require in general. If RubyGems is so special that you need to hijack it, that may be a very special case. The libraries even live in the same repository, they can be as coupled as they need to.

But what about other libraries?

(PS: Fortunately anything run via bundle exec is not subject to this because Bundler is loaded first.)

fxn avatar Jan 06 '22 19:01 fxn

Yes, that's unfortunate and I have no idea how to address this :(

In general, if Bundler is used, all dependencies should be managed by Bundler, so the require "zeitwerk" line in the script above should be removed (because zeitwerk is already in the inline Gemfile) and things should work as expected.

Ideally RubyGems wouldn't need to monkeypatch require, but RubyGems decorations are very established behavior so I don't think it's desirable to remove it. As a matter of fact, without RubyGems decorations, the script above would fail to require Zeitwerk because, like any other non default gem, it is not in the default $LOAD_PATH.

deivid-rodriguez avatar Jan 07 '22 09:01 deivid-rodriguez

@deivid-rodriguez indeed, it's an edge one.

In general, if Bundler is used, all dependencies should be managed by Bundler, so the require "zeitwerk" line in the script above should be removed (because zeitwerk is already in the inline Gemfile) and things should work as expected.

The gem dependency in the inline Gemfile is present only to make the script work with line 3 commented out, otherwise you could not instantiate a loader. The generic scenario may or may not have zeitwerk in the Gemfile. For example, a Bundler wrapper that uses Zeitwerk itself. For example, Rails executables under bin are Ruby programs that are executed first, and then they run Bundle.setup when its time arrives. I guess there could be other use cases.

And, again, this is not about Zeitwerk, but about not erasing possible wrappers of other gems. Maybe we can find more use cases in other contexts.

Also, some people argue: "If I am setting up a Docker container where I fully control the system, why should I be forced to run bundle exec sidekiq instead of just sidekiq? They have a point. Precisely, I discovered this incompatibility by understanding why the bare sidekiq command stopped working in development environments with Rails 7.0.0 (7.0.1 ships with a tweak that prevents that, but it is a workaround to this fundamental problem.)

One possibility would be to couple RubyGems and Bundler in a more gentle way. So, instead of Bundler hard-resetting require assuming everything in between can be erased, they could coordinate via a flag (sketched pseudo code to get the idea):

# require in RubyGems.
def require(path)
  return gem_original_require(path) if Gem.taken_over_by_bundler?
  ...
end

# At some point when Bundler is up, Bundler sets the flag.
Gem.taken_over_by_bundler = true

It's the same level of intimacy/coupling, only reversed, and doesn't erase other wrappers.

What do you think about that?

fxn avatar Jan 07 '22 11:01 fxn

That's seem like a nice idea to me, yeah.

deivid-rodriguez avatar Jan 07 '22 11:01 deivid-rodriguez

For example, Rails executables under bin are Ruby programs that are executed first, and then they run Bundle.setup when its time arrives. I guess there could be other use cases.

We try to handle all use cases as gracefully as possible, and we have recently got better there (forced by a significant part of the stdlib, very commonly used in binstubs like those, becoming gems), but historically bundler never expected people to activate gems before using Bundler.setup.

Also, some people argue: "If I am setting up a Docker container where I fully control the system, why should I be forced to run bundle exec sidekiq instead of just sidekiq? They have a point.

Yep, the idea is to completely avoid the need for bundle exec in the future, and do the right thing without it.

deivid-rodriguez avatar Jan 07 '22 11:01 deivid-rodriguez

but historically bundler never expected people to activate gems before using Bundler.setup.

That's an important one, and in that sense I take this issue as a conversation about it, not exactly as a technical proposal, because you have the right perspective to evaluate this.

For example, if the outcome was an official statement that says something like: "Activating gems before Bundler setup, either standard or 3rd party, is unsupported". That could be one way to address it.

However, in this particular case the rule wold be, "nobody can decorate require before Bundler even if they are not gems", which is weaker and sounds less clear to me. Because activating gems is core Bundler territory, but decorating require is not. I guess that is unlikely, but the rules of the game should be clear.

Yep, the idea is to completely avoid the need for bundle exec in the future, and do the right thing without it.

Awesome, maybe that vision can influence how to think about this topic.

fxn avatar Jan 07 '22 14:01 fxn

I just wanted to give some historical perspective and some guess on why Bundler started doing this the way it does it now. But your proposed solution feels good and fixes the issue in general, so I'm happy to go with that.

deivid-rodriguez avatar Jan 07 '22 15:01 deivid-rodriguez

I have found a new way to hit this.

  1. Gem dependency uses path.
  2. Bundler executes the local gemspec.
  3. The gemspec loads the library to set the version.
  4. The library loads Zeitwerk as a side-effect.
  5. RubyGems erases Zeitwerk's Kernel#require decoration later.

There are two things worth mentioning here:

  1. Idiomatically, step (3) cherry-picks the version file without loading the library, but loading the library is still correct.
  2. When a gemspec is executed, Bundler is still not ready, so you are loading gems from the system. However, if the versions in the lock file are the same, activation does not conflict and it works.

Bottom line, the author wondered how could things work when using the gem normally, but not when using path, same code. And the root cause is that RubyGems deletes the decoration.

fxn avatar May 01 '22 16:05 fxn

@fxn I finally got to working on this and proposed #6308. I set you as a commit co-author because I used your pseudocode mostly as is, just changed naming a bit :)

deivid-rodriguez avatar Jan 30 '23 12:01 deivid-rodriguez

@deivid-rodriguez that is fantastic, and thanks a lot for the credit ❤️.

fxn avatar Jan 30 '23 15:01 fxn