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

Improve bundler binstub error message #29967

Merged
merged 2 commits into from Aug 3, 2017
Merged

Conversation

naw
Copy link

@naw naw commented Jul 27, 2017

Rails displays an error message if you have a bundler-generated binstub at ./bin/rails instead of a Rails-generated binstub.

This error message is misleading because it makes it seem as though Rails 5 introduced recent changes in how binstubs are used, when these changes were actually introduced way back in Rails 4.

The suggested upgrade steps are appropriate for an app that was created in Rails 3, but they likely aren't the correct fix for someone who sees this error message today on a modern app. I believe the --binstubs option on bundler is a more likely culprit and troubleshooting path.

Rails displays an error message if you have a bundler-generated binstub
 at `./bin/rails` instead of a Rails-generated binstub.

This error message is misleading because it makes it seem as though
Rails 5 introduced recent changes in how binstubs are used, when these
changes were actually introduced way back in Rails 4.

The suggested upgrade steps are appropriate for an app that was created
in Rails 3, but they likely aren't the correct fix for someone who sees
this error message today on a modern app.  I believe the `--binstubs`
option on bundler is a more likely culprit and troubleshooting path.
@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

if your bundle install command is using the --binstubs option and thereby overriding the
standard Rails bin/rails binstub. You might need to stop using the --binstubs option,
or at least ensure that it is using a path other than ./bin. You may need to coordinate
this change with your environment's PATH.
Copy link
Member

Choose a reason for hiding this comment

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

you might need to stop using the --binstubs option, or at least ensure that it is using a path other than ./bin. You may need to coordinate this change with your environment's PATH.

I don't totally follow this. The rest of the change is good. Can you tell me more about what exactly is going on with this though?

Copy link
Author

Choose a reason for hiding this comment

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

Sure. I was inadvertently running bundle install --binstubs on a production server as part of a deploy step. This would generate new binstubs and override the Rails binstub that was already in ./bin, thus causing the bundle binstub error message to be shown even though my app already has the Rails binstub in ./bin in the repo. For me the solution, was just not to use --binstubs.

However, we probably don't want to suggest not using --binstubs as the only solution because there might be legitimate reasons to use that option. As far as I know, the Heroku buildpack still uses it, but with a non-default folder: bundle install --without development:test --path vendor/bundle --binstubs vendor/bundle/bin -j4 --deployment). If the binstubs are placed somewhere besides ./bin, Rails will see the correct Rails-generated ./bin/rails binstub and not raise an error. I'm presuming some people may need to modify their PATH if they place bundler binstubs in an alternate location (like vendor/bundle/bin), but perhaps I'm missing something.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense, but seems like a very specific failure mode. We should maybe either take that bit out, or re-write it.

It sounds like we're saying something along the lines of "rails binstubs are different than bundle install --binstubs. If you're using binstubs in the bin/ folder generated by bundler, you may be getting this message."

@naw
Copy link
Author

naw commented Aug 3, 2017

@schneems

Yes, good point regarding the specificity of the failure mode. My original assumption was that using --binstubs during deployment is a more likely cause of this error today than actually having the incorrect binstub files in source control. My assumption might be wrong.

Nevertheless, as far as I know, there are only two broad ways to receive this error message in an app created in Rails 4+:

  1. Overwriting the Rails binstubs with Bundle binstubs locally (and in source control).
  2. Having the correct binstubs in source control, but overwriting them on the production server with bundle install --binstubs.

In the first case, the correct step is to regenerate the Rails binstubs and fix them in source control. In the second case, the correct step likely involves changing how/whether you use the --binstubs option on bundle install during deployment.

One of the goals of this pull request is to clarify that upgrading binstubs in source control is not necessarily going to solve the problem. For me (and I assume for others), I already had the correct binstubs in source control.

I have added a commit to revise the error message to mention both possible solutions. Please let me know if you think it is still too specific.

@schneems
Copy link
Member

schneems commented Aug 3, 2017

Great, this works for me . Thanks!

@schneems schneems merged commit 23b0701 into rails:master Aug 3, 2017
@naw naw deleted the binstub-error-msg branch August 4, 2017 00:53
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

Successfully merging this pull request may close these issues.

None yet

4 participants