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: Skip archived stable URLs in formulae #10208

Merged
merged 4 commits into from Jan 13, 2021
Merged

Livecheck: Skip archived stable URLs in formulae #10208

merged 4 commits into from Jan 13, 2021

Conversation

samford
Copy link
Member

@samford samford commented Jan 2, 2021

  • 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?
  • Have you successfully run brew man locally and committed any changes?

This PR modifies livecheck to automatically skip formulae that have a stable URL from the Google Code Archive or Internet Archive, as these won't receive updates. I checked the PR history for related formulae and didn't see any that were updated while they were in this state, so I think it's safe to skip these formulae by default.

I initially considered restricting these automatic skips to formulae with a related stable archive URL but no head URL, as it may be possible to check for a new version in an upstream repository. However, I looked through formulae with this criteria and the upstream repositories weren't useful (i.e., they simply contained older versions than what Google Code Archive provides).

That said, it's possible to override these automatic skips with a livecheck block (if there's another source to check), so this should be fine. However, if there was another source to check, we should just update the stable URL to use that instead (so these skips wouldn't apply in that case).


Some time ago, I created a related PR to remove Google Code Archive livecheck blocks from formulae: Homebrew/homebrew-core#64606

I'm planning to create a follow-up PR in homebrew-core to remove the livecheck blocks that skip some of these formulae, as they'll no longer be necessary.

@BrewTestBot
Copy link
Member

Review period will end on 2021-01-05 at 00:00:00 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 2, 2021
@BrewTestBot
Copy link
Member

Review period ended.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 5, 2021
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.

Looks good but might be worth splitting into separate function/functions in livecheck.rb

@samford
Copy link
Member Author

samford commented Jan 5, 2021

might be worth splitting into separate function/functions in livecheck.rb

The #skip_conditions method was originally a way of separating this [growing] logic from what's currently the #run_checks method. Is there value in decomposing #skip_conditions further?

If so, could you explain a bit more about what form you would prefer this to take (e.g., a separate method only for the "skipped" part of #skip_conditions or separate methods for each of the if blocks in #skip_conditions)?

@MikeMcQuaid
Copy link
Member

The #skip_conditions method was originally a way of separating this [growing] logic from what's currently the #run_checks method. Is there value in decomposing #skip_conditions further?

Yes, I think probably.

or separate methods for each of the if blocks in #skip_conditions)?

This, ideally, unless you see no chance this list will ever be added to.

@samford
Copy link
Member Author

samford commented Jan 8, 2021

The latest commit incorporates the following changes:

  • Refactor Livecheck#skip_conditions into a Homebrew::Livecheck::SkipConditions module.
  • Split the if blocks in #skip_conditions into separate module methods.
  • Modify #skip_conditions and the related condition methods to always return the hash from Livecheck#status_hash (or nil), instead of printing any output.
  • Rename #skip_conditions to #skip_information to better reflect that it only returns information (a hash) now, rather than printing anything.
  • Create a #print_skip_information method that prints the livecheck output for the various skip conditions when provided with the hash output from #skip_information. This allows us to keep the printing logic in SkipConditions, so the related code in Livecheck#run_checks is simple/minimal.
  • Move related tests from livecheck/livecheck_spec.rb to skip_conditions_spec.rb.
  • Rework the SkipConditions tests to account for the related changes (and refactor a bit for better organization).

@MikeMcQuaid I may take another pass at this later but I don't think there will be substantial changes (if any). Feel free to review whenever you have time.

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.

Nice work so far! Made a bunch of comments that apply elsewhere, have avoided repeating myself too much but assume I mean the same thing everywhere with e.g. nilable and unless

skip_result = skip_conditions(formula_or_cask, json: json, full_name: full_name, quiet: quiet)
next skip_result if skip_result != false
skip_info = SkipConditions.skip_information(formula_or_cask, full_name: full_name, verbose: verbose)
if skip_info.is_a?(Hash)
Copy link
Member

Choose a reason for hiding this comment

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

This type checking feels non-idiomatic , particularly considering it's a no-op if the type doesn't match.

Library/Homebrew/livecheck/skip_conditions.rb Show resolved Hide resolved
Library/Homebrew/livecheck/skip_conditions.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/skip_conditions.rb Outdated Show resolved Hide resolved
Comment on lines 34 to 35
skip_message = if formula_or_cask.livecheck.skip_msg.is_a?(String) &&
formula_or_cask.livecheck.skip_msg.present?
Copy link
Member

Choose a reason for hiding this comment

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

This type check also feels like a smell given it's a no-op when it's not matched

Library/Homebrew/livecheck/skip_conditions.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/skip_conditions.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/skip_conditions.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/skip_conditions.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/skip_conditions.rb Show resolved Hide resolved
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.

Thanks again @samford!

@nandahkrishna nandahkrishna mentioned this pull request Jan 13, 2021
10 tasks
@samford samford merged commit ec93e91 into Homebrew:master Jan 13, 2021
@samford samford deleted the livecheck-skip-archived-stable-urls branch January 13, 2021 13:43
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Feb 13, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Feb 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants