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

Encourage use of bin/rails over rails where needed #33203

Closed
wants to merge 1 commit into from
Closed

Encourage use of bin/rails over rails where needed #33203

wants to merge 1 commit into from

Conversation

albertoalmagro
Copy link
Contributor

Summary

While reading Rails code I noted that in some places we used bin/railswhile in others we kept referring to rails when running tasks. This PR prefixes rails command with bin/ where bin/railsshould be preferred over rails. Occurrences like rails new haven't been updated because they aren't a use case of bin/rails.

Additional information

Test cases for generators usage have also been updated.

This commit prefixes rails command with 'bin/' where bin/rails should be preferred over rails. Occurrences like rails new and rails plugin haven't been updated because they aren't a use case of bin/rails
@rails-bot
Copy link

r? @sgrif

(@rails-bot has picked a reviewer for you, use r? to override)

Copy link
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Yay!

@matthewd
Copy link
Member

Why is this necessary? AFAIK these should only differ if people don't have rails on their path at all... but at that point, the same could be true for rake or ruby.

Am I forgetting a common situation where rails won't work but bin/rails will?

@deivid-rodriguez
Copy link
Contributor

The point is not that it doesn't work, but that the correct version is used.

@deivid-rodriguez
Copy link
Contributor

Why do you provide the binstubs with the application then?

@albertoalmagro
Copy link
Contributor Author

Exactly @deivid-rodriguez, thanks, you answered first ❤️

@matthewd on top of that I also think it is better that we encourage using the version we think is better. At the moment we are mixing both and that can be confusing.

@matthewd
Copy link
Member

If it works, I'm not sure I follow how it's not "the correct version".

Why do you provide the binstubs with the application then?

I can ask stand-offish questions too: how do you think rails works then?

@albertoalmagro I guess my question becomes whether/why we think the longer spelling is the better version. Standardizing on a consistent spelling seems sensible, but I'd think we'd default to the one that's easier to type.

@deivid-rodriguez
Copy link
Contributor

deivid-rodriguez commented Jun 25, 2018

Yeah, I didn't use the proper wording, I'll try again.

The point is not that rails may not be in the PATH, the point is that the Rails version in the PATH might not match the Rails version that you want to use for a specific application. You certainly don't want to run the rails 5.2 executable on a rails 5.0 application, for example, right? I thought this was the reason why Rails was shipping with binstubs.

@deivid-rodriguez
Copy link
Contributor

I can ask stand-offish questions too: how do you think rails works then?

I definitely did not mean this as "stand-offish", I'm sorry if it came off like that. Online async communication is tough, specially when not in your native language. The good news is that I learnt a new word :)

@matthewd
Copy link
Member

Ah yes, sorry, I misinterpreted your point about the version.

rails looks for, and runs, bin/rails if it's present.. that's how it works. The actual rails on the global path knows how to 1) do that, and 2) do rails new / rails plugin. It knows nothing about the other subcommands.

So when you run a 5.2, or 4.0, or whatever, rails from your PATH, its only job is to exec the bin/rails inside your app, which has the appropriate version. Thus my belief that they should be interchangeable.. we go out of our way to make it Just Work.

@deivid-rodriguez
Copy link
Contributor

deivid-rodriguez commented Jun 25, 2018

😮 I didn't know that, in that case yeah, I'm not sure about this PR.

I have a question though. How would my app break if I use a bundler generated binstub for rails, just like I do with all the other binstubs my app needs? I'm doing some quick tests and things seem to just work, except that I get a lot of scary verbose warnings together with the correct output for commands.

@deivid-rodriguez
Copy link
Contributor

Specifically I tried rails routes, rails generate, rails db:migrate:status, rails server and rails new.

@matthewd
Copy link
Member

It then ignores your binstub, and hopes it's right about what would be in bin/rails for your version (by assuming it matches the current version). In practice, bin/rails is simple enough that I don't think it's ever substantively changed... but that assumption is obviously pretty fragile over the long term, which is why we prefer the simplest possible API of exec bin/rails.

@deivid-rodriguez
Copy link
Contributor

deivid-rodriguez commented Jun 25, 2018

Aaah, you're right again, and explains why my tests just work.

What I mean is, could those three lines live inside the application loader in the general case (not only for the fallback), and leave the binstub that lives inside the application only for bundler to do its thing? Not sure if I communicated the idea correctly.

@albertoalmagro
Copy link
Contributor Author

Ah yes, sorry, I misinterpreted your point about the version.

rails looks for, and runs, bin/rails if it's present.. that's how it works. The actual rails on the global path knows how to 1) do that, and 2) do rails new / rails plugin. It knows nothing about the other subcommands.

So when you run a 5.2, or 4.0, or whatever, rails from your PATH, its only job is to exec the bin/rails inside your app, which has the appropriate version. Thus my belief that they should be interchangeable.. we go out of our way to make it Just Work.

@matthewd This explanation is gold, I had understood the same as @deivid-rodriguez, thanks for the lesson 📖 👏 !!!

Knowing this I think we should make the opposite to avoid this confusion. That is replacing bin/rails with the shorter rails. What do you think?

@albertoalmagro
Copy link
Contributor Author

My point is to unify this to be consistent., things like recommending here to run bin/rails:

msg = "Environment data not found in the schema. To resolve this issue, run: \n\n bin/rails db:environment:set"

While here only rails:
raise ArgumentError, "Missing `secret_key_base` for '#{Rails.env}' environment, set this string with `rails credentials:edit`"

Thanks

@matthewd
Copy link
Member

@deivid-rodriguez I worked out why that doesn't work [in the general case]: when we use that fallback behaviour, we're not assuming the app's Rails version can be booted in the same way the latest version can be (as I claimed above) -- we're booting the latest version (that is, the version that the path-found rails command belongs to).

If you've run it under bundler (or via said bundler binstub), then the chosen "latest" version will in fact be the right version for the app, and you'll be fine... but if you're using a truly bare rails, that's when you would have a version mismatch issue.


@albertoalmagro yeah, I originally understood this to be essentially a bug report that the "magic rails" behaviour was sometimes not working, so focused on why they weren't functionally identical. I agree it makes sense to standardize on one spelling -- and yes, I think the shorter / more easily typed one is the one to go with. (I suspect the spelling variety may date to the rake/rails unification circa ea4f0e2: rails has long had this special versioning behaviour, but of course rake does not.)

@albertoalmagro
Copy link
Contributor Author

@matthewd Thanks, then if you don't mind I will close this PR and will open a new PR (which references this one) with the corrected spelling, OK?

@albertoalmagro
Copy link
Contributor Author

@matthewd @deivid-rodriguez I'm closing this PR and opening #33229

I would be very happy to see you there. Thanks for your reviews and your comments here!

@albertoalmagro albertoalmagro deleted the alberto/add-bin-to-rails-command branch June 26, 2018 21:36
@deivid-rodriguez
Copy link
Contributor

@matthewd Thanks once again for your helpful explanations ❤️

So if I understood correctly, my idea would work only if Rails didn't intend to fully support running rails directly (without a bundle exec context or through a bundler binstub) in a reliable manner. So, since that is supported, the extra complexity is unavoidable and the rails binstub really needs to be different from the binstubs of other gems.

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

5 participants