298531 - NSPR support for Mac 10.4u ("universal") SDK

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Mark Mentovai]

NSPR companion to bug 298529.  Need to properly interpret SDK version number now
that Apple's broken with tradition and stuck letters in it.

Comment 1

[Mark Mentovai]

Attachment: SDK version detection, deployment target, and -fPIC

SDK detection as in bug 298529.

Use --enable-macos-target to set MACOSX_DEPLOYMENT_TARGET in the environment as
in bug 292530.

Use -fPIC for dynamic libraries now on OS X (already done in core) - even
though it's the default, certain other compiler flags may turn it off.	This
has been in my tree forever, and now seems as good a time as any to get it in.

Comment 2

[Wan-Teh Chang]

Comment on attachment 187096 [details] [diff] [review]
SDK version detection, deployment target, and -fPIC

Mark, you are really keeping up with the bleeding edge :-)

Some comments and questions.

>Index: mozilla/nsprpub/configure.in
...
>+    DSO_CFLAGS='-fPIC'

-fPIC doesn't need to be quoted.  Please remove the '' to be
consistent with the rest of the file.

>+    export MACOSX_DEPLOYMENT_TARGET="$MACOS_VERSION_MAJOR.$MACOS_VERSION_MINOR"

I don't think we need "export" here.

I'm wondering if we can replace the original MACOS_DEPLOYMENT_TARGET by
MACOSX_DEPLOYMENT_TARGET.  These two only differ in the existence of the third
version component.  Our code only uses 00 for the third version component in
MACOS_DEPLOYMENT_TARGET.

Is it documented that MACOSX_DEPLOYMENT_TARGET only has two version components
(major and minor)?

>-            TARGET_ARCH_LIB=powerpc-apple-darwin`echo $NEXT_ROOT | $PERL -pe 's/MacOSX10\.([^\.]*)//;if ($1) {$_=$1+4;} else {$_="'${target_os}'";s/(\d+)//;$_=$1;}'`
>+            TARGET_ARCH_LIB=${target_cpu}-${target_vendor}-darwin`echo $NEXT_ROOT | $PERL -pe 's/MacOSX10\.([\d]*)//;if ($1) {$_=$1+4;} else {$_="'${target_os}'";s/(\d+)//;$_=$1;}'`

Is it necessary to use ${target_vendor}?

Should we replace the hardcoded 10 (in MacOSX10) by ([\d]*) ?

Comment 3

[Wan-Teh Chang]

Comment on attachment 187096 [details] [diff] [review]
SDK version detection, deployment target, and -fPIC

Mark, please ignore my question about the 10 in MacOSX10.  I
understand why it should be a hardcoded 10 now.

Comment 4

[Mark Mentovai]

(In reply to comment #2)
> Mark, you are really keeping up with the bleeding edge :-)

I'm pretty sure I've fallen over it by now!

> >+    export MACOSX_DEPLOYMENT_TARGET="$MACOS_VERSION_MAJOR.$MACOS_VERSION_MINOR"
> 
> I don't think we need "export" here.

I'd rather use export to more closely mimic the build environment in the
configure environment in case the setting causes cc/ld heartburn.  I think it's
better to catch this sort of error at configure time than at compile time.

> I'm wondering if we can replace the original MACOS_DEPLOYMENT_TARGET by
> MACOSX_DEPLOYMENT_TARGET.  These two only differ in the existence of the third
> version component.  Our code only uses 00 for the third version component in
> MACOS_DEPLOYMENT_TARGET.

I'd like to rip MACOS_DEPLOYMENT_TARGET out altogether, but I was planning on
handling that in a future bug.  We don't need it any more, we have better and
more widely-accepted macros at our disposal courtesy of the system's build
environment.  MAC_OS_X_VERSION_MIN_REQUIRED gives the deployment target based on
the environment setting of MACOSX_DEPLOYMENT_TARGET, and
MAC_OS_X_VERSION_MAX_ALLOWED gives the SDK.  These are best used with macros
such as MAC_OS_X_VERSION_10_2 for safe comparisons.

See also bug 298543 - some of the deployment target stuff needs to change anyway.

> Is it documented that MACOSX_DEPLOYMENT_TARGET only has two version components
> (major and minor)?

Yes, ld vomits on anything other than 10.1 through 10.5, inclusive.  We never do
anything with greater resolution than major.minor, though, and I'm not really
sure it would ever be so helpful.

> >-            TARGET_ARCH_LIB=powerpc-apple-darwin`echo $NEXT_ROOT | $PERL -pe
's/MacOSX10\.([^\.]*)//;if ($1) {$_=$1+4;} else
{$_="'${target_os}'";s/(\d+)//;$_=$1;}'`
> >+            TARGET_ARCH_LIB=${target_cpu}-${target_vendor}-darwin`echo
$NEXT_ROOT | $PERL -pe 's/MacOSX10\.([\d]*)//;if ($1) {$_=$1+4;} else
{$_="'${target_os}'";s/(\d+)//;$_=$1;}'`
> 
> Is it necessary to use ${target_vendor}?

