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

Prevent webpacker:install failures on missing Gemfile #41104

Merged
merged 2 commits into from Feb 10, 2021
Merged

Prevent webpacker:install failures on missing Gemfile #41104

merged 2 commits into from Feb 10, 2021

Conversation

f6p
Copy link
Contributor

@f6p f6p commented Jan 13, 2021

fixes #41103 causing rails webpacker:install to fail when creating new app

Copy link
Member

@jonathanhefner jonathanhefner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is how we want to address #41103. See #41103 (comment).

Base automatically changed from master to main January 14, 2021 17:03
@f6p f6p changed the title Do not try to load spring if there are no locked_gems Prevent webpacker:install failures on missing Gemfile Jan 15, 2021
@f6p
Copy link
Contributor Author

f6p commented Feb 4, 2021

Hi

sorry for delay but I think skipping webpack together with bundle or gemfile is not so appealing as it may look. In such case rails commands fail randomly expecting webpacker configuration to be present. I believe the best solution is to pass silently on spring until it eventually can be started when lockfile is generated.

Cheers
🍷

@rafaelfranca
Copy link
Member

webpacker:install can't work if bundler didn't run yet. So we need to skip it. Yes, rails commands will fail, unless you run bundle install and rails webpacker:install. But I think it would be a problem if rails webpacker:install fails as well. Is that the case?

@f6p
Copy link
Contributor Author

f6p commented Feb 8, 2021

Sorry for my comments being overall chaotic. I will try to illustrate my reasoning in a more organized manner.
I must also add that I changed my perspective a little after giving the issue more thought.

Yes skipping webpack installation shouldn't happen unless gems are installed, that makes perfect sense. But I wonder if there may be an alternative worth a little consideration. I understand that without gems installed app shouldn't be in usable state but please let me illustrate my doubts by simple example.

First lets create app using rails new testapp --skip-webpack-install --skip-bundle. Then cd to app and run console. It will fail with ...webpacker/configuration.rb:99:in 'rescue in load': Webpacker configuration file not found /home/user/testapp/config/webpacker.yml. Please run rails webpacker:install.... Then run db:create - that command executes successfully, and after that running rails c will also result in success.

There are two interesting things going on here. First is that rails relies on non locked webpacker gem. If after creating new app and before running console I will edit non locked Gemfile and remove webpacker, rails c will execute successfully. Second is that db:create acutally generates Gemfile.lock without running installation (hence commands after it succeed). So in both cases what happens looks somewhat related to missing lockfile problem (but I have to admit I didn't investigated things deeply).

So I wonder if it would be a valid idea to enforce bundle installation before any rails command could actually be executed. Like for example by adding check for locked gems in binstubs.

Maybe it could simplify or replace gemfile related error handling in situations similar to those described above. The other potential benefit would be that rails new wouldn't need to rely on conditional execution depending on what options and libraries were enabled or disabled. It could just fail with default message saying something similar to: 'To do that you need to run bundle install first'. It would also allow to fail more gracefuly with descriptive message rather than throwing stacktrace to stdout in situations when somebody tries to interact with app that doesn't have gems installed (which was my original concern - improving user experience).

But if idea would turn out to be viable probably the biggest benefit would be that all similar issues would be prevented in the future.

Cheers
🍷

@rafaelfranca
Copy link
Member

So I wonder if it would be a valid idea to enforce bundle installation before any rails command could actually be executed. Like for example by adding check for locked gems in binstubs.

bundler already does that. bun/rails will not succeed if all the gems that the application defines in the Gemfile are not resolved and already installed.

Second is that db:create acutally generates Gemfile.lock without running installation.

This is not possible. It only generated the Gemfile.lock if all the gems are installed. See

[testapp (master #)]$ bundle check
Bundler can't satisfy your Gemfile's dependencies.
Install missing gems with `bundle install`.
[testapp (master #)]$ bin/rail db:create
bash: bin/rail: No such file or directory
[testapp (master #)]$ bin/rails db:create
/home/rafaelfranca/src/testapp/bin/spring:7:in `<top (required)>': undefined method `specs' for nil:NilClass (NoMethodError)
        from bin/rails:2:in `load'
        from bin/rails:2:in `<main>'

@f6p
Copy link
Contributor Author

f6p commented Feb 9, 2021

Thx for pointing this out, I have to admit I wasn't aware of that. I guess that makes my idea somewhat less appealing 😃 . Its also beyond of the scope of this PR anyways.

I did requested change and simplified message a bit. Please have a look.

Cheers
🍷

railties/lib/rails/generators/app_base.rb Outdated Show resolved Hide resolved
railties/test/generators/app_generator_test.rb Outdated Show resolved Hide resolved
railties/test/generators/app_generator_test.rb Outdated Show resolved Hide resolved
railties/test/generators/app_generator_test.rb Outdated Show resolved Hide resolved
@f6p
Copy link
Contributor Author

f6p commented Feb 10, 2021

Hello,

thx I'm not native speaker so my english isn't perfect :/ I went for less explicit message as it may skip bundle with other options as well (for now only --pretend) so I think it will be more universal without hardcoded option list.

Cheers
🍷

@rafaelfranca rafaelfranca merged commit de630cd into rails:main Feb 10, 2021
rafaelfranca added a commit that referenced this pull request Feb 10, 2021
Prevent webpacker:install failures on missing Gemfile
@jonathanhefner
Copy link
Member

@f6p This is great! Thank you for working on this!

@f6p
Copy link
Contributor Author

f6p commented Feb 14, 2021

@jonathanhefner Thank you sir for the kind words and feedback that made this PR possible. My contribution is a minor one but I'm glad I could help.

Cheers
🍷

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

Successfully merging this pull request may close these issues.

rails new fails at webpacker:install
3 participants