This repository has been archived by the owner. It is now read-only.

removing libiconv duplicate #10464

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
7 participants
@camillol
Contributor

camillol commented Feb 25, 2012

libiconv ships with OS X. A few build scripts have/used to have trouble detecting it correctly, so it was added as a formula. Then some other formulas, seeing that it was there, started using it as a dependency, even though they are capable of using the system libiconv.

The first commit patches all formulas that currently depend_on 'libiconv' to use the system iconv instead. All build on my machine; not all were tested thoroughly at runtime, but since the system iconv is GNU libiconv (check /usr/include/iconv.h), I don't expect programs to notice any difference once you get them to link with it.

The second commit removes the libiconv formula and blacklists it as a duplicate, otherwise spurious dependencies are bound to keep popping up. The formula should be moved to the homebrew-alt duplicates branch, imho.

camillol added some commits Feb 25, 2012

remove spurious libiconv dependencies
OS X ships with iconv. glib failed to detect it, and this snowballed into a few
formulas requiring a superfluous duplicate install of libiconv. All of the formulas in this commit compile without the libiconv dependency.
@Sharpie

This comment has been minimized.

Show comment Hide comment
@Sharpie

Sharpie Feb 25, 2012

Contributor

Which system did you test this on? For example, a newer libiconv could be required on Leopard or Snow Leopard.

Contributor

Sharpie commented Feb 25, 2012

Which system did you test this on? For example, a newer libiconv could be required on Leopard or Snow Leopard.

@MikeMcQuaid

This comment has been minimized.

Show comment Hide comment
@MikeMcQuaid

MikeMcQuaid Feb 25, 2012

Member

Then we should at least disable it on 10.7 I think. I think we need to start having per-Xcode/OS blacklists.

Member

MikeMcQuaid commented Feb 25, 2012

Then we should at least disable it on 10.7 I think. I think we need to start having per-Xcode/OS blacklists.

@adamv

This comment has been minimized.

Show comment Hide comment
@adamv

adamv Feb 25, 2012

Contributor

We added iconv in the first place because glib wouldn't detect the system version, but we patch it now. So I think this analysis is correct, but of course we need to do a little testing first.

Contributor

adamv commented Feb 25, 2012

We added iconv in the first place because glib wouldn't detect the system version, but we patch it now. So I think this analysis is correct, but of course we need to do a little testing first.

@jacknagel

This comment has been minimized.

Show comment Hide comment
@jacknagel

jacknagel Feb 25, 2012

Contributor

Looks to me like the system libiconv has been v1.11 in 10.5, 10.6, and 10.7.

Contributor

jacknagel commented Feb 25, 2012

Looks to me like the system libiconv has been v1.11 in 10.5, 10.6, and 10.7.

@camillol

This comment has been minimized.

Show comment Hide comment
@camillol

camillol Feb 26, 2012

Contributor

I tested on 10.7. Could anybody test on 10.6?

Contributor

camillol commented Feb 26, 2012

I tested on 10.7. Could anybody test on 10.6?

@adamv

This comment has been minimized.

Show comment Hide comment
@adamv

adamv Feb 26, 2012

Contributor

What would be a good test?

Contributor

adamv commented Feb 26, 2012

What would be a good test?

@camillol

This comment has been minimized.

Show comment Hide comment
@camillol

camillol Feb 26, 2012

Contributor

I think it would be sufficient to apply the patch and build all the formulas that were changed.

Contributor

camillol commented Feb 26, 2012

I think it would be sufficient to apply the patch and build all the formulas that were changed.

@jacknagel

This comment has been minimized.

Show comment Hide comment
@jacknagel

jacknagel Feb 26, 2012

Contributor

Everything checks out on 10.6.

Contributor

jacknagel commented Feb 26, 2012

Everything checks out on 10.6.

@camillol

This comment has been minimized.

Show comment Hide comment
@camillol

camillol Feb 26, 2012

Contributor

Thanks!

Contributor

camillol commented Feb 26, 2012

Thanks!

@jacknagel

This comment has been minimized.

Show comment Hide comment
@jacknagel

jacknagel Mar 2, 2012

Contributor

glib fixed their iconv detection bug, finally: https://bugzilla.gnome.org/show_bug.cgi?id=529806

Contributor

jacknagel commented Mar 2, 2012

glib fixed their iconv detection bug, finally: https://bugzilla.gnome.org/show_bug.cgi?id=529806

@jacknagel

This comment has been minimized.

Show comment Hide comment
@jacknagel

jacknagel Mar 3, 2012

Contributor

Probably the easiest way to move this forward is to (a) phase out the existing libiconv dependency declarations from formula that grew them spuriously, (b) wait a week or two to in case any weird issues pop up, and then finally (c) remove the libiconv formula.

For example, the zsh formula gained its libiconv dep for no apparent reason recently (IOW, there wasn't any comments or discussion about it that I could find) so I am comfortable removing it now.

Contributor

jacknagel commented Mar 3, 2012

Probably the easiest way to move this forward is to (a) phase out the existing libiconv dependency declarations from formula that grew them spuriously, (b) wait a week or two to in case any weird issues pop up, and then finally (c) remove the libiconv formula.

For example, the zsh formula gained its libiconv dep for no apparent reason recently (IOW, there wasn't any comments or discussion about it that I could find) so I am comfortable removing it now.

@camillol

This comment has been minimized.

Show comment Hide comment
@camillol

camillol Mar 3, 2012

Contributor

I agree. Let's start with the first commit.

Contributor

camillol commented Mar 3, 2012

I agree. Let's start with the first commit.

@jacknagel

This comment has been minimized.

Show comment Hide comment
@jacknagel

jacknagel Mar 3, 2012

Contributor

Pulled the first commit.

Contributor

jacknagel commented Mar 3, 2012

Pulled the first commit.

@mxcl

This comment has been minimized.

Show comment Hide comment
@mxcl

mxcl Mar 5, 2012

Member

Looking forward to removing this. Good work @camillol.

Member

mxcl commented Mar 5, 2012

Looking forward to removing this. Good work @camillol.

@jacknagel

This comment has been minimized.

Show comment Hide comment
@jacknagel

jacknagel Mar 13, 2012

Contributor

We've been operating without any deps on libiconv for 11 days now; probably going to pull trigger on this tonight unless somebody yells.

Contributor

jacknagel commented Mar 13, 2012

We've been operating without any deps on libiconv for 11 days now; probably going to pull trigger on this tonight unless somebody yells.

@jacknagel jacknagel closed this in ad70207 Mar 14, 2012

@MikeMcQuaid

This comment has been minimized.

Show comment Hide comment
@MikeMcQuaid

MikeMcQuaid Mar 14, 2012

Member

Nice.

Member

MikeMcQuaid commented Mar 14, 2012

Nice.

@camillol

This comment has been minimized.

Show comment Hide comment
@camillol

camillol Mar 14, 2012

Contributor

:D

Contributor

camillol commented Mar 14, 2012

:D

@hale

This comment has been minimized.

Show comment Hide comment
@hale

hale Mar 16, 2012

Could this be the reason I'm now having trouble installing nokogiri, a Rails gem which depends on libiconv? I remember even on snow leopard there were instructions to instal libiconv through homebrew before trying to install the gem.

Sorry if I've misunderstood~

hale commented Mar 16, 2012

Could this be the reason I'm now having trouble installing nokogiri, a Rails gem which depends on libiconv? I remember even on snow leopard there were instructions to instal libiconv through homebrew before trying to install the gem.

Sorry if I've misunderstood~

@jacknagel

This comment has been minimized.

Show comment Hide comment
@jacknagel

jacknagel Mar 16, 2012

Contributor

libiconv can be obtained from Homebrew-alt if you need it for the gem. Though technically it's a bug that the gem can't pick up the system iconv symbols correctly.

Contributor

jacknagel commented Mar 16, 2012

libiconv can be obtained from Homebrew-alt if you need it for the gem. Though technically it's a bug that the gem can't pick up the system iconv symbols correctly.

etehtsea added a commit to etehtsea/homebrew that referenced this pull request Mar 17, 2012

Remove libiconv duplicate
Commit ee2c3ab ("Remove spurious libiconv dependencies") pruned all
existing "depends_on 'libiconv'" usages from Homebrew in preparation for
removing the libiconv dupe itself.

Now that is done, and we can remove and blacklist it. It can be obtained
from Homebrew-alt.

Closes #10464.

Signed-off-by: Jack Nagel <jacknagel@gmail.com>

Conflicts:

	Library/Homebrew/cmd/doctor.rb
@camillol

This comment has been minimized.

Show comment Hide comment
@camillol

camillol Mar 20, 2012

Contributor

@hale I have no problem installing nokogiri on Lion without the libiconv formula, so I don't think that's your problem.

Contributor

camillol commented Mar 20, 2012

@hale I have no problem installing nokogiri on Lion without the libiconv formula, so I don't think that's your problem.

@hale

This comment has been minimized.

Show comment Hide comment
@hale

hale Apr 1, 2012

Yes sorry, it was a problem with my RVM installation / configuration.

hale commented Apr 1, 2012

Yes sorry, it was a problem with my RVM installation / configuration.

Sharpie pushed a commit to Sharpie/homebrew that referenced this pull request Jun 18, 2012

Remove libiconv duplicate
Commit ee2c3ab ("Remove spurious libiconv dependencies") pruned all
existing "depends_on 'libiconv'" usages from Homebrew in preparation for
removing the libiconv dupe itself.

Now that is done, and we can remove and blacklist it. It can be obtained
from Homebrew-alt.

Closes #10464.

Signed-off-by: Jack Nagel <jacknagel@gmail.com>

Sharpie pushed a commit to Sharpie/homebrew that referenced this pull request Sep 12, 2012

Remove libiconv duplicate
Commit ee2c3ab ("Remove spurious libiconv dependencies") pruned all
existing "depends_on 'libiconv'" usages from Homebrew in preparation for
removing the libiconv dupe itself.

Now that is done, and we can remove and blacklist it. It can be obtained
from Homebrew-alt.

Closes #10464.

Signed-off-by: Jack Nagel <jacknagel@gmail.com>

snakeyroc3 pushed a commit to snakeyroc3/homebrew that referenced this pull request Dec 17, 2012

Remove libiconv duplicate
Commit ee2c3ab ("Remove spurious libiconv dependencies") pruned all
existing "depends_on 'libiconv'" usages from Homebrew in preparation for
removing the libiconv dupe itself.

Now that is done, and we can remove and blacklist it. It can be obtained
from Homebrew-alt.

Closes #10464.

Signed-off-by: Jack Nagel <jacknagel@gmail.com>

@xu-cheng xu-cheng locked and limited conversation to collaborators Feb 16, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.