bzip2 and zip dependencies_if_needed #3813

maxim-belkin · 2018-02-16T21:21:50Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew tests with your changes locally?

Continuing to merge changes from Linuxbrew to Homebrew. Therefore, mentioning @sjackman @iMichka

Brining zip and bzip2 related changes from Linux. The PR addresses the issue that not all distros are created equal and lack bzip2 and zip.

I have written simple tests based on p7zip example.

On some systems identified as Linux, zip and bzip2 might not be available. Therefore, on such platforms we add them unconditionally as dependencies when required. On Mac, these dependencies are always satisfied.

sjackman · 2018-02-16T22:53:56Z

I'd suggest modelling bzip2_dep_if_needed and unzip_dep_if_needed after xz_dep_if_needed. The generic implementation is…

  def bzip2_dep_if_needed(tags)
    Dependency.new("bzip2", tags)
  end

The macOS implementation is…

def bzip2_dep_if_needed(*); end

and no Linux-specific implementation.

sjackman · 2018-02-16T22:55:55Z

Library/Homebrew/dependency_collector.rb

@@ -72,6 +72,10 @@ def xz_dep_if_needed(tags)

  def ld64_dep_if_needed(*); end

+  def zip_dep_if_needed(*); end
+
+  def bzip2_dep_if_needed(*); end


Model these after xz_dep_if_needed.

sjackman · 2018-02-16T22:56:08Z

Library/Homebrew/extend/os/dependency_collector.rb

@@ -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?


Remove this line.

had to add it back to override xz_dep_if_needed

sjackman · 2018-02-16T22:56:18Z

Library/Homebrew/extend/os/linux/dependency_collector.rb

+  def bzip2_dep_if_needed(tags)
+    Dependency.new("bzip2", tags)
+  end
+end


Remove this file.

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

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

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

sjackman · 2018-02-16T22:56:33Z

Library/Homebrew/extend/os/mac/dependency_collector.rb

+
+  def zip_dep_if_needed(*); end
+
+  def bzip2_dep_if_needed(*); end


Name the argument tags like subversion_dep_if_needed above.

Also: put these below xz_dep_if_needed

MikeMcQuaid · 2018-02-17T07:59:53Z

and no Linux-specific implementation.

I agree with this.

MikeMcQuaid · 2018-02-17T08:00:59Z

Brining zip and bzip2 related changes from Linux. The PR addresses the issue that not all distros are created equal and lack bzip2 and zip.

That's not what you're handling here, though, you're instead installing Homebrew's bzip2 and zip unconditionally regardless of their presence on the system. I don't think that's a good idea and it's not in keeping with the Homebrew ethos.

MikeMcQuaid · 2018-02-17T08:45:23Z

Some more questions here:

What Linux distros have gzip installed but not bzip2?
Again: why are you unconditionally making everyone using a Linuxbrew bzip2 formula even when the vast majority will have bzip2 present on their system already?
Where does this end in terms of assuming things are/aren't present in Linuxbrew?
Will this approach not cause issues with bottles that are reused between systems that assume the presence of some system libraries and others don't?

I think really what Linuxbrew should start doing to address those problems is:

Make bottles conditional on the distro version and the default (or minimal) distro install
Introduce portable- or similar packages to match the first Linuxbrew run with what's provided by the minimal macOS or OS X system

