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

audit: Check if uses_from_macos usage is correct #7280

Merged
merged 2 commits into from
Apr 7, 2020

Conversation

issyl0
Copy link
Member

@issyl0 issyl0 commented Apr 5, 2020

  • 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?

@issyl0 issyl0 force-pushed the audit-uses-from-macos branch from c0c4ae6 to f53ca5f Compare April 5, 2020 16:13
@issyl0
Copy link
Member Author

issyl0 commented Apr 5, 2020

OK, via abusing problem as a way to output stuff to the console (no amount of ohai with --verbose or --debug worked, and I couldn't get pry to trigger), I've found why some dependencies output as an empty string while others are fine:

problem "#{parameters(method).first.inspect}"

leads to:

nettle:
  * C: 19: col 3: FormulaAuditStrict/UsesFromMacos: s(:hash,
      s(:pair,
        s(:str, "m4"),
        s(:sym, :build)))
newt:
  * C: 19: col 3: FormulaAuditStrict/UsesFromMacos: s(:str, "python")

Now to work out how to handle the hash elements... 🤔

@issyl0 issyl0 force-pushed the audit-uses-from-macos branch from 9622fa4 to e884a19 Compare April 5, 2020 20:39
@issyl0
Copy link
Member Author

issyl0 commented Apr 5, 2020

With the current code, I get Error: 85 problems in 85 formulae detected. I think that sounds vaguely plausible?

@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Apr 6, 2020

Yeh, sounds about right. You can use brew style homebrew/core --only-cops=FormulaStrictAudit/Urls to run just that RuboCop (which should be much faster). Additionally: what do you think about this being a non-strict cop?

Once tests are 💚 (or if you need help getting them there): give me a shout and I'll review!

@issyl0
Copy link
Member Author

issyl0 commented Apr 6, 2020

The strict vs. non-strict cop was discussed in the original review and it was decided to make it a strict cop because of reliability concerns.

But that was back in July 2019, and uses_from_macos is way more widely used now, so I think a non-strict cop is good. I'll work on it later! Thanks, Mike.

@MikeMcQuaid
Copy link
Member

The strict vs. non-strict cop was discussed in the original review and it was decided to make it a strict cop because of reliability concerns.

👍. I'm confident this approach will be sufficiently robust.

@issyl0
Copy link
Member Author

issyl0 commented Apr 6, 2020

The one remaining style failure is:

Library/Homebrew/test/rubocops/uses_from_macos_spec.rb:5:1: C: RSpec/FilePath: Spec path should end with rubocop/cop/formula_audit/uses_from_macos*_spec.rb.
describe RuboCop::Cop::FormulaAudit::UsesFromMacos do
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

756 files inspected, 1 offense detected

After I've figured out how to fix this, I'll squash the commits and this'll be ready for review.

The results when running brew audit --only-cops=FormulaAudit/UsesFromMacos are different on macOS compared to Linux (there's more output on Linux). I guess this is because we're (oh god) not done with syncing uses_from_macos to Homebrew/homebrew-core yet. We can add more to the safelist when we come across them, I guess.

@MikeMcQuaid
Copy link
Member

We can add more to the safelist when we come across them, I guess.

👍

- This builds on @jonchang's work that started in Homebrew#6265.
- We now use `uses_from_macos` to declare dependencies that are implicit
  on macOS because they ship with macOS, but they're needed on Linux. We
  have to be sure that the dependencies people specify as
  `uses_from_macos` are actually shipped with macOS. So, we maintain a
  safelist of those dependencies and check against it.
- Also add more legitimate `uses_from_macos` dependencies to the list.
- This is runnable with `brew audit --only-cops=FormulaAudit/UsesFromMacos`.
- It produces different number of failures on macOS vs. Linux, because
  apparently we've not synced Homebrew/linuxbrew-core upstream thoroughly
  enough yet.
- Originally this was designed as a `--strict` audit, but we flipped it
  to be a normal audit because - to quote Mike - this is "sufficiently
  robust" now.
@issyl0 issyl0 force-pushed the audit-uses-from-macos branch from 8a92c61 to 857393c Compare April 6, 2020 12:44
@issyl0 issyl0 marked this pull request as ready for review April 6, 2020 12:44
@issyl0
Copy link
Member Author

issyl0 commented Apr 6, 2020

@MikeMcQuaid and anyone else: I squashed the commits, CI is 💚and this is ready for review, please!

Library/Homebrew/rubocops/uses_from_macos.rb Outdated Show resolved Hide resolved
@MikeMcQuaid MikeMcQuaid merged commit bd08201 into Homebrew:master Apr 7, 2020
@MikeMcQuaid
Copy link
Member

Thanks again @issyl0, great work!

@issyl0 issyl0 deleted the audit-uses-from-macos branch April 7, 2020 13:23
@lock lock bot added the outdated PR was locked due to age label May 20, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants