Merge pull request #4585 from reitermarkus/refactor-download-strategies

Refactor download strategies.

Library/Homebrew/download_strategy.rb

@@ -14,14 +14,13 @@ class AbstractDownloadStrategy
     end
   end
 
-  attr_reader :meta, :name, :version, :resource
+  attr_reader :meta, :name, :version
   attr_reader :shutup
 
-  def initialize(name, resource)
+  def initialize(name, version, resource)
     @name = name
-    @resource = resource
     @url = resource.url
-    @version = resource.version
+    @version = version
     @meta = resource.specs
     @shutup = false
     extend Pourable if meta[:bottle]
@@ -91,7 +90,7 @@ end
 class VCSDownloadStrategy < AbstractDownloadStrategy
   REF_TYPES = [:tag, :branch, :revisions, :revision].freeze
 
-  def initialize(name, resource)
+  def initialize(name, version, resource)
     super
     @ref_type, @ref = extract_ref(meta)
     @revision = meta[:revision]
@@ -137,7 +136,9 @@ class VCSDownloadStrategy < AbstractDownloadStrategy
     @clone
   end
 
-  delegate head?: :version
+  def head?
+    version.respond_to?(:head?) && version.head?
+  end
 
   # Return last commit's unique identifier for the repository.
   # Return most recent modified timestamp unless overridden.
@@ -208,7 +209,7 @@ end
 class CurlDownloadStrategy < AbstractFileDownloadStrategy
   attr_reader :mirrors, :tarball_path, :temporary_path
 
-  def initialize(name, resource)
+  def initialize(name, version, resource)
     super
     @mirrors = resource.mirrors.dup
     @tarball_path = HOMEBREW_CACHE/"#{name}-#{version}#{ext}"
@@ -255,6 +256,8 @@ class CurlDownloadStrategy < AbstractFileDownloadStrategy
       ohai "Downloading from #{url}"
     end
 
+    temporary_path.dirname.mkpath
+
     curl_download resolved_url(url), to: temporary_path
   end
 
@@ -386,7 +389,7 @@ class GitHubPrivateRepositoryDownloadStrategy < CurlDownloadStrategy
   require "utils/formatter"
   require "utils/github"
 
-  def initialize(name, resource)
+  def initialize(name, version, resource)
     super
     parse_url_pattern
     set_github_token
@@ -492,7 +495,7 @@ end
 class ScpDownloadStrategy < AbstractFileDownloadStrategy
   attr_reader :tarball_path, :temporary_path
 
-  def initialize(name, resource)
+  def initialize(name, version, resource)
     super
     @tarball_path = HOMEBREW_CACHE/"#{name}-#{version}#{ext}"
     @temporary_path = Pathname.new("#{cached_location}.incomplete")
@@ -543,7 +546,7 @@ class ScpDownloadStrategy < AbstractFileDownloadStrategy
 end
 
 class SubversionDownloadStrategy < VCSDownloadStrategy
-  def initialize(name, resource)
+  def initialize(name, version, resource)
     super
     @url = @url.sub("svn+http://", "")
   end
@@ -628,7 +631,7 @@ class GitDownloadStrategy < VCSDownloadStrategy
     %r{http://llvm\.org},
   ].freeze
 
-  def initialize(name, resource)
+  def initialize(name, version, resource)
     super
     @ref_type ||= :branch
     @ref ||= "master"
@@ -800,7 +803,7 @@ class GitDownloadStrategy < VCSDownloadStrategy
 end
 
 class GitHubGitDownloadStrategy < GitDownloadStrategy
-  def initialize(name, resource)
+  def initialize(name, version, resource)
     super
 
     return unless %r{^https?://github\.com/(?<user>[^/]+)/(?<repo>[^/]+)\.git$} =~ @url
@@ -854,7 +857,7 @@ class GitHubGitDownloadStrategy < GitDownloadStrategy
 end
 
 class CVSDownloadStrategy < VCSDownloadStrategy
-  def initialize(name, resource)
+  def initialize(name, version, resource)
     super
     @url = @url.sub(%r{^cvs://}, "")
 
@@ -924,7 +927,7 @@ class CVSDownloadStrategy < VCSDownloadStrategy
 end
 
 class MercurialDownloadStrategy < VCSDownloadStrategy
-  def initialize(name, resource)
+  def initialize(name, version, resource)
     super
     @url = @url.sub(%r{^hg://}, "")
   end
@@ -980,7 +983,7 @@ class MercurialDownloadStrategy < VCSDownloadStrategy
 end
 
 class BazaarDownloadStrategy < VCSDownloadStrategy
-  def initialize(name, resource)
+  def initialize(name, version, resource)
     super
     @url.sub!(%r{^bzr://}, "")
     ENV["BZR_HOME"] = HOMEBREW_TEMP
@@ -1031,7 +1034,7 @@ class BazaarDownloadStrategy < VCSDownloadStrategy
 end
 
 class FossilDownloadStrategy < VCSDownloadStrategy
-  def initialize(name, resource)
+  def initialize(name, version, resource)
     super
     @url = @url.sub(%r{^fossil://}, "")
   end

Library/Homebrew/formulary.rb

@@ -105,7 +105,7 @@ module Formulary
         formula_name = File.basename(bottle_name)[/(.+)-/, 1]
         resource = Resource.new(formula_name) { url bottle_name }
         resource.specs[:bottle] = true
-        downloader = CurlDownloadStrategy.new resource.name, resource
+        downloader = resource.downloader
         cached = downloader.cached_location.exist?
         downloader.fetch
         ohai "Pouring the cached bottle" if cached

Library/Homebrew/resource.rb

@@ -30,10 +30,6 @@ class Resource
       @resource.specs
     end
 
-    def version
-      @resource.version
-    end
-
     def mirrors
       @resource.mirrors
     end
@@ -57,7 +53,7 @@ class Resource
   end
 
   def downloader
-    download_strategy.new(download_name, Download.new(self))
+    download_strategy.new(download_name, version, Download.new(self))
   end
 
   # Removes /s from resource names; this allows go package names
@@ -68,7 +64,9 @@ class Resource
   end
 
   def download_name
-    name.nil? ? owner.name : "#{owner.name}--#{escaped_name}"
+    return owner.name if name.nil?
+    return escaped_name if owner.nil?
+    "#{owner.name}--#{escaped_name}"
   end
 
   def cached_download

Library/Homebrew/test/download_strategies_spec.rb

@@ -1,11 +1,12 @@
 require "download_strategy"
 
 describe AbstractDownloadStrategy do
-  subject { described_class.new(name, resource) }
+  subject { described_class.new(name, version, resource) }
 
   let(:specs) { {} }
   let(:name) { "foo" }
   let(:url) { "http://example.com/foo.tar.gz" }
+  let(:version) { nil }
   let(:resource) { double(Resource, url: url, mirrors: [], specs: specs, version: nil) }
   let(:args) { %w[foo bar baz] }
 
@@ -35,22 +36,24 @@ end
 
 describe VCSDownloadStrategy do
   let(:url) { "http://example.com/bar" }
-  let(:resource) { double(Resource, url: url, mirrors: [], specs: {}, version: nil) }
+  let(:version) { nil }
+  let(:resource) { double(Resource, url: url, mirrors: [], specs: {}) }
 
   describe "#cached_location" do
     it "returns the path of the cached resource" do
       allow_any_instance_of(described_class).to receive(:cache_tag).and_return("foo")
-      downloader = described_class.new("baz", resource)
+      downloader = described_class.new("baz", version, resource)
       expect(downloader.cached_location).to eq(HOMEBREW_CACHE/"baz--foo")
     end
   end
 end
 
 describe GitHubPrivateRepositoryDownloadStrategy do
-  subject { described_class.new("foo", resource) }
+  subject { described_class.new("foo", version, resource) }
 
   let(:url) { "https://github.com/owner/repo/archive/1.1.5.tar.gz" }
-  let(:resource) { double(Resource, url: url, mirrors: [], specs: {}, version: nil) }
+  let(:version) { nil }
+  let(:resource) { double(Resource, url: url, mirrors: [], specs: {}) }
 
   before do
     ENV["HOMEBREW_GITHUB_API_TOKEN"] = "token"
@@ -71,10 +74,11 @@ describe GitHubPrivateRepositoryDownloadStrategy do
 end
 
 describe GitHubPrivateRepositoryReleaseDownloadStrategy do
-  subject { described_class.new("foo", resource) }
+  subject { described_class.new("foo", version, resource) }
 
   let(:url) { "https://github.com/owner/repo/releases/download/tag/foo_v0.1.0_darwin_amd64.tar.gz" }
-  let(:resource) { double(Resource, url: url, mirrors: [], specs: {}, version: nil) }
+  let(:version) { nil }
+  let(:resource) { double(Resource, url: url, mirrors: [], specs: {}) }
 
   before do
     ENV["HOMEBREW_GITHUB_API_TOKEN"] = "token"
@@ -122,11 +126,12 @@ describe GitHubPrivateRepositoryReleaseDownloadStrategy do
 end
 
 describe GitHubGitDownloadStrategy do
-  subject { described_class.new(name, resource) }
+  subject { described_class.new(name, version, resource) }
 
   let(:name) { "brew" }
   let(:url) { "https://github.com/homebrew/brew.git" }
-  let(:resource) { double(Resource, url: url, mirrors: [], specs: {}, version: nil) }
+  let(:version) { nil }
+  let(:resource) { double(Resource, url: url, mirrors: [], specs: {}) }
 
   it "parses the URL and sets the corresponding instance variables" do
     expect(subject.instance_variable_get(:@user)).to eq("homebrew")
@@ -135,11 +140,12 @@ describe GitHubGitDownloadStrategy do
 end
 
 describe GitDownloadStrategy do
-  subject { described_class.new(name, resource) }
+  subject { described_class.new(name, version, resource) }
 
   let(:name) { "baz" }
   let(:url) { "https://github.com/homebrew/foo" }
-  let(:resource) { double(Resource, url: url, mirrors: [], specs: {}, version: nil) }
+  let(:version) { nil }
+  let(:resource) { double(Resource, url: url, mirrors: [], specs: {}) }
   let(:cached_location) { subject.cached_location }
 
   before do
@@ -202,14 +208,15 @@ describe GitDownloadStrategy do
 end
 
 describe S3DownloadStrategy do
-  subject { described_class.new(name, resource) }
+  subject { described_class.new(name, version, resource) }
 
   let(:name) { "foo" }
   let(:url) { "http://bucket.s3.amazonaws.com/foo.tar.gz" }
-  let(:resource) { double(Resource, url: url, mirrors: [], specs: {}, version: nil) }
+  let(:version) { nil }
+  let(:resource) { double(Resource, url: url, mirrors: [], specs: {}) }
 
   describe "#_fetch" do
-    subject { described_class.new(name, resource)._fetch }
+    subject { described_class.new(name, version, resource)._fetch }
 
     context "when given Bad S3 URL" do
       let(:url) { "http://example.com/foo.tar.gz" }
@@ -224,18 +231,19 @@ describe S3DownloadStrategy do
 end
 
 describe CurlDownloadStrategy do
-  subject { described_class.new(name, resource) }
+  subject { described_class.new(name, version, resource) }
 
   let(:name) { "foo" }
   let(:url) { "http://example.com/foo.tar.gz" }
-  let(:resource) { double(Resource, url: url, mirrors: [], specs: { user: "download:123456" }, version: nil) }
+  let(:version) { nil }
+  let(:resource) { double(Resource, url: url, mirrors: [], specs: { user: "download:123456" }) }
 
   it "parses the opts and sets the corresponding args" do
     expect(subject.send(:_curl_opts)).to eq(["--user", "download:123456"])
   end
 
   describe "#tarball_path" do
-    subject { described_class.new(name, resource).tarball_path }
+    subject { described_class.new(name, version, resource).tarball_path }
 
     context "when URL ends with file" do
       it { is_expected.to eq(HOMEBREW_CACHE/"foo-.tar.gz") }
@@ -251,12 +259,13 @@ end
 
 describe ScpDownloadStrategy do
   def resource_for(url)
-    double(Resource, url: url, mirrors: [], specs: {}, version: nil)
+    double(Resource, url: url, mirrors: [], specs: {})
   end
 
-  subject { described_class.new(name, resource) }
+  subject { described_class.new(name, version, resource) }
   let(:name) { "foo" }
   let(:url) { "scp://example.com/foo.tar.gz" }
+  let(:version) { nil }
   let(:resource) { resource_for(url) }
 
   describe "#initialize" do
@@ -271,7 +280,7 @@ describe ScpDownloadStrategy do
       context "with invalid URL #{invalid_url}" do
         it "raises ScpDownloadStrategyError" do
           expect {
-            described_class.new(name, resource_for(invalid_url))
+            described_class.new(name, version, resource_for(invalid_url))
           }.to raise_error(ScpDownloadStrategyError)
         end
       end