From e367e7eea8724c063c3a791ea48ad52e811559e0 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Thu, 2 Aug 2018 09:59:22 +0200 Subject: [PATCH] Explicitly pass url and mirrors to download strategy. --- Library/Homebrew/download_strategy.rb | 28 +++++------ Library/Homebrew/resource.rb | 11 +--- .../Homebrew/test/download_strategies_spec.rb | 50 +++++++++---------- 3 files changed, 39 insertions(+), 50 deletions(-) diff --git a/Library/Homebrew/download_strategy.rb b/Library/Homebrew/download_strategy.rb index 5d9d3f25c0..911deac410 100644 --- a/Library/Homebrew/download_strategy.rb +++ b/Library/Homebrew/download_strategy.rb @@ -17,12 +17,13 @@ class AbstractDownloadStrategy attr_reader :meta, :name, :version attr_reader :shutup - def initialize(name, version, resource) + def initialize(url, name, version, resource, mirrors: []) + @url = url @name = name - @url = resource.url @version = version @meta = resource.specs @shutup = false + @mirrors = mirrors extend Pourable if meta[:bottle] end @@ -90,7 +91,7 @@ end class VCSDownloadStrategy < AbstractDownloadStrategy REF_TYPES = [:tag, :branch, :revisions, :revision].freeze - def initialize(name, version, resource) + def initialize(url, name, version, resource, mirrors: []) super @ref_type, @ref = extract_ref(meta) @revision = meta[:revision] @@ -209,9 +210,8 @@ end class CurlDownloadStrategy < AbstractFileDownloadStrategy attr_reader :mirrors, :tarball_path, :temporary_path - def initialize(name, version, resource) + def initialize(url, name, version, resource, mirrors: []) super - @mirrors = resource.mirrors.dup @tarball_path = HOMEBREW_CACHE/"#{name}-#{version}#{ext}" @temporary_path = Pathname.new("#{cached_location}.incomplete") end @@ -389,7 +389,7 @@ class GitHubPrivateRepositoryDownloadStrategy < CurlDownloadStrategy require "utils/formatter" require "utils/github" - def initialize(name, version, resource) + def initialize(url, name, version, resource, mirrors: []) super parse_url_pattern set_github_token @@ -495,7 +495,7 @@ end class ScpDownloadStrategy < AbstractFileDownloadStrategy attr_reader :tarball_path, :temporary_path - def initialize(name, version, resource) + def initialize(url, name, version, resource, mirrors: []) super @tarball_path = HOMEBREW_CACHE/"#{name}-#{version}#{ext}" @temporary_path = Pathname.new("#{cached_location}.incomplete") @@ -546,7 +546,7 @@ class ScpDownloadStrategy < AbstractFileDownloadStrategy end class SubversionDownloadStrategy < VCSDownloadStrategy - def initialize(name, version, resource) + def initialize(url, name, version, resource, mirrors: []) super @url = @url.sub("svn+http://", "") end @@ -631,7 +631,7 @@ class GitDownloadStrategy < VCSDownloadStrategy %r{http://llvm\.org}, ].freeze - def initialize(name, version, resource) + def initialize(url, name, version, resource, mirrors: []) super @ref_type ||= :branch @ref ||= "master" @@ -803,7 +803,7 @@ class GitDownloadStrategy < VCSDownloadStrategy end class GitHubGitDownloadStrategy < GitDownloadStrategy - def initialize(name, version, resource) + def initialize(url, name, version, resource, mirrors: []) super return unless %r{^https?://github\.com/(?[^/]+)/(?[^/]+)\.git$} =~ @url @@ -857,7 +857,7 @@ class GitHubGitDownloadStrategy < GitDownloadStrategy end class CVSDownloadStrategy < VCSDownloadStrategy - def initialize(name, version, resource) + def initialize(url, name, version, resource, mirrors: []) super @url = @url.sub(%r{^cvs://}, "") @@ -927,7 +927,7 @@ class CVSDownloadStrategy < VCSDownloadStrategy end class MercurialDownloadStrategy < VCSDownloadStrategy - def initialize(name, version, resource) + def initialize(url, name, version, resource, mirrors: []) super @url = @url.sub(%r{^hg://}, "") end @@ -983,7 +983,7 @@ class MercurialDownloadStrategy < VCSDownloadStrategy end class BazaarDownloadStrategy < VCSDownloadStrategy - def initialize(name, version, resource) + def initialize(url, name, version, resource, mirrors: []) super @url.sub!(%r{^bzr://}, "") ENV["BZR_HOME"] = HOMEBREW_TEMP @@ -1034,7 +1034,7 @@ class BazaarDownloadStrategy < VCSDownloadStrategy end class FossilDownloadStrategy < VCSDownloadStrategy - def initialize(name, version, resource) + def initialize(url, name, version, resource, mirrors: []) super @url = @url.sub(%r{^fossil://}, "") end diff --git a/Library/Homebrew/resource.rb b/Library/Homebrew/resource.rb index 6343331a72..7dde0deca8 100644 --- a/Library/Homebrew/resource.rb +++ b/Library/Homebrew/resource.rb @@ -22,17 +22,9 @@ class Resource @resource = resource end - def url - @resource.url - end - def specs @resource.specs end - - def mirrors - @resource.mirrors - end end def initialize(name = nil, &block) @@ -53,7 +45,8 @@ class Resource end def downloader - download_strategy.new(download_name, version, Download.new(self)) + download_strategy.new(url, download_name, version, Download.new(self), + mirrors: mirrors.dup) end # Removes /s from resource names; this allows go package names diff --git a/Library/Homebrew/test/download_strategies_spec.rb b/Library/Homebrew/test/download_strategies_spec.rb index d78ae2437c..c84f332f8d 100644 --- a/Library/Homebrew/test/download_strategies_spec.rb +++ b/Library/Homebrew/test/download_strategies_spec.rb @@ -1,13 +1,13 @@ require "download_strategy" describe AbstractDownloadStrategy do - subject { described_class.new(name, version, resource) } + subject { described_class.new(url, 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(:resource) { double(Resource, specs: specs) } let(:args) { %w[foo bar baz] } specify "#source_modified_time" do @@ -37,23 +37,23 @@ end describe VCSDownloadStrategy do let(:url) { "http://example.com/bar" } let(:version) { nil } - let(:resource) { double(Resource, url: url, mirrors: [], specs: {}) } + let(:resource) { double(Resource, 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", version, resource) + downloader = described_class.new(url, "baz", version, resource) expect(downloader.cached_location).to eq(HOMEBREW_CACHE/"baz--foo") end end end describe GitHubPrivateRepositoryDownloadStrategy do - subject { described_class.new("foo", version, resource) } + subject { described_class.new(url, "foo", version, resource) } let(:url) { "https://github.com/owner/repo/archive/1.1.5.tar.gz" } let(:version) { nil } - let(:resource) { double(Resource, url: url, mirrors: [], specs: {}) } + let(:resource) { double(Resource, specs: {}) } before do ENV["HOMEBREW_GITHUB_API_TOKEN"] = "token" @@ -74,11 +74,11 @@ describe GitHubPrivateRepositoryDownloadStrategy do end describe GitHubPrivateRepositoryReleaseDownloadStrategy do - subject { described_class.new("foo", version, resource) } + subject { described_class.new(url, "foo", version, resource) } let(:url) { "https://github.com/owner/repo/releases/download/tag/foo_v0.1.0_darwin_amd64.tar.gz" } let(:version) { nil } - let(:resource) { double(Resource, url: url, mirrors: [], specs: {}) } + let(:resource) { double(Resource, url: url, specs: {}) } before do ENV["HOMEBREW_GITHUB_API_TOKEN"] = "token" @@ -126,12 +126,12 @@ describe GitHubPrivateRepositoryReleaseDownloadStrategy do end describe GitHubGitDownloadStrategy do - subject { described_class.new(name, version, resource) } + subject { described_class.new(url, name, version, resource) } let(:name) { "brew" } let(:url) { "https://github.com/homebrew/brew.git" } let(:version) { nil } - let(:resource) { double(Resource, url: url, mirrors: [], specs: {}) } + let(:resource) { double(Resource, specs: {}) } it "parses the URL and sets the corresponding instance variables" do expect(subject.instance_variable_get(:@user)).to eq("homebrew") @@ -140,12 +140,12 @@ describe GitHubGitDownloadStrategy do end describe GitDownloadStrategy do - subject { described_class.new(name, version, resource) } + subject { described_class.new(url, name, version, resource) } let(:name) { "baz" } let(:url) { "https://github.com/homebrew/foo" } let(:version) { nil } - let(:resource) { double(Resource, url: url, mirrors: [], specs: {}) } + let(:resource) { double(Resource, specs: {}) } let(:cached_location) { subject.cached_location } before do @@ -208,15 +208,15 @@ describe GitDownloadStrategy do end describe S3DownloadStrategy do - subject { described_class.new(name, version, resource) } + subject { described_class.new(url, name, version, resource) } let(:name) { "foo" } let(:url) { "http://bucket.s3.amazonaws.com/foo.tar.gz" } let(:version) { nil } - let(:resource) { double(Resource, url: url, mirrors: [], specs: {}) } + let(:resource) { double(Resource, specs: {}) } describe "#_fetch" do - subject { described_class.new(name, version, resource)._fetch } + subject { described_class.new(url, name, version, resource)._fetch } context "when given Bad S3 URL" do let(:url) { "http://example.com/foo.tar.gz" } @@ -231,19 +231,19 @@ describe S3DownloadStrategy do end describe CurlDownloadStrategy do - subject { described_class.new(name, version, resource) } + subject { described_class.new(url, name, version, resource) } let(:name) { "foo" } let(:url) { "http://example.com/foo.tar.gz" } let(:version) { nil } - let(:resource) { double(Resource, url: url, mirrors: [], specs: { user: "download:123456" }) } + let(:resource) { double(Resource, 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, version, resource).tarball_path } + subject { described_class.new(url, name, version, resource).tarball_path } context "when URL ends with file" do it { is_expected.to eq(HOMEBREW_CACHE/"foo-.tar.gz") } @@ -258,15 +258,11 @@ describe CurlDownloadStrategy do end describe ScpDownloadStrategy do - def resource_for(url) - double(Resource, url: url, mirrors: [], specs: {}) - end - - subject { described_class.new(name, version, resource) } + subject { described_class.new(url, name, version, resource) } let(:name) { "foo" } let(:url) { "scp://example.com/foo.tar.gz" } let(:version) { nil } - let(:resource) { resource_for(url) } + let(:resource) { double(Resource, specs: {}) } describe "#initialize" do invalid_urls = %w[ @@ -278,10 +274,10 @@ describe ScpDownloadStrategy do invalid_urls.each do |invalid_url| context "with invalid URL #{invalid_url}" do + let(:url) { invalid_url } + it "raises ScpDownloadStrategyError" do - expect { - described_class.new(name, version, resource_for(invalid_url)) - }.to raise_error(ScpDownloadStrategyError) + expect { subject }.to raise_error(ScpDownloadStrategyError) end end end