-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
Cask::Audit: reduce livecheck fetches #21308
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
Conversation
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.
Pull request overview
This PR optimizes cask audit performance by reducing redundant livecheck URL fetches from up to three times down to one in most cases. This is achieved by introducing a caching mechanism for the livecheck version audit result.
- Adds
@livecheck_resultinstance variable to cache livecheck audit results - Modifies
audit_livecheck_versionto cache and reuse results across multiple audit methods - Updates
audit_hosting_with_livecheckto callaudit_livecheck_versiondirectly instead of accepting it as a parameter - Adds early return optimization in
audit_livecheck_https_availabilityto skip unnecessary checks when livecheck already uses HTTPS and succeeds
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
40ae65b to
45c08f7
Compare
MikeMcQuaid
left a comment
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 once small nit addressed! Can self-merge without rereview.
The existing livecheck-related cask audits can end up fetching the livecheck URL four times and this can cause problems if a server has sensitive rate limiting. This modifies the `livecheck_version` audit to cache the result that's also used in the `hosting_with_livecheck` and `livecheck_https_availability` audits. This will avoid a duplicate fetch and can avoid another if the livecheck URL uses HTTPS and the check doesn't fail. Most casks meet that criteria, so this should be a notable improvement. There's still room for improvement, as the `min_os` audit calls `cask_sparkle_min_os`, which can also fetch the livecheck URL. It would be ideal if we could cache the livecheck content to avoid this duplicate fetch but I don't think that's possible with the current setup, so I'll have to revisit that later.
45c08f7 to
9ba98a6
Compare
brew lgtm(style, typechecking and tests) with your changes locally?The existing livecheck-related cask audits can end up fetching the livecheck URL four times and this can cause problems if a server has sensitive rate limiting (e.g., Homebrew/homebrew-cask#241429 (comment)). This modifies the
livecheck_versionaudit to cache the result that's also used in thehosting_with_livecheckandlivecheck_https_availabilityaudits. This will avoid a duplicate fetch and can avoid another if the livecheck URL uses HTTPS and the check doesn't fail. Most casks meet that criteria, so this should be a notable improvement.There's still room for improvement, as the
min_osaudit callscask_sparkle_min_os, which can also fetch the livecheck URL. It would be ideal if we could cache the livecheck content to avoid this duplicate fetch but I don't think that's possible with the current setup, so I'll have to revisit that later. [I have some in-progress work to add caching to livecheck and I may be able to leverage that in the future as a way of addressing this.]