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

CI: Add audit steps for formulae and casks #15047

Merged

Conversation

apainintheneck
Copy link
Contributor

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

Now the steps that say they run on all taps are after the one where we set up all taps. This might have been done on purpose before for performance reasons though. I'm not sure.

Now the steps that say they run on all taps are
after the one where we set up all taps.
@apainintheneck
Copy link
Contributor Author

Error: 338 problems in 102 casks detected

Well well well, look what we've got here.

@apainintheneck
Copy link
Contributor Author

Not able to reproduce locally. Probably has something to do with running these audits on Linux and it not being able to decide which on_* block to use.

~ $ brew audit --eval-all --skip-style --except=version --display-failures-only
==> Auditing a sample of available languages: zh-TW, pt, es-ES, ru, it, es-CL, nl, es-AR, pl and en-
==> Auditing a sample of available languages: cs, nl, fr, zh-TW, ru, ja, it, de, en and en-CA
==> Auditing a sample of available languages: zh, nl, de, en, en-GB, pt, en-CA, gl, zh-TW and it
==> Auditing a sample of available languages: ko, in, gl, es-ES, es-CL, pl, zh-TW, nl, pt and it
==> Auditing a sample of available languages: zh, zh-TW, ja, uk, nl, pt-BR, pl, de, fr and ru
==> Auditing a sample of available languages: bn, gu, eo, fa, he, sr, uk, en-CA, es-ES and zh-TW
==> Auditing a sample of available languages: oc, pa-IN, ar, sr, en-GB, as, nb, nr, mk and af
==> Auditing a sample of available languages: en-GB, pt, uk, ja, nl, cs, pl, en, fr and gl

That's probably why it wasn't set up like this before.

@MikeMcQuaid
Copy link
Member

Thanks @apainintheneck!

Compared to https://github.com/Homebrew/brew/actions/runs/4506520721/jobs/7933410010?pr=15046: this seems to slow things down a lot. I don't think it makes sense to rely on brew readall working on Linux for Casks. This could be moved to a (new?) macOS job.

Note that even before this: tap syntax was the slowest CI job by a fairly long way so I think it may make sense to split this up into multiple jobs.

Obviously I'd rather we ran more tests more often but, given Linux parallelisation is basically free, I think it makes sense to split out a new job here, perhaps doing just brew audit?

@apainintheneck
Copy link
Contributor Author

apainintheneck commented Mar 25, 2023

Looks like running the audit on MacOS pulls in other proprietary taps that have been installed there so I'll need to specify which ones should be included manually. Also, the tap-syntax step still took around 11 minutes even without the audit. If we're running brew readall on MacOS it shouldn't be needed on Linux anymore, right?

The new tap-audit step took 16 minutes but that might partially be related to auditing formulae and casks in other taps. I'll rerun it but it might end up being too slow to be worth it. In a pinch we could consider just running the cask stuff on MacOS and the formula stuff on Linux.

This will reduce the time it takes for the tap-syntax job
to complete (currently that is the slowest one) and will
allow us to audit casks as well as formulae (casks weren't
getting audited before in CI).
@apainintheneck
Copy link
Contributor Author

Still quite slow. It failed after the homebrew/core audit and it still took like 10 minutes. Probably not worth it.

@MikeMcQuaid
Copy link
Member

In a pinch we could consider just running the cask stuff on MacOS and the formula stuff on Linux.

This would make sense to me!

There will be one for casks which runs on MacOS
and the other for formulae which runs on Linux.
@apainintheneck
Copy link
Contributor Author

apainintheneck commented Mar 25, 2023

So now it seems to run pretty well after creating two new audit jobs, one for casks on MacOS and another for formulae on Linux.

TLDR: It adds 15 minutes to the total runtime but the two new jobs are run in parallel so it actually takes less time for the user. The tap-syntax job now takes around 11 minutes instead of 12 minutes previously.

run: brew readall --aliases homebrew/core

- name: Run brew audit --skip-style on homebrew/core
run: brew audit --skip-style --except=version --display-failures-only --tap=homebrew/core
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to use --display-failures-only here? We used to only use it for the general audit and not the one of homebrew/core. I'm going to rerun it without this option and see if it takes longer to complete.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to run at the same speed so I think it should be fine without it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does end up being kind of noisy on the cask side though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apainintheneck Yeh, let's definitely do --display-failures-only here, it's incredibly difficult to find the actual error otherwise when there's failures.

IMO --display-failures-only shouldn't even be a valid option, we should deprecate it at the next major/minor release and instead use --strict like we do with formula audits.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an interesting point. I'll remove that commit and then this should be good.

@apainintheneck apainintheneck changed the title CI: Change order of tap-syntax step CI: Add audit steps for formulae and casks Mar 25, 2023
@MikeMcQuaid
Copy link
Member

@apainintheneck Great work! We've historically had some issues with macOS worker parallelisation but these seem to be gone now. Just a heads up in case we need to adjust this in future.

@MikeMcQuaid MikeMcQuaid merged commit 4aa9595 into Homebrew:master Mar 28, 2023
45 checks passed
@MikeMcQuaid
Copy link
Member

Thanks again @apainintheneck!

@github-actions github-actions bot added the outdated PR was locked due to age label Apr 28, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 28, 2023
@apainintheneck apainintheneck deleted the change-order-of-tap-syntax branch May 14, 2023 19:42
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.

None yet

2 participants