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

Improve cask audit #15977

Merged
merged 1 commit into from Sep 8, 2023
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?

Another follow-up to #15943 and #15956.

  • check for cask.url in audit steps
  • check for cask.version in audit steps
  • check for cask.sha256 in fetch command
  • stop omitting casks based on nil url in audit command

It would be nice to be able to omit casks from the audit if the os is not supported but there is not easy way to do that without updating the SimulateSystem code or refactoring how MacOSRequirement's are defined in the DSL.

At the end of the day, these changes make it so that you won't try to fetch a cask if the url or sha256 is missing (we'd expect the sha256 value to be :no_check if it's been omitted on purpose). It also allows the audit to accept casks with nil urls and versions since we already check for those. The problem was that we had some audit steps which assumed those values would be valid (non-nil) and now they don't.

Results for brew audit --skip-style homebrew/cask/calhash -os=all --debug

/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::TapLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-cask/Formula/calhash.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Cask::CaskLoader::FromTapLoader): loading homebrew/cask/calhash
==> Auditing Cask calhash on os sonoma and arch intel
/usr/local/Homebrew/Library/Homebrew/brew.rb (Cask::CaskLoader::FromTapPathLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-cask/Casks/c/calhash.rb
==> Auditing pkg stanza: allow_untrusted
==> Auditing stanzas which require an uninstall
==> Auditing preflight and postflight stanzas
==> Auditing single uninstall_* and zap stanzas
==> Auditing required stanzas
==> Auditing version :latest does not appear as a string ('latest')
==> Auditing sha256 :no_check with version :latest
==> Auditing sha256 string is a legal SHA-256 digest
==> Auditing sha256 is not a known invalid value
==> Auditing URL format
==> Auditing GitHub prerelease
==> Auditing Cask calhash on os ventura and arch intel
/usr/local/Homebrew/Library/Homebrew/brew.rb (Cask::CaskLoader::FromTapPathLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-cask/Casks/c/calhash.rb
==> Auditing pkg stanza: allow_untrusted
==> Auditing stanzas which require an uninstall
==> Auditing preflight and postflight stanzas
==> Auditing single uninstall_* and zap stanzas
==> Auditing required stanzas
==> Auditing version :latest does not appear as a string ('latest')
==> Auditing sha256 :no_check with version :latest
==> Auditing sha256 string is a legal SHA-256 digest
==> Auditing sha256 is not a known invalid value
==> Auditing URL format
==> Auditing GitHub prerelease
==> Auditing Cask calhash on os monterey and arch intel
/usr/local/Homebrew/Library/Homebrew/brew.rb (Cask::CaskLoader::FromTapPathLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-cask/Casks/c/calhash.rb
==> Auditing pkg stanza: allow_untrusted
==> Auditing stanzas which require an uninstall
==> Auditing preflight and postflight stanzas
==> Auditing single uninstall_* and zap stanzas
==> Auditing required stanzas
==> Auditing version :latest does not appear as a string ('latest')
==> Auditing sha256 :no_check with version :latest
==> Auditing sha256 string is a legal SHA-256 digest
==> Auditing sha256 is not a known invalid value
==> Auditing URL format
==> Auditing GitHub prerelease
==> Auditing Cask calhash on os big_sur and arch intel
/usr/local/Homebrew/Library/Homebrew/brew.rb (Cask::CaskLoader::FromTapPathLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-cask/Casks/c/calhash.rb
==> Auditing pkg stanza: allow_untrusted
==> Auditing stanzas which require an uninstall
==> Auditing preflight and postflight stanzas
==> Auditing single uninstall_* and zap stanzas
==> Auditing required stanzas
==> Auditing version :latest does not appear as a string ('latest')
==> Auditing sha256 :no_check with version :latest
==> Auditing sha256 string is a legal SHA-256 digest
==> Auditing sha256 is not a known invalid value
==> Auditing URL format
==> Auditing GitHub prerelease
==> Auditing Cask calhash on os catalina and arch intel
/usr/local/Homebrew/Library/Homebrew/brew.rb (Cask::CaskLoader::FromTapPathLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-cask/Casks/c/calhash.rb
==> Auditing pkg stanza: allow_untrusted
==> Auditing stanzas which require an uninstall
==> Auditing preflight and postflight stanzas
==> Auditing single uninstall_* and zap stanzas
==> Auditing required stanzas
==> Auditing version :latest does not appear as a string ('latest')
==> Auditing sha256 :no_check with version :latest
==> Auditing sha256 string is a legal SHA-256 digest
==> Auditing sha256 is not a known invalid value
==> Auditing URL format
==> Auditing GitHub prerelease
==> Auditing Cask calhash on os mojave and arch intel
/usr/local/Homebrew/Library/Homebrew/brew.rb (Cask::CaskLoader::FromTapPathLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-cask/Casks/c/calhash.rb
==> Auditing pkg stanza: allow_untrusted
==> Auditing stanzas which require an uninstall
==> Auditing preflight and postflight stanzas
==> Auditing single uninstall_* and zap stanzas
==> Auditing required stanzas
==> Auditing sha256 string is a legal SHA-256 digest
==> Auditing sha256 is not a known invalid value
==> Auditing GitHub prerelease
audit for calhash: failed
 - a version stanza is required
 - a url stanza is required
==> Auditing Cask calhash on os high_sierra and arch intel
/usr/local/Homebrew/Library/Homebrew/brew.rb (Cask::CaskLoader::FromTapPathLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-cask/Casks/c/calhash.rb
==> Auditing pkg stanza: allow_untrusted
==> Auditing stanzas which require an uninstall
==> Auditing preflight and postflight stanzas
==> Auditing single uninstall_* and zap stanzas
==> Auditing required stanzas
==> Auditing sha256 string is a legal SHA-256 digest
==> Auditing sha256 is not a known invalid value
==> Auditing GitHub prerelease
audit for calhash: failed
 - a version stanza is required
 - a url stanza is required
==> Auditing Cask calhash on os sierra and arch intel
/usr/local/Homebrew/Library/Homebrew/brew.rb (Cask::CaskLoader::FromTapPathLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-cask/Casks/c/calhash.rb
==> Auditing pkg stanza: allow_untrusted
==> Auditing stanzas which require an uninstall
==> Auditing preflight and postflight stanzas
==> Auditing single uninstall_* and zap stanzas
==> Auditing required stanzas
==> Auditing sha256 string is a legal SHA-256 digest
==> Auditing sha256 is not a known invalid value
==> Auditing GitHub prerelease
audit for calhash: failed
 - a version stanza is required
 - a url stanza is required
==> Auditing Cask calhash on os el_capitan and arch intel
/usr/local/Homebrew/Library/Homebrew/brew.rb (Cask::CaskLoader::FromTapPathLoader): loading /usr/local/Homebrew/Library/Taps/homebrew/homebrew-cask/Casks/c/calhash.rb
==> Auditing pkg stanza: allow_untrusted
==> Auditing stanzas which require an uninstall
==> Auditing preflight and postflight stanzas
==> Auditing single uninstall_* and zap stanzas
==> Auditing required stanzas
==> Auditing sha256 string is a legal SHA-256 digest
==> Auditing sha256 is not a known invalid value
==> Auditing GitHub prerelease
audit for calhash: failed
 - a version stanza is required
 - a url stanza is required
calhash
  * a version stanza is required
  * a url stanza is required
Error: 2 problems in 1 cask detected.

@apainintheneck apainintheneck added bug Reproducible Homebrew/brew bug cask Homebrew Cask labels Sep 7, 2023
@apainintheneck apainintheneck self-assigned this Sep 7, 2023
@apainintheneck
Copy link
Contributor Author

@EricFromCanada Does this seem reasonable?

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Makes sense to me and would be good to fix affected casks.

Copy link
Member

@EricFromCanada EricFromCanada left a comment

Choose a reason for hiding this comment

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

Makes sense from here 👍

@apainintheneck apainintheneck marked this pull request as ready for review September 8, 2023 03:01
@apainintheneck
Copy link
Contributor Author

I guess I should check if a nil cask.homepage is accounted for too.

[:version, :sha256, :url, :homepage].each do |sym|

@apainintheneck apainintheneck marked this pull request as draft September 8, 2023 03:08
@EricFromCanada
Copy link
Member

I guess I should check if a nil cask.homepage is accounted for too.

Depends on how likely is it for a cask's homepage to vary depending on the version it installs.

- check for cask.url in audit steps
- check for cask.version in audit steps
- check for cask.sha256 in fetch command
- stop omitting casks based on nil url in audit command

It would be nice to be able to omit casks from the audit
if the os is not supported but there is not easy way to
do that without updating the SimulateSystem code or
refactoring how MacOSRequirement's are defined in the DSL.
@apainintheneck apainintheneck marked this pull request as ready for review September 8, 2023 03:39
@apainintheneck
Copy link
Contributor Author

I added a two more online only ones. There are probably a few more out there but we can fix them as they come up.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Thanks again @apainintheneck!

@MikeMcQuaid MikeMcQuaid merged commit 9b7b283 into Homebrew:master Sep 8, 2023
24 checks passed
@github-actions github-actions bot added the outdated PR was locked due to age label Oct 9, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Reproducible Homebrew/brew bug cask Homebrew Cask outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants