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

Add back the old imagemagick under the name imagemagick@6 #8756

Closed
wants to merge 1 commit into from

Conversation

crazymykl
Copy link
Contributor

@crazymykl crazymykl commented Jan 12, 2017

This is necessary since rmagick does not work with ImageMagick 7.0.x.

Apologies if I'm not following protocol 100%, it was not super clear how new versions of existing formulae should be formatted.

I bought back the formula for the last 6.9.x ImageMagick nearly verbatim, only changing the class name.

@ilovezfs
Copy link
Contributor

I do not think sufficient need for this formula has been demonstrated, especially given that there are no dependents in Homebrew itself that need it. The formula will be broken in terms of the ability of build systems to detect it via pkg-config unless it's made keg only or is marked as conflicting with the main imagemagick formula, which will then interfere with the installation of formulae that depend on imagemagick. My recommendation is that you host this in your own tap for the time being.

@ilovezfs ilovezfs added legacy Relates to a versioned @ formula new formula PR adds a new formula to Homebrew/homebrew-core labels Jan 12, 2017
@ilovezfs ilovezfs mentioned this pull request Jan 12, 2017
22 tasks
@zmwangx
Copy link
Contributor

zmwangx commented Jan 12, 2017

My recommendation is that you host this in your own tap for the time being.

The problem with that, in the case of IM, is that it is updated frequently, and whenever an update is issued the old tarball is removed from the server. That's why we brew mirror the tarballs.

But indeed, a formula like this wouldn't work very well. The best compromise is to mark it as keg only, and deal with the hassle of having to specify the opt paths (PATH, LDFLAGS, CPPFLAGS, PKG_CONFIG_PATH) where necessary (which isn't always possible in obscure/hand-crafted build systems), or force link it if you don't need any revdeps.

At the very least it comes down to

I do not think sufficient need for this formula has been demonstrated.

@ilovezfs
Copy link
Contributor

The problem with that, in the case of IM, is that it is updated frequently, and whenever an update is issued the old tarball is removed from the server.

Perhaps "someone" should convince upstream to knock that behavior off.

@zmwangx
Copy link
Contributor

zmwangx commented Jan 12, 2017

Perhaps "someone" should convince upstream to knock that behavior off.

That "someone" might need to foot the bill (which might be negligible; I've no idea).

@ilovezfs
Copy link
Contributor

Is there something deficient about the _orig tarballs on all Debian mirrors? https://mirrors.ocf.berkeley.edu/debian/pool/main/i/imagemagick/

@zmwangx
Copy link
Contributor

zmwangx commented Jan 12, 2017

Not sure whether dfsg stripping causes problems, I assume not. Two issues:

  1. Only a handful of versions are available, most are skipped;
  2. Debian removes old tarballs on a regular basis, too.

@ilovezfs
Copy link
Contributor

Only a handful of versions are available, most are skipped;

Is there some benefit the plethora of updates is providing?

Debian removes old tarballs on a regular basis, too.

I guess this is one of the reasons brew update and the Internet exist.

@zmwangx
Copy link
Contributor

zmwangx commented Jan 12, 2017

Is there some benefit the plethora of updates is providing?

Probably not until you have a CVE.

I guess this is one of the reasons brew update and the Internet exist.

Well, we're talking about user-hosted taps here... Not really our problem, but still.

@ilovezfs
Copy link
Contributor

User-hosted taps get updated by brew update.

@zmwangx
Copy link
Contributor

zmwangx commented Jan 12, 2017

It depends on said user updating the formula diligently :)

@ilovezfs
Copy link
Contributor

It depends on said user updating the formula diligently

Indeed.

@crazymykl
Copy link
Contributor Author

Again, the rmagick gem, by far the most common reason I have seen ImageMagick installed on macOS, does not build with major version 7. It is unlikely that a version that builds against both 6 and 7 will be forthcoming.,

rmagick has almost 8 million downloads.

@ilovezfs
Copy link
Contributor

perhaps we've just given them a reason to add support for 7

@crazymykl
Copy link
Contributor Author

There's also the question of deployment. A lot of apps are ultimately hosted on OSes which won't have a package for ImageMagick@7 for year, or possibly ever (e. g. CentOS).

Flipping the question: what exists that only builds against ImageMagick@7? Maybe 7 should be made keg-only until it is better supported in the wild than version 6.

@ilovezfs
Copy link
Contributor

It handles all of the Homebrew dependents. There's no reason to make it keg_only.

@zmwangx
Copy link
Contributor

zmwangx commented Jan 12, 2017

by far the most common reason I have seen ImageMagick installed on macOS

Experience varies, but I'm pretty sure IM on macOS is more often used as standalone tools. Most of those 8 million downloads could well be on Linux web servers, and I'm pretty sure PHP bindings for IM are more prevalent than the Ruby binding on servers.

what exists that only builds against ImageMagick@7?

Homebrew ships cutting edge software. That's the nature of the project.

@crazymykl
Copy link
Contributor Author

I have made a tap, for whoever finds this after googling rmagick homebrew imagemagick 7.

brew install crazymykl/homebrew-custom/imagemagick@6

@jacquerie
Copy link

jacquerie commented Jan 14, 2017

Thanks, @crazymykl! I'm using your tap because Wand 0.4.4 appears to support only ImageMagick@6. With ImageMagick@7 I was getting:

ImportError: MagickWand shared library not found.
You probably had not installed ImageMagick library.
Try to install:
  brew install freetype imagemagick

(typing this for the benefit of people searching for the error above).

@MikeMcQuaid
Copy link
Member

I talked with @ilovezfs and my understanding of the conversation was that we (or at least I) am happy to accept this as long as it's keg-only.

@@ -130,6 +130,9 @@ def install

def caveats
s = <<-EOS.undent
rmagick 2.x does not work with ImageMagick 7, and some invokations of
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not 100% thrilled with the verbiage here.

@crazymykl crazymykl force-pushed the version-imagemagick branch 2 times, most recently from d4b08be to 3bfd951 Compare January 16, 2017 16:38
@crazymykl
Copy link
Contributor Author

Updated. Let me know if there's more to do.

s = <<-EOS.undent
base = <<-EOS.undent
rmagick 2.x does not work with ImageMagick 7, and some invocations of
`convert` may have changed syntax.
Copy link
Member

Choose a reason for hiding this comment

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

Let's omit this; there's more than just rmagick and this isn't in keeping with other formulae updates.

sha256 "9258c4fc88187040a23e8c62e451ef725eb504b721181eba4e6dda382884bf8e" => :sierra
sha256 "6bb941944b07a17da426a1fae986ef8b13ba091dd52728e2ed59adb19d5fcce0" => :el_capitan
sha256 "7246f8e60983e429ee088e8a3d2f2b3fb780c868cc5f5d3effff4146ddafe12e" => :yosemite
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 the bottle block, it'll get re-added when we pull.

export PERL5LIB="#{HOMEBREW_PREFIX}/lib/perl5/site_perl":$PERL5LIB
EOS
s if build.with? "perl"
end
Copy link
Member

Choose a reason for hiding this comment

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

Probably just worth removing these caveats (or adjusting them to point to opt_lib )

depends_on "libtiff" => :recommended
depends_on "freetype" => :recommended

depends_on :x11 => :optional
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove this option.

@crazymykl
Copy link
Contributor Author

I did the thing.

@MikeMcQuaid
Copy link
Member

Thanks for your contribution to Homebrew! Without people like you submitting PRs we couldn't run this project. You rock!

@MikeMcQuaid
Copy link
Member

@crazymykl You may want to now remove your tap and/or add the according tap_migrations.json line.

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
legacy Relates to a versioned @ formula new formula PR adds a new formula to Homebrew/homebrew-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants