Rakefile seems to be loaded twice

dmke commented 3 months ago

I have some Rake files, which define some constants. When running bin/rails db:migrate, the output is flooded with warnings of already defined constants.

This doesn't happen when calling bundle exec annotaterb models.

Commands

$ bin/rails db:migrate
RAILS_ROOT/lib/tasks/codegen.rake:48: warning: already initialized constant Codegen::RAILS_ROOT
RAILS_ROOT/lib/tasks/codegen.rake:48: warning: previous definition of RAILS_ROOT was here
...
Annotating models
Model files unchanged.

Reproduction

Start with an empty Rails project (rails new --minimal --api or similar)
Create a model (rails g model User name email:unique) (not sure if this is necessary)

Create a lib/tasks/codegen.rake (the actual name is not relevant, it must end in .rake though):

module Codegen
RAILS_ROOT = Pathname.new(__dir__).join("../..").expand_path

# imagine some code generation methods here
end

namespace :codegen do
task :ts_types do
  # imagine calling Codegen.ts_types or similar here
end
end

Create/migrate database (bin/rails db:{create,migrate}). Observe no warning.
Add annotaterb, configure skip_on_db_migrate: false.
Migrate again (bin/rails db:migrate). Observe warnings.

Breakdown

Adding a puts caller before the Codegen::RAILS_ROOT constant reveals the following:

--- first encounter ---
GEM_HOME/gems/railties-7.1.3.4/lib/rails/engine.rb:684:in `load'
GEM_HOME/gems/railties-7.1.3.4/lib/rails/engine.rb:684:in `block in run_tasks_blocks'
GEM_HOME/gems/railties-7.1.3.4/lib/rails/engine.rb:684:in `each'
GEM_HOME/gems/railties-7.1.3.4/lib/rails/engine.rb:684:in `run_tasks_blocks'
GEM_HOME/gems/railties-7.1.3.4/lib/rails/application.rb:583:in `run_tasks_blocks'
GEM_HOME/gems/railties-7.1.3.4/lib/rails/engine.rb:471:in `load_tasks'
RAILS_ROOT/Rakefile:6:in `<top (required)>'
GEM_HOME/gems/rake-13.2.1/lib/rake/rake_module.rb:29:in `load'
GEM_HOME/gems/rake-13.2.1/lib/rake/rake_module.rb:29:in `load_rakefile'
GEM_HOME/gems/rake-13.2.1/lib/rake/application.rb:740:in `raw_load_rakefile'
GEM_HOME/gems/rake-13.2.1/lib/rake/application.rb:126:in `block in load_rakefile'
GEM_HOME/gems/rake-13.2.1/lib/rake/application.rb:214:in `standard_exception_handling'
GEM_HOME/gems/rake-13.2.1/lib/rake/application.rb:125:in `load_rakefile'
GEM_HOME/gems/rake-13.2.1/lib/rake/application.rb:82:in `block in run'
GEM_HOME/gems/rake-13.2.1/lib/rake/application.rb:214:in `standard_exception_handling'
GEM_HOME/gems/rake-13.2.1/lib/rake/application.rb:80:in `run'
GEM_HOME/gems/rake-13.2.1/exe/rake:27:in `<top (required)>'
GEM_HOME/bin/rake:25:in `load'
GEM_HOME/bin/rake:25:in `<top (required)>'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/cli/exec.rb:58:in `load'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/cli/exec.rb:58:in `kernel_load'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/cli/exec.rb:23:in `run'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/cli.rb:455:in `exec'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/vendor/thor/lib/thor/command.rb:28:in `run'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/vendor/thor/lib/thor/invocation.rb:127:in `invoke_command'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/vendor/thor/lib/thor.rb:527:in `dispatch'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/cli.rb:35:in `dispatch'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/vendor/thor/lib/thor/base.rb:584:in `start'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/cli.rb:29:in `start'
GEM_HOME/gems/bundler-2.5.10/exe/bundle:28:in `block in <top (required)>'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/friendly_errors.rb:117:in `with_friendly_errors'
GEM_HOME/gems/bundler-2.5.10/exe/bundle:20:in `<top (required)>'
GEM_HOME/bin/bundle:23:in `load'
GEM_HOME/bin/bundle:23:in `<main>'

--- second encounter ---
GEM_HOME/gems/railties-7.1.3.4/lib/rails/engine.rb:684:in `load'
GEM_HOME/gems/railties-7.1.3.4/lib/rails/engine.rb:684:in `block in run_tasks_blocks'
GEM_HOME/gems/railties-7.1.3.4/lib/rails/engine.rb:684:in `each'
GEM_HOME/gems/railties-7.1.3.4/lib/rails/engine.rb:684:in `run_tasks_blocks'
GEM_HOME/gems/railties-7.1.3.4/lib/rails/application.rb:583:in `run_tasks_blocks'
GEM_HOME/gems/railties-7.1.3.4/lib/rails/engine.rb:471:in `load_tasks'
RAILS_ROOT/Rakefile:6:in `<main>'
GEM_HOME/gems/annotaterb-4.10.0/lib/annotate_rb/rake_bootstrapper.rb:16:in `load'
GEM_HOME/gems/annotaterb-4.10.0/lib/annotate_rb/rake_bootstrapper.rb:16:in `call'
GEM_HOME/gems/annotaterb-4.10.0/lib/annotate_rb/runner.rb:21:in `run'
GEM_HOME/gems/annotaterb-4.10.0/lib/annotate_rb/runner.rb:7:in `run'
GEM_HOME/gems/annotaterb-4.10.0/lib/annotate_rb/tasks/annotate_models_migrate.rake:32:in `block (3 levels) in <main>'
GEM_HOME/gems/rake-13.2.1/lib/rake/task.rb:281:in `block in execute'
GEM_HOME/gems/rake-13.2.1/lib/rake/task.rb:281:in `each'
GEM_HOME/gems/rake-13.2.1/lib/rake/task.rb:281:in `execute'
GEM_HOME/gems/rake-13.2.1/lib/rake/task.rb:219:in `block in invoke_with_call_chain'
GEM_HOME/gems/rake-13.2.1/lib/rake/task.rb:199:in `synchronize'
GEM_HOME/gems/rake-13.2.1/lib/rake/task.rb:199:in `invoke_with_call_chain'
GEM_HOME/gems/rake-13.2.1/lib/rake/task.rb:188:in `invoke'
GEM_HOME/gems/rake-13.2.1/lib/rake/application.rb:188:in `invoke_task'
GEM_HOME/gems/rake-13.2.1/lib/rake/application.rb:138:in `block (2 levels) in top_level'
GEM_HOME/gems/rake-13.2.1/lib/rake/application.rb:138:in `each'
GEM_HOME/gems/rake-13.2.1/lib/rake/application.rb:138:in `block in top_level'
GEM_HOME/gems/rake-13.2.1/lib/rake/application.rb:147:in `run_with_threads'
GEM_HOME/gems/rake-13.2.1/lib/rake/application.rb:132:in `top_level'
GEM_HOME/gems/rake-13.2.1/lib/rake/application.rb:83:in `block in run'
GEM_HOME/gems/rake-13.2.1/lib/rake/application.rb:214:in `standard_exception_handling'
GEM_HOME/gems/rake-13.2.1/lib/rake/application.rb:80:in `run'
GEM_HOME/gems/rake-13.2.1/exe/rake:27:in `<top (required)>'
GEM_HOME/bin/rake:25:in `load'
GEM_HOME/bin/rake:25:in `<top (required)>'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/cli/exec.rb:58:in `load'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/cli/exec.rb:58:in `kernel_load'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/cli/exec.rb:23:in `run'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/cli.rb:455:in `exec'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/vendor/thor/lib/thor/command.rb:28:in `run'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/vendor/thor/lib/thor/invocation.rb:127:in `invoke_command'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/vendor/thor/lib/thor.rb:527:in `dispatch'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/cli.rb:35:in `dispatch'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/vendor/thor/lib/thor/base.rb:584:in `start'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/cli.rb:29:in `start'
GEM_HOME/gems/bundler-2.5.10/exe/bundle:28:in `block in <top (required)>'
GEM_HOME/gems/bundler-2.5.10/lib/bundler/friendly_errors.rb:117:in `with_friendly_errors'
GEM_HOME/gems/bundler-2.5.10/exe/bundle:20:in `<top (required)>'
GEM_HOME/bin/bundle:23:in `load'
GEM_HOME/bin/bundle:23:in `<main>'

--- warning ---
RAILS_ROOT/lib/tasks/codegen.rake:48: warning: already initialized constant Codegen::RAILS_ROOT
RAILS_ROOT/lib/tasks/codegen.rake:48: warning: previous definition of RAILS_ROOT was here

The culprit likely lies in rake_bootstrapper.rb (omitting some details), which loads the Rakefile unconditionally if it exists:

module AnnotateRb
  class RakeBootstrapper
    class << self
      def call(options)
        # ...
        require "rake"
        load "./Rakefile" if File.exist?("./Rakefile")
        # ...

AFAICT, the reason for this is to ensure that the environment task will be defined (through Rails.application.load_tasks in standard Rails-generated Rakefiles).

We could eliminate the warning by checking whether that task is defined:

        require "rake"
        load "./Rakefile" if File.exist?("./Rakefile") && !Rake::Task.task_defined?(:environment)

This works with both bin/rails db:migrate and bundle exec annotaterb models. I'm not sure whether are other forms of invocations, so this might need a more thorough testing.

Version

annotaterb version: 4.10.0
rails version: 7.1.3.4
ruby version: 3.0.7
database version: PostgreSQL 16.3
database adapter version (if available): pg 1.5.6

drwl commented 3 months ago

@dmke thanks for a detailed explanation + steps to reproduce.

I think you're 100% right on the cause + fix. Would you mind putting up a PR for this? I think just that change itself should be sufficient for supporting both the direct gem invocation (bundle exec annotaterb models) and hooking to db rake task invocation (bin/rails db:migrate).

dmke commented 3 months ago

Would you mind putting up a PR for this?

Will do, just don't know when :)

drwl commented 3 months ago

Will do, just don't know when :)

No worries, whenever you have a moment it would be appreciated. It doesn't have to be much more than just the conditional change you proposed. It's nice for me when other folks feel empowered to add PRs :).

drwl commented 2 months ago

I'm revisiting this issue now that I'm done with refactors. The more that I think about this, the more I'm beginning to wonder why AnnotateRb::RakeBootstrapper needs to load all the rake tasks at all.

I'll do some more investigation but that piece of code is 15+ years old: https://github.com/ctran/annotate_models/commit/2813dbd9f6e7f18449ff8c9c753f439ca22e5e47

dmke commented 2 months ago

The call method's primary goals seem to be:

if in a Rails project: find and load the Rails environment
otherwise: load ActiveSupport dependencies

Loading rake was just a means to the end of loading the defaults defined in lib/tasts/annotate_models.rake, which this fork has superseded by a YAML file.

I believe, the call method could be condensed to:

def call(_)
  require_relative "./config/environment.rb" if File.exist?("./config/environment.rb")
end

since ActiveSupport will already be loaded unconditionally in lib/annotate_rb.rb.

I'll try to experiment with that later.

dmke commented 2 months ago

I'll try to experiment with that later.

Reducing the code to simply load config/environment.rb (when present) does not suffice (see commit, CI)...

drwl commented 2 months ago

Thanks for trying to dig into this. I spent some time today and will give it another go later this week, but I realized I was mistaken by what actually happens.

In the commit I referenced above, it actually loads the annotaterb/lib/annotate_rb/tasks/annotate_models_migrate.rake and not lib/tasks file in the Rails app.

--

Unless I'm mistaken, there's 3 ways to run AnnotateRb with the context of a Rails app:

bundle exec annotaterb models, assumes annotaterb gem is in the Gemfile
annotaterb models, assumes annotaterb gem is installed but not necessarily specified in Gemfile
Hooks into Rails db tasks as a callback

I don't usually test 2) but I believe there might be setups that do this, especially if there's Rails engines. I'll try and understand the behavior of the RakeBootstrapper in all 3 contexts and report my findings.

drwl commented 2 months ago

@dmke Okay, I think I got a better sense of how things work given those 3 different entry points. The AnnotateRb::RakeBootstrapper only needs to be called if run using methods 1 or 2 (bundle exec annotaterb or annotaterb). If AnnotateRb is triggered through db migrations (method 3), then the Rails context already exists.

annotaterb models
- With an installed gem, the binstub gets called
- Calls: Gem.activate_bin_path
- Calls exe/annotaterb
- Calls AnnotateRb::Runner
- Calls AnnotateRb::Bootstrapper
- Loads the Rails App's Rakefile, if it exists
- Rails.application.load_tasks (in the Rakefile), tells Rails to load Rake tasks for the main app

There's no double loading of codegen.rake

bundle exec annotaterb models
- More or less the same as 1), except runs Bundle/Bundler code prior to executing the command
Hooked into rake db tasks (bin/rails db:migrate)
- Uses Rails app's bin/rails binstub
- Uses Railties gem code to invoke the command (db:migrate)
- Ultimately loads Rails app's Rakefile
- Loads the installed Rake task (in Rails app's lib/tasks/annotate_rb.rake)
- Calls AnnotateRb::Core.load_rake_tasks
- Loads AnnotateRb's annotaterb/lib/annotate_rb/tasks/annotate_models_migrate.rake
- annotaterb/lib/annotate_rb/tasks/annotate_models_migrate.rake: triggers AnnotateRb::Runner
- AnnotateRb::Runner calls AnnotateRb::RakeBootstrapper which loads the Rails app's Rakefile (again), leading to double loading of rake tasks in lib/tasks

drwl commented 2 months ago

I released a new gem version, 4.10.2, which contains your change. Let me know if you have any issues.

dmke commented 2 months ago

Thanks, will try later!

dmke commented 1 month ago

So, it took a moment, but it works as expected.

Thanks again!

drwl / annotaterb