Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rakefile seems to be loaded twice #130

Closed
dmke opened this issue Jun 28, 2024 · 12 comments
Closed

Rakefile seems to be loaded twice #130

dmke opened this issue Jun 28, 2024 · 12 comments

Comments

@dmke
Copy link
Contributor

dmke commented Jun 28, 2024

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
Copy link
Owner

drwl commented Jul 1, 2024

@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
Copy link
Contributor Author

dmke commented Jul 1, 2024

Would you mind putting up a PR for this?

Will do, just don't know when :)

@drwl
Copy link
Owner

drwl commented Jul 3, 2024

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
Copy link
Owner

drwl commented Jul 13, 2024

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:
ctran/annotate_models@2813dbd

@dmke
Copy link
Contributor Author

dmke commented Jul 15, 2024

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
Copy link
Contributor Author

dmke commented Jul 16, 2024

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
Copy link
Owner

drwl commented Jul 18, 2024

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:

  1. bundle exec annotaterb models, assumes annotaterb gem is in the Gemfile
  2. annotaterb models, assumes annotaterb gem is installed but not necessarily specified in Gemfile
  3. 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
Copy link
Owner

drwl commented Jul 21, 2024

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

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

  1. bundle exec annotaterb models
  • More or less the same as 1), except runs Bundle/Bundler code prior to executing the command
  1. 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 pushed a commit that referenced this issue Jul 22, 2024
See #130 for problem discussion.

This also drops a lot of legacy and/or unnecessary code from
`AnnotateRb::RakeBootstrapper`.
@drwl
Copy link
Owner

drwl commented Jul 23, 2024

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

@dmke
Copy link
Contributor Author

dmke commented Jul 23, 2024

Thanks, will try later!

@dmke
Copy link
Contributor Author

dmke commented Aug 5, 2024

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

Thanks again!

@dmke dmke closed this as completed Aug 5, 2024
@Youngv
Copy link

Youngv commented Dec 29, 2024

I encountered the issue where rake db:seed was running twice. I was able to resolve it by upgrading the gem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants