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: CPAN strategy #9510

Merged
merged 2 commits into from
Dec 12, 2020
Merged

livecheck: CPAN strategy #9510

merged 2 commits into from
Dec 12, 2020

Conversation

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

This adds a new livecheck strategy for software obtained from CPAN.

@maxim-belkin
Copy link
Member

Hey, @vladimyr! Thanks for the PR. What package can I test it with?
// btw, here is one typo -- "Exteract"

@vladimyr
Copy link
Contributor Author

Hi @maxim-belkin 👋

I'm AFK, going after typo later on and in the meantime, you can test it using the following packages:

  • btparse
  • carton
  • cpm
  • rex

Be aware that this is not an exhaustive list, just some examples I recalled from my testing spree.

Copy link
Member

@maxim-belkin maxim-belkin left a comment

Choose a reason for hiding this comment

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

Couple of general comments, but, in general a 👍🏻 from me.
I didn't comment on the part below package_path as I'm sure this is something that @samford and @nandahkrishna have to carefully review and approve.
But all in all, this is fantastic! It once again shows me that modular approach works great and it's easy to extend livecheck's support for specific sites on a rolling basis.
Thanks, @vladimyr!

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

There are currently five formulae in homebrew/core that have a stable URL using an archive file from CPAN:

  • btparse
  • carton
  • cpm
  • rex
  • sql-translator

I've tested a modified version of this strategy with my suggestions incorporated and this works as expected.


Testing this with the 255 unique cpan.metacpan.org URLs in homebrew/core formulae (out of 340 URLs and not just the five stable URLs), there were only two that didn't work. These were https://cpan.metacpan.org/authors/id/J/JS/JSTOWE/TermReadKey-2.37.tar.gz and https://cpan.metacpan.org/authors/id/J/JS/JSTOWE/TermReadKey-2.38.tar.gz.

The reason is because https://cpan.metacpan.org/authors/id/J/JS/JSTOWE/ isn't a directory listing page. Instead, page's title is "HAR DI HAR" and the body content is "HARR!". It seems like authors may be able to upload their own index.html file that prevents the directory listing from appearing.

Since this is the only outlier, I feel comfortable with my modified approach here. All the other URLs worked as expected without any additional modifications to the strategy beyond my suggestions here.

There were a few where the directory contained versions we wouldn't want (e.g., a mix of versions like 1.2.3 and date-based versions like 20201211) but we would simply address those using a livecheck block with a more specific regex, like we do with other strategies.

Library/Homebrew/livecheck/strategy/cpan.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/strategy/cpan.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/strategy/cpan.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/strategy/cpan.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/strategy/cpan.rb Outdated Show resolved Hide resolved
Library/Homebrew/livecheck/strategy/cpan.rb Outdated Show resolved Hide resolved
Library/Homebrew/test/livecheck/strategy/cpan_spec.rb Outdated Show resolved Hide resolved
Library/Homebrew/test/livecheck/strategy/cpan_spec.rb Outdated Show resolved Hide resolved
@samford
Copy link
Member

samford commented Dec 11, 2020

For what it's worth, if you agree with my suggested changes above, I can simply push my local changes to this branch and save you the trouble of incorporating them.

@vladimyr
Copy link
Contributor Author

vladimyr commented Dec 11, 2020

@samford I believe we are now on the same page? I think I addressed all your points, let me know if I accidentally missed something.

And I believe it goes without saying but still. Thank you for your patient and extensive guidance!

vladimyr and others added 2 commits December 11, 2020 17:43
Co-authored-by: Dario Vladovic <d.vladimyr@gmail.com>
@vladimyr vladimyr mentioned this pull request Dec 11, 2020
7 tasks
@samford
Copy link
Member

samford commented Dec 11, 2020

I'm good with where this is now and I don't see any other areas that could be improved. It's customary to not merge a Homebrew/brew PR until at least 24 hours have passed since it was opened (not sure what happened to our usual restriction here), so I'll merge this in a few hours.

Thanks, @vladimyr!

@vladimyr vladimyr mentioned this pull request Dec 12, 2020
7 tasks
@samford samford merged commit 52647e2 into Homebrew:master Dec 12, 2020
@vladimyr vladimyr deleted the livecheck-cpan branch December 12, 2020 04:14
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 11, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 11, 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

5 participants