Skip to content
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: add support for casks #8578

Merged
merged 27 commits into from
Dec 12, 2020

Conversation

SeekingMeaning
Copy link
Contributor

  • 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 tests with your changes locally?

This allows brew livecheck to work on casks and adds a new livecheck stanza to the Cask DSL. Since many casks use methods such as version.before_comma, a new version field is added to the Livecheck class that accepts either a String or a valid Symbol (before_comma, after_comma, before_colon, after_colon)

Example livecheck for ansible-dk:

livecheck do
  url :homepage
  version :before_comma
end

@reitermarkus reitermarkus added the cask Homebrew Cask label Sep 2, 2020
@reitermarkus reitermarkus requested a review from a team September 2, 2020 20:35
@miccal
Copy link
Member

miccal commented Sep 3, 2020

Great work.

Would the url in the livecheck block accept :appcast?

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! I'd love @samford and @nandahkrishna to take a look before this is merged.

@core-code
Copy link
Contributor

Would the url in the livecheck block accept :appcast?

also implicitly using the :appcast if nothing is specified would make sense.

Copy link
Member

@nandahkrishna nandahkrishna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @SeekingMeaning for this PR! Just a few minor comments. Also, would --formulae-only and --casks-only flags for the brew livecheck developer command be useful?

Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck.rb Outdated Show resolved Hide resolved
@samford
Copy link
Member

samford commented Sep 3, 2020

I don't often find myself in homebrew-cask but I happened to be looking at a cask the other day and thinking about adding support to livecheck. Good timing!

I'm in the process of reviewing and testing this and I'll have some feedback and requested changes once I'm finished.

Library/Homebrew/cask/dsl.rb Outdated Show resolved Hide resolved
Library/Homebrew/cask/dsl.rb Show resolved Hide resolved
@SeekingMeaning
Copy link
Contributor Author

Would the url in the livecheck block accept :appcast?

@miccal Yes

also implicitly using the :appcast if nothing is specified would make sense.

we might want to have the appcast URL take highest priority (and be the first addition to the urls array).

@core-code @nandahkrishna Done (I'm assuming all casks at least have url since a cask would be useless without it)

Also, would --formulae-only and --casks-only flags for the brew livecheck developer command be useful?

Yes, probably

"version"  => @version,

Would we want to omit this from the hash unless @formula_or_cask is a cask?

Maybe, but the doc comment says "Returns a Hash of all instance variable values"

-      @livecheck ||= Livecheck.new(self)
-      return @livecheck unless block_given?
-      raise CaskInvalidError.new(cask, "'livecheck' stanza may only appear once.") if @livecheckable
-      @livecheckable = true
-      @livecheck.instance_eval(&block)
+      set_unique_stanza(:livecheck, !block_given?) do
+        livecheck = LiveCheck.new(self)
+        livecheck.instance_eval(&block)
+        livecheck
+      end

@reitermarkus I actually did this initially but the Livecheck utility requires livecheck method to return a non-nil value

Copy link
Member

@samford samford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work here, @SeekingMeaning.

There may be more room for improvement outside of my specific comments below but this is my first pass through this. Since this is a significant addition, I would prefer that we take our time with this to make sure it's in good shape before merging.

One thing that sticks out to me right now is the relative verbosity of the various formula_or_cask language. It would be nice if we could find a generic way to refer to the concept of a formula or cask that's shorter but maybe this is just me.


There may be value in having a flag that allows us to restrict livecheck to only work with either formula or casks. Maybe something like --type=formula/--type=cask or individual flags like --formula-only/--cask-only. This would allow users to only work with one or the other for a given run, which may be useful in conjunction with the --all or --tap= options. This should be relatively easy to handle, since it only affects the Livecheck#livecheck method.

Edit: I just now saw Nanda's suggestion about the same flags, ahah. I'm not surprised we had the same idea.


From what I'm seeing, roughly 1/3 of the ~3645 casks have a URL that matches a strategy (~87% use the Git strategy) and a fair number of those produce an okay result. Hopefully we can look through the URLs and identify patterns that can be handled with additional strategies. If not, we're looking at 2500+ additional items to write livecheck blocks for, on top of the ~600 formulae that still need livecheck blocks.

Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/dev-cmd/livecheck.rb Show resolved Hide resolved
Library/Homebrew/dev-cmd/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/dev-cmd/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck.rb Show resolved Hide resolved
Library/Homebrew/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/livecheck.rb Outdated Show resolved Hide resolved
Library/Homebrew/test/livecheck_spec.rb Outdated Show resolved Hide resolved
@SeekingMeaning
Copy link
Contributor Author

One thing that sticks out to me right now is the relative verbosity of the various formula_or_cask language. It would be nice if we could find a generic way to refer to the concept of a formula or cask that's shorter but maybe this is just me.

Yes, I agree — what about recipe or item?

Hopefully we can look through the URLs and identify patterns that can be handled with additional strategies.

I'll try my best

@nandahkrishna
Copy link
Member

nandahkrishna commented Sep 4, 2020

I just now saw Nanda's suggestion about the same flags, ahah.

😄

As for the flags, it seems brew list has --formula/--formulae and --cask/--casks so we could go with those for consistency.

@SeekingMeaning
Copy link
Contributor Author

List of casks without an automatic livecheck strategy (2,319 total): https://gist.github.com/SeekingMeaning/6c7a6d072a562c6ac474d5c0bb558076

@MikeMcQuaid
Copy link
Member

One thing that sticks out to me right now is the relative verbosity of the various formula_or_cask language. It would be nice if we could find a generic way to refer to the concept of a formula or cask that's shorter but maybe this is just me.

I'm fine with this, personally. Would definitely prefer over anything like fc being used more widely.

@core-code
Copy link
Contributor

List of casks without an automatic livecheck strategy (2,319 total): https://gist.github.com/SeekingMeaning/6c7a6d072a562c6ac474d5c0bb558076

if you are looking for text patterns to find new versions of casks - i can contribute a few dozen of those.
patterns.txt

@reitermarkus
Copy link
Member

@SeekingMeaning, anything still missing here?

@MikeMcQuaid
Copy link
Member

@SeekingMeaning Would love to get this in in the nearish future. We can iterate on this in future PRs; I don't think this needs to be perfect to be merged.

@SeekingMeaning SeekingMeaning added the in progress Maintainers are working on this label Dec 8, 2020
@BrewTestBot
Copy link
Member

Review period ended.

@samford
Copy link
Member

samford commented Dec 11, 2020

With this having been rebased, I'll take another pass to review and test this as soon as I can. Thanks for helping to get this into shape, @reitermarkus.

@reitermarkus
Copy link
Member

I have removed version completely for now, since I don't think it is really that useful for casks to know whether the version actually increased.

SeekingMeaning and others added 24 commits December 12, 2020 17:43
- Skip blank lines in watchlist
- Initialize Livecheck#version as nil
- Simplify livecheck_version logic
- Make test a bit more understandable
When running `brew livecheck --cask` or `brew livecheck --formula`,
livecheck wasn't properly respecting these flags. It should have
worked by only including the casks or formulae in the watchlist but
instead these flags were treating all the names in the watchlist as
formulae or casks, which doesn't work properly.

This addresses the issue by always using `#to_formulae_and_casks`
on the watchlist names and then using `#reject` to conditionally
exclude formulae or casks based on whether the related flags were
passed to `brew livecheck`.
@samford samford merged commit baac12b into Homebrew:master Dec 12, 2020
@samford
Copy link
Member

samford commented Dec 12, 2020

Thanks, everyone!

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 12, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cask Homebrew Cask outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants