New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
livecheck: allow inheriting from a formula/cask #11743
livecheck: allow inheriting from a formula/cask #11743
Conversation
9f4dcea
to
9aeae58
Compare
Review period will end on 2021-07-20 at 16:06:02 UTC. |
d9bb228
to
7d71dde
Compare
3af4be7
to
e9ea1e8
Compare
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 If you've done this already and I just missed it my apologies |
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. |
Review period ended. |
e9ea1e8
to
0ad5825
Compare
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 Some things worth noting:
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. |
@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 If we're talking about a simple situation where formula A references formula B but formula B is untapped, then that will give a 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 If I've misunderstood your question, just let me know and I can explain further. |
@samford Thanks I should have been clearer. You've answered perfectly. I was referring to an instance where the referenced |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
ac1dcc9
to
1e58ba2
Compare
Outside of
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Yeah, I think it's much clearer now!
end | ||
``` | ||
|
||
The referenced formula/cask should be in the same tap, as a reference to a formula/cask from another tap will generate an error if the user doesn't already have it tapped. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 FormulaCop
s), 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).
I think it's fine as it is currently |
e9c1d44
to
8b29a6b
Compare
The latest push adds tests for
|
8b29a6b
to
0bc3d6c
Compare
The latest push simply rebases this on the current |
brew style
with your changes locally?brew typecheck
with your changes locally?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 alivecheck
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 theselivecheck
blocks. If a change is made to thesqlite
formula'slivecheck
block, it has to be manually replicated in each of the related formulae. Ideally, a change in thesqlite
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 duplicatelivecheck
blocks can be replaced with: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 thelivecheck
block. For example, you can inherit aurl
,strategy
block, etc. from another formula/cask and override only theregex
: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 alivecheck
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 usingformula "a52dec"
anda52dec
is usingcask "0-ad"
):The references are also surfaced in the verbose JSON output like so:
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 theLivecheck
module and I've disabledMetrics/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.