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

Use have_header() to check for the existence of iconv.h #1218

Merged
merged 1 commit into from Oct 30, 2015

Conversation

Projects
None yet
8 participants
@neonichu
Contributor

neonichu commented Jan 7, 2015

Simply checking /usr/include is no longer working on the latest versions of Xcode and OS X, because there is no /usr/include anymore. On OS X 10.10 with Xcode 6, the header resides in /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/usr/include/iconv.h

In addition to that, the simple check also ignores things like --with-iconv-dir, of course.

@knu

This comment has been minimized.

Show comment
Hide comment
@knu

knu Jan 7, 2015

Member

Are you using Xcode 6.0? As far as I know Xcode 6.1 should have fixed the problem.
http://stackoverflow.com/questions/27328049/missing-usr-include-after-yosemite-and-xcode-install

Member

knu commented Jan 7, 2015

Are you using Xcode 6.0? As far as I know Xcode 6.1 should have fixed the problem.
http://stackoverflow.com/questions/27328049/missing-usr-include-after-yosemite-and-xcode-install

@neonichu

This comment has been minimized.

Show comment
Hide comment
@neonichu

neonichu Jan 7, 2015

Contributor

I am using Xcode 6.1, not sure what the exact conditions for the existence of /usr/include are, then. Clang will find the header from the 10.10 SDK automatically, anyway.

In any case, I don't think it makes sense to hardcode a path when options like --with-iconv-dir exist.

Contributor

neonichu commented Jan 7, 2015

I am using Xcode 6.1, not sure what the exact conditions for the existence of /usr/include are, then. Clang will find the header from the 10.10 SDK automatically, anyway.

In any case, I don't think it makes sense to hardcode a path when options like --with-iconv-dir exist.

@knu

This comment has been minimized.

Show comment
Hide comment
@knu

knu Jan 7, 2015

Member

This test is merely for checking if user has properly installed Xcode Command Line Tools. Using have_header() here will not serve the purpose of detecting missing Command Line Tools because it would locate iconv.h installed by Homebrew (or MacPort, etc.) even if the Tools were not installed.

The reason behind the test is that we have seen so many users fail to install nokogiri because they did not have the Command Line Tools properly installed. One common problem was, when a user had (lib)iconv installed via Homebrew while not installing Xcode Command Line Tools libxml2 would pick the header from Homebrew (typically /usr/local/include/iconv.h) as a fallback from missing iconv.h in the system include path (which should be installed as part of the Tools), but the library found would be the one from the system (/usr/lib/libiconv.dylib; part of the base system) which is incompatible with the said header.

Member

knu commented Jan 7, 2015

This test is merely for checking if user has properly installed Xcode Command Line Tools. Using have_header() here will not serve the purpose of detecting missing Command Line Tools because it would locate iconv.h installed by Homebrew (or MacPort, etc.) even if the Tools were not installed.

The reason behind the test is that we have seen so many users fail to install nokogiri because they did not have the Command Line Tools properly installed. One common problem was, when a user had (lib)iconv installed via Homebrew while not installing Xcode Command Line Tools libxml2 would pick the header from Homebrew (typically /usr/local/include/iconv.h) as a fallback from missing iconv.h in the system include path (which should be installed as part of the Tools), but the library found would be the one from the system (/usr/lib/libiconv.dylib; part of the base system) which is incompatible with the said header.

@neonichu

This comment has been minimized.

Show comment
Hide comment
@neonichu

neonichu Jan 7, 2015

Contributor

That makes sense, but it is apparently an insufficient check. Since 10.9, checking the return value of xcode-select -p is enough, since installing Xcode brings the CLT along automatically. So what would your opinion be on amending the check with this on newer systems?

Contributor

neonichu commented Jan 7, 2015

That makes sense, but it is apparently an insufficient check. Since 10.9, checking the return value of xcode-select -p is enough, since installing Xcode brings the CLT along automatically. So what would your opinion be on amending the check with this on newer systems?

@zenspider

This comment has been minimized.

Show comment
Hide comment
@zenspider

zenspider Jan 9, 2015

Contributor

Did you run 'Xcode-select --install'?

Contributor

zenspider commented Jan 9, 2015

Did you run 'Xcode-select --install'?

@neonichu

This comment has been minimized.

Show comment
Hide comment
@neonichu

neonichu Jan 12, 2015

Contributor

@zenspider No, as it is not needed if one has Xcode installed anymore, according to https://developer.apple.com/library/ios/technotes/tn2339/_index.html. FWIW, building the C extension works just fine - it is just the check which trips it up.

Contributor

neonichu commented Jan 12, 2015

@zenspider No, as it is not needed if one has Xcode installed anymore, according to https://developer.apple.com/library/ios/technotes/tn2339/_index.html. FWIW, building the C extension works just fine - it is just the check which trips it up.

@flavorjones flavorjones added the osx label Jan 20, 2015

@flavorjones

This comment has been minimized.

Show comment
Hide comment
@flavorjones

flavorjones Jan 20, 2015

Member

Do we have consensus on whether this change is an improvement over the existing behavior? I'm a Linux guy, so I don't have a dog in this hunt and am looking for guidance.

Member

flavorjones commented Jan 20, 2015

Do we have consensus on whether this change is an improvement over the existing behavior? I'm a Linux guy, so I don't have a dog in this hunt and am looking for guidance.

@zenspider

This comment has been minimized.

Show comment
Hide comment
@zenspider

zenspider Jan 20, 2015

Contributor

@flavorjones I believe that my version of the instructions work when actually followed, so I don't think they need to be changed at this time.

ETA: I don't really have an opinion either way as I have ZERO interest in owning extconf.rb.

Contributor

zenspider commented Jan 20, 2015

@flavorjones I believe that my version of the instructions work when actually followed, so I don't think they need to be changed at this time.

ETA: I don't really have an opinion either way as I have ZERO interest in owning extconf.rb.

@flavorjones

This comment has been minimized.

Show comment
Hide comment
@flavorjones

flavorjones Jan 21, 2015

Member

Roger that, closing. Thanks, everybody!

Member

flavorjones commented Jan 21, 2015

Roger that, closing. Thanks, everybody!

@tamird

This comment has been minimized.

Show comment
Hide comment
@tamird

tamird May 15, 2015

Hey, this change is the right thing to do. Can we reopen this, please?

I just ran into this issue on a system that has Xcode (but not CLT) installed, and the file /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/usr/include/iconv.h does exist as @neonichu mentioned.

tamird commented May 15, 2015

Hey, this change is the right thing to do. Can we reopen this, please?

I just ran into this issue on a system that has Xcode (but not CLT) installed, and the file /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/usr/include/iconv.h does exist as @neonichu mentioned.

@neonichu

This comment has been minimized.

Show comment
Hide comment
@neonichu

neonichu May 15, 2015

Contributor

As a workaround, you can:

sudo mkdir -p /usr/include
sudo touch /usr/include/iconv.h

as the header isn't actually used when building on current OS X.

Contributor

neonichu commented May 15, 2015

As a workaround, you can:

sudo mkdir -p /usr/include
sudo touch /usr/include/iconv.h

as the header isn't actually used when building on current OS X.

@tamird

This comment has been minimized.

Show comment
Hide comment
@tamird

tamird May 15, 2015

Yikes, so the check should be removed entirely?

tamird commented May 15, 2015

Yikes, so the check should be removed entirely?

@neonichu

This comment has been minimized.

Show comment
Hide comment
@neonichu

neonichu May 15, 2015

Contributor

Sorry for the misunderstanding, what I meant is that /usr/include/iconv.h is never used because the iconv.h from the platform SDK always takes precedence.

Contributor

neonichu commented May 15, 2015

Sorry for the misunderstanding, what I meant is that /usr/include/iconv.h is never used because the iconv.h from the platform SDK always takes precedence.

@flavorjones flavorjones added this to the 1.6.7 milestone Sep 18, 2015

@flavorjones flavorjones reopened this Sep 18, 2015

@flavorjones

This comment has been minimized.

Show comment
Hide comment
@flavorjones

flavorjones Sep 18, 2015

Member

I think I'd like to get this into 1.6.7, since it seems to be turning into an "installation improvement" release. ;)

Thanks to everyone for helping me understand what's going on here. As a Linux guy, it's great to see OSX users stepping up.

Member

flavorjones commented Sep 18, 2015

I think I'd like to get this into 1.6.7, since it seems to be turning into an "installation improvement" release. ;)

Thanks to everyone for helping me understand what's going on here. As a Linux guy, it's great to see OSX users stepping up.

@neonichu

This comment has been minimized.

Show comment
Hide comment
@neonichu

neonichu Oct 7, 2015

Contributor

@flavorjones let me know if there's anything I can/should do to get this merged.

Contributor

neonichu commented Oct 7, 2015

@flavorjones let me know if there's anything I can/should do to get this merged.

@neonichu

This comment has been minimized.

Show comment
Hide comment
@neonichu

neonichu Oct 8, 2015

Contributor

BTW, I have been using this workaround:

$ sudo mkdir /usr/include
$ sudo touch /usr/include/iconv.h

but am now realising that it doesn't work anymore because of OS X 10.11's System Integrity Protection. Directories in /usr cannot be created anymore without turning it off.

Contributor

neonichu commented Oct 8, 2015

BTW, I have been using this workaround:

$ sudo mkdir /usr/include
$ sudo touch /usr/include/iconv.h

but am now realising that it doesn't work anymore because of OS X 10.11's System Integrity Protection. Directories in /usr cannot be created anymore without turning it off.

@kjs3

This comment has been minimized.

Show comment
Hide comment
@kjs3

kjs3 Oct 12, 2015

I hope this gets merged soon. The workaround is annoying.

Here's what I did:

  1. Disable SIP
  2. Do what @neonichu did above.
  3. Re-enable SIP csrutil enable

kjs3 commented Oct 12, 2015

I hope this gets merged soon. The workaround is annoying.

Here's what I did:

  1. Disable SIP
  2. Do what @neonichu did above.
  3. Re-enable SIP csrutil enable
@neonichu

This comment has been minimized.

Show comment
Hide comment
@neonichu

neonichu Oct 13, 2015

Contributor

FWIW, I'm building from source for now — posted some quick instructions here: http://buegling.com/blog/2015/4/26/building-nokogiri-on-os-x

Contributor

neonichu commented Oct 13, 2015

FWIW, I'm building from source for now — posted some quick instructions here: http://buegling.com/blog/2015/4/26/building-nokogiri-on-os-x

@segiddins

This comment has been minimized.

Show comment
Hide comment
@segiddins

segiddins Oct 29, 2015

Is there anything I can do to get this merged? It's literally impossible to install nokogiri without patching it on my machine at the moment, and that's quite annoying.

segiddins commented Oct 29, 2015

Is there anything I can do to get this merged? It's literally impossible to install nokogiri without patching it on my machine at the moment, and that's quite annoying.

@knu

This comment has been minimized.

Show comment
Hide comment
@knu

knu Oct 29, 2015

Member

Will handle this shortly!

Member

knu commented Oct 29, 2015

Will handle this shortly!

knu added a commit that referenced this pull request Oct 30, 2015

Merge pull request #1218 from neonichu/master
Use have_header() to check for the existence of iconv.h

@knu knu merged commit 7c14888 into sparklemotion:master Oct 30, 2015

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci The Travis CI build passed
Details
@knu

This comment has been minimized.

Show comment
Hide comment
@knu

knu Oct 30, 2015

Member

Sorry to keep you all waiting!

This issue is closed but we are open to additional improvements to the check and instructions in the error message. Thanks you for your continued support!

Member

knu commented Oct 30, 2015

Sorry to keep you all waiting!

This issue is closed but we are open to additional improvements to the check and instructions in the error message. Thanks you for your continued support!

@flavorjones

This comment has been minimized.

Show comment
Hide comment
@flavorjones

flavorjones Nov 17, 2015

Member

Can anybody confirm that this addressed the original poster's issue?

Member

flavorjones commented Nov 17, 2015

Can anybody confirm that this addressed the original poster's issue?

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Nov 18, 2015

@flavorjones Definitely works in my env 👍

Released version

$ gem install nokogiri
Building native extensions.  This could take a while...
ERROR:  Error installing nokogiri:
    ERROR: Failed to build gem native extension.
-----
The file "/usr/include/iconv.h" is missing in your build environment,
which means you haven't installed Xcode Command Line Tools properly.

To install Command Line Tools, try running `xcode-select --install` on
terminal and follow the instructions.  If it fails, open Xcode.app,
select from the menu "Xcode" - "Open Developer Tool" - "More Developer
Tools" to open the developer site, download the installer for your OS
version and run it.
-----
*** extconf.rb failed ***

Master

$ bundle exec rake
checking for iconv.h... yes
checking for iconv... yes

And then:

$ gem install pkg/nokogiri-1.6.7.rc3.gem 
Building native extensions.  This could take a while...
Successfully installed nokogiri-1.6.7.rc3
1 gem installed

alloy commented Nov 18, 2015

@flavorjones Definitely works in my env 👍

Released version

$ gem install nokogiri
Building native extensions.  This could take a while...
ERROR:  Error installing nokogiri:
    ERROR: Failed to build gem native extension.
-----
The file "/usr/include/iconv.h" is missing in your build environment,
which means you haven't installed Xcode Command Line Tools properly.

To install Command Line Tools, try running `xcode-select --install` on
terminal and follow the instructions.  If it fails, open Xcode.app,
select from the menu "Xcode" - "Open Developer Tool" - "More Developer
Tools" to open the developer site, download the installer for your OS
version and run it.
-----
*** extconf.rb failed ***

Master

$ bundle exec rake
checking for iconv.h... yes
checking for iconv... yes

And then:

$ gem install pkg/nokogiri-1.6.7.rc3.gem 
Building native extensions.  This could take a while...
Successfully installed nokogiri-1.6.7.rc3
1 gem installed
@flavorjones

This comment has been minimized.

Show comment
Hide comment
@flavorjones

flavorjones Nov 22, 2015

Member

@alloy Thank you so much for the confirmation!

Member

flavorjones commented Nov 22, 2015

@alloy Thank you so much for the confirmation!

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Nov 22, 2015

No problemo 🎩👌

alloy commented Nov 22, 2015

No problemo 🎩👌

@tamird tamird referenced this pull request Dec 10, 2015

Merged

Update nokogiri to 1.6.7 #22552

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