Check environment before loading Spring in boot.rb#39746
Check environment before loading Spring in boot.rb#39746rafaelfranca merged 3 commits intorails:masterfrom
Conversation
055f56b to
22ef5f7
Compare
There was a problem hiding this comment.
I initially created my application with Rails@6.0 but recently have been running off Rails master.
I stumbled upon the Rails upgrade tool to upgrade my application from 6.0 to 6.1. Running the upgrade task caused this snippet code to be added to boot.rb.
However, this change prevented my application from deploying on Heroku. It appears that this code still looked for Spring although it wasn't installed as Spring is located in the development group within my Gemfile. I'm not sure why this is, but removing this snipped caused my application to successfully build.
-----> Detecting rake tasks
!
! Could not detect rake tasks
! ensure you can run `$ bundle exec rake -P` against your app
! and using the production group of your Gemfile.
! /tmp/build_acae070e_/config/boot.rb:4:in `rescue in <top (required)>': undefined method `end_with?' for nil:NilClass (NoMethodError)
! from /tmp/build_acae070e_/config/boot.rb:1:in `<top (required)>'
! from /tmp/build_acae070e_/bin/rake:7:in `require_relative'
! from /tmp/build_acae070e_/bin/rake:7:in `<main>'
! /tmp/build_acae070e_/vendor/ruby-2.7.1/lib/ruby/2.7.0/rubygems/dependency.rb:311:in `to_specs': Could not find 'spring' (= 2.1.1) among 140 total gem(s) (Gem::MissingSpecError)
! Checked in 'GEM_PATH=/tmp/build_acae070e_/vendor/bundle/ruby/2.7.0', execute `gem env` for more information
! from /tmp/build_acae070e_/vendor/ruby-2.7.1/lib/ruby/2.7.0/rubygems/dependency.rb:323:in `to_spec'
! from /tmp/build_acae070e_/vendor/ruby-2.7.1/lib/ruby/2.7.0/rubygems/core_ext/kernel_gem.rb:62:in `gem'
! from /tmp/build_acae070e_/bin/spring:14:in `<top (required)>'
! from /tmp/build_acae070e_/config/boot.rb:2:in `load'
! from /tmp/build_acae070e_/config/boot.rb:2:in `<top (required)>'
! from /tmp/build_acae070e_/bin/rake:7:in `require_relative'
! from /tmp/build_acae070e_/bin/rake:7:in `<main>'
The previous version of the test was fragile due to over-specificity.
The previous version of the test was fragile, and it skipped running on JRuby even though its purpose is to test environments like JRuby.
In rails#39632, `boot.rb` was changed to load `bin/spring`, with the intention of adding a check to Spring itself that would prevent Spring from running in production environments. However, in a production environment, the Spring gem may not be installed. Furthermore, `bin/spring` may raise an error other than `LoadError` if it has been overwritten by e.g. `bundle binstubs` as part of the deployment process. Therefore, this commit adds the environment check to `boot.rb`. This commit also changes the app generator to generate `bin/spring` directly, instead of delegating to `bundle exec spring binstub`. This addresses an issue with the `--skip-bundle` flag. Previously, `--skip-bundle` caused `bin/spring` to not be generated. Thus the user had to manually run `bundle exec spring binstub` later, though that was not documented nor explained. Now, `bin/spring` is always generated. Additionally, by guaranteeing that `bin/stub` is generated, we avoid the need for `rescue LoadError` in `boot.rb`.
22ef5f7 to
72d10e2
Compare
| @@ -1,13 +1,13 @@ | |||
| <% unless options.skip_spring? -%> | |||
| begin | |||
| # frozen_string_literal: true | |||
There was a problem hiding this comment.
Yes. Let's remove this one.
There was a problem hiding this comment.
@rafaelfranca Thank you for fixing this in 80f7fd5! 🙏
| <% if spring_install? -%> | ||
|
|
||
| if !defined?(Spring) && [nil, "development", "test"].include?(ENV["RAILS_ENV"]) | ||
| load File.expand_path("../bin/spring", __dir__) |
There was a problem hiding this comment.
If we're generating this file ourselves, it seems odd to continue to put it in bin/, when it's only ever going to be loaded from a file in config/. Or is the idea to let applications use it as a "regular" Spring binstub later?
There was a problem hiding this comment.
bin/spring is still a valid command. It is there so people can write custom spring commands and call them. bin/spring rails for example works. That file is a binstube. I don't think it should be in the config folder.
There was a problem hiding this comment.
bin/spring railsfor example works
TIL! It seemed to me like the goal of #39225 was to make the Spring binstub unnecessary, but you're right that custom commands will still need it. To be honest I'm not totally clear on what we're trying to achieve with these changes, so I might bow out of reviewing this one 😬
Follow-up to rails#39746. This shifts the responsibility of loading Spring from `config/boot.rb` back to the relevant bin files. Fixes rails#40518. Closes rails#40521.
Follow-up to rails#39746. This shifts the responsibility of loading Spring from `config/boot.rb` back to the relevant bin files. Fixes rails#40518. Closes rails#40521. (cherry picked from commit acc837e)
| if !defined?(Spring) && [nil, "development", "test"].include?(ENV["RAILS_ENV"]) | ||
| load File.expand_path("../bin/spring", __dir__) | ||
| rescue LoadError => e | ||
| raise unless e.path.end_with?("/bin/spring") |
There was a problem hiding this comment.
Hey, @eugeneius why this has been removed? By default rails do not include spring into test env, so on CI with rails env test is not available. And before these changes, CI skipped spring. Now we have an error. I need to understand the reason in order to create a hot fix for that change.
| ENV['BUNDLE_GEMFILE'] ||= File.expand_path('../Gemfile', __dir__) | ||
| <% if spring_install? -%> | ||
|
|
||
| if !defined?(Spring) && [nil, "development", "test"].include?(ENV["RAILS_ENV"]) |
There was a problem hiding this comment.
for CI these changes are not working with the default Gemfile on the board and recommendations from rails/spring. Load error handling has provided support of the CI.
So bin/spring only available on a developers machine, on production and test instances we do not have it.
Since rails#39746, the Spring binstub can be generated without having to run `bundle install` first, and thus the `skip_bundle` option does not prevent the Spring binstub from being generated. Therefore, explicitly set the `skip_spring` option for plugin dummy apps.
In #39632,
boot.rbwas changed to loadbin/spring, with the intention of adding a check to Spring itself that would prevent Spring from running in production environments. However, in a production environment, the Spring gem may not be installed. Furthermore,bin/springmay raise an error other thanLoadErrorif it has been overwritten by e.g.bundle binstubsas part of the deployment process. Therefore, this commit adds the environment check toboot.rb.This commit also changes the app generator to generate
bin/springdirectly, instead of delegating tobundle exec spring binstub. This addresses an issue with the--skip-bundleflag. Previously,--skip-bundlecausedbin/springto not be generated. Thus the user had to manually runbundle exec spring binstublater, though that was not documented nor explained. Now,bin/springis always generated. Additionally, by guaranteeing thatbin/stubis generated, we avoid the need forrescue LoadErrorinboot.rb.