livecheck: allow inheriting from a formula/cask

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

---

I took a brief detour today to expand the livecheck block DSL to allow us to inherit livecheck information from another formula/cask (e.g., url, regex, etc.). This should work as expected for referenced formulae/casks regardless of whether they contain a livecheck block or simply use livecheck's default.

For example, we have various formulae in homebrew/core that use the same check as sqlite (e.g., dbhash, lemon, sqldiff, sqlite-analyzer) and we simply duplicate these livecheck blocks. If a change is made to the sqlite formula's livecheck block, it has to be manually replicated in each of the related formulae. Ideally, a change in the sqlite livecheck block would automatically be reflected in the related formulae and that's part of the goal of this feature. If/when this feature is merged, the duplicate livecheck blocks can be replaced with:

livecheck do
  formula "sqlite"
end

The way this is implemented, livecheck values that are inherited from another formula/cask (e.g., regex) can be overridden by supplying a value in the livecheck block. For example, you can inherit a url, strategy block, etc. from another formula/cask and override only the regex:

livecheck do
  formula "other-formula"
  regex(/href=.*?something[._-]v?(\d+(?:\.\d+)+)\.t/i)
end

---

One thing to note, I've implemented this such that it should correctly handle recursive situations. For example, a formula/cask contains a livecheck block that references another formula/cask which contains a livecheck block that references another formula/cask, etc. In this situation, livecheck will resolve the links to the final formula/cask and use that as the referenced formula/cask.

The debug output prints the references like so (where a2ps is using formula "a52dec" and a52dec is using cask "0-ad"):

Formula:          a2ps
Livecheckable?:   Yes
Formula Ref:      a52dec
Cask Ref:         0-ad

URL:              https://play0ad.com/download/mac/
Strategy:         PageMatch
Regex:            /href=.*?0ad[._-]v?(.*?)-osx64\.dmg/i

Matched Versions:
0.0.24b-alpha
a2ps : 4.14 ==> 0.0.24b-alpha

The references are also surfaced in the verbose JSON output like so:

[
  {
    "formula": "a2ps",
    "version": {
      "current": "4.14",
      "latest": "0.0.24b-alpha",
      "outdated": false,
      "newer_than_upstream": true
    },
    "meta": {
      "livecheckable": true,
      "references": [
        {
          "formula": "a52dec"
        },
        {
          "cask": "0-ad"
        }
      ],
      "url": {
        "original": "https://play0ad.com/download/mac/"
      },
      "strategy": "PageMatch",
      "strategies": [
        "PageMatch"
      ],
      "regex": "/href=.*?0ad[._-]v?(.*?)-osx64\\.dmg/i"
    }
  }
]

---

I'm working on another part of livecheck at the moment but this has been an itch that I've wanted to scratch for a while and it came up again today, so I figured I would finally take care of it.

Looking forward, I intend to factor this information into future caching behavior (i.e., avoiding multiple checks to the same source in a given run) when I finish implementing that. There are a few other things in the pipeline before that but I figured I would mention it in case someone gets a similar idea after looking at this.

Lastly, I've disabled the Metrics/ModuleLength rubocop for the Livecheck module and I've disabled Metrics/BlockLength in a couple areas. These changes pushed these areas just over the limits and I don't think it makes sense to refactor livecheck in this PR. I intend to do some refactoring in the future to address this situation but I don't think the length cops should hold us up in this PR.

[BrewTestBot]

Review period will end on 2021-07-20 at 16:06:02 UTC.

[Rylan12]

This is neat!

Just one thing that popped in my head: we should probably check for circular references here. That is, A uses B's livecheck and B uses A's livecheck. Probably just a matter of checking that at no point the next formula/cask to look at recursively is the initial one.

If you've done this already and I just missed it my apologies

[samford]

Just one thing that popped in my head: we should probably check for circular references here. That is, A uses B's livecheck and B uses A's livecheck. Probably just a matter of checking that at no point the next formula/cask to look at recursively is the initial one.

Ah, good call. This started out pretty simple but it gradually became more complex as I thought of situations that needed to be addressed. I missed that one, so I appreciate the reminder.

It's getting a bit late over here but I'll try to work on updating this to account for more unusual situations tomorrow.

[BrewTestBot]

Review period ended.

[samford]

The latest push now handles circular dependencies and, as a nice side effect, can also print the full chain of references in the debug output (not just formula/cask reference in the original livecheck block).

Some things worth noting:

• The return type of #resolve_livecheck_reference uses T.untyped as the array value (i.e., T.nilable(T::Array[T.untyped])) because Sorbet doesn't seem to understand array dereferencing. That is to say, when Sorbet encountered livecheck_formula_or_cask, livecheck_references = resolve_livecheck_reference(...), it thinks that livecheck_formula_or_cask is an array, not the first value from the array. I couldn't find a way to make Sorbet properly understand this and using T.untyped basically tells it not to bother in this particular area.
• I keep track of original_formula_or_cask separately in #resolve_livecheck_reference, since we want to keep it out of the references array but still need it catch a circular reference back to the original formula/cask. Originally, I had the starting formula/cask as the first member of references and had some logic where we tracked the depth of the recursion and used drop(1) to remove the first reference on the final return (i.e., depth.zero?) but I felt like this was harder to reason about.
• SkipConditions for a referenced formula/cask are treated differently than normal. Only errors and a skip message found in the referenced formula/cask's livecheck block are treated normally. All the other conditions are treated as an error, as we shouldn't reference a formula/cask that's automatically skipped (e.g., due to being deprecated, disabled, discontinued, etc.). I created a SkipConditions#referenced_skip_information method for this logic, as it also needs to be used in brew bump.

I'll probably take another pass at this once my brain's less tired, in case there's more room for improvement or anything that can be tidied up. That said, it's probably functionally complete at this point, so feel free to review.

[bevanjkay]

@samford What happens if the parent reference is in a formula/cask that is untapped?

[samford]

What happens if the parent reference is in a formula/cask that is untapped?

I'm not sure what "parent reference" would entail in this context, as I don't think of this as a parent/child relationship. Are you thinking of the parent as the formula/cask that's making the reference or the formula/cask that's referenced by formula/cask in a livecheck block?

If we're talking about a simple situation where formula A references formula B but formula B is untapped, then that will give a TapFormulaOrCaskUnavailableError when livecheck tries to load formula B (i.e., a 'No available formula with the name "x/y/z"' error along with the message "Please tap it and then try again: brew tap x/y"). This error only affects the check for formula A (i.e., brew livecheck will continue to check other formulae/casks), so I think this is fine. I don't think automatically tapping a missing tap to allow the check to work would be desirable (i.e., the error tells users how to fix the issue and it should be up to them whether they want to tap something).

I plan to add some documentation in this PR and I think the guidance should be that keeping formula/cask references within a tap will give the best results. It's technically possible right now to reference a formula/cask across taps but, as mentioned, it will give an error if the user doesn't have a given tap tapped. There's at least one formula in homebrew/core where we could potentially use a related cask's check but, with this in mind, I'm not sure that it's worth doing and it may be better to avoid establishing that precedent. I don't know if we want to explicitly disable cross-tap referencing (third-party taps may want to do it) but we could probably create an audit to prevent it in first-party taps, if necessary. It's something to think about, at least.

If you're instead talking about running brew livecheck on untapped formulae/casks, this is unsupported and isn't how it's made to be used. brew livecheck needs formulae/casks to be tapped to do certain things (e.g., use tap strategies) and this formula/cask feature also won't work with untapped formulae/casks. This isn't something that I intend to support, as ensuring a tap is tapped is very easy (either using brew tap or symlinking a local tap directory into Taps).

If I've misunderstood your question, just let me know and I can explain further.

[bevanjkay]

@samford Thanks I should have been clearer. You've answered perfectly. I was referring to an instance where the referenced formula #{token} is in an untapped tap.

[Rylan12]

I just took a look, and I think this looks good. This is also the first time I've looked through the livecheck codebase so I'm definitely not super familiar.

The only suggestion I have that I mentioned below is that I think the naming of the variables like formula_or_cask and livecheck_formula_or_cack are a bit confusing. I often found it hard to remember what was stored in each variable when reading the code. If the names could be clarified to something like original_formula_or_cask or current_formula_or_cask (or something that better describes what is being stored) I think the code would be a bit easier to read.

[samford]

The only suggestion I have that I mentioned below is that I think the naming of the variables like formula_or_cask and livecheck_formula_or_cask are a bit confusing. I often found it hard to remember what was stored in each variable when reading the code. If the names could be clarified to something like original_formula_or_cask or current_formula_or_cask (or something that better describes what is being stored) I think the code would be a bit easier to read.

Outside of #resolve_livecheck_reference, formula_or_cask is simply the current formula/cask in the main #run_checks loop. #resolve_livecheck_reference may benefit from a more explicit name for formula_or_cask but I'm not sure it would be worth the additional verbosity outside of that method.

#resolve_livecheck_reference contains formula_or_cask, first_formula_or_cask, referenced_formula_or_cask, and next_referenced_formula_or_cask variables, so I can understand that there may be some confusion. Would current_formula_or_cask be more clear in that context or do you think formula_or_cask is fine with the recent renaming of livecheck_formula_or_cask to referenced_formula_or_cask?

I also added some more comments to that method that may help to make the behavior a little more clear and that may help to explain the variables a bit. If it's still unclear, I can take another pass at it.

[Rylan12]

Nice! Yeah, I think it's much clearer now!

[Rylan12]

Think an audit for this is needed or is just knowing that it will error on CI good enough? Think I'm fine either way

[samford]

I tried to create an audit for it but I'm not sure we can load a formula/cask in the context of a Rubocop. I don't have much experience with creating audits but my in-progress audit encounters an undefined method `metadata' for nil:NilClass error when run on a formula using formula/cask in a livecheck block:

# This cop ensures that the argument to `#formula` or `#cask` only
# references a formula/cask in the same tap.
#
# @api private
class LivecheckReferenceSameTap < FormulaCop
  def audit_formula(_node, _class_node, _parent_class_node, body_node)
    tap_name = formula_tap
    return unless tap_name == "homebrew-core"

    livecheck_node = find_block(body_node, :livecheck)
    return if livecheck_node.blank?

    formula_node = find_every_method_call_by_name(livecheck_node, :formula).first
    formula_node_string = find_strings(formula_node).first if formula_node
    livecheck_formula = string_content(formula_node_string) if formula_node_string

    cask_node = find_every_method_call_by_name(livecheck_node, :cask).first
    cask_node_string = find_strings(cask_node).first if cask_node
    livecheck_cask = string_content(cask_node_string) if cask_node_string

    # Load the referenced formula or cask
    formula_or_cask, method_name, plural_name = if livecheck_formula.present?
      [Formulary.factory(livecheck_formula), "formula", "formulae"]
    elsif livecheck_cask.present?
      [Cask::CaskLoader.load(livecheck_cask), "cask", "casks"]
    end
    return unless formula_or_cask

    # Follow the formula/cask reference
    referenced_formula_or_cask, livecheck_references =
      Homebrew::Livecheck.resolve_livecheck_reference(formula_or_cask)

    # Compare the tap of each referenced formula/cask to formula_tap
    livecheck_references.each do |ref_formula_or_cask|
      if ref_formula_or_cask&.tap&.full_name&.split("/")&.last != tap_name
        offending_node(livecheck_node)
        problem "`#{method_name}` should only reference #{plural_name} in #{tap_name}."
      end
    end
  end
end

It would be nice to get this working but I'm stuck here at the moment and haven't dug into it further. In the interim time, I updated the other livecheck audits with respect to the new formula/cask methods.

---

homebrew/core CI's livecheck test would fail on a reference to a different tap, so that may be fine for now.

homebrew/cask CI taps both cask and core, so a reference to a homebrew/core formula in a tap wouldn't fail in that context. However, the audits we have for livecheck blocks don't apply to casks yet anyway (they're FormulaCops), so this wouldn't change the status quo. I haven't looked into how we may be able to share audits for something that's used in both formulae/casks (e.g., livecheck blocks) but it's on my to-do list (along with bringing livecheck blocks in casks up to standard).

[Rylan12]

Would current_formula_or_cask be more clear in that context or do you think formula_or_cask is fine with the recent renaming of livecheck_formula_or_cask to referenced_formula_or_cask?

I think it's fine as it is currently

[samford]

The latest push adds tests for SkipConditions#referenced_skip_information and a basic test for Livecheck#resolve_livecheck_references. Creating real tests for #resolve_livecheck_reference will require test formulae/casks in a tap. It looks like there's some prior art with the cask tests and the test/support/fixtures/third-party tap but it may take a bit of work to sort out something similar.

#formula/#cask can't be used in a livecheck block until these commits are in a brew release and that probably won't happen until next Monday, so I may try to expand the tests for #resolve_livecheck_references in the interim time.

[samford]

The latest push simply rebases this on the current master branch, as I'm going to go ahead and merge this to make sure it's in the next tagged brew version. I'll revisit the aforementioned #resolve_livecheck_references testing setup in a follow-up PR when I have a chance and figure out how to make it work.