From 3d27b2aa8d87c87dd0fa3958deff77bfe33fa8a3 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Wed, 1 Aug 2018 05:18:00 +0200 Subject: [PATCH 1/3] Explicitly pass version to download strategy. --- Library/Homebrew/download_strategy.rb | 33 +++++++------ Library/Homebrew/formulary.rb | 2 +- Library/Homebrew/resource.rb | 6 +-- .../Homebrew/test/download_strategies_spec.rb | 49 +++++++++++-------- 4 files changed, 48 insertions(+), 42 deletions(-) diff --git a/Library/Homebrew/download_strategy.rb b/Library/Homebrew/download_strategy.rb index e3e7b03b57..6525c98b55 100644 --- a/Library/Homebrew/download_strategy.rb +++ b/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] @@ -88,7 +87,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] @@ -134,7 +133,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. @@ -205,7 +206,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}" @@ -383,7 +384,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 @@ -489,7 +490,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") @@ -540,7 +541,7 @@ class ScpDownloadStrategy < AbstractFileDownloadStrategy end class SubversionDownloadStrategy < VCSDownloadStrategy - def initialize(name, resource) + def initialize(name, version, resource) super @url = @url.sub("svn+http://", "") end @@ -625,7 +626,7 @@ class GitDownloadStrategy < VCSDownloadStrategy %r{http://llvm\.org}, ].freeze - def initialize(name, resource) + def initialize(name, version, resource) super @ref_type ||= :branch @ref ||= "master" @@ -797,7 +798,7 @@ class GitDownloadStrategy < VCSDownloadStrategy end class GitHubGitDownloadStrategy < GitDownloadStrategy - def initialize(name, resource) + def initialize(name, version, resource) super return unless %r{^https?://github\.com/(?[^/]+)/(?[^/]+)\.git$} =~ @url @@ -851,7 +852,7 @@ class GitHubGitDownloadStrategy < GitDownloadStrategy end class CVSDownloadStrategy < VCSDownloadStrategy - def initialize(name, resource) + def initialize(name, version, resource) super @url = @url.sub(%r{^cvs://}, "") @@ -921,7 +922,7 @@ class CVSDownloadStrategy < VCSDownloadStrategy end class MercurialDownloadStrategy < VCSDownloadStrategy - def initialize(name, resource) + def initialize(name, version, resource) super @url = @url.sub(%r{^hg://}, "") end @@ -977,7 +978,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 @@ -1028,7 +1029,7 @@ class BazaarDownloadStrategy < VCSDownloadStrategy end class FossilDownloadStrategy < VCSDownloadStrategy - def initialize(name, resource) + def initialize(name, version, resource) super @url = @url.sub(%r{^fossil://}, "") end diff --git a/Library/Homebrew/formulary.rb b/Library/Homebrew/formulary.rb index 17dbd6f9a5..4a552b1be7 100644 --- a/Library/Homebrew/formulary.rb +++ b/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 = CurlDownloadStrategy.new(resource.download_name, resource.version, resource) cached = downloader.cached_location.exist? downloader.fetch ohai "Pouring the cached bottle" if cached diff --git a/Library/Homebrew/resource.rb b/Library/Homebrew/resource.rb index e86f48ac1d..b5088a84f6 100644 --- a/Library/Homebrew/resource.rb +++ b/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 diff --git a/Library/Homebrew/test/download_strategies_spec.rb b/Library/Homebrew/test/download_strategies_spec.rb index 66518015fa..d78ae2437c 100644 --- a/Library/Homebrew/test/download_strategies_spec.rb +++ b/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 From 9ffc7dd46516e1d94ca4c386fc6006c61bd0e6ca Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Wed, 1 Aug 2018 05:33:03 +0200 Subject: [PATCH 2/3] Use `Resource#downloader` for `BottleLoader`. --- Library/Homebrew/formulary.rb | 2 +- Library/Homebrew/resource.rb | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/formulary.rb b/Library/Homebrew/formulary.rb index 4a552b1be7..974ed85cea 100644 --- a/Library/Homebrew/formulary.rb +++ b/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.download_name, resource.version, resource) + downloader = resource.downloader cached = downloader.cached_location.exist? downloader.fetch ohai "Pouring the cached bottle" if cached diff --git a/Library/Homebrew/resource.rb b/Library/Homebrew/resource.rb index b5088a84f6..6343331a72 100644 --- a/Library/Homebrew/resource.rb +++ b/Library/Homebrew/resource.rb @@ -64,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 From 85a6d81e2a03421043c4baa40240b4823b8b12c9 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Wed, 1 Aug 2018 05:39:28 +0200 Subject: [PATCH 3/3] Make sure download directory exists. --- Library/Homebrew/download_strategy.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Library/Homebrew/download_strategy.rb b/Library/Homebrew/download_strategy.rb index 6525c98b55..4d8eb53520 100644 --- a/Library/Homebrew/download_strategy.rb +++ b/Library/Homebrew/download_strategy.rb @@ -253,6 +253,8 @@ class CurlDownloadStrategy < AbstractFileDownloadStrategy ohai "Downloading from #{url}" end + temporary_path.dirname.mkpath + curl_download resolved_url(url), to: temporary_path end