clang version string has changed on OS X; CLANG_CXX and CLANG_CC need generalization.

RESOLVED FIXED in mozilla21

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: pnkfelix, Assigned: pnkfelix)

Tracking

unspecified
mozilla21
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

The Mac OS X version of clang included with Xcode circa 4.5.2 has changed the output format so that it no longer includes the substring "clang version"

See below:

% xcodebuild -version
Xcode 4.5.2
Build version 4G2008a
% which clang++
/usr/bin/clang++
% ls -l /usr/bin/clang++ 
lrwxr-xr-x  1 root  wheel  5 Feb 12 11:34 /usr/bin/clang++ -> clang
% ls -l /usr/bin/clang
-rwxr-xr-x  1 root  wheel  26291280 Feb 12 11:34 /usr/bin/clang
% clang++ --version
Apple LLVM version 4.2 (clang-425.0.24) (based on LLVM 3.2svn)
Target: x86_64-apple-darwin12.2.1
Thread model: posix


This breaks the way that we detect the presence of clang in toolchain.m4:

CLANG_CC=
CLANG_CXX=
if test "$GCC" = yes; then
   if test "`$CC -v 2>&1 | grep -c 'clang version'`" != "0"; then
     CLANG_CC=1
   fi
fi


And subsequently in the configure script, we end up not setting certain parameters that were put in place to accommodate clang.
Created attachment 712909 [details] [diff] [review]
patch A v1: generalize test command in both copies of toolchain.m4
Created attachment 712911 [details] [diff] [review]
patch A v2: generalize test command in both copies of toolchain.m4

Took out ansi color annotations that corrupted patch output.  Otherwise same as patch A v1.
Attachment #712909 - Attachment is obsolete: true
Comment on attachment 712911 [details] [diff] [review]
patch A v2: generalize test command in both copies of toolchain.m4

(folks in #jsapi channel said :ted or :glandium are good potential reviewers.)
Attachment #712911 - Flags: review?(mh+mozilla)
Comment on attachment 712911 [details] [diff] [review]
patch A v2: generalize test command in both copies of toolchain.m4

Review of attachment 712911 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/toolchain.m4
@@ +55,5 @@
>  
>  CLANG_CC=
>  CLANG_CXX=
>  if test "$GCC" = yes; then
> +   if test "`$CC -v 2>&1 | grep -c 'clang version\\|Apple.*clang'`" != "0"; then

I doubt this works with two backslashes.
Attachment #712911 - Flags: review?(mh+mozilla) → review-
Comment on attachment 712911 [details] [diff] [review]
patch A v2: generalize test command in both copies of toolchain.m4

Review of attachment 712911 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/toolchain.m4
@@ +55,5 @@
>  
>  CLANG_CC=
>  CLANG_CXX=
>  if test "$GCC" = yes; then
> +   if test "`$CC -v 2>&1 | grep -c 'clang version\\|Apple.*clang'`" != "0"; then

Hmm, it works for me (and I had thought it was necessary to escape the backslash in the quoted string), but yes, retesting shows they are not actually necessary.  So I will remove them.
Created attachment 713336 [details] [diff] [review]
patch A v3: generalize test command in both copies of toolchain.m4

(Removed backslash-escape in response to review comments.)
Attachment #712911 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #713336 - Flags: review?(mh+mozilla)
Attachment #713336 - Flags: review?(mh+mozilla) → review+
Created attachment 713355 [details] [diff] [review]
patch A v4: generalize test command in both copies of toolchain.m4
Attachment #713336 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #713355 - Attachment description: revised version of patch made using hg instead of git (for proper formatting) → patch A v4: generalize test command in both copies of toolchain.m4
(Assignee)

Updated

5 years ago
Assignee: nobody → pnkfelix
Status: NEW → ASSIGNED
I've done a try build (using all targets except windows, as suggested in #developers):

  https://tbpl.mozilla.org/?tree=Try&rev=be6908e27730

So now someone with appropriate permissions can check this in.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dc58b4dbeb3
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1dc58b4dbeb3
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Depends on: 842131
You need to log in before you can comment on or make changes to this bug.