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: refactor url preprocessing #9074
Conversation
Makes sense to me but would like @samford or @nandahkrishna to chip in. Thanks @vladimyr! |
I'm in the process of doing [verbose JSON] livecheck runs across homebrew/core with and without these changes, to identify any regressions/improvements. I'll review this once that's finished and I've done some manual testing to check specific cases as well. Off hand, there's one type of GitHub URL that the previous code handled but this PR doesn't, so I'm going to label this "do not merge" for the moment. I'll address this in the forthcoming review but I at least wanted to give a brief update while I'm testing this. |
I made a quick test comparing the output of the Source:#!/bin/bash
formulae=(
mednafen
camlp5
kotlin
osrm-backend
prometheus
pyenv-virtualenv
sysdig
shairport-sync
yuicompressor
)
export HOMEBREW_NO_AUTO_UPDATE=1
for formula in "${formulae[@]}"; do
brew livecheck --debug "$formula" 2>/dev/null
done
Results:results.before.txt FWIW is worth those are identical but sweep through the whole formula set is very much needed 👍
Let me know what did I miss and I'll try to fix it so you don't have to do redundant tests. 😉 |
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've finished testing this and identifying regressions/improvements.
I like the idea of simplifying this code but this PR currently omits handling of certain types of URLs that the previous code supports. Namely:
- Older
https://github.com/downloads/owner/repository/...
URLs were handled properly before but aren't handled in this PR. These URLs should be processed intohttps://github.com/owner/repository.git
but this PR produces a processed URL likehttps://github.com/downloads/owner.git
. - GitLab URLs that aren't on gitlab.com were handled properly before (this is the purpose of the
url.include?("/-/archive/")
block) but this PR only supports gitlab.com. Removing the broader GitLab support in favor of a domain-specific approach breaks checks for a number of formulae.
That said, it's easy to fix these issues and I've added suggestions that do so. I also did a livecheck run across homebrew/core with my suggestions below and confirmed they resolve these two regressions that I saw in the original run for this PR's changes.
There are some modifications that need to be made to specific formulae with this PR in mind but I've taken care of some of them and I'm in the process of working through the others. Most of these can be done before this is merged but at least one needs to be handled afterward.
Formulae with a https://github.com/downloads/...
stable
URL
The following formulae are affected by the removal of the code that handles https://github.com/downloads/owner/repository/...
URLs. Some of the checks continue to work even though the stable
URL can't be checked, as either the head
URL is tried first or livecheck falls back to the homepage
(since livecheck checks in order of head
, stable
, homepage
when there isn't a livecheck
block in the formula).
btpd
: check only works because it falls back to thehomepage
URLcssembed
: broken, as no formula URLs are usable by defaultgrowly
: check only works because it falls back to thehomepage
URLhenplus
: broken, as no formula URLs are usable by defaultmidgard2
: only works because thehead
URL is tried before thestable
URLrpg
: only works because thehead
URL is tried before thestable
URLtidyp
: check only works because it falls back to thehomepage
URL
Formulae with a Self-Hosted GitLab stable
URL
The following formulae are affected by the removal of the /-/archive/
URL handling code, which was replaced with code that only handles gitlab.com URLs. As above, some of these checks are broken by this change and others continue to work due to the existence of other URLs in the formula (e.g., head
, homepage
).
dav1d
:https://code.videolan.org/videolan/dav1d/-/archive/0.7.1/dav1d-0.7.1.tar.bz2
libcerf
:https://jugit.fz-juelich.de/mlz/libcerf/-/archive/v1.14/libcerf-v1.14.tar.gz
libhandy
:https://gitlab.gnome.org/GNOME/libhandy/-/archive/1.0.1/libhandy-1.0.1.tar.gz
libolm
:https://gitlab.matrix.org/matrix-org/olm/-/archive/3.2.1/olm-3.2.1.tar.gz
libxp
:https://gitlab.freedesktop.org/xorg/lib/libxp/-/archive/libXp-1.0.3/libxp-libXp-1.0.3.tar.bz2
lmdb
:https://git.openldap.org/openldap/openldap/-/archive/LMDB_0.9.27/openldap-LMDB_0.9.27.tar.bz2
mat2
:https://0xacab.org/jvoisin/mat2/-/archive/0.11.0/mat2-0.11.0.tar.gz
menhir
:https://gitlab.inria.fr/fpottier/menhir/-/archive/20200624/menhir-20200624.tar.bz2
ortp
:https://gitlab.linphone.org/BC/public/ortp/-/archive/4.3.2/ortp-4.3.2.tar.bz2
posh
:https://salsa.debian.org/clint/posh/-/archive/debian/0.14.1/posh-debian-0.14.1.tar.bz2
Other Issues
URI.parse
will give a URI::InvalidURIError
error for some head
URLs. The examples I encountered were:
mandoc
:URI::InvalidURIError (bad URI(is not URI?): "anoncvs@mandoc.bsd.lv:/cvs")
(fromhead
URL)ncp
:URI::InvalidURIError (bad URI(is not URI?): ":pserver:cvs:@cvs.fefe.de:/cvs")
(fromhead
URL)
I'll defer to @MikeMcQuaid about what the best way of handling this would be. The basic idea is that these particular URLs shouldn't be processed. In general, livecheck won't do anything with these URLs (since no strategies will apply to them), so we only have to deal with this from the standpoint of #preprocess_url
.
Improvements
bombadillo
:Unable to get versions
before and now working properly using theGit
strategy with thetildegit.org
repository.scdoc
:Unable to get versions
before and now working properly using theGit
strategy with thegit.sr.ht
repository.
@samford Before I start reading/addressing comments I just want to take a moment to appreciate both the effort and the outcome, tips 🎩 |
@samford I took another shot based on helpful comments and my own ideas and here is what I got: Formulae with a
|
For reference here are router implementations from:
Gogs seems to lack the latest release ( |
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'm calling it a night over here but I'll take another pass at the latest revision when I get a chance.
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.
This pass addresses the strictness of the host
comparisons in the if
/else
conditions, as described in the comments below.
If #9105 is merged before this PR, I'll push a commit to your branch to expand the #preprocess_url
test cases to include the additional URLs supported here. I already have them worked out and this PR passed when I tested it locally.
Otherwise, if this is merged before #9105, I'll simply update that PR to include the additional test cases.
Running CI now #9105 is merged. |
@vladimyr Any news here? |
- support both `github.com/downloads/<owner>/<repo>` and `github.s3.amazonaws.com/<owner>/<repo>` URL patterns - support self-hosted GitLab installations (with project groups) - support _well-known_ Gitea and Gogs instances
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.
To help move this forward, I rebased this branch and added a commit that expands the #preprocess_url
test cases to also include the newly-supported URLs in this PR.
I also added a suggestion here to handle the remaining issue that hadn't yet been addressed (i.e., URI.parse
will give a URI::InvalidURIError
error for some head
URLs).
At this point, we just need to incorporate the suggestions, give this a final review, and I believe it should be good to go. @vladimyr, let me know if you won't have time to finish this, as I can simply incorporate the requested changes and help shepherd this towards merging.
@samford if you could do that when you get a chance it'd be good 🙇🏻 |
I'm already incorporating the suggestions here on my local branch as part of testing, so I'll push a commit once I'm done 👍 |
I did another before/after livecheck run across homebrew/core and the only changes I observed were as follows:
I'll create a follow-up PR in homebrew-core to address At this point, this just needs a final review to address any areas that may need to be improved/modified. |
Requested changes have been incorporated.
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.
Looks good to me at this point. I'm going to go ahead and merge this, so we can revisit another open livecheck PR (cask support) and try to move that one toward merging.
Thanks, @vladimyr!
brew style
with your changes locally?brew tests
with your changes locally?brew man
locally and committed any changes?Working with
URI
instances instead ofStrings
makes code much shorter and more readable. Special cases handling isn't needed anymore because matching is done on a host(name) level, paths ending with/releases/latest
do not get altered and all those listed formulas already have formula specific livecheck blocks.Added goodies are introducing support for tildegit.org (which follows Github's routing patterns) and sourcehut 🎉