Move Spring machinery to boot.rb#39632
Conversation
There was a problem hiding this comment.
I left out rescue LoadError in favor of options.skip_spring?, but is that sufficient?
There was a problem hiding this comment.
The currently failing tests clearly rely on the rescue, but I think that's only because the call to bundle exec spring binstub is stubbed out. I will look into making those tests pass without the rescue.
There was a problem hiding this comment.
I see now that it's not because bundle exec spring binstub is stubbed out, but rather because bundle_install? is false. That seems like an unfortunate circumstance to deal with. While it's obvious that you must run bundle install manually after using --skip-bundle, it's not obvious that you must run bundle exec spring binstub too.
One way to address this would be to move the binstub template inside of Rails itself. That doesn't seem too unreasonable since none of the code is particularly specific to Spring (other than require 'spring/binstub'). But that seems out of scope for this PR. For now, I just added the rescue.
This partially reverts rails#39225, and folds `boot_with_spring.rb` into `boot.rb`. Fixes rails#39622.
ec3dd42 to
a6c958b
Compare
| begin | ||
| load File.expand_path("../bin/spring", __dir__) | ||
| rescue LoadError => e | ||
| raise unless e.path.end_with?("/bin/spring") | ||
| end |
There was a problem hiding this comment.
I left out the RAILS_ENV check based on #39225 (comment).
In rails#39632, `boot.rb` was changed to load `bin/spring`, with the intention of adding a guard 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, the contents of `bin/spring` may not be guaranteed as it can be overwritten by e.g. `bundle binstubs`, and so it can raise unexpected errors. Therefore, this commit modifies `boot.rb` to include the (code-golfed) contents of `bin/spring` directly. Additionally, to keep these two files in sync, `bin/spring` is now generated by Rails itself rather than via the `spring binstub` command. Incidentally, generating `bin/spring` via Rails itself also addresses an issue with the `--skip-bundle` flag. Previously, `bin/spring` was not generated when using `--skip-bundle`, and the user would have to run both `bundle install` *and* `bundle exec spring binstub` manually, though the latter step was not documented or explained.
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`.
This partially reverts #39225, and folds
boot_with_spring.rbintoboot.rb.Fixes #39622.
/cc @dhh Is this what you had in mind for #39622 (comment)?