fxn / zeitwerk

Efficient and thread-safe code loader for Ruby
MIT License
1.99k stars 118 forks source link

Zeitwerk::NameError on calling Zeitwerk::Loader.eager_load_all #284

Closed sajan45 closed 9 months ago

sajan45 commented 9 months ago

I am using the same setup as in issue #282 to upgrade Gruf. The workaround helped me with running the app with the development environment but I am getting the below exception if the app has config.eager_load = true in the config. We have that for our test environment. The below traces are for a rake command in in test environment. I also get the same error(with obvious different traces) if the application is calling Zeitwerk::Loader.eager_load_all explicitly anywhere with eager_load set to false.

/home/ruby/3.1.0/gems/zeitwerk-2.6.12/lib/zeitwerk/loader/callbacks.rb:33:in `on_file_autoloaded': expected file /home/ruby/3.1.0/gems/gruf-2.18.0/lib/gruf/version.rb to define constant Gruf::Version, but didn't (Zeitwerk::NameError)

      raise Zeitwerk::NameError.new(msg, cref.last)
      ^^^^^
    from /home/ruby/3.1.0/gems/zeitwerk-2.6.12/lib/zeitwerk/kernel.rb:31:in `require'
    from /home/ruby/3.1.0/gems/zeitwerk-2.6.12/lib/zeitwerk/loader/helpers.rb:135:in `const_get'
    from /home/ruby/3.1.0/gems/zeitwerk-2.6.12/lib/zeitwerk/loader/helpers.rb:135:in `cget'
    from /home/ruby/3.1.0/gems/zeitwerk-2.6.12/lib/zeitwerk/loader/eager_load.rb:175:in `block in actual_eager_load_dir'
    from /home/ruby/3.1.0/gems/zeitwerk-2.6.12/lib/zeitwerk/loader/helpers.rb:40:in `block in ls'
    from /home/ruby/3.1.0/gems/zeitwerk-2.6.12/lib/zeitwerk/loader/helpers.rb:25:in `each'
    from /home/ruby/3.1.0/gems/zeitwerk-2.6.12/lib/zeitwerk/loader/helpers.rb:25:in `ls'
    from /home/ruby/3.1.0/gems/zeitwerk-2.6.12/lib/zeitwerk/loader/eager_load.rb:170:in `actual_eager_load_dir'
    from /home/ruby/3.1.0/gems/zeitwerk-2.6.12/lib/zeitwerk/loader/eager_load.rb:17:in `block (2 levels) in eager_load'
    from /home/ruby/3.1.0/gems/zeitwerk-2.6.12/lib/zeitwerk/loader/eager_load.rb:16:in `each'
    from /home/ruby/3.1.0/gems/zeitwerk-2.6.12/lib/zeitwerk/loader/eager_load.rb:16:in `block in eager_load'
    from /home/ruby/3.1.0/gems/zeitwerk-2.6.12/lib/zeitwerk/loader/eager_load.rb:10:in `synchronize'
    from /home/ruby/3.1.0/gems/zeitwerk-2.6.12/lib/zeitwerk/loader/eager_load.rb:10:in `eager_load'
    from /home/ruby/3.1.0/gems/zeitwerk-2.6.12/lib/zeitwerk/loader.rb:379:in `block in eager_load_all'
    from /home/ruby/3.1.0/gems/zeitwerk-2.6.12/lib/zeitwerk/loader.rb:377:in `each'
    from /home/ruby/3.1.0/gems/zeitwerk-2.6.12/lib/zeitwerk/loader.rb:377:in `eager_load_all'
    from /home/ruby/3.1.0/gems/railties-6.0.4.8/lib/rails/application/finisher.rb:122:in `block in <module:Finisher>'
    from /home/ruby/3.1.0/gems/railties-6.0.4.8/lib/rails/initializable.rb:32:in `instance_exec'
    from /home/ruby/3.1.0/gems/railties-6.0.4.8/lib/rails/initializable.rb:32:in `run'
    from /home/my_app/config/environment.rb:20:in `block in run_initializers'
    from /usr/local/lib/ruby/3.1.0/tsort.rb:228:in `block in tsort_each'
    from /usr/local/lib/ruby/3.1.0/tsort.rb:350:in `block (2 levels) in each_strongly_connected_component'
    from /usr/local/lib/ruby/3.1.0/tsort.rb:431:in `each_strongly_connected_component_from'
    from /usr/local/lib/ruby/3.1.0/tsort.rb:349:in `block in each_strongly_connected_component'
    from /usr/local/lib/ruby/3.1.0/tsort.rb:347:in `each'
    from /usr/local/lib/ruby/3.1.0/tsort.rb:347:in `call'
    from /usr/local/lib/ruby/3.1.0/tsort.rb:347:in `each_strongly_connected_component'
    from /usr/local/lib/ruby/3.1.0/tsort.rb:226:in `tsort_each'
    from /usr/local/lib/ruby/3.1.0/tsort.rb:205:in `tsort_each'
    from /home/my_app/config/environment.rb:16:in `run_initializers'
    from /home/ruby/3.1.0/gems/railties-6.0.4.8/lib/rails/application.rb:363:in `initialize!'
    from /home/my_app/config/environment.rb:29:in `<top (required)>'
    from <internal:/usr/local/lib/ruby/site_ruby/3.1.0/rubygems/core_ext/kernel_require.rb>:85:in `require'
    from <internal:/usr/local/lib/ruby/site_ruby/3.1.0/rubygems/core_ext/kernel_require.rb>:85:in `require'
    from /home/my_app/bin/rake:3:in `<main>'

