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

Fix iconv and clang version detection on OSX #6917

Merged
merged 3 commits into from Jan 5, 2019

Conversation

LordAro
Copy link
Member

@LordAro LordAro commented Sep 24, 2018

Rationale:

Under OSX, clang is actually all compilers - cc, gcc & clang. We already grep the output of --version to get that gcc might actually be clang, but the version number is hidden. For reasons best known to themselves, Apple have decided to completely remove the "actual" clang version number and use their own.

This screws with the CFLAGS somewhat, as several of them are specific to compiler versions.

It turns out that gcc -v under OSX actually outputs

Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.13.sdk/usr/include/c++/4.2.1
Apple LLVM version 9.1.0 (clang-902.0.39.2)
Target: x86_64-apple-darwin17.7.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

which isn't exactly what we were expecting. Coupled with the fact that the clang "version number" is determined by taking the first 2 digits from the first line of output, and we get version... 10

This is solved by only taking the lines containing "version", which seems to be constant between all variants. We then get version "91", which is still inaccurate, but actually works for our purposes

As for iconv, for some reason on @andythenorth's system iconv.h is only in /usr/local/Cellar/libiconv/1.15/include/iconv.h, and the configure script only searches within /usr/include & /usr/local/include. This seems fairly arbitrary anyway, so some magic involving the compiler output (which seems to be consistent between gcc & clang) and awk gets us a list of search paths to ...search within

@LordAro LordAro force-pushed the macosx-clang-iconv-fix branch 2 times, most recently from 5a714af to cfc9fde Compare September 24, 2018 21:48
config.lib Outdated Show resolved Hide resolved
config.lib Outdated Show resolved Hide resolved
@LordAro
Copy link
Member Author

LordAro commented Oct 21, 2018

In the interests of actually fixing #6880, would it be reasonable to just "fake" the clang version if it's on OSX? arbitrarily set it to 5.0 or something

@LordAro
Copy link
Member Author

LordAro commented Nov 25, 2018

Given recent additions (C++11 enum types), I figure it is better to just force C++11 globally for all compilers. A bit of a stop-gap measure until CMake happens, but it does make it work...

@andythenorth
Copy link
Contributor

Works on macOS 10.13.6 with Clang "Apple LLVM version 10.0.0 (clang-1000.11.45.5)"

@orudge
Copy link
Contributor

orudge commented Dec 3, 2018

Also works for me on Mac OS X 10.11.6 with Clang "Apple LLVM version 8.0.0 (clang-800.0.42.1).

orudge
orudge previously approved these changes Dec 3, 2018
Copy link
Contributor

@orudge orudge left a comment

Choose a reason for hiding this comment

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

Can't see anything that looks obviously wrong to me; tested on OS X 10.11 with Apple LLVM 8 and works for me.

@nielsmh
Copy link
Contributor

nielsmh commented Dec 27, 2018

Oddly, this does not seem to cause problems with the macOS CI tests being run right now, even with no extra CXXFLAGS.

@TrueBrain TrueBrain merged commit 4fbfe34 into OpenTTD:master Jan 5, 2019
@LordAro LordAro deleted the macosx-clang-iconv-fix branch January 5, 2019 16:40
@JGRennison
Copy link
Contributor

With this PR applied:
A version string of "Apple LLVM version 8.0.0 (clang-800.0.42.1)" produces a cc_version of 80.
A version string of "Apple LLVM version 10.0.0 (clang-1000.11.45.5)" produces a cc_version of 10, which doesn't seem correct. This causes some of the subsequent tests of the cc_version variable to be incorrect, though as these only control warning flags compilation still succeeds.

@LordAro
Copy link
Member Author

LordAro commented Jan 13, 2019

Yeah, this was known. It's unfortunate, but there wasn't really any other way around it, without hugely complicating everything. We decided it was better to have the version a bit wrong (and to force c++11) and not worry about it until someone rewrites the buildsystem in cmake or similar

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

Successfully merging this pull request may close these issues.

None yet

7 participants