As a more general point: try to think more about designing Linuxbrew as a system. A bunch of these changes (#3809 included) are essentially just hacks to try and get Linuxbrew working on more Linux systems. This way lies madness. Linuxbrew should define (both in terms of documentation and code) what they are willing to support. If you'd rather take the approach of "we're going to try and support literally every case of running Linuxbrew on any Linux distro" then I think the upstreaming efforts may need to part ways (and perhaps be partially reverted).

sjackman · 2018-02-17T17:49:25Z

What Linux distros have gzip installed but not bzip2?

Bare-bones distros, most commonly used in Docker and CI systems.

Again: why are you unconditionally making everyone using a Linuxbrew bzip2 formula even when the vast majority will have bzip2 present on their system already?

This could be conditional on which("bzip2")

Where does this end in terms of assuming things are/aren't present in Linuxbrew?
Will this approach not cause issues with bottles that are reused between systems that assume the presence of some system libraries and others don't?

Linuxbrew uses host executables when possible. Linuxbrew uses no host libraries, except for glibc and libstdc++. Linuxbrew uses brewed versions of glibc and libstdc++ when the host versions are too old.

Make bottles conditional on the distro version and the default (or minimal) distro install

There's nearly unlimited variation in what packages may or may not be installed for a given distribution.

Make bottles conditional on the distro version and the default (or minimal) distro install

Linuxbrew uses a single bottle that's portable to all Linux systems by provided brewed versions of all the necessary libraries.

sjackman · 2018-02-17T18:00:53Z

As a more general point: try to think more about designing Linuxbrew as a system. A bunch of these changes (#3809 included) are essentially just hacks to try and get Linuxbrew working on more Linux systems. This way lies madness. Linuxbrew should define (both in terms of documentation and code) what they are willing to support. If you'd rather take the approach of "we're going to try and support literally every case of running Linuxbrew on any Linux distro" then I think the upstreaming efforts may need to part ways (and perhaps be partially reverted).

We actively support the major distributions of Linux, such as Debian, Ubuntu, Fedora, and CentOS.

I believe Homebrew also had issues with the system version of curl being too old on older distributions of macOS. The solution that worked or Homebrew on macOS should also work for Linuxbrew.

the upstreaming efforts may need to part ways (and perhaps be partially reverted).

Although I appreciate your effort to help upstream Linux patches, Maxim—Thank you!—you may not have the context of when and why patches were introduced, and whether those patches are still needed upstream, or could instead be reverted in Linuxbrew. I suggest we have more discussion within the Linuxbrew team before opening PRs to upstream Linuxbrew patches. Reverting changes to Linuxbrew/brew that are no longer needed is an important component of merging Linuxbrew/brew into Homebrew/brew.

maxim-belkin · 2018-02-17T19:38:48Z

why are you unconditionally making everyone using a Linuxbrew bzip2 formula even when the vast majority will have bzip2 present on their system already?

I could not agree more: everything should be conditional and if the system provides required components - Homebrew should use these. In fact, I was disappointed to see :python and the like go, but, unfortunatly, I don't have enough time to read all the reasoning behind that decision. I think that all tools that Homebrew uses should be checked not only for existence but also that they function properly, have required version, etc. and not rely on MacOS.version. MacOS.version is convenient as it converts multiple comparisons to only a handful, but implies you have to make changes every single time macOS is updated and that's not a super sustainable model. Though, of course, it will survive given the interest that project has in the community.

This PR is merely an attempt to bring changes that currently exist in Linuxbrew to Homebrew.

Although I appreciate your effort to help upstream Linux patches, Maxim—Thank you!—you may not have the context of when and why patches were introduced, and whether those patches are still needed upstream, or could instead be reverted in Linuxbrew. I suggest we have more discussion within the Linuxbrew team before opening PRs to upstream Linuxbrew patches.

🔪 First of all, thank you for editing the comment thus making it a bit easier to pull out from my back 😄 The context I usually use consists of (1) git log messages (2) comments within the code (not the strongest side of Linuxbrew/Homebrew) (3) Slack (when needed) (4) related GitHub PRs and Issues if changes are hard to make sense of otherwise. Please, let me know of any other context I'm missing. Changes introduced in this as well as #3809 PRs seem straightforward so I only mentioned you here.

I suggest we have more discussion within the Linuxbrew team before opening PRs to upstream Linuxbrew patches.

sounds good

MikeMcQuaid · 2018-02-18T10:38:21Z

Bare-bones distros, most commonly used in Docker and CI systems.

Which distros specifically, though? Can you name a few?

We actively support the major distributions of Linux, such as Debian, Ubuntu, Fedora, and CentOS.

My point was more: it doesn't seem there's a definition of what you don't support and won't try to e.g. the bare-bones distros you mention above. It's also worth asking what Linuxbrew wants to do differently to tools like Gentoo Prefix which are much more established. With a smaller team and fewer resources spreading yourselves too thin means a tool that's less useful to anyone.

Linuxbrew uses host executables when possible.

That's not the case in #3809 with curl or here with bzip2.

There's nearly unlimited variation in what packages may or may not be installed for a given distribution.

That's true but the standard or minimal installation for a distribution provides a minimal subset.

Linuxbrew uses a single bottle that's portable to all Linux systems by provided brewed versions of all the necessary libraries.

I believe Homebrew also had issues with the system version of curl being too old on older distributions of macOS. The solution that worked or Homebrew on macOS should also work for Linuxbrew.

This is not the same solution, though. In this case you're saying "if there's a Homebrew curl: always use it". In the macOS case the relevant formulae have audit checks so that a modern curl can be built and then used. If there was some sort of feature check before this was enabled with curl that would be a smarter move e.g. some test to see what sorts of crypto are supported by the system curl.

In fact, I was disappointed to see :python and the like go, but, unfortunatly, I don't have enough time to read all the reasoning behind that decision.

This was as the vast majority of users are now using bottles exclusively with Homebrew which means that these requirements were only using system versions when building from source and untested by CI. That's not the case here where the vast majority of Linuxbrew users will have bzip2 installed but this requires everyone to have it installed.

I think that all tools that Homebrew uses should be checked not only for existence but also that they function properly, have required version, etc. and not rely on MacOS.version. MacOS.version is convenient as it converts multiple comparisons to only a handful, but implies you have to make changes every single time macOS is updated and that's not a super sustainable model.

Our model is completely sustainable because we rely on a macOS base system that only changes versions once a year. With respect, MacOS.version makes life much easier for Homebrew but just not for Linuxbrew. This is why I've been saying for years Linuxbrew should create their own formulae that are not designed to be used on macOS (i.e. don't use if OS.mac? or MacOS.version).

Reverting changes to Linuxbrew/brew that are no longer needed is an important component of merging Linuxbrew/brew into Homebrew/brew.

👍 . I also think it's worth a discussion at some point between Linuxbrew and Homebrew maintainers as to what the end goal is, a (very loosely) estimated timescale and the similarities and differences in ethos between the projects. This could result in documentation that would make PRs and issues between the projects less confusing.

My starter for ten on this:

Linuxbrew/brew should eventually go away as something that's used by end-users in favour of Linux support being baked sufficiently into Homebrew/brew.
Linuxbrew/brew will remain as a fork (and could be used for those currently on master rather than stable branches) purely to upstream work to Homebrew.
Completing this migration should be aimed to be done by the end of 2018
Homebrew is primarily a non-optioned binary package manager installed into /usr/local and run on macOS. The longer tail we support are older macOS versions, custom prefixes and custom options in formulae.
Linuxbrew/homebrew-core changes never go upstream to Homebrew/homebrew-core unless there's a bug reproducible on both platforms.
Making a change in Homebrew/homebrew-core to add consistency or ease merge conflicts between Homebrew and Linuxbrew is not acceptable.

iMichka · 2018-02-18T11:24:15Z

which distros specifically, though? Can you name a few?

Our ubuntu docker image we use for the circle.ci build does not have bzip2 nor zip installed.
Another (better) example would be Alpine Linux: docker pull alpine && docker run -it alpine, and then run apk -vv info|sort. You will see that the list of packages is quite small :)

Our model is completely sustainable because we rely on a macOS base system that only changes versions once a year. With respect, MacOS.version makes life much easier for Homebrew but just not for Linuxbrew. This is why I've been saying for years Linuxbrew should create their own formulae that are not designed to be used on macOS (i.e. don't use if OS.mac? or MacOS.version).

I agree that it is easier on mac. As I have been doing the core merges for the last year, I see that on Linux it is a little bit more of work; but at the end it is just "some" dependencies missing (libxml2, zip, ncurses, ....). I think we got that already pretty well covered now, as we have now more than 1600 bottles. And I do not get many requests to fix other packages, so I guess the other 3000 or so formulae are less used (or just work out of the box).

When you say we should create our own formulae, how should that work? As patch files that we would apply on top of the homebrew-core repo? I think whatever way we choose, the amount of work is probably equivalent.

I must say that the merges from homebrew-core to linuxbrew-core are working quite well; it is a lot o work during the week, but I think it is because I am doing this mostly alone. You have the same bottleneck in homebrew where most of the updates are done by one or two dedicated persons :) If we had 5 maintainers more everything would be fine :)

I am open for changes though, if it can reduce the workload: but most of the time is spent fixing dependencies and rebuilding bottles, so I am not sure how we can improve this without more maintainers.

Homebrew is primarily a non-optioned binary package manager installed into /usr/local and run on macOS. The longer tail we support are older macOS versions, custom prefixes and custom options in formulae.

One small note: Linuxbrew tries to follow that philosophy too. Though we have some users that can not create a /home/linuxbrew/ folder, and these are stuck with building stuff from source. And these are the cases where we hit the most bugs ...

Linuxbrew/homebrew-core changes never go upstream to Homebrew/homebrew-core unless there's a bug reproducible on both platforms.

Right. I think there was never such a case, at least not last year. We just have the regular sha256 mismatches (at least a few per month), which then get fixed by me or reported if I have no time to fix them myself.

Making a change in Homebrew/homebrew-core to add consistency or ease merge conflicts between Homebrew and Linuxbrew is not acceptable.

+1 on that. I think I never backported anything to core. Maybe once I wanted some linker flags in a different order for a test, but that's all. Merge conflicts are not a problem, I fix some of them each day, and it is working quite well. In fact, there are not so many merge conflicts at the end, and they are all trivial to solve.

iMichka · 2018-02-18T13:35:43Z

BTW, I am not sure this PR is the best place to discuss this :)

Defaulting zip_dep_if_needed(tags) and bzip2_dep_if_needed(tags) methods to those on Linux and overriding them on macOS.

sjackman · 2018-02-19T07:05:16Z

Library/Homebrew/dependency_collector.rb

@@ -72,6 +72,14 @@ def xz_dep_if_needed(tags)

  def ld64_dep_if_needed(*); end

+  def zip_dep_if_needed(tags)
+    Dependency.new("zip", tags)


Use which to check first whether zip is provided by the host, and make this dependency conditional.

Any preference as to which one to use:

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

or

return if which("zip") Dependency.new("zip", tags)

?

I prefer the former.

sjackman · 2018-02-19T07:05:24Z

Library/Homebrew/dependency_collector.rb

+  end
+
+  def bzip2_dep_if_needed(tags)
+    Dependency.new("bzip2", tags)


Ditto here.

Here, we are adding `unless which("zip")` and `unless which("bzip2")` and, thus, make `zip` and `bzip2` dependencies conditional.

Files are globbed based on their name. Therefore, we have to rename them so tests for Linux are not executed on a Mac.

MikeMcQuaid · 2018-02-19T09:23:00Z

Library/Homebrew/test/dependency_collector_spec.rb

@@ -140,3 +140,4 @@ def find_requirement(klass)
    end
  end
 end
+require "test/os/dependency_collector_spec"


Don't need this line for tests.

MikeMcQuaid · 2018-02-19T09:23:03Z

Library/Homebrew/test/os/dependency_collector_spec.rb

@@ -0,0 +1 @@
+require "test/os/linux/dependency_collector" if OS.linux?


This can just be dependency_collector_spec.rb without the if or the os/linux file.

MikeMcQuaid · 2018-02-19T09:23:26Z

Library/Homebrew/extend/os/mac/dependency_collector.rb

+
+  def zip_dep_if_needed(*); end
+
+  def bzip2_dep_if_needed(*); end


Name the argument tags like subversion_dep_if_needed above.

MikeMcQuaid · 2018-02-19T09:24:20Z

Library/Homebrew/dependency_collector.rb

@@ -162,6 +170,8 @@ def parse_url_spec(url, tags)
    when ".lz"          then Dependency.new("lzip", tags)
    when ".rar"         then Dependency.new("unrar", tags)
    when ".7z"          then Dependency.new("p7zip", tags)
+    when ".zip"         then zip_dep_if_needed(tags)


Put these at the top below xz_dep_if_needed

MikeMcQuaid · 2018-02-19T09:24:38Z

Library/Homebrew/dependency_collector.rb

+
+  def bzip2_dep_if_needed(tags)
+    Dependency.new("bzip2", tags) unless which("bzip2")
+  end


Put these below xz_dep_if_needed

MikeMcQuaid · 2018-02-19T09:24:47Z

Library/Homebrew/extend/os/mac/dependency_collector.rb

