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.

Sign up for GitHub

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

Jump to bottom

Fix iconv and clang version detection on OSX #6917

Merged
merged 3 commits into from
Jan 5, 2019
Merged

Fix iconv and clang version detection on OSX #6917

merged 3 commits into from
Jan 5, 2019
+15 −19

Conversation

LordAro
Copy link
Member

@LordAro LordAro commented Sep 24, 2018 •
edited
Loading

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
@LordAro LordAro requested a review from michicc September 24, 2018 21:48
michicc
michicc reviewed Sep 30, 2018
View reviewed changes
michicc
michicc previously requested changes Sep 30, 2018
View reviewed changes
@LordAro LordAro force-pushed the macosx-clang-iconv-fix branch from cfc9fde to c53a7d9 Compare September 30, 2018 19:39
@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 LordAro force-pushed the macosx-clang-iconv-fix branch from c53a7d9 to a72729d Compare November 25, 2018 11:27
@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)"

@LordAro LordAro force-pushed the macosx-clang-iconv-fix branch from a72729d to 30e05e5 Compare November 25, 2018 11:35
@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
View reviewed changes
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.

Sorry, something went wrong.

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

Sorry, something went wrong.

@LordAro LordAro force-pushed the macosx-clang-iconv-fix branch from 30e05e5 to 7ed3153 Compare January 5, 2019 16:16
@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
Reviewers

@TrueBrain TrueBrain

@michicc michicc

@orudge orudge

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@LordAro @andythenorth @orudge @nielsmh @JGRennison @michicc @TrueBrain