The value is always apple for now under Mac OS X, but we don't know what the
future holds.

I'll dequote -fPIC on this one, and wait on killing MACOS_DEPLOYMENT_TARGET
unless you feel strongly that it ought to be done here and now.

Comment 5

[Wan-Teh Chang]

Comment on attachment 187096 [details] [diff] [review]
SDK version detection, deployment target, and -fPIC

r=wtc.

Comment 6

[Mark Mentovai]

Comment on attachment 187096 [details] [diff] [review]
SDK version detection, deployment target, and -fPIC

Only change to the patch  here is that -fPIC won't be wrapped in quotes.

Comment 7

[Wan-Teh Chang]

Comment on attachment 187096 [details] [diff] [review]
SDK version detection, deployment target, and -fPIC

Mark, Chris,

Another suggested change is to move the export MACOSX_DEPLOYMENT_TARGET
statement to the inside of the previous if statement, because the
MACOS_VERSION_MAJOR and MACOS_VERSION_MINOR are only defined there.

>@@ -961,16 +962,18 @@
>         fi
>         if test -z "$MACOS_VERSION_MICRO"; then
>             MACOS_VERSION_MICRO=0
>         fi
>         MACOS_DEPLOYMENT_TARGET=`printf "%02d%02d%02d" "$MACOS_VERSION_MAJOR" "$MACOS_VERSION_MINOR" "$MACOS_VERSION_MICRO"`
>         AC_DEFINE_UNQUOTED(MACOS_DEPLOYMENT_TARGET, $MACOS_DEPLOYMENT_TARGET)
>     fi
> 
>+    export MACOSX_DEPLOYMENT_TARGET="$MACOS_VERSION_MAJOR.$MACOS_VERSION_MINOR"
>+

Comment 8

[Mark Mentovai]

Comment on attachment 187096 [details] [diff] [review]
SDK version detection, deployment target, and -fPIC

Tell ya what: let's not do the deployment target here at all, I'll get it in
bug 298543.  The export and AC_SUBST in configure.in and the change to
autoconf.mk.in are out.

Comment 9

[Wan-Teh Chang]

Comment on attachment 187096 [details] [diff] [review]
SDK version detection, deployment target, and -fPIC

Mark, I just checked in this patch on the NSPR trunk.
Are you saying that I should back it out?

Comment 10

[Mark Mentovai]

Yes, please, if MACOSX_DEPLOYMENT_TARGET is ever ".", as it will be on the
tinderboxes, the build will break.

Comment 11

[Mark Mentovai]

Oh, wait, you moved it into the "if" block.  That's safe.

Comment 12

[Wan-Teh Chang]

Attachment: Patch as checked into the NSPR trunk

Mark, I am confused.  Here is what I checked into the
NSPR trunk.  Is it okay, or should I back it out and
wait for your new patch?

Comment 13

[Mark Mentovai]

Comment on attachment 187560 [details] [diff] [review]
Patch as checked into the NSPR trunk

This is OK.  As long as the export MACOSX_DEPLOYMENT_TARGET is inside the
conditional block where the variables it depends on are defined (it is, and
nice catch), it's safe.

Comment 14

[Wan-Teh Chang]

Comment on attachment 187096 [details] [diff] [review]
SDK version detection, deployment target, and -fPIC

Aha, I see.  Thanks for explaining.  I actually just
thought it is better to put the definition and use of
a variable next to each other and didn't realize it
fixed a build failure :-)

Comment 15

[Wan-Teh Chang]

Mark, I am confused again.

The conditional block is
if test -f /System/Library/Frameworks/Carbon.framework/Carbon.
That test is intended to tell Mac OS X from barebone Darwin.

So, with your original patch, MACOSX_DEPLOYMENT_TARGET is "."
only if we are on a barebone Darwin system.  I believe all of
our tinderboxes are Mac OS X.  So how can MACOSX_DEPLOYMENT_TARGET
be "."?

Comment 16

[Mark Mentovai]

I spoke hastily - it actually wouldn't have been '.' on any of the tinderboxen.
 Sorry for the confusion.

Safe to close this one out?

Comment 17

[Wan-Teh Chang]

Mozilla trunk is using NSPRPUB_PRE_4_2_CLIENT_BRANCH, so
we need to get the patch into that branch.  Right now that
requires 1.8b3 approval.  Would you like to request approval
or can this wait until Mozilla 1.9?

Comment 18

[Mark Mentovai]

Comment on attachment 187560 [details] [diff] [review]
Patch as checked into the NSPR trunk

This (and others) will be needed to support x86 Macs.  Given the current
timeframes, I think 1.8b3 is appropriate.

Comment 19

[Benjamin Smedberg]

Comment on attachment 187560 [details] [diff] [review]
Patch as checked into the NSPR trunk

a+ for nspr client branches matching 1.8b3

Comment 20

[Wan-Teh Chang]

Patch checked in on the NSPRPUB_PRE_4_2_CLIENT_BRANCH
for mozilla1.8b3 and aviary1.1a2.