livecheck: support throttle DSL #16918

cho-m · 2024-03-19T15:18:38Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?

Essentially, we need to figure out best way to combine autobump, livecheck, and throttle so that autobumps can properly handle this. Preferably avoiding code duplication across various parts (i.e. bump commands, livecheck logic, github action, etc.)

Some ideas:

Throttle DSL - This approach
Livecheck throttling - ~~e.g. Homebrew/homebrew-core@184767f
Time-based throttling - would need to change version-based throttling to time-based (days between bump) in JSON, provide a way of detecting some datetime info (e.g. modulo current day of year, attempt during specific timeslot in a day).
- Con: This functionality mainly works when on autobump, but is trickier when user bumping is allowed as cannot guarantee cadence here.

cho-m · 2024-03-19T15:48:18Z

Library/Homebrew/livecheck/livecheck.rb

@@ -810,6 +810,28 @@ def latest_version(
 latest: Version.new(match_version_map.values.max_by { |v| LivecheckVersion.create(formula_or_cask, v) }),
 }

+ if (throttle = livecheck.throttle || referenced_livecheck&.throttle)
+ match_version_map.delete_if { |_match, version| !version.patch.to_i.modulo(throttle).zero? }


Could even consider extending DSL to allow minor version and other fine control. Maybe like one of following styles:

throttle 10, :minor throttle 10, :minor_version throttle 10, :version_minor

carlocab

Seems sensible to me, but I think we want to deprecate the throttle JSON in favour of this so we don't have two different lists to carry around.

cho-m · 2024-03-19T18:03:41Z

deprecate the throttle JSON in favour of this so we don't have two different lists to carry around.

That was my plan if we go this route. But may be a follow up so we can migrate homebrew/core usage and then deprecate support. I need to figure out how to handle renovate and vim since we don't collect sufficient version info.

MikeMcQuaid

Looks good! There's a bunch of suboptimal type usage in existing code that would be nice to clean up but doesn't need to block this PR.

Library/Homebrew/dev-cmd/bump.rb

MikeMcQuaid · 2024-03-20T08:42:27Z

Library/Homebrew/dev-cmd/bump.rb

- livecheck_latest = livecheck_result(loaded_formula_or_cask)
+ livecheck_latest, livecheck_latest_throttled = livecheck_result(loaded_formula_or_cask)
+ # TODO: Pass down `livecheck_latest` info to print output for throttled formulae or casks
+ livecheck_latest = livecheck_latest_throttled if livecheck_latest_throttled

 new_version_value = if (livecheck_latest.is_a?(Version) && livecheck_latest >= current_version_value) ||


Doesn't need cleaned up in this PR but: I hate that it's using different types here to indicate success/failure.

MikeMcQuaid · 2024-03-20T08:43:03Z

Library/Homebrew/livecheck.rb

+ rate: T.nilable(Integer),
+ ).returns(T.nilable(Integer))
+ }
+ def throttle(rate = T.unsafe(nil))


Would be nicer to avoid unsafe here if possible.

I think this may relate to sig @reitermarkus suggested. If we type the parameter as T.nilable(Integer) then it is no longer T.unsafe.

On other hand, if we type it as Integer, then we need to make it T.unsafe.

Whichever we choose should be consistent with Livecheck code so can discuss in follow up.

Whichever we choose should be consistent with Livecheck code so can discuss in follow up.

Agreed.

Generally I favour (in descending order of preference):

non-nilable types

nilable types

use of unsafe

use of must

samford

From a quick review/test, we still need to add some throttle information to the debug and JSON output. Namely:

The JSON output is missing an (optional) latest_throttled field. The value is present in the data from #latest_version but we need to handle it in #run_checks.
We should print the throttle value in the debug output.
We should include the throttle value in the meta part of the JSON output.

These are simple changes and I worked them up while reviewing this, so I'll just go ahead and push a commit to take care of it (since I can't add suggestions to all the code here).

Past that, this seems reasonable to me. With these changes, livecheck continues to return the latest version and the latest throttled version is simply provided as supplemental information, which is how I would handle this. brew bump benefits from the extra information while livecheck continues to return the latest version 👍

Library/Homebrew/livecheck.rb

MikeMcQuaid · 2024-03-21T12:15:40Z

Past that, this seems reasonable to me.

@samford can you ✅ in future when you feel that way? Thanks 🙇🏻

MikeMcQuaid · 2024-03-21T12:15:52Z

@cho-m Good to 🚢 once rebased and 🟢, thanks!

Library/Homebrew/livecheck.rb

samford · 2024-03-21T12:28:48Z

@samford can you ✅ in future when you feel that way? Thanks 🙇🏻

My wording was soft but I still wanted to see the throttle variable/method in the Livecheck DSL reordered before approving/merging. I should have been more explicit about that up front. I pushed a commit to take care of those changes, so I'll go ahead and approve.

I also rebased this and resolved the merge conflicts, so it should pass CI this time around.

Library/Homebrew/livecheck.rb

cho-m · 2024-03-21T13:00:41Z

@cho-m Good to 🚢 once rebased and 🟢, thanks!

I will do one more sweep through code. Mostly going to clean up some of my unused logic though I may defer some/all changes to follow up (along with tests and audits).

Library/Homebrew/dev-cmd/bump-formula-pr.rb

Library/Homebrew/dev-cmd/bump.rb

cho-m · 2024-03-21T13:58:08Z

Going to have to figure out why test for ZSH completions is failing. I don't think it is related to this PR given we didn't modify description. Can't repro locally.

Error: Homebrew::Completions when generating completions .update_shell_completions! generates shell completions

Failure/Error: described_class.update_shell_completions!

NoMethodError:
  undefined method `split' for nil:NilClass

          cmd_parser.description.split(DESCRIPTION_SPLITTING_PATTERN).first
                                ^^^^^^

Signed-off-by: Michael Cho <[email protected]>

Co-authored-by: Markus Reiter <[email protected]>

Signed-off-by: Michael Cho <[email protected]>

cho-m mentioned this pull request Mar 19, 2024

formulae: add livecheck throttle, throttled_formulae: removals Homebrew/homebrew-core#166501

Merged

6 tasks

cho-m changed the title ~~livecheck: support throttle DSL~~ [POC] livecheck: support throttle DSL Mar 19, 2024

cho-m commented Mar 19, 2024

View reviewed changes

cho-m force-pushed the livecheck-throttle branch 3 times, most recently from d7ada6c to c863181 Compare March 19, 2024 16:10

cho-m changed the title ~~[POC] livecheck: support throttle DSL~~ livecheck: support throttle DSL Mar 19, 2024

cho-m force-pushed the livecheck-throttle branch from c863181 to 7ff01d8 Compare March 19, 2024 16:40

cho-m marked this pull request as ready for review March 19, 2024 16:49

carlocab reviewed Mar 19, 2024

View reviewed changes

p-linnane requested a review from samford March 19, 2024 19:03

MikeMcQuaid approved these changes Mar 20, 2024

View reviewed changes

samford reviewed Mar 21, 2024

View reviewed changes

Library/Homebrew/livecheck.rb Outdated Show resolved Hide resolved

samford force-pushed the livecheck-throttle branch from d28d2df to ad477e0 Compare March 21, 2024 12:16

MikeMcQuaid approved these changes Mar 21, 2024

View reviewed changes

reitermarkus reviewed Mar 21, 2024

View reviewed changes

Library/Homebrew/livecheck.rb Outdated Show resolved Hide resolved

samford force-pushed the livecheck-throttle branch from ad477e0 to 0f947e0 Compare March 21, 2024 12:26

samford approved these changes Mar 21, 2024

View reviewed changes

reitermarkus reviewed Mar 21, 2024

View reviewed changes

Library/Homebrew/livecheck.rb Outdated Show resolved Hide resolved

cho-m commented Mar 21, 2024

View reviewed changes

Library/Homebrew/dev-cmd/bump-formula-pr.rb Outdated Show resolved Hide resolved

cho-m commented Mar 21, 2024

View reviewed changes

Library/Homebrew/dev-cmd/bump.rb Outdated Show resolved Hide resolved

MikeMcQuaid approved these changes Mar 21, 2024

View reviewed changes

MikeMcQuaid enabled auto-merge March 21, 2024 13:50

livecheck: support throttle DSL

bfec6ee

Signed-off-by: Michael Cho <[email protected]>

samford and others added 4 commits March 21, 2024 10:11

livecheck: Add throttle info to debug, JSON output

25d07c3

livecheck: Reorder throttle

7681621

livecheck: Update throttle rate parameter type

0f06392

Co-authored-by: Markus Reiter <[email protected]>

dev-cmd/bump-formula-pr: clean up unused code

43e2e28

Signed-off-by: Michael Cho <[email protected]>

cho-m force-pushed the livecheck-throttle branch from a9eddb7 to 43e2e28 Compare March 21, 2024 14:11

MikeMcQuaid merged commit 857838a into Homebrew:master Mar 21, 2024
25 checks passed

cho-m deleted the livecheck-throttle branch March 21, 2024 14:33

cho-m mentioned this pull request Mar 21, 2024

livecheck: replace T.unsafe with T.nilable #16933

Closed

7 tasks

github-actions bot added the outdated PR was locked due to age label Apr 21, 2024

github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

livecheck: support throttle DSL #16918

livecheck: support throttle DSL #16918

cho-m commented Mar 19, 2024 •

edited

cho-m Mar 19, 2024

carlocab left a comment

cho-m commented Mar 19, 2024

MikeMcQuaid left a comment

MikeMcQuaid Mar 20, 2024

MikeMcQuaid Mar 20, 2024

cho-m Mar 21, 2024

MikeMcQuaid Mar 21, 2024

samford left a comment •

edited

MikeMcQuaid commented Mar 21, 2024

MikeMcQuaid commented Mar 21, 2024

samford commented Mar 21, 2024 •

edited

cho-m commented Mar 21, 2024

cho-m commented Mar 21, 2024

livecheck: support throttle DSL #16918

livecheck: support throttle DSL #16918

Conversation

cho-m commented Mar 19, 2024 • edited

cho-m Mar 19, 2024

Choose a reason for hiding this comment

carlocab left a comment

Choose a reason for hiding this comment

cho-m commented Mar 19, 2024

MikeMcQuaid left a comment

Choose a reason for hiding this comment

MikeMcQuaid Mar 20, 2024

Choose a reason for hiding this comment

MikeMcQuaid Mar 20, 2024

Choose a reason for hiding this comment

cho-m Mar 21, 2024

Choose a reason for hiding this comment

MikeMcQuaid Mar 21, 2024

Choose a reason for hiding this comment

samford left a comment • edited

Choose a reason for hiding this comment

MikeMcQuaid commented Mar 21, 2024

MikeMcQuaid commented Mar 21, 2024

samford commented Mar 21, 2024 • edited

cho-m commented Mar 21, 2024

cho-m commented Mar 21, 2024

cho-m commented Mar 19, 2024 •

edited

samford left a comment •

edited

samford commented Mar 21, 2024 •

edited