-
-
Notifications
You must be signed in to change notification settings - Fork 905
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
Conversation
Are you using Xcode 6.0? As far as I know Xcode 6.1 should have fixed the problem. |
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 |
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. |
That makes sense, but it is apparently an insufficient check. Since 10.9, checking the return value of |
Did you run 'Xcode-select --install'? |
@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. |
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. |
@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 |
Roger that, closing. Thanks, everybody! |
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 |
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. |
Yikes, so the check should be removed entirely? |
Sorry for the misunderstanding, what I meant is that |
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. |
@flavorjones let me know if there's anything I can/should do to get this merged. |
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 |
I hope this gets merged soon. The workaround is annoying. Here's what I did:
|
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 |
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. |
Will handle this shortly! |
Use have_header() to check for the existence of iconv.h
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! |
Can anybody confirm that this addressed the original poster's issue? |
@flavorjones Definitely works in my env 👍 Released version
Master
And then:
|
@alloy Thank you so much for the confirmation! |
No problemo 🎩👌 |
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.