At the moment, `#use_homebrew_curl?` can only be true for a
`homepage` or `stable`/cask `url` with `using: :homebrew_curl`. If
the checked URL differs from these URLs, livecheck won't use brewed
curl. This limitation prevents livecheck from using brewed curl for a
`livecheck` block URL that's a string literal (not a symbol for a
`#checkable_url` like `:stable`, `:head`, `:url`). `libzip` was the
original formula referenced in the related brew issue and it meets
this criterion, so it doesn't appear to be handled by the existing
`#use_homebrew_curl?` implementation.
Additionally, the existing behavior can cause livecheck to
unnecessarily use brewed curl for a completely different website
(e.g., `cubelib`, `otf2`). For example, if the `stable` URL has
`using: :homebrew_curl` and the `livecheck` block has `url
:homepage`, livecheck will use brewed curl when checking the
`homepage`. If these are completely different domains/servers, it's
unlikely that we would need to use brewed curl when checking the
`homepage`, so this particular behavior may not be beneficial.
This commit reimplements `use_homebrew_curl?` to apply brewed curl
when the checked URL's root domain is the same as the root domain of
an aforementioned formula/cask URL with `using: :homebrew_curl`. For
example, this looser approach would allow a `livecheck` block
checking `https://www.example.com/downloads/` to use brewed curl if
the `stable` URL was `https://downloads.example.com/example.zip` with
`using: :homebrew_curl`. These could be different servers but, based
on related formulae, this looseness is necessary for the moment.
This approach aims to resolve both issues, allowing brewed curl to be
applied to a slightly broader range of URLs (i.e., not limited to
just the `#checkable_urls`) while also helping to avoid unnecessarily
applying brewed curl when it's less likely to be useful (completely
different domains). Neither approach is perfect but this one may be
more useful in the interim time.
Depending on how this looser approach works in practice, we may want
to consider returning to a stricter approach once we have something
like `using: :homebrew_curl` in `livecheck` blocks (this is
forthcoming). Being explicit in a `livecheck` block is the most
reliable approach (i.e., only use brewed curl when needed), so we
could favor that and pare down the automated approach to only what's
needed to support implicit checks (i.e., with no `livecheck` block).
Of course, it's also possible to drop the automated approach entirely
and simply require a `livecheck` block in this scenario but we can
decide on how to handle this when the time comes.
The default redirection maximum for `curl` is 50 but we should use
something more reasonable in livecheck. It's rare but a misconfigured
server with an endless redirection loop will hit the 50 redirection
limit. Unfortunately, we've encountered this in the wild (e.g., the
server for `getmail` and `memtester` endlessly redirects), so it's
not an idle concern. This commit basically adds `--max-redirs 5` to
`Livecheck::Strategy::DEFAULT_CURL_ARGS` to enforce a more reasonable
redirection maximum.
To be clear, the `max_iterations` logic in `#parse_curl_output`
(which was previously found in `Strategy#page_content`) doesn't
restrict the number of redirections that `curl` follows. At the point
the `curl` output is being parsed, the requests have already been
made and `max_iterations` simply restricts the number of responses
`#parse_curl_output` is willing to parse. If we use `--max-redirs`
and properly set `max_iterations` to `max-redirs + 1`, we shouldn't
encounter the "Too many redirects" error in `#parse_curl_output`.
Currently, only `Livecheck::Strategy::PAGE_HEADERS_CURL_ARGS` uses
the `--silent` option and `PAGE_CONTENT_CURL_ARGS` does not (though
there's no intention behind this omission). However, the
`#page_content` method should also use the `--silent` flag, to
prevent progress bar text (`#=#=#`, etc.) from appearing in output.
This is an issue because the regex that's used to identify `curl`
error messages in `stderr` (`^curl:.+$/`) will fail if leading
progress bar text is present. This leads to an ambiguous "cURL
failed without a detectable error" message instead of the actual
error message(s) from `curl`.
This commit addresses the issue by adding `--silent` to
`Livecheck::Strategy::DEFAULT_CURL_ARGS`, which both
`PAGE_HEADERS_CURL_ARGS` and `PAGE_CONTENT_CURL_ARGS` inherit.
Some formulae/casks contain duplicate checkable URLs or contain
URLs that become duplicates after `#preprocess_url` is used. With
the former, a formula's `stable` and `head` URLs can be the same if
they both use the Git repository. With the latter, a formula's
`homepage` and `stable` URLs are different but `#preprocess_url` can
return the same Git repository URL for each (which can also be
a duplicate of the `head` URL).
The `fabric-completion` formula is a worst case scenario, as it
contains both of these conditions but the repository has no tags.
By default, livecheck would needlessly check the repository three
times (i.e., no versions are found so livecheck tries the next URL,
which happens to be the same URL).
This commit avoids duplicate URLs in `#checkable_urls` and keeps
track of checked URLs in `#latest_version` to avoid a duplicate
caused by `#preprocess_url`. This should effectively resolve the
issue of checking the same URL more than once for a given
formula/cask. Checking the same URL only once across all the
formulae/casks in a given livecheck run will be handled in a later
commit (though I've done most of the groundwork already in previous
PRs).
This aligns `PageMatch` with how cask strategies handle requirements
in their `#find_versions` methods. This moves related logic from
`Livecheck#latest_version` into the `PageMatch` strategy, which feels
a bit more appropriate.
There are times where a regex isn't needed in a `strategy` block and
these changes explicitly handle that situation.
This allows the Symbol Proc format used in some `Sparkle` `livecheck`
blocks (e.g., `strategy :sparkle, &:version`) to continue working
instead of failing with a "wrong number of arguments (given 1,
expected 0)" error. This error would occur because a Symbol Proc only
only expects one argument (e.g., an `Item`, not an `Item` and a
regex/nil).
There's an argument to be made for avoiding the Symbol Proc format
for `strategy` blocks but I haven't found a way of selectively
disabling the Style/SymbolProc cop only for a `strategy` DSL method
call. That is to say, if we don't use the Symbol Proc format, `brew
style` will give a "Pass &:version as an argument to strategy instead
of a block." offense.
Besides that, this also replaces the `block` type signatures in
livecheck strategies with `T.untyped`. Sorbet doesn't know how to
handle a `Proc` with a variable number of arguments and can't be
taught how (i.e., `T.any` with a `Proc` signature for each variation
doesn't work). The aforementioned changes cause Sorbet to complain
about there being both too many and too few arguments, so the only
way to win is not to play the game. Hopefully we can restore the
`block` type signatures in the future (if upstream resolves this
years-old issue) but `T.untyped` seems to be our only option for now.
The `ExtractPlist` strategy doesn't return certain values (e.g.,
`:url`), so it's necessary to also ensure these values are present
before printing related debug info. Without these changes, we end up
printing info for a blank value (e.g., "URL (strategy):").
This modifies cask-related livecheck strategies to allow passing a
regex into a `strategy` block, when appropriate. These strategies
were outliers that explicitly rejected a regex even if a `strategy`
block was used, forcing any regex to be inlined in the `strategy`
block (instead of being defined using `#regex`).
With these changes, all `strategy` blocks will be able to accept a
regex, further simplifying the mental model. This also helps to
better align the various `find_versions` and `versions_from_*`
methods across strategies.
Some Apache URLs use a `filename` query string parameter instead of
`path` and the `Apache` strategy isn't applied. The parameter value
is the same as `path` (i.e., a path to a file), so this updates the
strategy's `URL_MATCH_REGEX` to handle this URL format as well.
The existing regex wasn't able to match errors like:
curl: option --something: is unknown
Additionally, the existing approach wouldn't capture multi-line
errors, whereas this captures all the `curl:` lines from `stderr`.
Valid `strategy` block return types currently vary between
strategies. Some only accept a string whereas others accept a string
or array of strings. [`strategy` blocks also accept a `nil` return
(to simplify early returns) but this was already standardized across
strategies.]
While some strategies only identify one version by default (where a
string is an appropriate return type), it could be that a strategy
block identifies more than one version. In this situation, the
strategy would need to be modified to accept (and work with) an
array from a `strategy` block.
Rather than waiting for this to become a problem, this modifies all
strategies to standardize on allowing `strategy` blocks to return a
string or array of strings (even if only one of these is currently
used in practice). Standardizing valid return types helps to further
simplify the mental model for `strategy` blocks and reduce cognitive
load.
This commit extracts related logic from `#find_versions` into
methods like `#versions_from_content`, which is conceptually similar
to `PageMatch#page_matches` (renamed to `#versions_from_content`
for consistency). This allows us to write tests for the related code
without having to make network requests (or stub them) at this point.
In general, this also helps to better align the structure of
strategies and how the various `#find_versions` methods work with
versions.
There's still more planned work to be done here but this is a step
in the right direction.
Up to this point, we've had to rely on making `Strategy` constants
private to ensure that the only available constants are strategies.
With the current setup, the existence of a constant that's not a
strategy would break `Strategy#strategies` and
`Livecheck#livecheck_strategy_names`.
Instead, we can achieve the same goal by skipping over constants
that aren't a class. Other than saving us from having to make these
constants private, this is necessary to be able to create a
`Strategy` constant that can be used in all strategies.
