style: call rubocop directly

[Rylan12]

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes? Here's an example.
• Have you successfully run brew style with your changes locally?
• Have you successfully run brew typecheck with your changes locally?
• Have you successfully run brew tests with your changes locally?
• Have you successfully run brew man locally and committed any changes?

---

This PR attempts to solve part of #10505

The idea is that we can initiate rubocop by calling RuboCop::CLI.new.run instead of using a system call to rubocop. Once this is done, we can selectively ignore the warnings to remove the message.

It appears that this first draft works well. I've noticed two issues, though:

1. Calling rubocop directly raises style warnings that were not raised previously. I think that this is likely due to a misconfiguration, but it is surprising.
2. Calling rubocop directly doesn't display rubocop errors. This isn't an issue for most brew style users, but for those who create and modify style rules, this lack of indication of a rubocop problem would be problematic.

I'm going to keep poking at this to see if I can figure it out. If anyone has any ideas, please share.

[BrewTestBot]

Review period will end on 2021-02-11 at 00:09:01 UTC.

[Bo98]

Does running it in a more isolated environment (i.e. a new Ruby process running a custom .rb file that does the RuboCop::CLI.new.run) make any of the weirdness go away?

The one new style warning it raises in the CI does seem correct - not entirely sure why it wasn't caught before though.

[MikeMcQuaid]

Great work here so far @Rylan12!

[MikeMcQuaid]

I wonder if there's any APIs that allow the JSON output to be obtained directly?

[Rylan12]

Possibly. I've been looking through the code and slowly starting to wrap my head around it. I'd like to avoid using any of the private API (just seems like good practice) so that may be a limiting factor. So far, I know that RuboCop::CLI#run is part of the public API while other parts that may be helpful (like RuboCop::Runner) are private. I'll look into the JSON stuff, though.

[MikeMcQuaid]

• Calling rubocop directly raises style warnings that were not raised previously. I think that this is likely due to a misconfiguration, but it is surprising.

Yes, no idea what's happening here.

2. Calling rubocop directly doesn't display rubocop errors. This isn't an issue for most brew style users, but for those who create and modify style rules, this lack of indication of a rubocop problem would be problematic.

Can you elaborate on with an example here?

Does running it in a more isolated environment (i.e. a new Ruby process running a custom .rb file that does the RuboCop::CLI.new.run) make any of the weirdness go away?

I'd be surprised if it did but copying the code in the bin stub (Library/Homebrew/vendor/bundle/ruby/2.6.0/bin/rubocop) with the warning gem added seems likely to.

[Rylan12]

• Calling rubocop directly raises style warnings that were not raised previously. I think that this is likely due to a misconfiguration, but it is surprising.

The one new style warning it raises in the CI does seem correct - not entirely sure why it wasn't caught before though.

I think the change is that style warnings (as opposed to style errors) weren't shown by brew style before. I don't know why that was the case. Unless there's a reason we want to ignore style errors, I'd say that the "new way" is better (i.e. including the style warnings)

Does running it in a more isolated environment (i.e. a new Ruby process running a custom .rb file that does the RuboCop::CLI.new.run) make any of the weirdness go away?

I'll give it a shot

I'd be surprised if it did but copying the code in the bin stub (Library/Homebrew/vendor/bundle/ruby/2.6.0/bin/rubocop) with the warning gem added seems likely to.

Thanks for the tip. I'll see what I can do. Totally venturing into new waters here so I may need some guidance.

[MikeMcQuaid]

I think the change is that style warnings (as opposed to style errors) weren't shown by brew style before. I don't know why that was the case. Unless there's a reason we want to ignore style errors, I'd say that the "new way" is better (i.e. including the style warnings)

Agreed 👍🏻

Totally venturing into new waters here so I may need some guidance.

Happy to help however I can!

[Rylan12]

Just realized that I forgot to give an example of the issues not being reported.

