Skip to content

Conversation

samford
Copy link
Member

@samford samford commented Apr 4, 2025

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

Casks now support an os DSL method, similar to arch. This makes it available in livecheck blocks, like we do with arch.

Besides that, this also adds tests for the version and arch delegate methods in the livecheck block DSL. This doesn't affect test coverage but it ensures that the methods work as expected in livecheck blocks. For what it's worth, it was necessary to use Cask::Cask.new in these tests, as the delegate methods don't work when using the existing Cask::CaskLoader.load approach (for whatever reason).

Related to Homebrew/homebrew-cask#207471.

@samford samford force-pushed the livecheck/make-os-available branch from f9f7359 to 2aa6816 Compare April 4, 2025 03:43
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, thanks!

I got very excited and thought you provided a way of detecting which OS is supported by a Cask with a single method. If I can nerd-snipe you into doing that next: I'd be delighted 😉

samford added 3 commits April 4, 2025 09:17
Casks now support an `os` DSL method, similar to `arch`. This makes
it available in `livecheck` blocks, like we do with `arch`.
This adds the ability to specify tests that depend on a certain CPU
architecture using `:needs_arm` or `:needs_intel`, similar to the
existing `:needs_macos` and `:needs_linux` metadata for tests that
depend on a certain OS.
This adds tests for the livecheck DSL's `version` and `arch`
delegates. This doesn't affect test coverage but it ensures that the
methods work as expected in `livecheck` blocks.
@samford samford force-pushed the livecheck/make-os-available branch from 2aa6816 to 8d52d9b Compare April 4, 2025 13:18
Copy link
Member

@p-linnane p-linnane left a comment

Choose a reason for hiding this comment

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

Thanks for the super fast turnaround on this @samford! 😄

@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue Apr 4, 2025
@samford
Copy link
Member Author

samford commented Apr 4, 2025

The latest push reworks the arch/os tests to scope them to the supported arch/OS values, as the failure on "generic OS" CI made it clear that I need to handle this properly or it can fail in different environments. We already have :needs_macos and :needs_linux metadata but I had to add :needs_arm and :needs_intel to handle arch.


I got very excited and thought you provided a way of detecting which OS is supported by a Cask with a single method. If I can nerd-snipe you into doing that next: I'd be delighted 😉

Unfortunately, I think that will require some heavy lifting (per the latter part of #19689 (comment)). I don't think there's anything in the cask DSL that's guaranteed to explicitly provide that information based on how it's currently used.

os is only used in casks when we need to specify an OS-dependent string (e.g., darwin for macOS, linux for Linux) and it only includes the minimum arguments that are necessary. The existing examples all have macos and linux arguments but neither is guaranteed (e.g., it's unlikely but if an upstream project only used a suffix for macOS or Linux in the filename but not both, the os call would only include one OS or the other), similar to how arch isn't guaranteed to have both arm and intel values (and it's omitted entirely for casks that are x86_64-only or universal).

depends_on is missing from some macOS-only casks and the return value defaults to the oldest allowed macOS version, so it's not reliable information either. It's also omitted for casks that don't depend on a certain OS (e.g., fonts). Since some omissions are intentional and others are unintentional, we can't reliably make assumptions around depends_on.

If we add depends_on macos to all of the macOS-only casks (and somehow ensure that it's always used for OS-specific casks in the future) and make depends_on not return anything if it's not used in a cask, that may allow us to make assumptions about what OSs a cask supports (i.e., macOS-only if depends_on macos is present, otherwise all [Homebrew-supported] OSs apply). I don't think we have any Linux-only casks yet but depends_on should probably be expanded to add Linux support, though it would probably have to be something like depends_on linux: true to indicate generic Linux support (unless a cask requires a specific distribution/version).

Long story short, I probably have too much on my plate already at the moment but I would be excited if someone else picked it up. I can create an issue (in brew or homebrew-cask) if that would help.

I think making minimum macOS dependencies (i.e., oldest allowed) explicit instead of implicit would get us most of the way there (and it's arguably the right thing to do with casks not being macOS-only now). No promises but I'll try to dig into it when I have time (unless someone wants to beat me to it).

Merged via the queue into master with commit 4c56360 Apr 4, 2025
36 checks passed
@MikeMcQuaid MikeMcQuaid deleted the livecheck/make-os-available branch April 4, 2025 14:44
@MikeMcQuaid
Copy link
Member

Unfortunately, I think that will require some heavy lifting (per the latter part of #19689 (comment)). I don't think there's anything in the cask DSL that's guaranteed to explicitly provide that information based on how it's currently used.

I think the way @on_system_blocks_exist is used my provide a path here; this could set @supports_linux, @supports_arm too in a similar place.

@samford
Copy link
Member Author

samford commented Apr 4, 2025

I think the way @on_system_blocks_exist is used my provide a path here; this could set @supports_linux, @supports_arm too in a similar place.

That would handle explicit indicators but, as mentioned, we have casks that support an arch/os without any explicit indication.

We can better identify supported OSs if we:

  • Add support for depends_on :macos and depends_on :linux to casks, dev-cmd/generate-cask-ci-matrix.rb, etc.
  • Add depends_on :macos to macOS casks that use the implicit macOS dependency (i.e., >= oldest allowed macOS) and remove the implicit logic, so cask.depends_on[:macos] is only present if the cask explicitly depends on macOS. [To be clear, depends_on :macos would use the same oldest-allowed-macOS requirement as the current implicit logic.] We would probably have to figure out how to handle the migration for third-party casks (and/or save the removal for a notable brew version), though.

That setup would technically allow us to assume that the absence of a depends_on OS value means that the cask isn't OS-dependent (e.g., fonts). However, we can only rely on this assumption for first-party casks, so I think we're just going to have to require casks to always explicitly specify which OSs are supported or if the cask is OS-independent (e.g., fonts). depends_on could be adapted to support the former (though it may be somewhat awkward when specifying multiple OSs) but I'm not sure about the latter (maybe something like depends_on os: :any?).

Arch is harder because we can't rely on arch/on_arm/on_intel to reliably indicate what architectures are supported, as the absence of a block/call doesn't mean lack of support. depends_on arch is the only reliable indicator and that's currently only used when a cask supports one architecture but not another (e.g., depends_on arch: :arm64 for whisky). I'm not sure how to resolve that other than requiring casks to explicitly specify supported archs in depends_on arch.

[Side note: I'm working on a PR to modify bump-cask-pr to respect depends_on arch values. It's pretty much done, I'm just testing/tweaking now.]

@MikeMcQuaid
Copy link
Member

That would handle explicit indicators but, as mentioned, we have casks that support an arch/os without any explicit indication.

I think at least handling those with explicit indicators better would be a good starting point.

CC @SMillerDev for thoughts on the above

@SMillerDev
Copy link
Member

I think at least handling those with explicit indicators better would be a good starting point.

That would be my starting point as well. The only ones that are not explicitly for a single platform are, in order of popularity: font, artifact and the shell completions.

In my mental model you can therefore assume that: if a cask contains something other then those and has no os stanza, it must only support a single OS. With the history of casks I think we can safely assume that it only supports macOS in that case.

@samford
Copy link
Member Author

samford commented Apr 7, 2025

I'm working on providing more granular booleans for on_system usage, so we will be able to tell if a formula/cask uses OS- and arch-specific on_system calls. The general idea is that we can set specific boolean values (for ARM, Intel, macOS, and Linux) in place of @on_system_blocks_exist. If we need to know if any on_system calls were made we can simply check if any of those booleans were set.

I'm currently approaching this as a class (with an any? method), so we can replace @on_system_blocks_exist with an object instead of having to manage a bunch of boolean instance variables in both formulae and casks. I should have a draft PR up with more details after I work through some technical hurdles.

Edit: PR can be found at #19721

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants