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

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

Merged
merged 1 commit into from
Oct 30, 2015

Conversation

neonichu
Copy link
Contributor

@neonichu 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
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor

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

@neonichu
Copy link
Contributor Author

@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
Copy link
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.

@zenspider
Copy link
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.

@flavorjones
Copy link
Member

Roger that, closing. Thanks, everybody!

@tamird
Copy link

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
Copy link
Contributor Author

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
Copy link

tamird commented May 15, 2015

Yikes, so the check should be removed entirely?

@neonichu
Copy link
Contributor Author

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
Copy link
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.

@neonichu
Copy link
Contributor Author

neonichu commented Oct 7, 2015

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

@neonichu
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Member

knu commented Oct 29, 2015

Will handle this shortly!

knu added a commit that referenced this pull request Oct 30, 2015
Use have_header() to check for the existence of iconv.h
@knu knu merged commit 7c14888 into sparklemotion:master Oct 30, 2015
@knu
Copy link
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
Copy link
Member

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

@alloy
Copy link

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
Copy link
Member

@alloy Thank you so much for the confirmation!

@alloy
Copy link

alloy commented Nov 22, 2015

No problemo 🎩👌

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

Successfully merging this pull request may close these issues.

8 participants