What I did was add a line with something like a = 1 / 0 to one of the cop files (I think I used homepage.rb to force an exception. Then, running rubocop directly or with the old brew style shows an error but the new brew style doesn't and just says 0 files checked.

[MikeMcQuaid]

Then, running rubocop directly or with the old brew style shows an error but the new brew style doesn't and just says 0 files checked.

Hmm, that seems non-ideal unless we can figure out a way to optionally show it again.

[Rylan12]

Agreed. This is its current state is not something that should be merged. I'd rather live with a warning than have errors silenced.

I looked at the code a little more. It looks like the error message are displayed using the warn method. I think this also puts them to stderr. So, it seems that the warn method isn't working properly when we're calling it (this is reproducible in brew irb as well). Do we override/modify warn anywhere? I wonder if rubocop does? It's also possible that stderr output is being silenced somehow. I'll be home in ~30 minutes and I'll spend some more time working on this.

[Bo98]

So, it seems that the warn method isn't working properly when we're calling it (this is reproducible in brew irb as well).

It's because we run brew with -W0.

The warn method should work with HOMEBREW_DEVELOPER=1 HOMEBREW_RUBY_WARNINGS=-W1 brew irb

[Rylan12]

Bingo, thanks!

I modified this to use with_warnings 1 to always show warnings generated with #warn just for the rubocop run. This seems to mostly solve that issue, but it's not perfect. I'm still getting this message when running an intentionally broken brew style:

$ brew style
An error occurred while FormulaAudit/Homepage cop was inspecting /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/htop.rb:1:0.
To see the complete backtrace run rubocop -d.
An error occurred while FormulaAudit/Homepage cop was inspecting /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/htop.rb:1:0.
To see the complete backtrace run rubocop -d.

1 file inspected, no offenses detected

1 error occurred:
An error occurred while FormulaAudit/Homepage cop was inspecting /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/htop.rb:1:0.
undefined method `metadata' for nil:NilClass
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/2.6.0/gems/rubocop-1.9.1/lib/rubocop/cli/command/execute_runner.rb:79:in `display_error_summary'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/2.6.0/gems/rubocop-1.9.1/lib/rubocop/cli/command/execute_runner.rb:58:in `display_summary'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/2.6.0/gems/rubocop-1.9.1/lib/rubocop/cli/command/execute_runner.rb:27:in `block in execute_runner'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/2.6.0/gems/rubocop-1.9.1/lib/rubocop/cli/command/execute_runner.rb:52:in `with_redirect'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/2.6.0/gems/rubocop-1.9.1/lib/rubocop/cli/command/execute_runner.rb:25:in `execute_runner'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/2.6.0/gems/rubocop-1.9.1/lib/rubocop/cli/command/execute_runner.rb:17:in `run'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/2.6.0/gems/rubocop-1.9.1/lib/rubocop/cli/command.rb:11:in `run'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/2.6.0/gems/rubocop-1.9.1/lib/rubocop/cli/environment.rb:18:in `run'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/2.6.0/gems/rubocop-1.9.1/lib/rubocop/cli.rb:65:in `run_command'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/2.6.0/gems/rubocop-1.9.1/lib/rubocop/cli.rb:72:in `execute_runners'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/2.6.0/gems/rubocop-1.9.1/lib/rubocop/cli.rb:41:in `run'
/usr/local/Homebrew/Library/Homebrew/style.rb:147:in `block in run_rubocop'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/2.6.0/gems/activesupport-6.1.1/lib/active_support/core_ext/kernel/reporting.rb:28:in `with_warnings'
/usr/local/Homebrew/Library/Homebrew/style.rb:146:in `run_rubocop'
/usr/local/Homebrew/Library/Homebrew/style.rb:54:in `check_style_impl'
/usr/local/Homebrew/Library/Homebrew/style.rb:16:in `check_style_and_print'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/style.rb:75:in `style'
/usr/local/Homebrew/Library/Homebrew/brew.rb:122:in `<main>'

It seems unable to display the normal rubocop error message:

Errors are usually caused by RuboCop bugs.
Please, report your problems to RuboCop's issue tracker.
https://github.com/rubocop-hq/rubocop/issues

Mention the following information in the issue report:
1.9.1 (using Parser 3.0.0.0, rubocop-ast 1.4.1, running on ruby 2.6.3 universal.x86_64-darwin20)

The traceback points here. Looks like a gem issue. I need to research more here I'm not that familiar with how gems interact with ruby code, etc.

Also, brew tests is still failing. Not yet sure why.

[Bo98]

Bundler's standalone setup.rb (the require "vendor/bundle/bundler/setup") skips spec loading. Rubocop assumes that its spec is loaded.

[Bo98]

Basically, Bundler's standalone mode is a very basic version of how gem loading works so it doesn't support the full feature set.

But it means that it works without having Bundler/Rubygems installed. Which matters less nowadays because Bundler ships with Ruby now. In fact, require still does gem searching under standalone mode if Rubygems is present (fixed in Bundler 2.2.7).

[BrewTestBot]

Review period ended.

[Rylan12]

Does running it in a more isolated environment (i.e. a new Ruby process running a custom .rb file that does the RuboCop::CLI.new.run) make any of the weirdness go away?

I went back to give this a try and it worked! I've updated the PR to implement this. Interestingly, though, the warning that reappeared is now gone again. Not sure why.

I didn't know where to put the rubocop.rb file that actually runs rubocop. For now, I put it in Library/Homebrew/rubocops/bin/rubocop.rb. I could use some advice on that, though. Maybe something like Library/Homebrew/bin/rubocop.rb?

If this looks good, I'll try to do a similar thing with brew tests. I haven't looked there at all to see what the situation is, though, so that may go in a separate PR.

[Bo98]

Interestingly, though, the warning that reappeared is now gone again. Not sure why.

Probably because Rubocop doesn't know what a FormulaUnavailableError is by just parsing the file under an isolated environment. When called within the brew environment, it will know since it will have been defined beforehand.

I didn't know where to put the rubocop.rb file that actually runs rubocop. For now, I put it in Library/Homebrew/rubocops/bin/rubocop.rb. I could use some advice on that, though. Maybe something like Library/Homebrew/bin/rubocop.rb?

I think utils is currently the dumping ground for scripts like that. I don't really have any preference though.

[Rylan12]

Probably because Rubocop doesn't know what a FormulaUnavailableError is by just parsing the file under an isolated environment. When called within the brew environment, it will know since it will have been defined beforehand.

👍 makes sense

I think utils is currently the dumping ground for scripts like that.

Thanks, sounds good

[MikeMcQuaid]

Ooo, nice. I wonder if we can use this to get -W0 removed entirely. Opened #10593 as another issue to track this.

[Rylan12]

One other thing I just noticed: should test/rubocop.rb be updated as well?

I quickly tried to modify the Open3.capture command to call ruby $HOMEBREW_LIBRARY/Homebrew/utils/rubocop.rb instead of ruby -S rubocop but it seems that the rubocop.rb file doesn't actually exist in the test HOMBREW_LIBRARY. Not sure what the best way to handle this is.

Edit: looking again at the test, I'm not sure this should be changed after all. It says when calling `rubocop` outside of the Homebrew environment, so I think it should use ruby -S rubocop after all.

Edit 2: and.... the reason it wasn't working was that I was trying to run from the master branch 🤦‍♂️. Guess I'm a little tired atm...

[MikeMcQuaid]

Edit: looking again at the test, I'm not sure this should be changed after all. It says when calling `rubocop` outside of the Homebrew environment, so I think it should use ruby -S rubocop after all.

Agreed 👍🏻

---

Another thing that I don't think requires a fix but is just worth thinking about is dev-cmd/rubocop.sh: does that need any changes/thought/consolidation?

[Rylan12]

Another thing that I don't think requires a fix but is just worth thinking about is dev-cmd/rubocop.sh: does that need any changes/thought/consolidation?

I just modified it to also use the rubocop utility and that seems to work with no warnings (or significant slowdowns), so let's give it a go!