Conversation
7dc3be9 to
881e9ce
Compare
There was a problem hiding this comment.
Pull request overview
Updates Homebrew Cask’s Linux-support detection so casks that use on_linux/on_macos blocks (but no explicit os stanza) can still be recognized as Linux-supported by simulating a Linux load and checking whether any artifacts are produced.
Changes:
- Enhance
Cask::Cask#supports_linux?to simulate Linux loading when OS blocks exist butosstanza isn’t present. - Add a new fixture cask exercising
on_macos+on_linuxartifacts and extend thesupports_linux?spec accordingly. - Change
generate-cask-ci-matrixto defaultGITHUB_REPOSITORYtohomebrew/cask.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Library/Homebrew/cask/cask.rb | Adds Linux simulation fallback logic for OS-block-based casks without an os stanza. |
| Library/Homebrew/test/support/fixtures/cask/Casks/with-os-blocks.rb | New fixture cask with on_macos and on_linux blocks producing artifacts. |
| Library/Homebrew/test/cask/cask_spec.rb | Adds an assertion that the new OS-block fixture reports Linux support. |
| Library/Homebrew/dev-cmd/generate-cask-ci-matrix.rb | Defaults GITHUB_REPOSITORY env var value. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
881e9ce to
28b8451
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks @bevanjkay! Appreciate this but think probably going down the wrong approach here.
I think @samford just added depends_on :macos to everything in homebrew/cask?
I think we should work with them and instead aim to reduce and remove all this logic around specific artefact checks and instead the # Bare depends_on :macos explicitly marks a cask as macOS-only logic above should be sufficient.
If anything, let's just add cask audits for casks using invalid artefacts for the OS.
|
In that case I think we're looking at fairly large overhaul of the logic that determines if Linux is supported. Essentially Linux is supported unless we say that it isn't? The same as how it works on For casks with more complicated artifact sets we are presently checking for the presence of the But if now is the time to hoist and reverse this logic then I'm ok with that. |
Yes, that's the goal here.
Yes, I think so. In hindsight: we should have done this in the first place. @samford has now done enough of the manual work here that I think we should be well placed to do how we should have done in the first place. |
on_linuxsupports_linux?
28b8451 to
9f196ca
Compare
|
I have simplified the logic significantly. Semi-relatedly, at the moment the below are seen as mutually exclusive; and result in an error; However I think we need to be able to set both declarations - one for macos dependency, and one for minimum system requirement. A cask that is available on linux may still require a minimum macos version, so we need to be able to apply it there also. |
I agree we semantically need both but I think specifying both is pretty ugly so it'd be worth figuring out some other syntax for this like |
|
@bevanjkay I'd suggest adding to this PR an audit that uses the existing invalid artefacts logic and fails on any cask that doesn't specify |
brew lgtm(style, typechecking and tests) with your changes locally?I used
claude-codeto help debug the issue and implement the functionality.Cask.supports_linux?previously returned false for casks that useon_linux do / on_macos doblocks without an explicitosstanza (e.g. winbox) https://github.com/Homebrew/homebrew-cask/pull/261129/changes.When
on_os_blocks_exist?is true but noosstanza value is present, this PR adds the functionality to simulate loading the cask on Linux and return true if either produces any artifacts.