From 54e15cf3612f6ff4c585c8903b180e43b4622422 Mon Sep 17 00:00:00 2001 From: Sean Molenaar Date: Mon, 10 Jan 2022 18:57:56 +0200 Subject: [PATCH 1/4] pr-pull: allow pulling casks --- Library/Homebrew/dev-cmd/pr-pull.rb | 129 +++++++++++------- Library/Homebrew/test/dev-cmd/pr-pull_spec.rb | 85 +++++++++++- 2 files changed, 163 insertions(+), 51 deletions(-) diff --git a/Library/Homebrew/dev-cmd/pr-pull.rb b/Library/Homebrew/dev-cmd/pr-pull.rb index be03cea20d..78f3bf9546 100644 --- a/Library/Homebrew/dev-cmd/pr-pull.rb +++ b/Library/Homebrew/dev-cmd/pr-pull.rb @@ -108,51 +108,61 @@ module Homebrew end end - def determine_bump_subject(old_contents, new_contents, formula_path, reason: nil) - formula_path = Pathname(formula_path) - formula_name = formula_path.basename.to_s.chomp(".rb") + def get_package(tap, subject_name, subject_path, content) + if subject_path.dirname == tap.cask_dir + return begin + Cask::CaskLoader.load(content.dup) + rescue Cask::CaskUnavailableError + nil + end + end - new_formula = begin - Formulary.from_contents(formula_name, formula_path, new_contents, :stable) + begin + Formulary.from_contents(subject_name, subject_path, content, :stable) rescue FormulaUnavailableError nil end + end - return "#{formula_name}: delete #{reason}".strip if new_formula.blank? + def determine_bump_subject(old_contents, new_contents, subject_path, reason: nil) + subject_path = Pathname(subject_path) + tap = Tap.from_path(subject_path) + subject_name = subject_path.basename.to_s.chomp(".rb") + name = subject_path.dirname == tap.cask_dir ? "cask" : "formula" - old_formula = begin - Formulary.from_contents(formula_name, formula_path, old_contents, :stable) - rescue FormulaUnavailableError - nil - end + new_package = get_package(tap, subject_name, subject_path, new_contents) - return "#{formula_name} #{new_formula.stable.version} (new formula)" if old_formula.blank? + return "#{subject_name}: delete #{reason}".strip if new_package.blank? - if old_formula.stable.version != new_formula.stable.version - "#{formula_name} #{new_formula.stable.version}" - elsif old_formula.revision != new_formula.revision - "#{formula_name}: revision #{reason}".strip + old_package = get_package(tap, subject_name, subject_path, old_contents) + + return "#{subject_name} #{new_package.version} (new #{name})" if old_package.blank? + + if old_package.version != new_package.version + "#{subject_name} #{new_package.version}" + elsif old_package.respond_to?(:revision) && old_package.revision != new_package.revision + "#{subject_name}: revision #{reason}".strip else - "#{formula_name}: #{reason || "rebuild"}".strip + "#{subject_name}: #{reason || "rebuild"}".strip end end # Cherry picks a single commit that modifies a single file. # Potentially rewords this commit using {determine_bump_subject}. - def reword_formula_commit(commit, file, reason: "", verbose: false, resolve: false, path: ".") - formula_file = Pathname.new(path) / file - formula_name = formula_file.basename.to_s.chomp(".rb") + def reword_package_commit(commit, file, reason: "", verbose: false, resolve: false, path: ".") + package_file = Pathname.new(path) / file + package_name = package_file.basename.to_s.chomp(".rb") - odebug "Cherry-picking #{formula_file}: #{commit}" + odebug "Cherry-picking #{package_file}: #{commit}" Utils::Git.cherry_pick!(path, commit, verbose: verbose, resolve: resolve) - old_formula = Utils::Git.file_at_commit(path, file, "HEAD^") - new_formula = Utils::Git.file_at_commit(path, file, "HEAD") + old_package = Utils::Git.file_at_commit(path, file, "HEAD^") + new_package = Utils::Git.file_at_commit(path, file, "HEAD") - bump_subject = determine_bump_subject(old_formula, new_formula, formula_file, reason: reason).strip + bump_subject = determine_bump_subject(old_package, new_package, package_file, reason: reason).strip subject, body, trailers = separate_commit_message(path.git_commit_message) - if subject != bump_subject && !subject.start_with?("#{formula_name}:") + if subject != bump_subject && !subject.start_with?("#{package_name}:") safe_system("git", "-C", path, "commit", "--amend", "-q", "-m", bump_subject, "-m", subject, "-m", body, "-m", trailers) ohai bump_subject @@ -164,7 +174,7 @@ module Homebrew # Cherry picks multiple commits that each modify a single file. # Words the commit according to {determine_bump_subject} with the body # corresponding to all the original commit messages combined. - def squash_formula_commits(commits, file, reason: "", verbose: false, resolve: false, path: ".") + def squash_package_commits(commits, file, reason: "", verbose: false, resolve: false, path: ".") odebug "Squashing #{file}: #{commits.join " "}" # Format commit messages into something similar to `git fmt-merge-message`. @@ -197,10 +207,10 @@ module Homebrew Utils::Git.cherry_pick!(path, "--no-commit", *commits, verbose: verbose, resolve: resolve) # Determine the bump subject by comparing the original state of the tree to its current state. - formula_file = Pathname.new(path) / file - old_formula = Utils::Git.file_at_commit(path, file, "#{commits.first}^") - new_formula = File.read(formula_file) - bump_subject = determine_bump_subject(old_formula, new_formula, formula_file, reason: reason) + package_file = Pathname.new(path) / file + old_package = Utils::Git.file_at_commit(path, file, "#{commits.first}^") + new_package = File.read(package_file) + bump_subject = determine_bump_subject(old_package, new_package, package_file, reason: reason) # Commit with the new subject, body, and trailers. safe_system("git", "-C", path, "commit", "--quiet", @@ -215,7 +225,7 @@ module Homebrew commits = Utils.safe_popen_read("git", "-C", tap.path, "rev-list", "--reverse", "#{original_commit}..HEAD").lines.map(&:strip) - # Generate a bidirectional mapping of commits <=> formula files. + # Generate a bidirectional mapping of commits <=> formula/cask files. files_to_commits = {} commits_to_files = commits.to_h do |commit| files = Utils.safe_popen_read("git", "-C", tap.path, "diff-tree", "--diff-filter=AMD", @@ -223,10 +233,13 @@ module Homebrew files.each do |file| files_to_commits[file] ||= [] files_to_commits[file] << commit - next if (tap.path/file).dirname == tap.formula_dir && File.extname(file) == ".rb" + tap_file = tap.path/file + if (tap_file.dirname == tap.formula_dir || tap_file.dirname == tap.cask_dir) && File.extname(file) == ".rb" + next + end odie <<~EOS - Autosquash can't squash commits that modify non-formula files. + Autosquash can only squash commits that modify formula or cask files. File: #{file} Commit: #{commit} EOS @@ -246,13 +259,13 @@ module Homebrew files = commits_to_files[commit] if files.length == 1 && files_to_commits[files.first].length == 1 # If there's a 1:1 mapping of commits to files, just cherry pick and (maybe) reword. - reword_formula_commit(commit, files.first, path: tap.path, reason: reason, verbose: verbose, resolve: resolve) + reword_package_commit(commit, files.first, path: tap.path, reason: reason, verbose: verbose, resolve: resolve) processed_commits << commit elsif files.length == 1 && files_to_commits[files.first].length > 1 # If multiple commits modify a single file, squash them down into a single commit. file = files.first commits = files_to_commits[file] - squash_formula_commits(commits, file, path: tap.path, reason: reason, verbose: verbose, resolve: resolve) + squash_package_commits(commits, file, path: tap.path, reason: reason, verbose: verbose, resolve: resolve) processed_commits += commits else # We can't split commits (yet) so just raise an error. @@ -293,27 +306,45 @@ module Homebrew return false if labels.include?("CI-syntax-only") || labels.include?("CI-no-bottles") - changed_formulae(tap, original_commit).any? do |f| - !f.bottle_unneeded? && !f.bottle_disabled? + changed_packages(tap, original_commit).any? do |f| + !f.instance_of?(Cask::Cask) && !f.bottle_unneeded? && !f.bottle_disabled? end end - def changed_formulae(tap, original_commit) - if Homebrew::EnvConfig.disable_load_formula? - opoo "Can't check if updated bottles are necessary as HOMEBREW_DISABLE_LOAD_FORMULA is set!" - return - end - - Utils.popen_read("git", "-C", tap.path, "diff-tree", - "-r", "--name-only", "--diff-filter=AM", - original_commit, "HEAD", "--", tap.formula_dir) - .lines - .map do |line| + def changed_packages(tap, original_commit) + formulae = Utils.popen_read("git", "-C", tap.path, "diff-tree", + "-r", "--name-only", "--diff-filter=AM", + original_commit, "HEAD", "--", tap.formula_dir) + .lines + .map do |line| next unless line.end_with? ".rb\n" name = "#{tap.name}/#{File.basename(line.chomp, ".rb")}" - Formula[name] + if Homebrew::EnvConfig.disable_load_formula? + opoo "Can't check if updated bottles are necessary as HOMEBREW_DISABLE_LOAD_FORMULA is set!" + break + end + begin + Formulary.resolve(name) + rescue FormulaUnavailableError + nil + end end.compact + casks = Utils.popen_read("git", "-C", tap.path, "diff-tree", + "-r", "--name-only", "--diff-filter=AM", + original_commit, "HEAD", "--", tap.cask_dir) + .lines + .map do |line| + next unless line.end_with? ".rb\n" + + name = "#{tap.name}/#{File.basename(line.chomp, ".rb")}" + begin + Cask::CaskLoader.load(name) + rescue Cask::CaskUnavailableError + nil + end + end.compact + formulae + casks end def download_artifact(url, dir, pr) diff --git a/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb b/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb index 1b6e68c929..955739b872 100644 --- a/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb +++ b/Library/Homebrew/test/dev-cmd/pr-pull_spec.rb @@ -40,11 +40,38 @@ describe "brew pr-pull" do end EOS end + let(:cask_rebuild) do + <<~EOS + cask "food" do + desc "Helpful description" + version "1.0" + url "https://brew.sh/food-\#{version}.tgz" + end + EOS + end + let(:cask_version) do + <<~EOS + cask "food" do + version "2.0" + url "https://brew.sh/food-\#{version}.tgz" + end + EOS + end + let(:cask) do + <<~EOS + cask "food" do + version "1.0" + url "https://brew.sh/food-\#{version}.tgz" + end + EOS + end let(:tap) { Tap.fetch("Homebrew", "foo") } let(:formula_file) { tap.path/"Formula/foo.rb" } + let(:cask_file) { tap.cask_dir/"food.rb" } + let(:path) { (Tap::TAP_DIRECTORY/"homebrew/homebrew-foo").extend(GitRepositoryExtension) } describe "#autosquash!" do - it "squashes a formula correctly" do + it "squashes a formula or cask correctly" do secondary_author = "Someone Else " (tap.path/"Formula").mkpath formula_file.write(formula) @@ -61,11 +88,26 @@ describe "brew pr-pull" do expect(tap.path.git_commit_message).to include("foo 2.0") expect(tap.path.git_commit_message).to include("Co-authored-by: #{secondary_author}") end + + (path/"Casks").mkpath + cask_file.write(cask) + cd path do + safe_system Utils::Git.git, "add", cask_file + safe_system Utils::Git.git, "commit", "-m", "food 1.0 (new cask)" + original_hash = `git rev-parse HEAD`.chomp + File.write(cask_file, cask_rebuild) + safe_system Utils::Git.git, "commit", cask_file, "-m", "rebuild" + File.write(cask_file, cask_version) + safe_system Utils::Git.git, "commit", cask_file, "-m", "version", "--author=#{secondary_author}" + described_class.autosquash!(original_hash, tap: tap) + expect(path.git_commit_message).to include("food 2.0") + expect(path.git_commit_message).to include("Co-authored-by: #{secondary_author}") + end end end describe "#signoff!" do - it "signs off a formula" do + it "signs off a formula or cask" do (tap.path/"Formula").mkpath formula_file.write(formula) cd tap.path do @@ -75,6 +117,33 @@ describe "brew pr-pull" do end described_class.signoff!(tap.path) expect(tap.path.git_commit_message).to include("Signed-off-by:") + + (path/"Casks").mkpath + cask_file.write(cask) + cd path do + safe_system Utils::Git.git, "add", cask_file + safe_system Utils::Git.git, "commit", "-m", "food 1.0 (new cask)" + end + described_class.signoff!(tap.path) + expect(tap.path.git_commit_message).to include("Signed-off-by:") + end + end + + describe "#get_package" do + it "returns a formula" do + expect(described_class.get_package(tap, "foo", formula_file, formula)).to be_a(Formula) + end + + it "returns nil for an unknown formula" do + expect(described_class.get_package(tap, "foo", formula_file, "")).to be_nil + end + + it "returns a cask" do + expect(described_class.get_package(tap, "foo", cask_file, cask)).to be_a(Cask::Cask) + end + + it "returns nil for an unknown cask" do + expect(described_class.get_package(tap, "foo", cask_file, "")).to be_nil end end @@ -83,10 +152,18 @@ describe "brew pr-pull" do expect(described_class.determine_bump_subject("", formula, formula_file)).to eq("foo 1.0 (new formula)") end + it "correctly bumps a new cask" do + expect(described_class.determine_bump_subject("", cask, cask_file)).to eq("food 1.0 (new cask)") + end + it "correctly bumps a formula version" do expect(described_class.determine_bump_subject(formula, formula_version, formula_file)).to eq("foo 2.0") end + it "correctly bumps a cask version" do + expect(described_class.determine_bump_subject(cask, cask_version, cask_file)).to eq("food 2.0") + end + it "correctly bumps a formula revision with reason" do expect(described_class.determine_bump_subject( formula, formula_revision, formula_file, reason: "for fun" @@ -100,6 +177,10 @@ describe "brew pr-pull" do it "correctly bumps a formula deletion" do expect(described_class.determine_bump_subject(formula, "", formula_file)).to eq("foo: delete") end + + it "correctly bumps a cask deletion" do + expect(described_class.determine_bump_subject(cask, "", cask_file)).to eq("food: delete") + end end end end From 0d8a3d8041f82063b182cec7f68d7655446020eb Mon Sep 17 00:00:00 2001 From: Sean Molenaar Date: Mon, 4 Apr 2022 13:24:05 +0200 Subject: [PATCH 2/4] pr-pull: fix style Co-authored-by: Mike McQuaid --- Library/Homebrew/dev-cmd/pr-pull.rb | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/Library/Homebrew/dev-cmd/pr-pull.rb b/Library/Homebrew/dev-cmd/pr-pull.rb index 78f3bf9546..070c5828a6 100644 --- a/Library/Homebrew/dev-cmd/pr-pull.rb +++ b/Library/Homebrew/dev-cmd/pr-pull.rb @@ -110,11 +110,12 @@ module Homebrew def get_package(tap, subject_name, subject_path, content) if subject_path.dirname == tap.cask_dir - return begin + cask = begin Cask::CaskLoader.load(content.dup) rescue Cask::CaskUnavailableError nil end + return cask end begin @@ -128,7 +129,8 @@ module Homebrew subject_path = Pathname(subject_path) tap = Tap.from_path(subject_path) subject_name = subject_path.basename.to_s.chomp(".rb") - name = subject_path.dirname == tap.cask_dir ? "cask" : "formula" + is_cask = subject_path.dirname == tap.cask_dir + name = is_cask ? "cask" : "formula" new_package = get_package(tap, subject_name, subject_path, new_contents) @@ -136,11 +138,11 @@ module Homebrew old_package = get_package(tap, subject_name, subject_path, old_contents) - return "#{subject_name} #{new_package.version} (new #{name})" if old_package.blank? - - if old_package.version != new_package.version + if old_package.blank? + "#{subject_name} #{new_package.version} (new #{name})" + elsif old_package.version != new_package.version "#{subject_name} #{new_package.version}" - elsif old_package.respond_to?(:revision) && old_package.revision != new_package.revision + elsif !is_cask && old_package.revision != new_package.revision "#{subject_name}: revision #{reason}".strip else "#{subject_name}: #{reason || "rebuild"}".strip @@ -209,7 +211,7 @@ module Homebrew # Determine the bump subject by comparing the original state of the tree to its current state. package_file = Pathname.new(path) / file old_package = Utils::Git.file_at_commit(path, file, "#{commits.first}^") - new_package = File.read(package_file) + new_package = package_file.read bump_subject = determine_bump_subject(old_package, new_package, package_file, reason: reason) # Commit with the new subject, body, and trailers. @@ -234,7 +236,8 @@ module Homebrew files_to_commits[file] ||= [] files_to_commits[file] << commit tap_file = tap.path/file - if (tap_file.dirname == tap.formula_dir || tap_file.dirname == tap.cask_dir) && File.extname(file) == ".rb" + if (tap_file.dirname == tap.formula_dir || tap_file.dirname == tap.cask_dir) && + File.extname(file) == ".rb" next end From 08341d1c8b74ce21251cdd1328ec6ab7d3e1ec52 Mon Sep 17 00:00:00 2001 From: Sean Molenaar Date: Mon, 4 Apr 2022 14:14:00 +0200 Subject: [PATCH 3/4] pr-pull: whitespace fix --- Library/Homebrew/dev-cmd/pr-pull.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/dev-cmd/pr-pull.rb b/Library/Homebrew/dev-cmd/pr-pull.rb index 070c5828a6..1f42bec812 100644 --- a/Library/Homebrew/dev-cmd/pr-pull.rb +++ b/Library/Homebrew/dev-cmd/pr-pull.rb @@ -139,7 +139,7 @@ module Homebrew old_package = get_package(tap, subject_name, subject_path, old_contents) if old_package.blank? - "#{subject_name} #{new_package.version} (new #{name})" + "#{subject_name} #{new_package.version} (new #{name})" elsif old_package.version != new_package.version "#{subject_name} #{new_package.version}" elsif !is_cask && old_package.revision != new_package.revision From 0ec9cf721c8e99d76c290ef6c2cc765171318142 Mon Sep 17 00:00:00 2001 From: Sean Molenaar Date: Mon, 4 Apr 2022 14:19:39 +0200 Subject: [PATCH 4/4] pr-pull: fix trailing whitespace --- Library/Homebrew/dev-cmd/pr-pull.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/dev-cmd/pr-pull.rb b/Library/Homebrew/dev-cmd/pr-pull.rb index 1f42bec812..a2c7e6b4be 100644 --- a/Library/Homebrew/dev-cmd/pr-pull.rb +++ b/Library/Homebrew/dev-cmd/pr-pull.rb @@ -236,7 +236,7 @@ module Homebrew files_to_commits[file] ||= [] files_to_commits[file] << commit tap_file = tap.path/file - if (tap_file.dirname == tap.formula_dir || tap_file.dirname == tap.cask_dir) && + if (tap_file.dirname == tap.formula_dir || tap_file.dirname == tap.cask_dir) && File.extname(file) == ".rb" next end