Add MPICH cop and test #6214

zbeekman · 2019-06-06T16:08:48Z

Tests are passing locally now, one CI build failure seemed like a cloud/network issue

Split off from PR Update documentation to codify MPI and BLAS/LAPACK library choices & link rubydoc #6209
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 tests with your changes locally?

zbeekman · 2019-06-06T16:25:28Z

Note: Edited
Reason: Minimize outdated information

Previous details about expected vs observed behavior

Outdated Contents

This is out of date because Markus showed me how to resolve my issue

What I expect to happen

Cop passes the test which I wrote for it.

I don't fully understand how the cop output relates to the expected spec output. Maybe I need to dig around in expect_offense.

What actually happens

I can run brew audit --strict <formula> on a formula where I have replaced the open-mpi dependency with an mpich dependency, and I see the cop working as expected. For example:

diff --git i/Formula/opencoarrays.rb w/Formula/opencoarrays.rb
 index 6e32a36394..58afc7826c 100644
 --- i/Formula/opencoarrays.rb
 +++ w/Formula/opencoarrays.rb
 @@ -14,7 +14,7 @@ class Opencoarrays < Formula

    depends_on "cmake" => :build
    depends_on "gcc"
 -  depends_on "open-mpi"
 +  depends_on "mpich"

    def install
      mkdir "build" do

$ brew audit --strict opencoarrays
opencoarrays:
  * C: 17: col 3: Use 'depends_on "open-mpi"' instead of 'depends_on "mpich"'.
Error: 1 problem in 1 formula detected
$ brew audit --strict scalapack # un-modified, uses open-mpi
# returns 0, no output

However, if I run

brew tests --verbose --debug --only=rubocops/lines

I get:

  1) RuboCop::Cop::FormulaAudit::Miscellaneous When auditing formula reports an offense when using depends_on "mpich"
     Failure/Error:
       expect_offense(<<~RUBY)
         class Foo < Formula
           desc "foo"
           url 'https://brew.sh/foo-1.0.tgz'
           depends_on "mpich"
           ^^^^^^^^^^^^^^^^^^  Use 'depends_on "open-mpi"' instead of 'depends_on "mpich"'.
         end
       RUBY

       expected: "class Foo < Formula\n  desc \"foo\"\n  url 'https://brew.sh/foo-1.0.tgz'\n  depends_on \"mpich\"\n  ^^^^^^^^^^^^^^^^^^  Use 'depends_on \"open-mpi\"' instead of 'depends_on \"mpich\"'.\nend\n"
            got: "class Foo < Formula\n  desc \"foo\"\n  url 'https://brew.sh/foo-1.0.tgz'\n  depends_on \"mpich\"\nend\n"

       (compared using ==)

       Diff:
       @@ -2,6 +2,5 @@
          desc "foo"
          url 'https://brew.sh/foo-1.0.tgz'
          depends_on "mpich"
       -  ^^^^^^^^^^^^^^^^^^  Use 'depends_on "open-mpi"' instead of 'depends_on "mpich"'.
        end

     # ./vendor/bundle/ruby/2.3.0/gems/rubocop-0.71.0/lib/rubocop/rspec/expect_offense.rb:99:in `expect_offense'
     # ./test/rubocops/lines_spec.rb:787:in `block (3 levels) in <top (required)>'
     # ./test/spec_helper.rb:163:in `block (2 levels) in <top (required)>'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.1/lib/rspec/retry.rb:123:in `block in run'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.1/lib/rspec/retry.rb:110:in `loop'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.1/lib/rspec/retry.rb:110:in `run'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.1/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.1/lib/rspec/retry.rb:37:in `block (2 levels) in setup'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-wait-0.0.9/lib/rspec/wait.rb:46:in `block (2 levels) in <top (required)>'

What I don't understand

Is there an instance variable that needs to be set somewhere to pickup the offense source range that's not getting set? @offensive_node.source prints the correct information in the cop, so this seems unlikely.

Am I not specify the test correctly? The test output outputs the dummy ruby formula source, but does not seem to underline it and print my error message from the cop.

zbeekman · 2019-06-06T17:36:49Z

Same error on CI as when run locally

Failures:

  1) RuboCop::Cop::FormulaAudit::Miscellaneous When auditing formula reports an offense when using depends_on "mpich"
     Failure/Error:
       expect_offense(<<~RUBY)
         class Foo < Formula
           desc "foo"
           url 'https://brew.sh/foo-1.0.tgz'
           depends_on "mpich"
           ^^^^^^^^^^^^^^^^^^  Use 'depends_on "open-mpi"' instead of 'depends_on "mpich"'.
         end
       RUBY

       expected: "class Foo < Formula\n  desc \"foo\"\n  url 'https://brew.sh/foo-1.0.tgz'\n  depends_on \"mpich\"\n  ^^^^^^^^^^^^^^^^^^  Use 'depends_on \"open-mpi\"' instead of 'depends_on \"mpich\"'.\nend\n"
            got: "class Foo < Formula\n  desc \"foo\"\n  url 'https://brew.sh/foo-1.0.tgz'\n  depends_on \"mpich\"\nend\n"

       (compared using ==)

       Diff:
       @@ -2,6 +2,5 @@
          desc "foo"
          url 'https://brew.sh/foo-1.0.tgz'
          depends_on "mpich"
       -  ^^^^^^^^^^^^^^^^^^  Use 'depends_on "open-mpi"' instead of 'depends_on "mpich"'.
        end
     # ./vendor/bundle/ruby/2.3.0/gems/rubocop-0.71.0/lib/rubocop/rspec/expect_offense.rb:99:in `expect_offense'
     # ./test/rubocops/lines_spec.rb:787:in `block (3 levels) in <top (required)>'
     # ./test/spec_helper.rb:163:in `block (2 levels) in <top (required)>'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.1/lib/rspec/retry.rb:123:in `block in run'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.1/lib/rspec/retry.rb:110:in `loop'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.1/lib/rspec/retry.rb:110:in `run'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.1/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.1/lib/rspec/retry.rb:37:in `block (2 levels) in setup'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-wait-0.0.9/lib/rspec/wait.rb:46:in `block (2 levels) in <top (required)>'

Library/Homebrew/rubocops/lines.rb

reitermarkus · 2019-06-06T17:43:06Z

Library/Homebrew/test/rubocops/lines_spec.rb

@@ -782,5 +782,16 @@ class Foo < Formula
        end
      RUBY
    end
+
+    it "reports an offense when using depends_on \"mpich\"" do
+      expect_offense(<<~RUBY)


Suggested change

expect_offense(<<~RUBY)

expect_offense(<<~RUBY, "/homebrew-core/")

@reitermarkus you rock! Thanks, that fixed it!

Should I break this back out into its own class again if I want to provide auto-correct functionality? I can add a test for that too, it looks like, now that I understand how that works. I think if I leave it in the miscellaneous class, which currently has no auto-correct, the auto-correct won't be specific to just the MPI lib choice test, but to the entire class (maybe?).

Yeah, I think a separate class for each auto-correctable cop is preferable.

Can I keep it in lines.rb or should I make a new file/module?

@reitermarkus why does your change resolve the problem?

I was looking for documentation on expect_offense() and came up with https://www.rubydoc.info/gems/rubocop/RuboCop%2FRSpec%2FExpectOffense:expect_offense

The second argument, file is not well documented there, IMO. Is that argument taken to be a hypothetical path to the source?

- Split off from PR Homebrew#6209 - Create stand alone class for cop w/ auto-correct

zbeekman · 2019-06-06T21:37:26Z

test failure

The agent: Hosted Agent lost communication with the server. Verify the machine is running and has a healthy network connection. For more information, see: https://go.microsoft.com/fwlink/?linkid=846610

EDIT:
This appears to be a cloud/network issue. I can't get a log for the failing step, it won't download.

further refactoring & testing ideas

There are a number of cops checking stuff about depends_on that are relatively straightforward to implement auto correction for. Most are in lines.rb but the veclibfort/lapack check in text.rb is also a prime candidate.

I was considering moving these to their own file, bad_dependency.rb or similar, ensuring all had tests, and autocorrect. Please let me know what you think of that idea.

MikeMcQuaid · 2019-06-07T08:29:33Z

I was considering moving these to their own file, bad_dependency.rb or similar, ensuring all had tests, and autocorrect. Please let me know what you think of that idea.

Makes sense! I'd suggest making it a separate/follow-up PR, though.

MikeMcQuaid · 2019-06-07T09:19:17Z

Fixed up brew style. Thanks again @zbeekman.

zbeekman · 2019-06-07T14:35:08Z

If I run brew style <some/path> do the same rules apply as brew style? I could swear I checked this, and there were no complaints, but maybe I missed something.

Thanks for cleaning up after me!

MikeMcQuaid · 2019-06-07T14:58:09Z

If I run brew style <some/path> do the same rules apply as brew style?

No.

zbeekman · 2019-06-07T16:16:18Z

Well, that's where I went wrong I guess. Thanks again!

zbeekman mentioned this pull request Jun 6, 2019

Update documentation to codify MPI and BLAS/LAPACK library choices & link rubydoc #6209

Merged

6 tasks

reitermarkus reviewed Jun 6, 2019

View reviewed changes

Library/Homebrew/rubocops/lines.rb Outdated Show resolved Hide resolved

reitermarkus reviewed Jun 6, 2019

View reviewed changes

zbeekman force-pushed the add-MPI-cop branch from 53a55c2 to 12eea86 Compare June 6, 2019 20:25

Add MPICH cop and test

ffe3005

- Split off from PR Homebrew#6209 - Create stand alone class for cop w/ auto-correct

zbeekman force-pushed the add-MPI-cop branch from 12eea86 to ffe3005 Compare June 6, 2019 20:31

Add test for MPI choice cop's autocorrect

65fbcc8

Limit veclibfort & lapack cop to core & add tests

75bc30b

rubocops/text: fix long line.

11de7de

MikeMcQuaid merged commit c95cf90 into Homebrew:master Jun 7, 2019

zbeekman deleted the add-MPI-cop branch June 7, 2019 14:35

lock bot added the outdated PR was locked due to age label Jan 1, 2020

lock bot locked as resolved and limited conversation to collaborators Jan 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MPICH cop and test #6214

Add MPICH cop and test #6214

zbeekman commented Jun 6, 2019 •

edited

Loading

zbeekman commented Jun 6, 2019 •

edited

Loading

This is out of date because Markus showed me how to resolve my issue

What I expect to happen

What actually happens

What I don't understand

zbeekman commented Jun 6, 2019

reitermarkus Jun 6, 2019

zbeekman Jun 6, 2019 •

edited

Loading

reitermarkus Jun 6, 2019

zbeekman Jun 6, 2019

zbeekman Jun 6, 2019

zbeekman commented Jun 6, 2019 •

edited

Loading

MikeMcQuaid commented Jun 7, 2019

MikeMcQuaid commented Jun 7, 2019

zbeekman commented Jun 7, 2019

MikeMcQuaid commented Jun 7, 2019

zbeekman commented Jun 7, 2019

	expect_offense(<<~RUBY)
	expect_offense(<<~RUBY, "/homebrew-core/")

Add MPICH cop and test #6214

Add MPICH cop and test #6214

Conversation

zbeekman commented Jun 6, 2019 • edited Loading

Tests are passing locally now, one CI build failure seemed like a cloud/network issue

zbeekman commented Jun 6, 2019 • edited Loading

Previous details about expected vs observed behavior

This is out of date because Markus showed me how to resolve my issue

What I expect to happen

What actually happens

What I don't understand

zbeekman commented Jun 6, 2019

reitermarkus Jun 6, 2019

Choose a reason for hiding this comment

zbeekman Jun 6, 2019 • edited Loading

Choose a reason for hiding this comment

reitermarkus Jun 6, 2019

Choose a reason for hiding this comment

zbeekman Jun 6, 2019

Choose a reason for hiding this comment

zbeekman Jun 6, 2019

Choose a reason for hiding this comment

zbeekman commented Jun 6, 2019 • edited Loading

test failure

further refactoring & testing ideas

MikeMcQuaid commented Jun 7, 2019

MikeMcQuaid commented Jun 7, 2019

zbeekman commented Jun 7, 2019

MikeMcQuaid commented Jun 7, 2019

zbeekman commented Jun 7, 2019

zbeekman commented Jun 6, 2019 •

edited

Loading

zbeekman commented Jun 6, 2019 •

edited

Loading

zbeekman Jun 6, 2019 •

edited

Loading

zbeekman commented Jun 6, 2019 •

edited

Loading