Cask::DSL::DependsOn: allow :macos and version#22146
Conversation
The `binary` call argument in the `with-depends-on-macos-bare` cask interpolates `appdir` but the cask doesn't have a preceding `app` call to install the app, so this produces an error when attempting to install the cask. This updates the `binary` argument to interpolate `staged_path` instead, which should work as expected. Besides that, the binary at this path is `Caffeine` rather than `caffeine`.
There was a problem hiding this comment.
Pull request overview
This PR updates the Cask depends_on DSL to allow combining bare depends_on :macos (marking a cask as macOS-only) with a versioned depends_on macos: ... requirement, and adjusts Linux-support detection to use an explicit macOS-only indicator.
Changes:
- Allow
depends_on :macosto coexist withdepends_on macos: ">= :catalina"without raising a duplicatedepends_on macoserror. - Track an explicit
@requires_macosflag and expose it viaDependsOn#macos?, updatingCask::Cask#supports_linux?to use it. - Extend tests/fixtures to cover the combined bare+version case and adjust an existing fixture’s
binarypath.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Library/Homebrew/cask/dsl/depends_on.rb | Alters macos= behavior and adds macos? flagging for macOS-only casks. |
| Library/Homebrew/cask/cask.rb | Switches Linux support detection to rely on depends_on.macos?. |
| Library/Homebrew/test/cask/dsl_spec.rb | Adds spec coverage for bare :macos alongside a version requirement. |
| Library/Homebrew/test/cask/depends_on_spec.rb | Adds install-time regression coverage for bare :macos and bare+version. |
| Library/Homebrew/test/support/fixtures/cask/Casks/with-depends-on-macos-bare.rb | Updates the fixture’s binary target path. |
| Library/Homebrew/test/support/fixtures/cask/Casks/with-depends-on-macos-bare-and-comparison.rb | Adds a new fixture cask combining :macos and a version comparison. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Currently `Cask::DSL::DependsOn.macos=` will raise an "Only a single 'depends_on macos' is allowed" error if a cask uses `depends_on :macos` and specifies a macOS requirement like `depends_on macos: ">= :catalina"`. These shouldn't be mutually exclusive, as the former is used to specify that a cask is macOS-only and the latter is used to specify a macOS version requirement and that can be used in casks that also support Linux (i.e., it implies macOS support but not macOS exclusivity). Since `depends_on macos:` can be used in casks that support both macOS and Linux, we also need to add `depends_on :macos` to casks that are macOS-only, like we recently did for casks that don't use `depends_on macos:`. To make that possible, this updates `Cask::DSL::DependsOn.macos=` to only raise this error if `depends_on macos:` is called with a version and a version has already been specified. I've set this up so that calling `depends_on :macos` won't overwrite an existing `@macos` value. Similarly, calling `depends_on macos:` with a version requirement will overwrite a blank `MacOSRequirement`, so `depends_on.macos` will reflect the macOS version requirement. This should work regardless of whether `depends_on :macos` comes before or after `depends_on macos:`. However, `Cask::Cask.supports_linux?` uses a false `version_specified?` value as the indicator that a cask uses `depends_on :macos` and this won't be true if a cask also uses `depends_on macos:`. To address this issue, this also modifies `Cask::DSL::DependsOn.macos=` to set a `@requires_macos` variable to true when `depends_on :macos` is used, so we have a stable indicator that we can rely on. I've also added a `macos?` method that simply returns the `@requires_macos` value and I've updated `supports_linux?` to use that in the related condition instead.
samford
force-pushed
the
cask-depends_on-allow-macos-and-version
branch
from
f02ecd3 to
667a299
Compare
MikeMcQuaid
left a comment
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Sorry, thanks for the work but I don't think this is the approach we want right now.
| depends_on :macos | ||
| depends_on macos: ">= :catalina" |
There was a problem hiding this comment.
I think this is pretty ugly and we should avoid this syntax. Instead, as mentioned before, I think we should aim to equalise on the formulae format. If we need a , required: true or something in the interim: that's fine.
There was a problem hiding this comment.
I think we should aim to equalise on the formulae format
Could you be more specific? As described in #22080 (comment), separate depends_on :macos and depends_on macos: calls is how formulae handle this situation (e.g., as in age-plugin-se). depends_on :macos indicates that a package is macOS-only and depends_on macos: only indicates the macOS version requirement (i.e., it doesn't serve the same purpose as :macos). If I'm overlooking something that formulae do differently, let me know.
The notable difference between the two is that casks use depends_on macos: ">= :catalina" and formulae use depends_on macos: :catalina to indicate the same "Catalina or higher" macOS requirement but that's not relevant to the issue. Aligning the macos: value format between formulae and casks is a good goal but not required here.
What I suggested in the other PR is to allow the :macos + macos: setup as a way of being able to denote casks that are mac0S-only when they also use depends_on macos: "...". This is something that we need to do before moving forward in other areas, as the status quo of only adding depends_on :macos to casks without depends_on macos: "..." isn't sufficient (i.e., folks seem to think the work is entirely done but that's not the case).
As mentioned in the linked comment, alternative approaches that use one depends_on call with multiple arguments come with implementation issues. If you're suggesting something like depends_on macos: ">= :catalina", required: true, Cask::DSL::DependsOn would need to be reimplemented in a different way, as the required value wouldn't be treated as specific to macOS (and something like macos_required: true would overlap with the purpose of depends_on :macos). It also complicates the mental model, as a cask using depends_on :macos where brew audit surfaces a macOS requirement would require removing depends_on :macos and replacing it with depends_on macos: ">= :catalina", required: true, rather than simply adding a separate depends_on macos: "..." call. Regardless, a multi-argument approach would be a lot more work (and would require similar changes to formulae), which is why I'm suggesting that we use the existing formula approach of :macos + macos: for now and work on potential improvements to both the formula and cask DSL afterward.
This approach works and aligns with how formulae currently handle this, so I took your previous "Makes sense to me" in the supports_linux? PR as saying that moving forward with this makes sense and we can work on better aligning formula/cask depends_on macos: values and other improvements afterward.
There was a problem hiding this comment.
The notable difference between the two is that casks use
depends_on macos: ">= :catalina"and formulae usedepends_on macos: :catalinato indicate the same "Catalina or higher" macOS requirement but that's not relevant to the issue.
If we're changing things: I think that would be good to keep in mind, if nothing else as a North Star here.
Regardless, a multi-argument approach would be a lot more work (and would require similar changes to formulae)
It may be more work but we should do that work to get to a better outcome. Let's not copy the subpar formula experience for casks but figure out the best version for both and implement that for casks first and migrate formulae after.
This approach works and aligns with how formulae currently handle this, so I took your previous "Makes sense to me" in the
supports_linux?PR as saying that moving forward with this makes sense and we can work on better aligning formula/caskdepends_on macos:values and other improvements afterward.
Apologies for confusion here. I don't think it makes sense to add new functionality now for casks that we don't want to have in the long-term in formulae or casks.
brew lgtm(style, typechecking and tests) with your changes locally?Currently
Cask::DSL::DependsOn.macos=will raise an "Only a single 'depends_on macos' is allowed" error if a cask usesdepends_on :macosand specifies a macOS requirement likedepends_on macos: ">= :catalina". These shouldn't be mutually exclusive, as the former is used to specify that a cask is macOS-only and the latter is used to specify a macOS version requirement and that can be used in casks that also support Linux (i.e., it implies macOS support but not macOS exclusivity). Sincedepends_on macos:can be used in casks that support both macOS and Linux, we also need to adddepends_on :macosto casks that are macOS-only, like we recently did for casks that don't usedepends_on macos:.To make that possible, this updates
Cask::DSL::DependsOn.macos=to only raise this error ifdepends_on macos:is called with a version and a version has already been specified. I've set this up so that callingdepends_on :macoswon't overwrite an existing@macosvalue. Similarly, callingdepends_on macos:with a version requirement will overwrite a blankMacOSRequirement, sodepends_on.macoswill reflect the macOS version requirement. This should work regardless of whetherdepends_on :macoscomes before or afterdepends_on macos:.However,
Cask::Cask.supports_linux?uses a falseversion_specified?value as the indicator that a cask usesdepends_on :macosand this won't be true if a cask also usesdepends_on macos:. To address this issue, this also modifiesCask::DSL::DependsOn.macos=to set a@requires_macosvariable to true whendepends_on :macosis used, so we have a stable indicator that we can rely on. I've also added amacos?method that simply returns the@requires_macosvalue and I've updatedsupports_linux?to use that in the related condition instead.