OnSystem: Add UsesOnSystem class

[samford]

• 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 adds a UsesOnSystem class to OnSystem, containing boolean instance variables to indicate which types of on_system methods are used in a formula or cask. This is intended as a replacement for @on_system_blocks_exist, which doesn't allow us to determine what kinds of on_system calls were used. This provides more granularity but we can still use @uses_on_system.present? to determine whether any on_system calls were used (and this doubles as a nil check in Formula, as the self.class instance variable has to use a nilable type).

The UsesOnSystem instance variables cover the current ARCH_OPTIONS and BASE_OS_OPTIONS. UsesOnSystem also provides a macos_requirements set, which is used to keep track of macOS requirements that are specified in on_system and on_<macos_version> method calls (e.g., on_system :linux, macos: :ventura_or_newer, on_monterey :or_older, on_ventura, on_sonoma :or_newer). This can be used to identify macOS dependency variations in a way that doesn't require simulating different OSs when loading the package.

As a practical example, if you wanted to determine whether a cask uses Linux on_system calls, you can call cask.uses_on_system.linux?. The linux boolean will be true if the cask has an on_linux block, an on_system block (which requires Linux), or uses os linux: .... This is something that would be challenging to determine from outside of OnSystem but it's relatively easy to collect the information in OnSystem methods and make it available like this.

---

Edit: I resolved the Method uses_on_system does not exist on Cask::Cask error by running brew typecheck --update and committing the RBI change (it suddenly occurred to me while I was doing something completely unrelated 😆).

[MikeMcQuaid]

This is great, a much improved API, nice work @samford!

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[samford]

Finally coming back to this. The latest push:

• Adds a UsesOnSystem.macos_requirements property and uses it to collect macOS requirements from macOS conditions specified in on_system and on_<macos_version> method calls (e.g., on_system :linux, macos: :ventura_or_newer, on_monterey :or_older, on_ventura, on_sonoma :or_newer). This can be used to identify macOS dependency variations in a way that doesn't require simulating different OSs when loading the package.
• Adds a MacOSRequirement#highest_allowed method, so we can easily identify the highest supported macOS version that a requirement allows, if any. This can be used when producing a minimal set of supported macOS versions that satisfy UsesOnSystem.macos_requirements (e.g., when determining simulation OSs for bump-cask-pr). That is to say, we can do something like cask.uses_on_system.macos_requirements.filter_map { |req| req.highest_allowed&.to_sym }.uniq.
• Adds on_system_spec.rb, including some baseline tests to ensure some of those methods work when tested in isolation.
• Adds tests to cover changes in OnSystem, Formula, and Cask::DSL.
• Modifies OnSystem.os_condition_met? to return false if the current OS is any non-macOS value. Previously, this method only accounted for Linux and macOS values, so this method was giving an error when tested with a generic OS (i.e., running brew tests --generic). [This shortcoming was present before but it didn't cause an error until tests were added to exercise OnSystem.]
• Adds more type signatures to OnSystem and changes it to typed: strict.
• Adds more type signatures to MacOSRequirement, moving it closer to typed: strict. There are a few areas that aren't quite clear to me based on the code and existing tests but I did what I could.

This is another one of those times where writing tests took notably longer than the implementation 😆. There may be more room for improvement here but it should be ready for review at this point.

[samford]

One thing worth discussing is whether UsesOnSystem is the best name for this, considering that the properties are also set outside of OnSystem in Cask::DSL methods like sha256, arch, and os. Those are conceptually similar to OnSystem methods but not strictly the same thing, so UsesOnSystem isn't an entirely accurate name. Maybe something more generic like SystemConditionals would be more appropriate?

[MikeMcQuaid]

Looks good to me, nice work @samford!

I can see what you mean about the naming; uses_on_system feels maybe too close to uses_on_macos given the two are unrelated? I like on_system_conditional as you've suggested.

Don't feel strongly though! Happy to merge as-is.

[Rylan12]

This is great!

Agreed about the naming being a little funky. Another possibility might be to call this class OnSystemDeclarations, and then you can do formula.on_system_declarations.any? or .none? or .linux?, etc

[MikeMcQuaid]

Agreed about the naming being a little funky. Another possibility might be to call this class OnSystemDeclatations, and then you can do formula.on_system_declarations.any? or .none? or .linux?, etc

I like this 👍🏻

[samford]

Any objections to omitting On from the proposed alternative names? The issue I was trying to highlight is that referencing OnSystem in the name isn't entirely accurate because the Cask::DSL sha256, arch, and os methods also set UsesOnSystem values and those methods aren't from OnSystem. All of the methods that set the values deal with system conditionals, so a generic System<Something> name makes sense but naming it OnSystem<Something> advertises an idea that isn't always true. SystemDeclarations sounds like the best name so far (if we're fine with omitting On).

I'm happy to add #any?/#none? if we need/want them but they would need to accept an argument or block and I'm not sure if they would provide any value beyond #empty?/#present?. Did you have any specific use case in mind or is it simply a matter of any?/none? feeling more natural when it comes to wording?

[Rylan12]

I see your point on the On, and have no preference about whether it's included or not. SystemDeclarations or SystemConditionals both seem good to me.

they would need to accept an argument or block

Sorry, I'm not sure I understand exactly what you mean

Did you have any specific use case in mind or is it simply a matter of any?/none? feeling more natural when it comes to wording?

The latter. The reason I suggested #any? is that this new class is sort of like a collection of system conditionals. When I see the #any? method, I know intuitively that it is used on collection-like objects to indicate whether they contain any items.

On the other hand, when I see #present?, my intuition is that this method is being used to check whether the variable is nil or some other uninitialized value.

I don't feel strongly about this, though, as everyone's intuition may be different! This is just my own

[samford]

This may just be me but I tend to think of #any?/#none? in relation to Array and Hash, as they're the common classes with those methods, you can iterate over them using #each/#map, etc. I think Struct is also Enumerable but it doesn't have #any?/#none? or #each/#map methods and neither does Sorbet's T::Struct. It's possible to implement those methods but it's more involved than #empty?/#present? and I don't imagine we need the additional functionality.

My interpretation of #present? is "not false-y or empty". Extending that idea to this struct, I think of #present? as indicating that none of the properties are false-y or empty (as I've set them all to default to false or an empty array). [Viewing this another way, #present? could also be seen as checking whether any values have been modified from their default values, which may align with part of your idea of #present?.] Whether my implementation/intention is in line with the Rails documentation for #present? and #blank? is debatable, so I'm interested in hearing if this goes against others' mental model of #present?.

What I mean by "they would need to accept an argument or block" is that #any?/#none? can accept an argument (e.g., [nil, false, 0].any?(Integer), %w[bar baz bat bam].any?(/m/), {foo: 0, bar: 1, baz: 2}.any?(Array)) or a block (e.g., (1..4).any? {|element| element < 2 }, {foo: 0, bar: 1, baz: 2}.any? {|key, value| value < 1 }). These examples are pulled from the Enumerable#any? documentation, which also explains the behavior (e.g., without an argument/block, #any? checks for truthy values). It's possible to collect the property values into an array or hash and use #any?/#none? from those objects but I'm not sure if it's worth the effort or if having those methods would be appropriate for a struct.

That's just my perspective and my understanding may be lacking or incorrect, so I'm open to other views.

[MikeMcQuaid]

It's possible to collect the property values into an array or hash and use #any?/#none? from those objects but I'm not sure if it's worth the effort or if having those methods would be appropriate for a struct.

It should be (very) easy to implement Enumerable, as from the document you've linked:

• include Enumerable
• Implement method #each

The module would handle all of the rest which should require less work than present/blank/any/none/etc. manually.

[samford]

It should be (very) easy to implement Enumerable

One way to approach it looks like this (note that Sorbet has additional requirements for implementing Enumerable):

class UsesOnSystem < T::Struct
  include Enumerable

  extend T::Generic
  Elem = type_member(:out) do
    { fixed: T.any(T::Boolean, T::Set[MacOSRequirement]) }
  end

  # [Omitted property declarations and aliases...]

  # Returns a `Hash` of all non-`nil` instance variables, using `String` keys.
  sig { returns(T::Hash[String, Elem]) }
  def to_hash
    T.let(serialize, T::Hash[String, Elem])
  end

  # Returns a `Hash` of all non-`nil` instance variables, using `Symbol` keys.
  sig { returns(T::Hash[Symbol, Elem]) }
  def to_h = to_hash.transform_keys(&:to_sym)

  # Calls a provided block with each property value. If a block is not
  # provided, it returns a new `Enumerator` instead.
  sig {
    override.params(
      block: T.nilable(T.proc.params(arg0: Elem).returns(BasicObject)),
    ).returns(T.anything)
  }
  def each(&block)
    block ? to_hash.values.each(&block) : to_hash.values.each
  end

  # Whether the object has only default values.
  sig { returns(T::Boolean) }
  def empty? = none?

  # Whether the object has any non-default values.
  sig { returns(T::Boolean) }
  def present? = any?
end

I initially implemented #each to also provide property names when iterating values (like Hash#each_pair) but this leads to problems if you reimplement methods like #any? (see below), so I set this up to only iterate over values instead. [We could potentially collect property values in a different way than to_hash.values but I opted for brevity/simplicity to start with.]

This provides all of the Enumerable methods as expected but the trouble is that all?/any?/none?/one? simply check whether elements are truthy. This works for booleans but the default value for macos_requirements is an empty set and that's also truthy, so these methods don't achieve what I would expect in this context (i.e., checking for non-default values).

With that in mind, it seems like we have a couple of choices if we want to implement Enumerable:

• Reimplement the aforementioned methods from Enumerable to check if elements are present? instead of truthy [when a block isn't provided], as that also accounts for empty/blank values.
• Remove the default value for macos_requirements (so it's nil) and update any code that modifies the value to conditionally initialize the value before pushing new values (e.g., @uses_on_system.macos_requirements ||= Set[], similar to how we handle @uses_on_system).

Having to always conditionally initialize macos_requirements before it's modified isn't great but overriding the aforementioned methods also takes a wee bit of work:

sig {
  override.params(
    block: T.nilable(T.proc.params(arg0: Elem).returns(BasicObject)),
  ).returns(T::Boolean)
}
def all?(&block)
  block ? super : super(&:present?)
end

sig {
  override.params(
    block: T.nilable(T.proc.params(arg0: Elem).returns(BasicObject)),
  ).returns(T::Boolean)
}
def any?(&block)
  block ? super : super(&:present?)
end

sig {
  override.params(
    block: T.nilable(T.proc.params(arg0: Elem).returns(BasicObject)),
  ).returns(T::Boolean)
}
def none?(&block) = !any?(&block)

sig {
  override.params(
    block: T.nilable(T.proc.params(arg0: Elem).returns(BasicObject)),
  ).returns(T::Boolean)
}
def one?(&block)
  block ? super : super(&:present?)
end

Thoughts on this?

[MikeMcQuaid]

@samford Thanks for the example! I don't feel strongly either way given that; I defer to either using Enumerable or not depending on what works best for you.