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

Infer TargetRubyVersion version from Gemfile #5610

Closed
roberts1000 opened this issue Feb 26, 2018 · 10 comments
Closed

Infer TargetRubyVersion version from Gemfile #5610

roberts1000 opened this issue Feb 26, 2018 · 10 comments

Comments

@roberts1000
Copy link
Contributor

roberts1000 commented Feb 26, 2018

This has been proposed before, but now that #5473 has been merged (which lets us infer the TargetRailsVersion from the bundler lock files), it makes sense to consider the bundler Gemfile|gems.rb when we choose the TargetRubyVersion. The new process would look like this:

  1. If the TargetRubyVersion is specified in the config, use it. Otherwise,
  2. Proposed new step: If the Ruby version is specified in the Gemfile or gems.rb file, use it. Otherwise,
  3. If there is a .ruby-version file, read the version from the file and use it, Otherwise,
  4. Use the default Ruby version (which is currently 2.1)

Steps 1, 3 and 4 make up the current process. It would be trivial to pluck the ruby line out of the Gemfile|gems.rb and set TargetRubyVersion based on that value.

Pain Point

My company has dozen of Rails apps, but we force them to use a shared RuboCop configuration. We've been setting the TargetRubyVersion in that configuration, but not all apps are on the same Ruby. This forces us to use the lowest Ruby version in our shared config, or have the apps override TargetRubyVersion in their own rubocop.yml (which we try to avoid because devs tend to start overriding other stuff we don't want to them override), or keep a .ruby-version file so RuboCop will read from there (this is our current solution).

Unfortunately, my company uses a PaaS that actually ignores the .ruby-version file. The PaaS pulls the ruby version from the Gemfile|gems.rb if it's present. The only reason we actually include a .ruby-version file today, in our particular projects, is to get rubocop to be more dynamic in how it chooses the TargetRubyVersion. (We do use RVM, but it happily reads the ruby version from the Gemfile|gems.rb. )

  1. We tried changing our Gemfiles to read the ruby version from the .ruby-version file, via code like:

    # Gemfile
    source 'https://rubygems.org
    
    ruby File.read('.ruby-version', mode: 'rb').chomp
    
    gem 'rails' ....

    This unfortunately causes errors in other tools which expect a raw version string to be present in the Gemfile directly after the ruby method name.

Benefits

  1. Many people could delete an unneeded .ruby-version file from their projects.
  2. Reading the ruby version out of the Gemfile would be conceptually similar to how we set the TargetRailsVersion - which we now infer by reading the Gemfile.lock|gems.locked file.

Implementation

This is a pretty small change. We simply need to require ruby 'x.x.x' be on a line by itself (which is how it's typically done) and regex the value out of the Gemfile or gems.rb.

I can submit a PR quickly so #5473, and this issue, can drop in the same release if there's interest.

@scottmatthewman
Copy link
Contributor

I like this idea. A quick question (and one in which I have no vested interest in the answer): should the check of the Gemfile come before, or after, the check for .ruby-version?

@roberts1000
Copy link
Contributor Author

roberts1000 commented Feb 27, 2018

I don't have a huge preference here either. I went with making the Gemfile the authority because I get the feeling more people are including the ruby version in the Gemfile these days. Since RMV|rbenv happily read the ruby version from the Gemfile, there's less of a need for bundler based projects to keep a .ruby-version file.

If ruby is specified in both the Gemfile and .ruby-version file:

  1. The PaaS, my team is using, (Pivotal Cloud Foundary) seems to totally ignore the .ruby-version file. It considers the Gemfile the authoritative source of the ruby version.
  2. RVM (and I think rbenv) will pull from .ruby-version file before pulling from the Gemfile so they clearly consider the .ruby-version file to be the authority.
  3. Bundler is a little more complex. Ultimately, bundler only cares that the active version of ruby is the same as the version that's specified in the Gemfile. If they're different, bundler will refuse to bundle when the bundle command is executed. However, it kind of defers to RVM|rbenv because those tools set the active version of Ruby when someone cd's into the project directory. Since they prefer pulling the ruby version out of the .ruby-version file, bundler essentially let's .ruby-version be the authority on the active ruby version, but not the authority on the ruby that should be used for the project. Interestingly enough, bundler will write out Ruby version in the Gemfile.lock file, but only when ruby is specified in the Gemfile file. I'd probably say bundler considers the Gemfile to be the authoritative source for the project, but it will play nice with RVM|rbenv and try to get keep things in sync.

Some tools are going to consider .ruby-version to be the authority. Some are going to consider the Gemfile to be the authority. So the question is, "what does RuboCop consider the authoritative source of the Ruby version?" I'm guessing it's the .ruby-version file since the current code in RuboCop reads out of the .ruby-version and ignores the Gemfile. If that's the case, we can check the .ruby-version file before the Gemfile. In practice it shouldn't matter. If a project defines ruby in both places, bundler will encourage/force the user to keep the info in sync (unless the user goes out of his way to keep it out of sync). If the ruby version is only specified in one place, then order is a non issue. (Although, it's going to be slightly faster to get the value out of the .ruby-version file.)

@scottmatthewman
Copy link
Contributor

If the ruby version is only specified in one place, then order is a non issue. (Although, it's going to be slightly faster to get the value out of the .ruby-version file.)

Yeah, for performance reasons alone I'd be inclined to check for .ruby-version first, as if that file doesn't exist it's easy enough to skip. However, the Gemfile requires parsing to determine whether or not it specifies a ruby version.

But honestly, if there's no big hit either way, then I'm not fussed. Glad we both put these thoughts down though 😄

@mikegee
Copy link
Contributor

mikegee commented Feb 27, 2018

We tried changing our Gemfiles to read the ruby version from the .ruby-version file, via code like:

# Gemfile
source 'https://rubygems.org

ruby File.read('.ruby-version', mode: 'rb').chomp

gem 'rails'

This unfortunately causes errors in other tools which expect a raw version string to be present in the Gemfile directly after the ruby method name.

...

We simply need to require ruby 'x.x.x' be on a line by itself (which is how it's typically done) and regex the value out of the Gemfile or gems.rb.

Are we ok with Rubocop being among the tools that are particular about the format of this line? Or, can we ask Bundler for the Ruby version after it parses the Gemfile?

I know that we skipped depending on Bundler in #5518, but the lockfile is safer to parse with a regular expression since it can't contain arbitrary Ruby code.

@ZASMan
Copy link

ZASMan commented Feb 27, 2018

This would be very helpful!

@roberts1000
Copy link
Contributor Author

roberts1000 commented Feb 27, 2018

@mikegee Good points. I was leaning towards searching the Gemfile because rubocop doesn't require the project to be bundled; it will happily execute without the Gemfile.lock being present. If we search the Gemfile, we can identify the ruby version when the user hasn't yet bundled the project.

I can make the parse logic a little smarter and handle the cases where ruby 'x.x.x' isn't on a line by itself, if needed. The regex is still pretty clean; I just need to make sure we don't pickup lines that have been commented out.

FWIW - I confirmed with RVM and it can't correctly identify the ruby version when it's on a compound line

source 'https://rubygems.org'; ruby '2.5.0'

I'm comfortable assuming/requiring ruby to be on it's own line in the Gemfile, but I'll code it whichever way is prefered.

@mikegee
Copy link
Contributor

mikegee commented Feb 27, 2018

@roberts1000 you don't want to depend on Bundler to parse the Gemfile?

I think it would be nice if this Rubocop feature could handle your example:

ruby File.read('.ruby-version', mode: 'rb').chomp

@roberts1000
Copy link
Contributor Author

roberts1000 commented Feb 27, 2018

I'm ok with using Bundler. I've been avoiding it so we wouldn't have to introduce a run time dependency on Bundler. It's easy enough to parse the Gemfile text and pluck out the necessary string, so I figured regexing was the quickest path to getting the thumbs up on this issue.

However, if we want to support strategies like ruby File.read('.ruby-version', mode: 'rb').chomp, then it makes more sense to use Bundler and actually evaluate the ruby code in the Gemfile.

@pocke
Copy link
Collaborator

pocke commented Mar 2, 2018

I think the feature should use Gemfile.lock instead of Gemfile.
If Ruby version is specified in a Gemfile, Bundler generates Gemfile.lock with Ruby version.
For example:

# Gemfile
source 'https://rubygems.org'
ruby '2.5.0'

Generated Gemfile.lock

GEM
  remote: https://rubygems.org/
  specs:

PLATFORMS
  ruby

DEPENDENCIES

RUBY VERSION
   ruby 2.5.0p0

BUNDLED WITH
   1.16.1

Pros:

  • RuboCop can detect ruby version if ruby File.read('.ruby-version', mode: 'rb').chomp is used.
  • Gemfile.lock is more parsable than Gemfile. Probably we can use simply regexp.

@roberts1000
Copy link
Contributor Author

Sounds good. PR incoming.

This was referenced Mar 21, 2018
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

No branches or pull requests

5 participants