Another piece of info that seems related to this: If I set config.eager_load to false and start the e2e tests that also starts a Gruf server, I get below exception

/home/ruby/3.1.0/gems/gruf-2.18.0/lib/gruf/server.rb:228:in `update_proc_title': uninitialized constant Gruf::VERSION (NameError)

      Process.setproctitle("gruf #{Gruf::VERSION} -- #{state}")
                                       ^^^^^^^^^
Did you mean?  Gruf::Version
               PgVersion
    from /home/ruby/3.1.0/gems/gruf-2.18.0/lib/gruf/server.rb:93:in `start!'
    from /home/ruby/3.1.0/gems/gruf-2.18.0/lib/gruf/cli/executor.rb:61:in `run'
    from /home/my_app/bin/gruf:9:in `<main>'
fxn commented 9 months ago

Fortunately, the root cause is the same one.

Gruf instantiates a Zeitwerk::GemInflector and passes __FILE__ to it. The gem inflector uses that argument to set a custom rule for the version file (here).

This is a particular case of using __dir__ and __FILE__, not strictly related to Zeitwerk, any dependency could potentially be using them with potential mismatches too, due to the symlinks.

This one will need that you fork Gruf. You'd replace this line:

loader.inflector = ::Zeitwerk::GemInflector.new(__FILE__)

with this singleton method definition on the default inflector:

def (loader.inflector).camelize(basename, abspath)
  abspath.end_with?('lib/gruf/version.rb') ? 'VERSION' : super
end

There is only one version.rb in the project today, so it would suffice to configure

loader.inflector.inflect('version' => 'VERSION')

That is simpler, but would inflect all potential future "version" basenames in Gruf, it's a trade-off.

For the same price, you could define the Gruf module before loader.setup here, instead of your app.

sajan45 commented 9 months ago

If I understand this correctly, this is happening because the condition abspath == @version_file returns false due to the mismatch between the path resolved by __FILE__ and __dir__, as FILE returns the symlink path but dir returns canonicalized absolute path. If this is correct, please help me understand why this is not happening with other gems. I searched in Github for other gems that setup Inflector in the same way, the first result was this telegram bot gem, I added this to the Gemfile and required the entry point but I don't see any error(I changed Gruf to a non-Zeitwerk version to test this). What's different here ?

fxn commented 9 months ago

If I understand this correctly, this is happening because the condition abspath == @version_file returns false due to the mismatch between the path resolved by FILE and dir, as FILE returns the symlink path but dir returns canonicalized absolute path.

Correct.

If this is correct, please help me understand why this is not happening with other gems.

As you say, as far as I can see it is the same setup, should raise the same way.

