Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bzip2 and zip dependencies_if_needed #3813

Merged
merged 14 commits into from
Feb 21, 2018
10 changes: 10 additions & 0 deletions Library/Homebrew/dependency_collector.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,14 @@ def xz_dep_if_needed(tags)
Dependency.new("xz", tags)
Copy link
Member

@sjackman sjackman Feb 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Mike agrees, change this to…

Dependency.new("xz", tags) unless which("xz")

end

def zip_dep_if_needed(tags)
Dependency.new("zip", tags) unless which("zip")
end

def bzip2_dep_if_needed(tags)
Dependency.new("bzip2", tags) unless which("bzip2")
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put these below xz_dep_if_needed


def ld64_dep_if_needed(*); end

def self.tar_needs_xz_dependency?
Expand Down Expand Up @@ -158,6 +166,8 @@ def resource_dep(spec, tags)
def parse_url_spec(url, tags)
case File.extname(url)
when ".xz" then xz_dep_if_needed(tags)
when ".zip" then zip_dep_if_needed(tags)
when ".bz2" then bzip2_dep_if_needed(tags)
when ".lha", ".lzh" then Dependency.new("lha", tags)
when ".lz" then Dependency.new("lzip", tags)
when ".rar" then Dependency.new("unrar", tags)
Expand Down
1 change: 1 addition & 0 deletions Library/Homebrew/extend/os/dependency_collector.rb
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
require "dependency_collector"
require "extend/os/mac/dependency_collector" if OS.mac?
require "extend/os/linux/dependency_collector" if OS.linux?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had to add it back to override xz_dep_if_needed

5 changes: 5 additions & 0 deletions Library/Homebrew/extend/os/linux/dependency_collector.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class DependencyCollector
def xz_dep_if_needed(tags)
Dependency.new("xz", tags) unless which("xz")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm 👍 on changing the default implementation in Library/Homebrew/dependency_collector.rb to do this for consistency. Could also do cvs_dep_if_needed if desired. The only potential gotcha is self.tar_needs_xz_dependency?; is it a safe assumption that xz in the PATH means tar is built with xz support? CC @sjackman

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from looking at the gnu-tar's ./configure script, it looks like tar is always built with "tar" support and the only thing one can override is which "xz" to use. By default, tar uses the first 'xz' that appears in the PATH first.
And I don't see anything related to xz hard-coded in tar:

$ strings /usr/local/Cellar/gnu-tar/1.30/bin/gtar | grep xz

$ strings /usr/local/Cellar/gnu-tar/1.30/libexec/gnubin/tar | grep xz

end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above: had to add it back to override xz_dep_if_needed (14d7a7a#r169467375)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't override xz_dep_if_needed for Linux. This implementation ought to be the generic implementation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does xz exist on all Linux systems? If so - sure. Otherwise, it would make sense to keep it conditional.

4 changes: 4 additions & 0 deletions Library/Homebrew/extend/os/mac/dependency_collector.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ def xz_dep_if_needed(tags)
Dependency.new("xz", tags)
end

def zip_dep_if_needed(tags); end

def bzip2_dep_if_needed(tags); end

def ld64_dep_if_needed(*)
# Tiger's ld is too old to properly link some software
return if MacOS.version > :tiger
Expand Down
53 changes: 53 additions & 0 deletions Library/Homebrew/test/os/linux/dependency_collector_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
require "dependency_collector"

describe DependencyCollector do
alias_matcher :be_a_build_requirement, :be_build

after(:each) do
described_class.clear_cache
end

describe "#add" do
resource = Resource.new

context "when xz, zip, and bzip2 are not available" do
it "creates a resource dependency from a '.xz' URL" do
resource.url("http://example.com/foo.xz")
allow_any_instance_of(Object).to receive(:which).with("xz")
expect(subject.add(resource)).to eq(Dependency.new("xz", [:build]))
end

it "creates a resource dependency from a '.zip' URL" do
resource.url("http://example.com/foo.zip")
allow_any_instance_of(Object).to receive(:which).with("zip")
expect(subject.add(resource)).to eq(Dependency.new("zip", [:build]))
end

it "creates a resource dependency from a '.bz2' URL" do
resource.url("http://example.com/foo.tar.bz2")
allow_any_instance_of(Object).to receive(:which).with("bzip2")
expect(subject.add(resource)).to eq(Dependency.new("bzip2", [:build]))
end
end

context "when xz, zip, and bzip2 are available" do
it "does not create a resource dependency from a '.xz' URL" do
resource.url("http://example.com/foo.xz")
allow_any_instance_of(Object).to receive(:which).with("xz").and_return(Pathname.new("foo"))
expect(subject.add(resource)).to be nil
end

it "does not create a resource dependency from a '.zip' URL" do
resource.url("http://example.com/foo.zip")
allow_any_instance_of(Object).to receive(:which).with("zip").and_return(Pathname.new("foo"))
expect(subject.add(resource)).to be nil
end

it "does not create a resource dependency from a '.bz2' URL" do
resource.url("http://example.com/foo.tar.bz2")
allow_any_instance_of(Object).to receive(:which).with("bzip2").and_return(Pathname.new("foo"))
expect(subject.add(resource)).to be nil
end
end
end
end
12 changes: 12 additions & 0 deletions Library/Homebrew/test/os/mac/dependency_collector_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,18 @@
expect(subject.add(resource)).to be nil
end

specify "Resource dependency from a '.zip' URL" do
resource = Resource.new
resource.url("http://example.com/foo.zip")
expect(subject.add(resource)).to be nil
end

specify "Resource dependency from a '.bz2' URL" do
resource = Resource.new
resource.url("http://example.com/foo.tar.bz2")
expect(subject.add(resource)).to be nil
end

specify "Resource dependency from a '.git' URL" do
resource = Resource.new
resource.url("git://example.com/foo/bar.git")
Expand Down