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

Enable use of latest formula version in resource livecheck URLs #14262

Merged

Conversation

nandahkrishna
Copy link
Member

@nandahkrishna nandahkrishna commented Dec 19, 2022

  • 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?

See Homebrew/homebrew-core#112155 (comment) for a great explanation from @samford. In short, we require access to the formula's latest version as found by livecheck in URLs for resource livecheck blocks – this is required for multiple formulae including influxdb, flux, luvit, etc.

This PR:

  • Adds a method to the livecheck DSL to return a placeholder string (<FORMULA_LATEST_VERSION>)
  • Enables the replacement of this placeholder string with a formula's latest version in its resources' livecheck URLs

I briefly tested this with the pkg-config-wrapper resource in influxdb and it seems to work fine.

The livecheck block for the resource:

livecheck do
  url "https://raw.githubusercontent.com/influxdata/influxdb/v#{latest_version}/go.mod"
  regex(/(pkg-config\sv)+(\d+(?:\.\d+)+)/i)
  strategy :page_match do |page, regex|
    page.scan(regex).map { |match| match[1] }
  end
end

The result:

...

Resource:         pkg-config-wrapper
Livecheckable?:   Yes

URL:              https://raw.githubusercontent.com/influxdata/influxdb/v<FORMULA_LATEST_VERSION>/go.mod
URL (processed):  https://raw.githubusercontent.com/influxdata/influxdb/v2.6.0/go.mod
Strategies:       PageMatch
Strategy:         PageMatch
Regex:            /(pkg-config\sv)+(\d+(?:\.\d+)+)/i

Matched Versions:
0.2.11 => #<Version:0x00007fc40c9d4608 @version="0.2.11", @detected_from_url=false>

...

  pkg-config-wrapper: 0.2.11 ==> 0.2.11

@BrewTestBot
Copy link
Member

Review period will end on 2022-12-20 at 05:21:52 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 19, 2022
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.

Nice work. This is in line with what I was thinking from our past discussion and it works as expected when I manually tested it.

Going through the changes, there may be something to be said for extracting the <FORMULA_LATEST_VERSION> string into a constant that can be used in both the Livecheck class (i.e., the DSL methods) and Livecheck#resource_version. I tinkered with this and the approach I ended up with involves creating a separate Homebrew::Livecheck::Constants module that can be loaded in both places.

That setup got me thinking about whether we should simply use the constant in resource livecheck blocks instead of a latest_version method. All the method buys us at that point is a lowercase name. However, it's a bit tricky to make the constant available within a livecheck block.

Luckily, that's something I experimented with in the past when I was trying to figure out how we could provide regex constants (i.e., regex-escaped strings that can be readily interpolated as well as full regex literals). I dug up my work from last year and basically I was taking a similar approach, where the constants were in a module. The way I made them available to livecheck blocks was by including the module in the Formula#livecheck method (after the early return but before @livecheck.instance_eval(&block) is called).

I've pushed a branch with a follow-up commit that uses this approach in my brew fork, if you would like to take a look and let me know what you think. The only public-facing difference is that the URL in the example livecheck block would be "https://raw.githubusercontent.com/influxdata/influxdb/v#{LATEST_VERSION}/go.mod" instead.

My thinking is that the constant approach would also lay the groundwork for regex constants (if we want to go that route) and it would be as simple as adding more values to Homebrew::Livecheck::Constants (i.e., no need to add methods to the DSL). It would also end up being more consistent for maintainers/contributors, as these would all be constants (i.e., we wouldn't have to remember that latest_version is a method but the regexes are constants).

@nandahkrishna
Copy link
Member Author

My thinking is that the constant approach would also lay the groundwork for regex constants (if we want to go that route) and it would be as simple as adding more values to Homebrew::Livecheck::Constants (i.e., no need to add methods to the DSL). It would also end up being more consistent for maintainers/contributors, as these would all be constants (i.e., we wouldn't have to remember that latest_version is a method but the regexes are constants).

I agree with this, we should make it as easy as possible for contributors to look at/modify/use these placeholders. Using constants (uppercase) instead of methods (lowercase) isn't less elegant at all, in my opinion.

I've pushed your commit to this PR branch as I'm in favour of the approach you suggested.

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, Nanda!

I believe we'll have to wait for this to be included in a new brew release before we can start using it in formulae but I'll try to take another pass through the related homebrew/core PRs sometime tomorrow (unless you beat me to it).

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 20, 2022
@BrewTestBot
Copy link
Member

Review period ended.

@nandahkrishna nandahkrishna merged commit 30c3fc5 into Homebrew:master Dec 20, 2022
@nandahkrishna nandahkrishna deleted the resource-livecheck-formula-latest branch December 20, 2022 06:02
@github-actions github-actions bot added the outdated PR was locked due to age label Jan 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 20, 2023
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

4 participants