However, it does raise in my machine, perhaps there is something else in yours? Was eager load enabled for certain? (Surely it was, but let me double-check since things do not match.)

fxn commented 9 months ago

Oh, BTW, another way to patch the gems is to pass to the gem inflector the real path to __FILE__.

fxn commented 9 months ago

Let me share the way I test this too. This script duplicates the gems that I previously cloned under vendor:

require 'fileutils'

def duplicate_directory(source_dir, destination_dir)
  FileUtils.mkdir_p(destination_dir)

  Dir.foreach(source_dir) do |entry|
    next if entry == '.' || entry == '..'

    source_path = File.join(source_dir, entry)
    destination_path = File.join(destination_dir, entry)

    if File.directory?(source_path)
      # If entry is a directory, recursively duplicate it
      duplicate_directory(source_path, destination_path)
    else
      # If entry is a file, create a symlink to the original file
      FileUtils.ln_s(File.realpath(source_path), destination_path)
    end
  end
end

["gruf", "telegram-bot-ruby"].each do |gem_name|
  source_directory = File.expand_path("vendor/#{gem_name}")
  destination_directory = File.expand_path("vendor/bug/#{gem_name}")
  FileUtils.rm_rf(destination_directory)

  duplicate_directory(source_directory, destination_directory)
end

This is a dummy work directory, not a Rails app.

Then, my test is

ruby -Ivendor/bug/telegram-bot-ruby/lib -rtelegram/bot -e 'p Telegram::Bot::VERSION'

and an execution looks like this:

issue-282 % sh test.sh
-e:1:in `<main>': uninitialized constant Telegram::Bot::VERSION (NameError)

p Telegram::Bot::VERSION
               ^^^^^^^^^
Did you mean?  Telegram::Bot::Version

If you enable logging in the loaders of the gems, (loader.log!), you'll see the autoload being set for Version because the paths being used are real. They are real because __dir__ is real. Therefore, the comparison in the gem inflector fails as you correctly interpreted.

sajan45 commented 9 months ago

Yes the test looks good, so it's probably something with my application, though I double checked that eager_loading is enabled and there is even a explicit call to Zeitwerk::Loader.eager_load_all in one of the rake task. Anyway, looks like I will have to fork the gem for now and spend some time to check if we can change the build system to avoid this kind of issue. Thank you for the help. You can close this.

fxn commented 9 months ago

My pleasure, please don't hesitate to followup if anything else shows up!

fxn commented 9 months ago

For the archives, I've written this script checking __dir__ and __FILE__ with a few symlinks:

require 'fileutils'

FileUtils.rm_rf('a')
FileUtils.mkdir_p('a/b/c')

FileUtils.ln_s(File.expand_path('a/b'), 'a/b_link')

File.write('a/b/c/foo.rb', 'puts [__dir__, File.dirname(__FILE__), __FILE__]')
FileUtils.ln_s(File.expand_path('a/b/c/foo.rb'), 'a/b/c/foo_link.rb')

paths = %w(
  a/b/c/foo
  a/b/c/foo_link
  a/b_link/c/foo
  a/b_link/c/foo_link
)

paths.each do |path|
  puts "Testing #{path}:"
  system %(ruby -I. -e 'require "#{path}"')
end

I wonder which is the rationale for not defining __dir__ (conceptually) as File.dirname(__FILE__), at first sight it does not seem coherent to me. @byroot do you know?

fxn commented 9 months ago

I have updated the docs (https://github.com/fxn/zeitwerk/commit/7daca616ff12f4e8eaf7a36649c4ca78082aa154).

Gems are expected to use for_gem, unless you have very custom needs. The README has a snippet unrolling conceptually what for_gem does, so there is no magic, but it used __dir__, and File.dirname is a better translation. I suspect Gruf just copied that snippet.

That said, it would be better if Gruf used for_gem, I might propose a patch for them.

fxn commented 9 months ago

@sajan45 I have opened https://github.com/bigcommerce/gruf/pull/202. In your fork, that patch should just work. No double loading or version.rb inflection mismatches.

sajan45 commented 9 months ago

@fxn This is really awesome. Thank you for the patch.