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

cmd/fetch & cmd/audit: handle unsupported cask os/arch combos #15956

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?

This is a follow-up to #15943 as described here: #15943 (comment)

It's possible for casks to only run on certain os/arch combinations but we don't check for them in the fetch and audit commands.

At first, I tried to check if the macos version was satisfied in a previous PR but that doesn't work correctly because MacOSRequirement and ArchRequirement don't respect SimulateSystem.

Instead I'm just checking to see if the url stanza is defined for each os/arch combination and skipping those casks where it ends up being nil. This is kind of brute forcing it but should work.

Testing:

  1. brew audit
    Run brew audit --skip-style --except=version homebrew/cask/gpgfrontend --os=all -d and five of the audits should warn about unsupported os/arch combinations.
  2. brew fetch
/u/l/Homebrew (handle-nil-urls-in-fetch-and-audit-cmds|✚1) $ brew fetch gpgfrontend --os=catalina
Warning: Cask gpgfrontend is not supported on os catalina and arch intel

Are there any scenarios where we'd want to audit a cask with a nil url?

It's possible for casks to only run on certain os/arch
combinations but we don't check for them in the fetch and
audit commands.

At first, I tried to check if the macos version was satisfied
in a previous PR but that doesn't work correctly because
MacOSRequirement and ArchRequirement don't respect SimulateSystem.

Instead I'm just checking to see if the url stanza is defined
for each os/arch combination and skipping those casks where
it ends up being nil. This is kind of brute forcing it but
should work.
@apainintheneck apainintheneck added bug Reproducible Homebrew/brew bug cask Homebrew Cask labels Sep 4, 2023
@apainintheneck apainintheneck changed the title cmd/fetch & cmd/audit: handle unsupported os/arch combos cmd/fetch & cmd/audit: handle unsupported cask os/arch combos Sep 4, 2023
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.

Would it work to check against cask.sha256 instead? The one cask for which the current approach doesn't work is calhash, which is a class of application where each release works only on its target OS version, but the URL is defined outside the on_<system> blocks so that it can be generated at runtime. (The alternative is to just modify that cask to mirror deeper, which has the URL statically set for each block.)

@p-linnane
Copy link
Member

@EricFromCanada We should just change calhash to match deeper and onyx since that developer handles all their apps the same way.

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! What's the user error message look like in this case?

@MikeMcQuaid MikeMcQuaid merged commit e439725 into Homebrew:master Sep 5, 2023
26 checks passed
@MikeMcQuaid
Copy link
Member

Thanks @apainintheneck!

@EricFromCanada
Copy link
Member

Makes sense to me! What's the user error message look like in this case?

$ brew audit --skip-style --except=version --tap=homebrew/cask
Warning: Cask apparency is not supported on os high_sierra and arch intel
audit for calhash: failed
 - exception while auditing calhash: undefined method `latest?' for nil:NilClass
==> Auditing a sample of available languages: fr, en, eo, te, de, th, my, hi, es-AR and pa-IN
Warning: Cask gpgfrontend is not supported on os high_sierra and arch intel
==> Auditing a sample of available languages: eo, mn, af, ve, lt, hu, el, br, it and bg
Warning: Cask saoimageds9 is not supported on os high_sierra and arch intel
audit for sf-symbols: failed
 - Use `sha256 :no_check` when URL is unversioned.
==> Auditing a sample of available languages: cs, zh-TW, nl, pt-BR, pl, ja, it, uk, pt and en-GB
calhash
  * exception while auditing calhash: undefined method `latest?' for nil:NilClass
sf-symbols
  * Use `sha256 :no_check` when URL is unversioned.
Error: 2 problems in 2 casks detected.

$ brew fetch --cask gpgfrontend
Warning: Cask gpgfrontend is not supported on os high_sierra and arch intel

@EricFromCanada
Copy link
Member

Another edge case: a cask can omit blocks for some system releases as long as there's a depends_on stanza outside them that blocks them off (example: Homebrew/homebrew-cask#154505). But for a formula like calhash, which only has versions for Catalina and later and has depends_on defined inside each system block to ensure each version runs only on that release, brew readall homebrew/cask will fail because it'll find neither URLs for some versions, nor any depends_on stating otherwise. The somewhat odd solution is to tack on system blocks at each end supplying an appropriate depends_on stanza. (Homebrew/homebrew-cask#154503)

EricFromCanada added a commit to EricFromCanada/homebrew-cask that referenced this pull request Sep 6, 2023
@apainintheneck
Copy link
Contributor Author

I think there are two things here.

  1. brew audit should be updated so that certain checks do not assume that stanzas are available when they might not be like the version stanza above.
  2. The on_* blocks should automatically update some sort of MacOSRequirement so that this workaround with depends_on macos: isn't necessary inside each block anymore.

@MikeMcQuaid
Copy link
Member

Makes sense to me! What's the user error message look like in this case?

$ brew audit --skip-style --except=version --tap=homebrew/cask

Thanks @EricFromCanada! Sorry, I more mean: what's it look like if I, as a user, try to install a Cask where the URL is nil.

2. The on_* blocks should automatically update some sort of MacOSRequirement so that this workaround with depends_on macos: isn't necessary inside each block anymore.

Agreed 👍🏻

@EricFromCanada
Copy link
Member

what's it look like if I, as a user, try to install a Cask where the URL is nil

If I remove the on_mojave :or_older block I just added:

$ brew install --cask calhash
Error: attempted to use a Downloadable without a URL!
/usr/local/Homebrew/Library/Homebrew/downloadable.rb:75:in `downloader'
/usr/local/Homebrew/Library/Homebrew/downloadable.rb:87:in `fetch'
/usr/local/Homebrew/Library/Homebrew/cask/download.rb:53:in `fetch'
/usr/local/Homebrew/Library/Homebrew/cask/installer.rb:167:in `download'
/usr/local/Homebrew/Library/Homebrew/cask/installer.rb:70:in `fetch'
/usr/local/Homebrew/Library/Homebrew/cask/installer.rb:99:in `install'
/usr/local/Homebrew/Library/Homebrew/cmd/install.rb:244:in `block in install'
/usr/local/Homebrew/Library/Homebrew/cmd/install.rb:233:in `each'
/usr/local/Homebrew/Library/Homebrew/cmd/install.rb:233:in `install'
/usr/local/Homebrew/Library/Homebrew/brew.rb:94:in `<main>'

@EricFromCanada
Copy link
Member

As suggested, I moved the list of macOS requirements into an array in Homebrew/homebrew-cask#154556. It passes CI, but this is what an unsupported system release sees:

$ brew info --cask calhash
==> calhash: 
https://www.titanium-software.fr/en/calhash.html
...

$ brew fetch --cask calhash
brew fetch --cask calhash
Error: Parameter 'val': Expected type T.any(PkgVersion, String, Version), got type NilClass
Caller: /usr/local/Homebrew/Library/Homebrew/cask/download.rb:39
Definition: /usr/local/Homebrew/Library/Homebrew/version.rb:492
...

$ brew install --cask calhash
Error: This software does not run on macOS versions other than Catalina, Big Sur, Monterey and Ventura.

$ brew audit --skip-style --except=version homebrew/cask/calhash
audit for calhash: failed
 - a version stanza is required
 - a sha256 stanza is required
 - exception while auditing calhash: undefined method `latest?' for nil:NilClass
calhash
  * a version stanza is required
  * a sha256 stanza is required
  * exception while auditing calhash: undefined method `latest?' for nil:NilClass
Error: 3 problems in 1 cask detected.

@apainintheneck
Copy link
Contributor Author

Yeah, some of the audits need to be updated to handle nil urls and versions (especially since we already check for them elsewhere). Other than that we should really be checking for MacOSRequirements here. I'll have another PR up that will hopefully improve things.

@apainintheneck apainintheneck mentioned this pull request Sep 7, 2023
7 tasks
@github-actions github-actions bot added the outdated PR was locked due to age label Oct 8, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 8, 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

4 participants