docs: add brew livecheck documentation #7937
Conversation
|
Your documentation looks great @samford :) |
docs/Brew-Livecheck.md
Outdated
|
|
||
| 4. **Select a source**. After researching and comparing sources, decide which one is the best available option and use it as the `url` in the `livecheck` block. | ||
|
|
||
| 5. **Create a regex, if necessary or beneficial**. If the check works fine without a regex and wouldn't benefit from having one, it's fine to omit it. However, when a default check isn't working properly and we need to create a `livecheck` block, a regex is almost always necessary as well. More information on creating regexes can be found in the [regex guidelines](#regex-guidelines) section. |
There was a problem hiding this comment.
3, 4, and 5 feel more like handholding than documentation. I think we can trust contributors to figure out that they need to pick something after doing research. If we describe the locations you can look in and the fact that you don't always need a block people will figure out that they can do research.
There was a problem hiding this comment.
Agreed but I'd add 6 (as you're telling people to do 1 again). I think 1 and 2 cover this sufficiently.
There was a problem hiding this comment.
When I integrated all the feedback for this section, it was very short and I felt that it was a lot less useful for people new to livecheck. I've tried to find some middle ground between my original version and the requested feedback.
docs/Brew-Livecheck.md
Outdated
|
|
||
| * **Regexes should be case insensitive unless case sensitivity is explicitly required for matching to work properly**. Case insensitivity is enabled by adding the `i` flag at the end of the regex literal (e.g. `/.../i` or `%r{...}i`). This helps to improve reliability and reduce maintenance, as a case-insensitive regex doesn't need to be manually updated if there are any upstream changes in letter case. | ||
|
|
||
| * **Regexes should only use a capturing group around the part of the matched text that corresponds to the version**. For example, in `/href=.*?example-v?(\d+(?:\.\d+)+)(?:-src)?\.t/i`, we're only using a capturing group around the version part (matching a version like `1.2`, `1.2.3`, etc.) and we're using non-capturing groups elsewhere (e.g. `(?:-src)?`). This allows livecheck to rely on the first capture group being the version string. |
There was a problem hiding this comment.
Why would you need a second capture group?
There was a problem hiding this comment.
Alternatively: if the second capture group is unused anyway: why does this matter as long as the first one is the one matching the version?
There was a problem hiding this comment.
I've massaged this language a bit to assert that capturing groups should only be used around version text, as there seemed to be some confusion around the "first capture group" language.
Why would you need a second capture group?
In the overwhelming majority of cases, a regex only contains one capture group: the version text. We now have a small number of regexes that contain more than one capture group, where the regex is used within a strategy block to combine the captured text into one version (e.g., a cask version like 1.2.3, abcdefgh).
Alternatively: if the second capture group is unused anyway: why does this matter as long as the first one is the one matching the version?
It's one of those situations where it's fine until it's not. It's easier and more reliable to say, "Only use a capture group around the version text" instead of "you can use a capture group as long as it comes after all the capture group(s) for version text". The latter case would allow people to be lazy about this rule in most cases but it would come back to bite us.
For a practical example, let's say an upstream repository historically tags versions like 1.2.3. The standard regex for Git tags like this (/^v?(\d+(?:\.\d+)+)$/i) only has a capture group around the version, so it works fine. Over time, upstream switches to a release-1.2.3 tag format but they're not always reliable about it, so we end up with 1.2.3, release-1.2.4, 1.2.5, and release-1.2.6.
Since we have to match both of these tag formats, we would need to add an optional prefix at the start of the existing regex. If we're following the strict guideline, we would use /^(?:release[._-])?v?(\d+(?:\.\d+)+)$/i, which uses a non-capturing group around the optional release- part. This works as expected.
If we aren't strict about this rule, someone may naively use a regex like /^(release[._-])?v?(\d+(?:\.\d+)+)$/i, which would treat release- as the version. This release- version is technically newer than 1.2.5 (due to how Version comparison works) and this check would pass CI since livecheck technically found a version. If a maintainer doesn't manually test the livecheck block outside of CI (I always do) or check the CI log to catch this, it would be merged and break the check.
That said, I believe we have to be strict about this anyway to be able to enforce it using a RuboCop. From an implementation standpoint, I imagine this RuboCop will basically be "ensure there's only one capture group in the regex when there isn't a strategy block".
docs/Brew-Livecheck.md
Outdated
|
|
||
| * **Regexes should only use a capturing group around the part of the matched text that corresponds to the version**. For example, in `/href=.*?example-v?(\d+(?:\.\d+)+)(?:-src)?\.t/i`, we're only using a capturing group around the version part (matching a version like `1.2`, `1.2.3`, etc.) and we're using non-capturing groups elsewhere (e.g. `(?:-src)?`). This allows livecheck to rely on the first capture group being the version string. | ||
|
|
||
| * **Regexes should only match stable versions**. Regexes should be written to avoid prerelease versions like `1.2-alpha1`, `1.2-beta1`, `1.2-rc1`, etc. |
There was a problem hiding this comment.
Is there a regex sample for this?
There was a problem hiding this comment.
I think most of our existing regexes, except for those Formulae where homebrew-core allows pre-releases (such asrecode or vbindiff) satisfy this criterion.
For example, the regex /^v?(\d+(?:\.\d+)+)$/i, which we use when matching git tags avoids matching these pre-release versions. If this example is good enough, it could be added to the doc.
Even when we're looking for versions from a HTML page (an index page of all sources), we use something like /someformula[._-]v?(\d+(?:\.\d+)+)\.t/i where the file extension .t check serves this purpose.
There was a problem hiding this comment.
This seems like a poor fit for living in every regex and instead something that livecheck itself should know to strip out.
There was a problem hiding this comment.
For what it's worth, I removed this guideline from the most recent draft. How to avoid matching unstable versions depends on the context of a given formula/cask, so it can be hard to explain and may not be something that we can enforce through automation anyway.
This seems like a poor fit for living in every regex and instead something that livecheck itself should know to strip out.
We have some simple logic in Livecheck#latest_version that's intended to filter out unstable versions (logic here and the UNSTABLE_VERSION_KEYWORDS array here). However, this currently only applies when the formula/cask doesn't have a livecheck block. Some formulae/casks use an unstable version, so we have to be able to match unstable versions using a livecheck block.
In general, Version has some built-in handling of alpha/beta/pre/rc, so it seems like this should probably be something that Version detects and livecheck simply checks. We're not allowed to access the underlying tokens of a Version (see #7340), so this would require something like Version#stable? (and/or Version#unstable?), which would check if a version's tokens are one of these unstable types.
In general, I think we can improve the unstable version filtering logic but I don't think we can reasonably avoid the situation where regexes only match stable versions. Though I put forth the idea of only matching stable versions as a guideline, it's honestly just a side effect of the way we have to create regexes for other reasons.
When it comes to Git tags, we create a regex that only matches the desired format because we frequently have to filter out all sorts of unexpected/unrelated tags that are perpetually treated as newest (from the standpoint of Version comparison). These tags prevent the check from working properly and it's necessary to exclude them. To illustrate why we can't use a looser regex, I'll provide some examples.
Say we have a repository with a stable tag format like 1.2.3, unstable tags like 1.2.3-rc4, and a weird tag like 1.2.3-some-fix. If we use a regex that allows it to match unstable versions (i.e., not restricting the end), it will also match 1.2.3-some-fix and this will be treated as newest until a later version shows up (e.g., 1.2.4).
As another example, a number of repositories contain tags for several different pieces of software. Some will use the 1.2.3 tags for the main software in the repository and use a prefix for other versions, like something-5.6.7.
If we're loose about the start of the regex, then it would match tags like 1.2.3 as well as something-5.6.7. Since we only capture the version part, it will return a version that looks plausible (5.6.7) but is actually related to a different piece of software that's not used in the formula/cask. This particular type of problem is only addressed when someone says, "Hm, that doesn't look right", checks upstream, and corrects the issue using a livecheck block.
Basically, there are benefits to restricting the format of Git tags and this naturally filters out unstable versions in the process.
It's kind of a similar situation with file name regexes. If we have a regex like /href=.*?example[._-]v?(\d+(?:\.\d+)+)\.t/i, the trailing \.t is primarily intended to only match versions from tarballs. However, a side effect is that it implicitly excludes unstable versions, as it would match example-1.2.3.tar.gz but not example-1.2.4-rc1.tar.gz.
To match unstable versions, we would need to add something like [^"' >]*? at the end of the capturing group, like (\d+(?:\.\d+)+[^"' >]*?). This would match example-1.2.4-rc1.tar.gz and produce a version like 1.2.4-rc1.
It would need to be done this way because the unstable indicator (alpha/beta/rc/pre) must be part of the version text for livecheck to [theoretically] filter it out without using the regex. If we allowed a looser match outside of the capture group (i.e., /href=.*?example[._-]v?(\d+(?:\.\d+)+)[^"' >]*?\.t/i), this would also match unstable versions but the returned version would appear to be stable (i.e., 1.2.3-rc4 would appear as 1.2.3).
As before, this would match more than just unstable versions. If we encounter a weird version that we don't want but it isn't filtered out as an unstable version, then it could erroneously appear as newest to livecheck and essentially break the check.
Since we would have to actively loosen file name regexes to match unstable versions and this would open us up to potential issues, it isn't something that I've seen value in pursuing. To me, it would be more work for a worse result without a compelling reason behind it (i.e., we're generally only interested in stable versions anyway).
There are a small number of formulae/casks using an unstable version and we do actively expand those regexes to be able to match unstable versions. However, if/when the formula/cask returns to a stable version, we would return the regex to a normal format.
Does this help to explain things?
docs/Brew-Livecheck.md
Outdated
|
|
||
| * **Regexes should only match stable versions**. Regexes should be written to avoid prerelease versions like `1.2-alpha1`, `1.2-beta1`, `1.2-rc1`, etc. | ||
|
|
||
| * **Restrict matching to `href` attributes when targeting file names in an HTML page (or `url` attributes in an RSS feed)**. Using `href=.*?` (or `url=.*?`) at the start of the regex will take care of any opening delimiter for the attribute (`"`, `'`, or nothing) as well as any leading part of the URL. This helps to keep the regex from being overly specific, reducing the need for maintenance in the future. A regex like `href=.*?example-...` is often fine but sometimes it's necessary to have something explicit before the file name to limit matching to only what's appropriate (e.g. `href=.*?/example-...` or `href=["']?example-...`). Similarly, `["' >]` can be used to target the end of the attribute, when needed. |
There was a problem hiding this comment.
I'm not sure what homebrew wants from me here. Should I use href? Should I not? What do I need to use it with?
Can we work with some examples here?
There was a problem hiding this comment.
Having read this I have no idea when I should or shouldn't use this.
There was a problem hiding this comment.
Basically, if we're matching a file name on an HTML page, restricting matching to href attribute URLs will reduce the scope and help to avoid unwanted matches. I reworked this particular guideline to the following:
- Anchor the start/end of the regex, to restrict the scope. For example, on HTML pages we often match file names or version directories in
hrefattribute URLs (e.g./href=.*?example[._-]v?(\d+(?:\.\d+)+)\.zip/i). The general idea is that limiting scope will help exclude unwanted matches.
Some of the previous information is handled by related examples in the new "Example livecheck blocks" section, which hopefully does a better job of providing context. Let me know if these areas are still unclear and I'll try to clarify further.
There was a problem hiding this comment.
The reason we use v?(\d+(?:\.\d+)+) (note the + instead of *) is because it will ensure we're only matching something like 1.2/v1.2, 1.2.3/v1.2.3, etc. When I started working on improving livecheck over a year ago, [0-9.]+ was used to match version text and the looseness was sometimes breaking checks due to it matching unwanted text and returning it as the newest version.
We created the more explicit regex in response to those issues and have standardized on using it when we want to match a version like 1.2/v1.2.3, etc. People see the relative complexity of this regex and suggest something simpler like [\d.]+ but we have experience with that not being sufficient.
The only problem we've had with the + regex is that it sometimes needs to be loosened to v?(\d+(?:\.\d+)*) (i.e., * instead of +) but this doesn't happen very often. For example, we use the * regex if a project predictably publishes the first release for a major version as 2 instead of 2.0/2.0.0 (e.g., openjdk). We also sometimes use the * regex for date-based versions that don't use delimiters (e.g., YYYYMMDD) but we're often more explicit with the regex when a date-based version does use a delimiter (e.g., (\d{2,4}-\d{2}-\d{2})).
Despite the looseness of the * regex, it's still better than [0-9.]+ as it won't match text that isn't a complete version like ., .1, etc. It's explicit enough that it will only match text like 1, 123, 1.0, etc. and this helps to avoid unexpected situations.
The + version is what we use by default (when we won't encounter versions like 1, 123, etc.) as the looser * version can match something it shouldn't if the text is only composed of digits. For example, a project may use versions like 1.2.3 for stable releases but also publish snapshots using date-based versions like 20210113. If they both use the same general file/tag name and are found in the same place, then the looser regex would match both and the date-based version would be erroneously returned as newest. This is something we've encountered a number of times, so we use the + regex by default instead of *.
To put it into perspective, there are only 45 instances of the * regex in homebrew/core, compared to 1000+ instances of the + regex.
However, I imagine you're primarily seeing (\d+(?:\.\d+)*) in homebrew/cask, where there are 330 instances of it (and growing fast). homebrew/cask only has 24 instances of the + regex (all created by me, it seems).
Support for casks in livecheck was only recently added and, unfortunately, homebrew/cask has standardized on the wrong regex. I explained this concept in PR review (i.e., use the + regex unless * is necessary) but it was ignored. I also don't have write access in homebrew/cask, so I don't regularly review livecheck block PRs over there (as I can't merge them) like I do in homebrew/core.
Rather than trying to keep up with the pace that livecheck blocks are being added to casks, I'm currently trying to focus on adding more RuboCops that will automatically enforce the most of the guidelines. We have some livecheck RuboCops for formulae but they don't apply to casks yet, so I need to figure out how to make them work for both.
Regarding the complexity of this regex compared to [0-9.]+ (or [\d.]+), these regexes will be replaced by constants, so the complexity will be abstracted away. At that point, we'll get the benefit of the additional specificity (over something loose like [0-9.]+) without asking people to engage with the additional complexity. [I imagine this may also help to improve the current situation with cask regexes not always following guidelines.]
I'm aiming to create a PR that adds these constants soon (the work's pretty much done) but I'm working to finish/merge some other PRs first.
I'm planning to eventually spend time bringing the livecheck blocks in homebrew/cask up to standard but I don't plan to work on this until sometime after I've implemented regex constants (as that will require modifying regexes across the entire tap anyway).
That or |
There was a problem hiding this comment.
Thanks for writing this up! I've suggested some edits but not 100%. Can you strip out all uses of the passive voice and generally "strengthen up" (i.e instead of saying "in most cases we generally prefer you to do X" just say "do X") and make wording more concise.
I think some of the latter half of this document would be better placed as documentation within the source code of brew livecheck itself; it doesn't seem overly relevant to the end-users in formulae.
As @SMillerDev has noted: I think we're going to need automated RuboCop checks to replace some/most of the style recommendations here; it's simply not feasible to expect maintainers to referent to this (long) document every time they are reviewing a livecheck change. Additionally, brew test-bot or similar should be configured so that it actually tests the livecheck change in formulae (once brew livecheck is in Homebrew/brew) to ensure that it doesn't error with the change.
docs/Brew-Livecheck.md
Outdated
|
|
||
| ### Tap strategies | ||
|
|
||
| Taps can add strategies to apply to their formulae by creating a `livecheck_strategy` folder in the root directory and placing strategy files within. At a minimum, strategies must provide a `#match?(url)` method and a `#find_versions(url, regex)` method. |
There was a problem hiding this comment.
What taps already use this? I'm unconvinced this is a better fit than duplication in formulae.
There was a problem hiding this comment.
To my knowledge, the only tap that has taken advantage of this was Linuxbrew/homebrew-xorg. There were a number of formulae in that tap checking the same sources and Maxim asked me about allowing taps to provide their own tap-specific strategies. It didn't end up being too difficult to support, so I added it and Maxim created the Xorg strategy in homebrew-xorg to handle related formulae without needing to create many individual livecheck blocks.
The strategy lived there until some X.org formulae started being integrated into homebrew-core. At that point, I added the Xorg strategy to Homebrew/homebrew-livecheck and refactored it. After it was merged, Maxim removed the original strategy from homebrew-xorg. At this point, I'm not aware of any taps with their own strategies but I also haven't looked. It's rare for a tap to use livecheck blocks in the first place, so I would be surprised if anyone who's not associated with Homebrew is using a tap strategy.
That said, the benefit of allowing taps to create a strategy is that you can potentially avoid having to create livecheck blocks for formulae. A strategy will automatically apply to matching URLs, so you can create a strategy that handles groups of formulae without any extra work.
This isn't useful for taps that offer first-party software (where you handle releases yourself and don't need livecheck) but it can be useful for taps that contain third-party software. If enough of the software comes from a specific source, a strategy may make more sense than individual livecheck blocks (especially if they're all very similar).
We only include livecheck strategies in Homebrew/brew that apply to first-party taps, so if someone comes to us and says, "Here's a PR for a strategy that's really useful for my tap" we would probably say, "Sorry, this doesn't apply to any of our formulae/casks and we're not going to include this just for your tap." A response I've seen regarding formulae in homebrew-core is, "Sorry, we're not going to merge this formula but you're free to create a tap for it", so allowing tap strategies is basically in the same vein.
That said, I removed this part of the documentation, as it may make more sense to include it in the rubydoc.brew.sh documentation. For the moment, I'm simply leaving this feature undocumented. Folks can discover it by digging into the code and I'll gladly explain it to anyone who comes asking for it but I'm not sure that anyone outside of Homebrew will ever request it.
However, it takes very little code to continue supporting this feature, so I don't think there's any value in dropping it. It may be that some large, external tap (e.g., brewsci/homebrew-bio) could benefit from this in the future.
Yup, given the comments above.
As mentioned above: I think it's sufficient but in almost all cases it's going to need to be mainly automated checks that do this checking. The stuff that's mentioned that can't be trivially automated I'm not sure how I'd review it after reading this above. I think some of this could be improved with e.g. more documented DSL elements, constants, etc. instead of "copy paste this".
I haven't so I pass! @maxim-belkin? |
I'll have a closer look at it later today or tomorrow. |
|
This is a thorough write-up of how things work in homebrew/livecheck internally, recommendations and guidelines for tap maintainers creating livecheck strategies and livecheck blocks, and requirements contributors might face upon submitting pull requests to homebrew/livecheck. I like thorough guides but I have to admit that it's difficult to disagree with some reviews here that found this guide to be a bit more prescriptive than descriptive. So, my suggestions are:
Nonetheless, this is a great description! I like that it's thorough and is very-well written. I didn't comment on some conversations, but I understood the part discussed here -- I'm not sure why or what is not clear there but I blame it on me being a non-native speaker. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
I'm planning on returning to this and moving it forward toward completion as soon as I can. Right now I'm busy with some of the remaining work on bringing livecheckables up to current standards, as that needs to be finished to unblock the homebrew-core migration. Finishing the docs is next on the list after that. |
|
@samford What's the status on this? No rush/worries! |
The number one item on my to-do list right now is taking care of the final GSoC evaluation, since that's time sensitive. I'm hoping to finish that today or tomorrow, time permitting. Once that's out of the way, finishing up this documentation PR is the next item on my list and I'll be responding to the feedback here once I start working on it again. After this PR is done, I'm planning to take a pass through the |
|
Perfect, sounds good @samford! |
|
@samford Can this get finished up or closed out? It's been blocked for several months. Thanks! |
|
@MikeMcQuaid Thanks for the nudge. I started working on the docs again a few weeks ago but, shortly after, other obligations took away my time. I'll work on this some more and hopefully I can push a new draft that incorporates most of the existing feedback within a week or so. I'm sure it will need more review at that point but I imagine it will be closer to what we're looking for than this initial draft. |
|
@samford Gentle nudge again on these, thanks! |
|
@samford without these docs I have to keep pinging you in core and hope you read it. And contributors are just guessing their way to the process. Please prioritize finishing this over changing/adding anything in core. |
|
I have some in-progress changes that aren't up yet, so I'll work on getting them to a point where I can do another push. Most of the existing feedback is addressed by the changes and I'll respond to comments where discussion is still needed.
Even after the docs are merged, I would still encourage folks to request my review on livecheck PRs. I'm trying to make the related documentation as useful as possible but, no matter how good it is, there's still some subjectivity involved in reviewing a The guidelines apply to most cases but there will always be exceptions to the rules, outliers, etc. and my experience is valuable in these areas. The documentation will provide a foundation for people to learn about livecheck but I don't think it can be exhaustive. I'm trying to dump as much of my knowledge into it as I can but, based on the feedback I've received so far, some of it may need to be left out. For what it's worth, I generally get to homebrew-core PRs that request my review within 24 hours (often within an hour if I'm in front of my computer and working on Homebrew), so I think "...hope you read it" is unwarranted. When you've requested my review on homebrew-core PRs, I've reviewed them as soon as possible. However, an issue I've been encountering in homebrew-core is that often my review is requested on a PR after I've gone to sleep and it's merged without my review less than eight hours later. To folks in other time zones, it may appear like I simply didn't bother to review a PR. I always check the PR in the morning and, if it's already merged, I create a follow-up PR to integrate any changes that should have been addressed in review. I usually copy the original author in the follow-up PR (or request their review), so they're aware. There may be benefit in instituting a policy in homebrew-core to give a maintainer 24 hours to review a PR after their review is requested. I generally get to homebrew-core PRs less than 12 hours after my review is requested but we already use 24 hours in Homebrew/brew, so it would be consistent. |
This is simply not sustainable, any maintainer needs to be able to review livechecks. The "hope you read it" was not based on an idea that you do not currently read it, it was simply based on an acknowledgement that all maintainers are human and sometimes have better things to do. |
Agreed. This is the oldest PR on Homebrew/brew by several months. Completing existing PRs should almost always (not just in this case) take priority over opening new ones.
Agreed with @SMillerDev: this is not going to scale. "What @samford knows" has to be replaced with documentation, automation and just trust. Even "please request me for review" should probably be replaced with automation that does so automatically.
I'm fine with this as a principle when review is specifically requested. Otherwise, I'd rather not and rely on fixes being made after the fact.
Agreed. Ultimately we need to be able to build a process that survives @samford (or any maintainer, myself included) being hit by a bus or just quitting the project. |
|
Review period ended. |
I agree with this and it's always been the goal. The documentation is a notable part of moving toward this goal and I'm sorry it's been delayed for so long. Another part of enabling more maintainers to effectively review I've planned to replace commonly-found patterns (and some entire regexes) with constants for a long time but haven't created a PR for this yet. I've worked on one approach to this (locally), so I'll wrap it up and create a PR once we're finished here. It may not be the exact implementation we end up using but it will help move us in the right direction (e.g., starting a discussion), at the very least. Looking at the most recent version of the regex guidelines, I think there are only a couple that we can't currently create RuboCops around. One is "use I spent the past few days finishing the work that I had started to address feedback and expand this documentation further. I'll comment on any areas where discussion is still needed but feel free to review when you have a chance. |
Co-authored-by: Mike McQuaid <mike@mikemcquaid.com> Co-authored-by: Sean Molenaar <1484494+SMillerDev@users.noreply.github.com>
|
Thanks @samford, really appreciate the timely turnaround here! Merging now. Any @Homebrew/maintainers with remaining comments can make them inline and changes can be addressed in follow-up PRs. |
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?brew manlocally and committed any changes?This is preliminary documentation for the
brew livecheckcommand. At the moment, it's primarily geared toward explaininglivecheckblocks and livecheck's strategies. I'm hoping to expand this in the near future to explain more about livecheck in general, as I feel the current documentation still assumes that the user has some existing knowledge of livecheck.Any feedback is appreciated but here are some specific areas that I'm curious to hear about:
livecheckblock PRs once they start appearing in homebrew-core? If not, what could be better?For what it's worth, we plan to turn as much of the
urlandregexguidelines into audits as we can, to reduce the mental burden on maintainers. I imagine the guidelines can be a bit much to remember for anyone who's not invested in livecheck.Lastly, can we hold off on publishing this document until after the migration of
livecheckblocks from homebrew-livecheck to homebrew-core? It's currently written as if this migration has already happened and I'm only posting this now so that other maintainers can reference the information in the interim time.