+
+  def zip_dep_if_needed(*); end
+
+  def bzip2_dep_if_needed(*); end


Also: put these below xz_dep_if_needed

MikeMcQuaid · 2018-02-19T10:10:25Z

Library/Homebrew/test/os/dependency_collector_spec.rb

@@ -0,0 +1 @@
+require "test/dependency_collector"


This file should instead be filled with the contents of Library/Homebrew/test/os/linux/dependency_collector.rb (which should be deleted)

MikeMcQuaid · 2018-02-19T10:27:29Z

Library/Homebrew/test/os/dependency_collector_spec.rb

@@ -0,0 +1,23 @@
+require "dependency_collector"


Sorry, I may have misread/typed before but this is still the wrong filename: please put it in test/dependency_collector_spec.rb and add tests in test/os/mac/dependency_collector_spec.rb (these files both already exist)

😕 I'm confused as to what goes where. Coincidentally, tests have to be updated to account for the "conditional nature" of bzip2 and zip dependencies. Shall I make tests for macOS unconditional?

Unconditional version that will work on macOS only:

it "creates a 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

Conditional version that (I think) will work on both macOS and Linux

though, I don't know whether if which("bzip2") works in tests.

it "creates a resource dependency from a '.bz2' URL" do resource = Resource.new resource.url("http://example.com/foo.tar.bz2") if which("bzip2") expect(subject.add(resource)).to be nil else expect(subject.add(resource)).to eq(Dependency.new("bzip2", [:build])) end end

Sorry, I've not been clear here at all as I kinda forgot how things were structured. Put macOS tests in test/os/mac/dependency_collector_spec.rb (and see how those are done there). If we want to have both macOS and Linux tests (for existing examples we don't have Linux ones) then have a look at test/os/linux/osxfuse_requirement_spec.rb and add relevant tests in there. You may also, while you're there, add tests for e.g. xz and other behaviours that differ.

Would it be useful to have docs to spell more of this out? Thanks and sorry again for all the back and forth.

sjackman · 2018-02-20T00:37:05Z

Bare-bones distros, most commonly used in Docker and CI systems.

Which distros specifically, though? Can you name a few?

Debian and Ubuntu for example

❯❯❯ docker run debian:9 which gzip bzip2 xz
/bin/gzip
❯❯❯ docker run ubuntu:xenial which gzip bzip2 xz
/bin/gzip

We actively support the major distributions of Linux, such as Debian, Ubuntu, Fedora, and CentOS.

My point was more: it doesn't seem there's a definition of what you don't support and won't try to e.g. the bare-bones distros you mention above.

We don't actively support 32-bit Intel, or distributions of Linux that use a libc other than glibc. We can add more systems to the unsupported list as they come up, but they don't come up too terribly often. Most issues are reported from users running the typical distributions. Docker images help a lot in confirming whether an issue is reproducible on their distribution of Linux, or somehow unique to their system configuration (not reproducible in Docker).

It's also worth asking what Linuxbrew wants to do differently to tools like Gentoo Prefix which are much more established. With a smaller team and fewer resources spreading yourselves too thin means a tool that's less useful to anyone.

I develop data analysis applications on macOS and then deploy them to Linux servers using the same package manager (Brew) on both macOS and Linux. I teach workshops where users install packages using a single set of instructions to install software on their laptops running macOS using Homebrew, on Linux using Linuxbrew, and on Windows using Linuxbrew thanks to Windows Subsystem for Linux. Being able to use the same package manager on multiple operating systems is quite powerful.

Linuxbrew uses host executables when possible.

That's not the case in #3809 with curl or here with bzip2.

@maxim-belkin is fixing that now.

There's nearly unlimited variation in what packages may or may not be installed for a given distribution.

That's true but the standard or minimal installation for a distribution provides a minimal subset.

Here's the list of essential (required) Debian packages. It's pretty safe to assume that these packages will be available on most Linux systems.
See https://www.reddit.com/r/debian/comments/6lnakq/what_are_the_essential_packages/

base-files              libattr1                libuuid1
base-passwd             libblkid1               login
bash                    libc-bin                lsb-base
bsdutils                libc6                   mawk
coreutils               libcomerr2              mount
dash                    libfdisk1               multiarch-support
debconf                 libgcc1                 ncurses-base
debianutils             liblzma5                ncurses-bin
diffutils               libmount1               passwd
dpkg                    libpam-modules          perl-base
e2fslibs                libpam-modules-bin      sed
e2fsprogs               libpam-runtime          sensible-utils
findutils               libpam0g                sysvinit-utils
gcc-6-base              libpcre3                tar
grep                    libselinux1             tzdata
gzip                    libsepol1               util-linux
hostname                libsmartcols1           zlib1g
init-system-helpers     libss2
libacl1                 libtinfo5

Reverting changes to Linuxbrew/brew that are no longer needed is an important component of merging Linuxbrew/brew into Homebrew/brew.

👍 . I also think it's worth a discussion at some point between Linuxbrew and Homebrew maintainers as to what the end goal is, a (very loosely) estimated timescale and the similarities and differences in ethos between the projects. This could result in documentation that would make PRs and issues between the projects less confusing.

My starter for ten on this:

Linuxbrew/brew should eventually go away as something that's used by end-users in favour of Linux support being baked sufficiently into Homebrew/brew.

Linuxbrew/brew will remain as a fork (and could be used for those currently on master rather than stable branches) purely to upstream work to Homebrew.

Completing this migration should be aimed to be done by the end of 2018

Homebrew is primarily a non-optioned binary package manager installed into /usr/local and run on macOS. The longer tail we support are older macOS versions, custom prefixes and custom options in formulae.

Linuxbrew/homebrew-core changes never go upstream to Homebrew/homebrew-core unless there's a bug reproducible on both platforms.

Making a change in Homebrew/homebrew-core to add consistency or ease merge conflicts between Homebrew and Linuxbrew is not acceptable.

I'm on board with all of these goals. End of 2018 is a good target date.

MikeMcQuaid · 2018-02-20T09:30:50Z

Thanks for all the information above.

I'm on board with all of these goals. End of 2018 is a good target date.

Is it worth having a Linuxbrew/brew tracking issue to discuss this? I'm happy to create it.

sjackman · 2018-02-20T16:47:31Z

👍 I've created a tracking issue at Linuxbrew/brew#612

maxim-belkin · 2018-02-20T21:57:39Z

bzip2 and zip have been conditional since b084a25
The latest commit (14d7a7a) does the following:

makes xz_dep_if_needed conditional on Linux systems (changes in extend/os/dependency_collector.rb and extend/os/linux/dependency_collector.rb)
removes test/os/dependency_collector_spec.rb
adds Linux-specific tests in test/os/linux/dependency_collector_spec.rb that account for conditional nature of xz, zip, and bzip2
adds macOS-specific tests in test/os/mac/dependency_collector_spec.rb for zip and bzip2 (making sure they are not added as dependencies because they're provided by the host system).

What else could be done:

default implementation of xz_dep_if_needed could be modified to include unless which("xz")

sjackman · 2018-02-20T22:17:54Z

Library/Homebrew/extend/os/linux/dependency_collector.rb

+  def bzip2_dep_if_needed(tags)
+    Dependency.new("bzip2", tags)
+  end
+end


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

sjackman · 2018-02-20T22:18:42Z

Library/Homebrew/dependency_collector.rb

@@ -70,6 +70,14 @@ def xz_dep_if_needed(tags)
    Dependency.new("xz", tags)


If Mike agrees, change this to…

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

sjackman · 2018-02-20T22:20:11Z

default implementation of xz_dep_if_needed could be modified to include unless which("xz")

That makes sense to me.

maxim-belkin · 2018-02-20T23:02:21Z

if default implementation of xz_dep_if_needed is augmented with unless ... then there will be no need in redefining these on Linux so some of changes will go away.

MikeMcQuaid · 2018-02-21T08:31:57Z

Library/Homebrew/extend/os/linux/dependency_collector.rb

@@ -0,0 +1,5 @@
+class DependencyCollector
+  def xz_dep_if_needed(tags)
+    Dependency.new("xz", tags) unless which("xz")


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

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

MikeMcQuaid · 2018-02-21T14:31:34Z

Thanks @maxim-belkin! A long, painful journey but we got there in the end!

sjackman · 2018-02-21T17:01:37Z

Thanks for this PR, Maxim! And thanks for merging, Mike!

maxim-belkin added 5 commits February 16, 2018 10:41

zip and bzip2 dependencies when needed

444b292

On some systems identified as Linux, zip and bzip2 might not be available. Therefore, on such platforms we add them unconditionally as dependencies when required. On Mac, these dependencies are always satisfied.

Fixing brew-style offenses

5a92976

Adding tests for dependency_collector on Linux

a53171d

Fixing test

5cfcd2a

Removing unnecessary arguments

1b74f21

sjackman suggested changes Feb 16, 2018

View reviewed changes

MikeMcQuaid mentioned this pull request Feb 18, 2018

brew.sh: changes from Linuxbrew (Linux fork) #3809

Merged

6 tasks

Refactoring based on suggestions

3dabebb

Defaulting zip_dep_if_needed(tags) and bzip2_dep_if_needed(tags) methods to those on Linux and overriding them on macOS.

sjackman suggested changes Feb 19, 2018

View reviewed changes

maxim-belkin added 2 commits February 19, 2018 07:21

Making zip and bzip2 dependecies conditional

b084a25

Here, we are adding `unless which("zip")` and `unless which("bzip2")` and, thus, make `zip` and `bzip2` dependencies conditional.

Renaming linux-specifix file with tests

73a3592

Files are globbed based on their name. Therefore, we have to rename them so tests for Linux are not executed on a Mac.

MikeMcQuaid requested changes Feb 19, 2018

View reviewed changes

maxim-belkin added 2 commits February 19, 2018 09:54

Code refactoring

d25fc5c

Code refactoring v2.0

306c190

MikeMcQuaid reviewed Feb 19, 2018

View reviewed changes

Code refactoring v3.0

c6dac68

MikeMcQuaid reviewed Feb 19, 2018

View reviewed changes

Code refactoring 4.0

14d7a7a

sjackman suggested changes Feb 20, 2018

View reviewed changes

MikeMcQuaid reviewed Feb 21, 2018

View reviewed changes

maxim-belkin added 2 commits February 21, 2018 13:44

brew style: replace tabs with spaces

c83dd0d

Make 'xz' and 'cvs' dependencies conditional

f887400

MikeMcQuaid approved these changes Feb 21, 2018

View reviewed changes

MikeMcQuaid merged commit 1811c77 into Homebrew:master Feb 21, 2018

maxim-belkin deleted the brew-unix-2 branch March 12, 2018 18:49

Homebrew locked and limited conversation to collaborators May 4, 2018


		def zip_dep_if_needed(*); end

		def bzip2_dep_if_needed(*); end

		@@ -0,0 +1 @@
		require "test/os/linux/dependency_collector" if OS.linux?

		@@ -70,6 +70,14 @@ def xz_dep_if_needed(tags)
		Dependency.new("xz", tags)

bzip2 and zip dependencies_if_needed #3813

bzip2 and zip dependencies_if_needed #3813

Conversation

maxim-belkin commented Feb 16, 2018

sjackman commented Feb 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Feb 17, 2018

MikeMcQuaid commented Feb 17, 2018

MikeMcQuaid commented Feb 17, 2018

sjackman commented Feb 17, 2018

sjackman commented Feb 17, 2018 • edited Loading

maxim-belkin commented Feb 17, 2018

MikeMcQuaid commented Feb 18, 2018

iMichka commented Feb 18, 2018

iMichka commented Feb 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Unconditional version that will work on macOS only:

Conditional version that (I think) will work on both macOS and Linux

Choose a reason for hiding this comment

sjackman commented Feb 20, 2018 • edited Loading

MikeMcQuaid commented Feb 20, 2018

sjackman commented Feb 20, 2018 • edited Loading

maxim-belkin commented Feb 20, 2018

Choose a reason for hiding this comment

sjackman Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

sjackman commented Feb 20, 2018

maxim-belkin commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Feb 21, 2018

sjackman commented Feb 21, 2018

sjackman commented Feb 17, 2018 •

edited

Loading

sjackman commented Feb 20, 2018 •

edited

Loading

sjackman commented Feb 20, 2018 •

edited

Loading

sjackman Feb 20, 2018 •

edited

Loading