Skip to content

bump-formula-pr: autobump livecheck-defined resources #20700

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

bump-formula-pr: autobump livecheck-defined resources #20700

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 91 additions & 51 deletions Library/Homebrew/dev-cmd/bump-formula-pr.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,12 +389,12 @@ def run
silent: args.quiet?,
ignore_non_pypi_packages: true

update_matching_version_resources! commit_formula,
version: new_formula_version.to_s
update_livecheck_defined_resources! commit_formula,
formula_version: new_formula_version.to_s
end

if resources_checked.nil? && commit_formula.resources.any? do |resource|
resource.livecheck.formula != :parent && !resource.name.start_with?("homebrew-")
!resource.livecheck_defined? && !resource.name.start_with?("homebrew-")
end
formula_pr_message += <<~EOS

Expand Down Expand Up @@ -557,60 +557,100 @@ def fetch_resource_and_forced_version(formula_or_resource, new_version, url, **s

sig {
params(
formula: Formula,
version: String,
formula: Formula,
formula_version: String,
).void
}
def update_matching_version_resources!(formula, version:)
formula.resources.select { |r| r.livecheck.formula == :parent }.each do |resource|
new_url = update_url(resource.url, resource.version.to_s, version)

if new_url == resource.url
opoo <<~EOS
You need to bump resource "#{resource.name}" manually since the new URL
and old URL are both:
#{new_url}
EOS
next
def update_livecheck_defined_resources!(formula, formula_version:)
formula.resources.each do |resource|
next unless resource.livecheck_defined?
next if resource.livecheck.skip?

old_url = resource.url
old_revision = resource.specs[:revision]
old_version = resource.version.to_s
new_version = formula_version

if resource.livecheck.formula != :parent
Comment on lines +572 to +574
Copy link
Member

@samford samford Oct 8, 2025

Choose a reason for hiding this comment

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

Suggested change
new_version = formula_version
if resource.livecheck.formula != :parent
if resource.livecheck.formula == :parent
new_version = formula_version
else

We only want to set new_version to formula_version when the livecheck block uses formula :parent, so an if/else setup may make this more explicit. The existing logic works but I think this suggestion is easier to follow.

require "livecheck/livecheck"
version_info = Homebrew::Livecheck.resource_version(resource, formula_version,
json: true,
debug: args.debug?,
quiet: args.quiet?,
verbose: args.verbose?)
new_version = version_info.dig(:version, :latest)
odie "Resource \"#{resource.name}\" livecheck failed to get latest version!" if new_version.blank?
next if old_version == new_version
Copy link
Member

@samford samford Oct 8, 2025

Choose a reason for hiding this comment

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

Suggested change
next if old_version == new_version
next if new_version == old_version

This may just be me but with comparisons, I prefer to put the variable that I'm interested in first. In this case, this suggestion would read as, "Don't proceed if the new version is the same as the old version" and I think that's easy to follow and makes it clear what we're interested in.

Copy link
Member Author

@cho-m cho-m Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, can update to this.

Unrelated but GitHub batch apply is suddenly not working so may have to apply each suggestion individually.

end

new_mirrors = resource.mirrors.map do |mirror|
update_url(mirror, resource.version.to_s, version)
if old_revision.present?
new_revision = if old_revision == old_version
forced_version = true
new_version
elsif (old_tag = resource.specs[:tag])
new_tag = old_tag.gsub(old_version, new_version)
odie "Resource \"#{resource.name}\" tag could not be updated!" if old_tag == new_tag
Copy link
Member

@samford samford Oct 8, 2025

Choose a reason for hiding this comment

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

Suggested change
odie "Resource \"#{resource.name}\" tag could not be updated!" if old_tag == new_tag
odie "Resource \"#{resource.name}\" tag could not be updated!" if new_tag == old_tag

Same here, if you agree with the previous comment.

resource_path, forced_version = fetch_resource_and_forced_version(resource, new_version, old_url,
tag: new_tag)
Utils.popen_read("git", "-C", resource_path.to_s, "rev-parse", "-q", "--verify", "HEAD").strip
else
odie <<~EOS
Unable to automatically bump resource "#{resource.name}"!
Either bump this formula manually or add support to `bump-formula-pr`.
EOS
end
# Allow skipping different tags on same revision
next if old_revision == new_revision
Copy link
Member

@samford samford Oct 8, 2025

Choose a reason for hiding this comment

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

Suggested change
next if old_revision == new_revision
next if new_revision == old_revision


inreplace_regex = /
^(?<ws>[ ]+)resource\ "#{resource.name}"\ do\s+
url\ "#{Regexp.escape(old_url)}",\s+
(using:[ ]+.*,\s+)?
(tag:[ ]+.*,\s+)?
Comment on lines +606 to +609
Copy link
Member

@samford samford Oct 8, 2025

Choose a reason for hiding this comment

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

Suggested change
^(?<ws>[ ]+)resource\ "#{resource.name}"\ do\s+
url\ "#{Regexp.escape(old_url)}",\s+
(using:[ ]+.*,\s+)?
(tag:[ ]+.*,\s+)?
^(?<ws> +)resource\ "#{resource.name}"\ do\s+
url\ "#{Regexp.escape(old_url)}",\s+
(using: +.*,\s+)?
(tag: +.*,\s+)?

Unless I'm overlooking something, I believe you should be able to use + here instead of [ ]+. The previous regex was using [ ]+ because the space was at the very start of the line but that's not the case here.

Copy link
Member Author

@cho-m cho-m Oct 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Extended mode (x) so either need \ or [ ]. Could switch to former as it is used it in other parts of regexp.

Copy link
Member

@MikeMcQuaid MikeMcQuaid Oct 8, 2025

Choose a reason for hiding this comment

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

I really don't think we should add more regexp for this when we have AST logic already. They will be brittle and error-prone. Ideally we'd move away from the existing regexp in bump* too because they predate our vendored AST logic.

Copy link
Member Author

@cho-m cho-m Oct 8, 2025

Choose a reason for hiding this comment

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

I really don't think we should add more regexp for this when we have AST logic already. They will be brittle and error-prone. Ideally we'd move away from the existing regexp in bump* too because they predate our vendored AST logic.

Sounds reasonable.

Won't have time to look into this so will close PR for now in case anyone else wants to work on it.

revision:\ "#{old_revision}"\n
(\s*version\ .*\n)?
/x

new_resource_block = "\\k<ws>resource \"#{resource.name}\" do\n"
new_resource_block += "\\k<ws> url \"#{old_url}\",\n"
new_resource_block += "\\k<ws> using: :#{resource.using},\n" if resource.using.is_a?(Symbol)
new_resource_block += "\\k<ws> tag: \"#{new_tag}\",\n" if new_tag
new_resource_block += "\\k<ws> revision: \"#{new_revision}\"\n"
new_resource_block += "\\k<ws> version \"#{new_version}\"\n" if forced_version
else
new_url = update_url(old_url, old_version, new_version)
if old_url == new_url
Copy link
Member

@samford samford Oct 8, 2025

Choose a reason for hiding this comment

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

Suggested change
if old_url == new_url
if new_url == old_url

opoo <<~EOS if resource.livecheck.formula == :parent
You need to bump resource "#{resource.name}" manually since the new URL
and old URL are both:
#{new_url}
EOS
next
end
new_mirrors = resource.mirrors.map do |mirror|
update_url(mirror, old_version, new_version)
end
resource_path, forced_version = fetch_resource_and_forced_version(resource, new_version, new_url)
Utils::Tar.validate_file(resource_path)
new_hash = resource_path.sha256

# Capture old_url to avoid possible HEAD resources
inreplace_regex = /
^(?<ws>[ ]+)resource\ "#{resource.name}"\ do\s+
Copy link
Member

@samford samford Oct 8, 2025

Choose a reason for hiding this comment

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

Suggested change
^(?<ws>[ ]+)resource\ "#{resource.name}"\ do\s+
^(?<ws> +)resource\ "#{resource.name}"\ do\s+

url\ "#{Regexp.escape(old_url)}"(?<using>,\ using:\ .*)?\s+
(mirror\ .*\s+)*
(version\ .*\s+)?
sha256\ .*\n
/x

new_resource_block = "\\k<ws>resource \"#{resource.name}\" do\n"
new_resource_block += "\\k<ws> url \"#{new_url}\"\\k<using>\n"
new_resource_block += new_mirrors.map { |m| "\\k<ws> mirror \"#{m}\"\n" }.join
new_resource_block += "\\k<ws> version \"#{new_version}\"\n" if forced_version
new_resource_block += "\\k<ws> sha256 \"#{new_hash}\"\n"
end
resource_path, forced_version = fetch_resource_and_forced_version(resource, version, new_url)
Utils::Tar.validate_file(resource_path)
new_hash = resource_path.sha256

inreplace_regex = /
[ ]+resource\ "#{resource.name}"\ do\s+
url\ .*\s+
(mirror\ .*\s+)*
sha256\ .*\s+
(version\ .*\s+)?
(\#.*\s+)*
livecheck\ do\s+
formula\ :parent\s+
end\s+
((\#.*\s+)*
patch\ (.*\ )?do\s+
url\ .*\s+
sha256\ .*\s+
end\s+)*
end\s
/x

leading_spaces = T.must(formula.path.read.match(/^( +)resource "#{resource.name}"/)).captures.first
new_resource_block = <<~EOS
#{leading_spaces}resource "#{resource.name}" do
#{leading_spaces} url "#{new_url}"#{new_mirrors.map { |m| "\n#{leading_spaces} mirror \"#{m}\"" }.join}
#{leading_spaces} sha256 "#{new_hash}"
#{"#{leading_spaces} version \"#{version}\"\n" if forced_version}
#{leading_spaces} livecheck do
#{leading_spaces} formula :parent
#{leading_spaces} end
#{leading_spaces}end
EOS

odebug "Updating \"#{resource.name}\" from #{old_version} to #{new_version}"
Utils::Inreplace.inreplace formula.path do |s|
s.sub! inreplace_regex, new_resource_block
end
Expand Down
Loading