Merge pull request #7347 from jonchang/jumbo-bottles-fix

pr-pull: eliminate another curl call

Library/Homebrew/dev-cmd/pr-pull.rb

@@ -1,18 +1,11 @@
 # frozen_string_literal: true
 
+require "download_strategy"
 require "cli/parser"
 require "utils/github"
 require "tmpdir"
 require "bintray"
 
-class CurlNoResumeDownloadStrategy < CurlDownloadStrategy
-  private
-
-  def _fetch(url:, resolved_url:)
-    curl("--location", "--remote-time", "--create-dirs", "--output", temporary_path, resolved_url)
-  end
-end
-
 module Homebrew
   module_function
 
@@ -148,6 +141,27 @@ module Homebrew
     nil
   end
 
+  def download_artifact(url, dir, pr)
+    token, username = GitHub.api_credentials
+    case GitHub.api_credentials_type
+    when :env_username_password, :keychain_username_password
+      curl_args = ["--user", "#{username}:#{token}"]
+    when :env_token
+      curl_args = ["--header", "Authorization: token #{token}"]
+    when :none
+      raise Error, "Credentials must be set to access the Artifacts API"
+    end
+
+    # Download the artifact as a zip file and unpack it into `dir`. This is
+    # preferred over system `curl` and `tar` as this leverages the Homebrew
+    # cache to avoid repeated downloads of (possibly large) bottles.
+    FileUtils.chdir dir do
+      downloader = GitHubArtifactDownloadStrategy.new(url, "artifact", pr, curl_args: curl_args, secrets: [token])
+      downloader.fetch
+      downloader.stage
+    end
+  end
+
   def pr_pull
     pr_pull_args.parse
 
@@ -163,7 +177,7 @@ module Homebrew
 
     workflow = args.workflow || "tests.yml"
     artifact = args.artifact || "bottles"
-    tap = Tap.fetch(args.tap || "homebrew/core")
+    tap = Tap.fetch(args.tap || CoreTap.instance.name)
 
     setup_git_environment!
 
@@ -187,9 +201,8 @@ module Homebrew
             next
           end
 
-          GitHub.fetch_artifact(user, repo, pr, dir, workflow_id:   workflow,
+          url = GitHub.get_artifact_url(user, repo, pr, workflow_id: workflow, artifact_name: artifact)
-                                                     artifact_name: artifact,
+          download_artifact(url, dir, pr)
-                                                     strategy:      CurlNoResumeDownloadStrategy)
 
           if Homebrew.args.dry_run?
             puts "brew bottle --merge --write #{Dir["*.json"].join " "}"
@@ -209,3 +222,32 @@ module Homebrew
     end
   end
 end
+
+class GitHubArtifactDownloadStrategy < AbstractFileDownloadStrategy
+  def fetch
+    ohai "Downloading #{url}"
+    if cached_location.exist?
+      puts "Already downloaded: #{cached_location}"
+    else
+      begin
+        curl "--location", "--create-dirs", "--output", temporary_path, url,
+             *meta.fetch(:curl_args, []),
+             secrets: meta.fetch(:secrets, [])
+      rescue ErrorDuringExecution
+        raise CurlDownloadStrategyError, url
+      end
+      ignore_interrupts do
+        cached_location.dirname.mkpath
+        temporary_path.rename(cached_location)
+        symlink_location.dirname.mkpath
+      end
+    end
+    FileUtils.ln_s cached_location.relative_path_from(symlink_location.dirname), symlink_location, force: true
+  end
+
+  private
+
+  def resolved_basename
+    "artifact.zip"
+  end
+end

Library/Homebrew/test/utils/github_spec.rb

@@ -42,17 +42,22 @@ describe GitHub do
     end
   end
 
-  describe "::fetch_artifact", :needs_network do
+  describe "::get_artifact_url", :needs_network do
     it "fails to find a nonexistant workflow" do
       expect {
-        subject.fetch_artifact("Homebrew", "homebrew-core", 1, ".")
+        subject.get_artifact_url("Homebrew", "homebrew-core", 1)
       }.to raise_error(/No matching workflow run found/)
     end
 
     it "fails to find artifacts that don't exist" do
       expect {
-        subject.fetch_artifact("Homebrew", "homebrew-core", 51971, ".", artifact_name: "false_bottles")
+        subject.get_artifact_url("Homebrew", "homebrew-core", 51971, artifact_name: "false_bottles")
       }.to raise_error(/No artifact .+ was found/)
     end
+
+    it "gets an artifact link" do
+      url = subject.get_artifact_url("Homebrew", "homebrew-core", 51971, artifact_name: "bottles")
+      expect(url).to eq("https://api.github.com/repos/Homebrew/homebrew-core/actions/artifacts/3557392/zip")
+    end
   end
 end

Library/Homebrew/utils/github.rb

@@ -1,6 +1,5 @@
 # frozen_string_literal: true
 
-require "download_strategy"
 require "tempfile"
 require "uri"
 
@@ -435,8 +434,7 @@ module GitHub
                   scopes:         CREATE_ISSUE_FORK_OR_PR_SCOPES)
   end
 
-  def fetch_artifact(user, repo, pr, dir,
+  def get_artifact_url(user, repo, pr, workflow_id: "tests.yml", artifact_name: "bottles")
-                     workflow_id: "tests.yml", artifact_name: "bottles", strategy: CurlDownloadStrategy)
     scopes = CREATE_ISSUE_FORK_OR_PR_SCOPES
     base_url = "#{API_URL}/repos/#{user}/#{repo}"
     pr_payload = open_api("#{base_url}/pulls/#{pr}", scopes: scopes)
@@ -479,28 +477,7 @@ module GitHub
       EOS
     end
 
-    artifact_url = artifact.first["archive_download_url"]
+    artifact.first["archive_download_url"]
-
-    token, username = api_credentials
-    case api_credentials_type
-    when :env_username_password, :keychain_username_password
-      curl_args = { user: "#{username}:#{token}" }
-    when :env_token
-      curl_args = { header: "Authorization: token #{token}" }
-    when :none
-      raise Error, "Credentials must be set to access the Artifacts API"
-    end
-
-    # Download the artifact as a zip file and unpack it into `dir`. This is
-    # preferred over system `curl` and `tar` as this leverages the Homebrew
-    # cache to avoid repeated downloads of (possibly large) bottles.
-    FileUtils.chdir dir do
-      curl_args[:cache] = Pathname.new(dir)
-      curl_args[:secrets] = [token]
-      downloader = strategy.new(artifact_url, "artifact", pr, **curl_args)
-      downloader.fetch
-      downloader.stage
-    end
   end
 
   def api_errors