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

Make the config loader Bundler-aware #7983

Merged
merged 1 commit into from May 27, 2020
Merged

Conversation

CvX
Copy link
Contributor

@CvX CvX commented May 16, 2020

Before this change, when inheriting a configuration from a gem, rubocop:

  1. would fail to find it, if the gem specified in the Gemfile has a path or git option (because rubygems is unaware of those gems)
  2. would always load the config of the latest installed version of a gem, regardless of the version specified in the Gemfile

This commit changes that, so if executed in the context of Bundler (defined?(Bundler)) it will first try to get the gem path via Bundler, and fallback to the previous Gem::Specification path when not found.

This solves the two mentioned issues while maintaining the possibility to run rubocop in a globally installed context, i.e. without Bundler.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@CvX CvX force-pushed the bundler-compat branch 2 times, most recently from 9e45c36 to db296ba Compare May 17, 2020 16:13
@bbatsov
Copy link
Collaborator

bbatsov commented May 17, 2020

The suggested change seems reasonable to me. @jonas054 @koic What do you think?

@CvX
Copy link
Contributor Author

CvX commented May 17, 2020

I can think of a couple open questions that should be answered before merging:

  1. While I added it to the changelog as a fix, should it be considered a (breaking) change? I can think of a situation where a rubocop user has a Gemfile with an outdated version a gem that's used to inherit the config from, and uses bundle exec rubocop to run the linting, but unknowingly has been using a different (globally installed) version of that gem. After this change, rubocop will start picking the correct version.
  2. I don't know how much of a private or public API Bundler.load.specs[gem_name].first.full_gem_path is. Also, this API might have been different in the older versions of Bundler. Should we have an escape hatch (rescue?) in case it blows up? Should we test the compatibility with different versions? (1.x, 2.x, master?) What about the cases where rubocop is run in a context of some ancient (i.e. pre 1.0) versions of Bundler?

@bbatsov
Copy link
Collaborator

bbatsov commented May 18, 2020

Good points.

  1. I don't think many users would be impacted by this, but we should probably highlight the potential for some issues in the changelog.
  2. If you've stuck on an ancient Bundler probably recent RuboCop's won't work for you either. :-) I guess if this works on 1.x and 2.x we should be fine.

@koic
Copy link
Member

koic commented May 18, 2020

This proposal looks good to me.

OTOH, I'm worried about it doesn't work with Bundler 1.2.0(Bundler's patch version is not investigated).
It may be better to handle Bundler::GemfileError and display a message prompting Bundler upgrade.

Bundler 2.1.4

% ruby -rbundler -e "p RUBY_VERSION; p Bundler::VERSION; p Bundler.load.specs['net-telnet'].first.full_gem_path"
"2.4.10"
"2.1.4"
"/Users/koic/.rbenv/versions/2.4.10/lib/ruby/gems/2.4.0/gems/net-telnet-0.2.0"

Bundler 1.3.0

% ruby -rbundler -e "p RUBY_VERSION; p Bundler::VERSION; p Bundler.load.specs['net-telnet'].first.full_gem_path"
"2.4.10"
"1.3.0"
"/Users/koic/.rbenv/versions/2.4.10/lib/ruby/gems/2.4.0/gems/net-telnet-0.2.0"

Bundler 1.2.0

% ruby -rbundler -e "p RUBY_VERSION; p Bundler::VERSION; p Bundler.load.specs['net-telnet'].first.full_gem_path"
"2.4.10"
"1.2.0"
/Users/koic/.rbenv/versions/2.4.10/lib/ruby/gems/2.4.0/gems/bundler-1.2.0/lib/bundler/dsl.rb:12:in `rescue in evaluate': There was an error in your Gemfile, and Bundler cannot continue. (Bundler::GemfileError)
        from /Users/koic/.rbenv/versions/2.4.10/lib/ruby/gems/2.4.0/gems/bundler-1.2.0/lib/bundler/dsl.rb:5:in `evaluate'
        from /Users/koic/.rbenv/versions/2.4.10/lib/ruby/gems/2.4.0/gems/bundler-1.2.0/lib/bundler/definition.rb:18:in `build'
        from /Users/koic/.rbenv/versions/2.4.10/lib/ruby/gems/2.4.0/gems/bundler-1.2.0/lib/bundler.rb:144:in `definition'
        from /Users/koic/.rbenv/versions/2.4.10/lib/ruby/gems/2.4.0/gems/bundler-1.2.0/lib/bundler.rb:132:in `load'
        from -e:1:in `<main>'

@bbatsov
Copy link
Collaborator

bbatsov commented May 18, 2020

I like this suggestion!

@CvX
Copy link
Contributor Author

CvX commented May 23, 2020

OTOH, I'm worried about it doesn't work with Bundler 1.2.0

@koic It does work! 😃 The Gemfile you ran your test on apparently isn't compatible with Bundler 1.2.0. When everything aligns just right (ruby version, Bundler version, and Gemfile syntax):

ruby -rbundler -e "p RUBY_VERSION; p Bundler::VERSION; p Bundler.load.specs['i18n'].first.full_gem_path"
"1.9.3"
"1.2.0"
"/Users/cvx/.rbenv/versions/1.9.3-p551/lib/ruby/gems/1.9.1/gems/i18n-0.9.5"

I don't think catching Bundler::GemfileError would be of any use, since it has less to do with rubocop's bundler usage and is more of a user error. If you tried to run bundle exec rubocop with an invalid config it would fail before reaching the config resolver.

I don't think many users would be impacted by this, but we should probably highlight the potential for some issues in the changelog

@bbatsov Do you have any suggestion how to succinctly explain these potential edge-cases? 🙂

Before this change, when inheriting a configuration from a gem, rubocop:

1. would fail to find it, if the gem specified in the Gemfile has a `path` or `git` option (because rubygems is unaware of those gems)
2. would always load the config of the latest installed version of a gem, regardless of the version specified in the Gemfile

This commit changes that, so if executed in the context of Bundler (`defined?(Bundler)`) it will first try to get the gem path via Bundler, and fallback to the previous `Gem::Specification` path when not found.

This solves the two mentioned issues while maintaining the possibility to run rubocop in a globally installed context, i.e. without Bundler.
@bbatsov
Copy link
Collaborator

bbatsov commented May 26, 2020

@bbatsov Do you have any suggestion how to succinctly explain these potential edge-cases? 🙂

Something like "Require Bundler X or newer" perhaps?

@CvX
Copy link
Contributor Author

CvX commented May 27, 2020

"Require Bundler X or newer"

This PR doesn't require Bundler, but if we're talking version requirement of an optional integration that it enables – I've tried the oldest version that's mentioned in bundler's changelog (0.9.5 from 2010) that snippet still worked, so I'm now even more confident that we're safe on that front. 😃

@bbatsov bbatsov merged commit b9f3a51 into rubocop:master May 27, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented May 27, 2020

No need to fret over this further then. :-)

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

3 participants