298543 - Always export MACOSX_DEPLOYMENT_TARGET

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mark Mentovai]

Following bug 298430, 10.1 ("Puma") is no longer supported.  The default
deployment target was changed to 10.2 in configure.in, but configure only
exports the value to MACOSX_DEPLOYMENT_TARGET in the environment when it's
specified explicitly.  That was fine when the default was 10.1, because that was
the default value in the absence of that variable anyway.

The default value should be 10.2 on PowerPC to match our new minimum and 10.4 on
Intel to match the minimum possible value for that architecture.

configure should export this variable:
 - when the user explicitly asks for --enable-macos-target
or
 - when configure implicitly selects a target and no MACOSX_DEPLOYMENT_TARGET
   variable exists in the environment

This will have the desired effect of preventing the app from launching on
systems that are too old (although someone should whip out a 10.1 system and
test it.)

Comment 1

[Mark Mentovai]

This would also be a good time to kill MACOS_DEPLOYMENT_TARGET (without the X).
 It's currently a preprocessor macro that corresponds to the value of
--enable-macos-target.  We can do better by using MAC_OS_X_VERSION_MIN_REQUIRED,
which the system gives us for free as part of the build environment.  It's set
based on the value of MACOSX_DEPLOYMENT_TARGET in the environment.  Also of
interest is MAC_OS_X_VERSION_MAX_ALLOWED, which corresponds to the selected SDK.

If we use this setup to the fullest extent possible, we'll be able to do some
neat release-dependent things with weak linking, but there will be some gotchas
now that Apple's changed some APIs.

lxr tells me that the only place outside of nspr that uses
MACOS_DEPLOYMENT_TARGET tests for target < 10.2, which isn't a factor now that
Jaguar is the minimum supported runtime.  Within nspr, which still supports
Puma, there should be no problem using the system's availability macros.

Comment 2

[Mark Mentovai]

Attachment: Export MACOSX_DEPLOYMENT_TARGET and kill MACOS_DEPLOYMENT_TARGET

This patch implements the logic in comment 0.  When no --enable-macos-target is
given, configure will use the preexisting MACOSX_DEPLOYMENT_TARGET variable, if
any, and fall back on a minimum value depending on the target architecture. 
For PowerPC, this is now 10.2.	For ia32, it is 10.4.

This patch also removes the custom MACOS_DEPLOYMENT_TARGET, as discussed in
comment 1.  Use MAC_OS_X_VERSION_MIN_REQUIRED as a substitute if and when
necessary.  See attachment 187099 [details] [diff] [review] to bug 298529 for an example.  The only
existing part of the code outside of NSPR that used MACOS_DEPLOYMENT_TARGET was
in js.	Because the test there was for a deployment target < 10.2, and the
minimum deployment target is now 10.2 on any platform, that conditional section
was removed.

Finally, this patch fixes the ia32 detector introduced in bug 297165.  The
patch there was initially against configure, but the square brackets present a
problem in m4 source like configure.in when the quote delimiters are [,]. 
Using x[3456]86* led to the configure script testing for x345686*.

An additional patch to NSPR will be required.

Comment 3

[Mark Mentovai]

Attachment: NSPR companion

This does for NSPR what the core patch does for the core.

This builds on the patch in bug 298531, and won't apply or work properly until
that patch is in.  That patch hasn't yet made it to
NSPRPUB_PRE_4_2_CLIENT_BRANCH, although it has approval to.

I will test this patch in a standalone build and verify that it still functions
with the 10.1 SDK.

Comment 4

[Wan-Teh Chang]

Comment on attachment 187718 [details] [diff] [review]
NSPR companion

> AC_ARG_ENABLE(macos-target,
>-             [  --enable-macos-target=VER (default=10.1)
>-                          Set the minimum MacOS version needed at runtime],
>-             [MACOS_DEPLOYMENT_TARGET_STR=$enableval],
>-             [MACOS_DEPLOYMENT_TARGET_STR=10.1])
>+             [  --enable-macos-target=VER (default=10.1/ppc, 10.4/x86)
>+                 Set the minimum MacOS version needed at runtime],
>+             [_MACOSX_DEPLOYMENT_TARGET_STR=$enableval])

I suggest we say "10.1 for ppc" or "10.1 on ppc" instead of
"10.1/ppc".

Just curious -- does the indentation of the "Set the minimum..."
line matter?

>+#ifdef XP_MACOSX
>+#if !defined(MAC_OS_X_VERSION_10_3) || \
>+ MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_3
> /*
>  * socket(AF_INET6) fails with EPROTONOSUPPORT on Mac OS X 10.1.
>  * IPv6 under OS X 10.2 and below is not complete (see bug 222031).
>  */
>-#if MACOS_DEPLOYMENT_TARGET < 100300

Could you explain why we need !defined(MAC_OS_X_VERSION_10_3)?

If MAC_OS_X_VERSION_10_3 is not defined, we blindly assume
the deployment target is < 10.3.  That doesn't seem right.

>-#if defined(MACOS_DEPLOYMENT_TARGET) && (MACOS_DEPLOYMENT_TARGET < 100200)
>+#if defined(XP_MACOSX) && (!defined(MAC_OS_X_VERSION_10_3) || \
>+                         MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_2)

!defined(MAC_OS_X_VERSION_10_3) should be
!defined(MAC_OS_X_VERSION_10_2), right?

>-        || (defined(MACOS_DEPLOYMENT_TARGET) \
>-        && MACOS_DEPLOYMENT_TARGET < 100200)))
>+        || (defined(XP_MACOSX) && (!defined(MAC_OS_X_VERSION_10_3) || \
>+        MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_3))))

Original code is DT < 10.2, new code is DT < 10.3.  This doesn't
seem right.

Comment 5

[Mark Mentovai]

Attachment: NSPR companion, v2

Sharp eye.  Adjustments made.

> Could you explain why we need !defined(MAC_OS_X_VERSION_10_3)?
> 
> If MAC_OS_X_VERSION_10_3 is not defined, we blindly assume
> the deployment target is < 10.3.  That doesn't seem right.

If MAC_OS_X_VERSION_10_3 is not defined, it's because it's not defined in the
SDK's headers.	Right now, the headers for the SDKs (10.1 - 10.4) I've checked
on both 10.3 and 10.4 have macros for 10.1 - 10.5.  But what if the perpetual
rumors of Apple's "beleagured" state are incorrect, and in the future, against
all odds, there's a 10.6 release?  The 10.6 SDK would surely have a macro for
it, but those still working in 10.4-land wouldn't.  So if MAC_OS_X_VERSION_10_6
is undefined, I think it's safe to assume that it's because the SDK is older
than 10.6.  Since SDK >= DT, it's also safe to assume that the DT is less than
10.6.  Although 10_3 is defined in all of the SDKs I checked, I can't examine
them all.  To set a better example for future conditionals, I thought it would
be a good idea to test for its being defined before using it and assuming older
if undefined.

Comment 6

[Wan-Teh Chang]

Comment on attachment 187725 [details] [diff] [review]
NSPR companion, v2

I'm still confused about the MAC_OS_X_VERSION_MIN_REQUIRED
and MAC_OS_X_VERSION_10_x macros.

I don't know which SDK I installed on my Mac.  NEXT_ROOT is
not set in my environment.  If I compile a simple C file
with "gcc foo.c", I found that MAC_OS_X_VERSION_10_1 and
MAC_OS_X_VERSION_10_3 are not defined.	Is this because
I am not using any SDK?  If so, this patch will make a SDK
a new requirement to compile NSPR.

Comment 7

[Mark Mentovai]

Attachment: NSPR companion, v3rd time's a charm

Those macros come in from AvailabilityMacros.h.  Accustomed to working in an
environment where, for example, Carbon/Carbon.h is already included, I omitted
including it.  That was sloppy.  I should have tested the #ifdefs.  Here, I
include it in md/_darwin.h if XP_MACOSX is defined.

If you don't specify an SDK, / will be treated as your SDK.  In other words,
you'll get the headers and libraries that shipped with your OS release (more
accurately, the toolcahin that belongs to your OS release.)

SDKs are not a requirement to build nspr.  This patch does not change nspr's
minimum requirements.

SDKs are a requirement to build Mozilla in certain situations, but that's
another story.

Comment 8

[Wan-Teh Chang]

Comment on attachment 187744 [details] [diff] [review]
NSPR companion, v3rd time's a charm

Mark, thanks for the explanation and the new patch.  This one
has an error.  The --enable-macos-target option sets the
_MACOSX_DEPLOYMENT_TARGET_STR variable.  It should set the
_MACOSX_DEPLOYMENT_TARGET variable instead.

For stylistic reason, would it be better to initialize
_MACOSX_DEPLOYMENT_TARGET to empty?

A nit (which I would just fix when I check in if this patch
were good): the continuation of a long line is usually indented
either 4 or 8 spaces to the right in NSPR code.

Comment 9

[Wan-Teh Chang]

Attachment: NSPR companion, v4

Comment 10

[Wan-Teh Chang]

Comment on attachment 187899 [details] [diff] [review]
NSPR companion, v4

I went ahead and fixed the error in Mark's patch, v3.

I've tested this patch.

I personally would replace

    (!defined(MAC_OS_X_VERSION_10_2) || \
	MAC_OS_X_VERSION_MIN_REQUIRED < MAC_OS_X_VERSION_10_2)

by

    MAC_OS_X_VERSION_MIN_REQUIRED < 1020

I find the latter much easier to understand even though it uses
hardcoded constant 1020.

As comment 5 shows, it takes at least three sentences to explain
why !defined(MAC_OS_X_VERSION_10_2) implies DT < 10.2.

But I am willing to be consistent with how this is coded in the
rest of Mozilla.

Comment 11

[Mark Mentovai]

(In reply to comment #10)
> 
>     MAC_OS_X_VERSION_MIN_REQUIRED < 1020
> 
> I find the latter much easier to understand even though it uses
> hardcoded constant 1020.

I think the link between 10.2 and 1020 is much more tenuous than
MAC_OS_X_VERSION_10_2.  I'd rather use the macro, for this reason only.  The
!defined nonsense seems intuitive to me, but I don't expect everyone to be
familiar with the intracacies of SDKs and their macros.  Ideally, the !defined
test and comparison would go off in some function with comments to explain it,
but cpp doesn't allow that kind of thing.

> As comment 5 shows, it takes at least three sentences to explain
> why !defined(MAC_OS_X_VERSION_10_2) implies DT < 10.2.

I was just being verbose!

Comment 12

[Mark Mentovai]

Comment on attachment 187899 [details] [diff] [review]
NSPR companion, v4

r+, we've got a winner

Comment 13

[Mark Mentovai]

(In reply to comment #8)
> For stylistic reason, would it be better to initialize
> _MACOSX_DEPLOYMENT_TARGET to empty?

Just a matter of style.  I never do that in raw sh, under the assumption that
I'm shaving off a handful of cycles.  I'd give r+ if you wanted to add a block
for when --enable-macos-target is missing and set the variable empty, though.

Comment 14

[Wan-Teh Chang]

Mark, did you notice that I added the initialization of
_MACOSX_DEPLOYMENT_TARGET to NSPR patch v4?  I did that
to be consistent with how the configure.in script handles
the other variables.

Comment 15

[Mark Mentovai]

Yes, that's fine.

Comment 16

[Josh Aas]

The first patch in the list will allow us to fail gracefully for Mac OS X 10.1
users. This will be the first release they can't use, so we should at least do
that for them. Nominating as blocker.

Comment 17

[Benjamin Smedberg]

Needs a release note for 1.8b3 (we should releasenote that we don't work with
mac10.1 anyway); need to fix for b4.

Comment 18

[Chase Phillips]

Comment on attachment 187596 [details] [diff] [review]
Export MACOSX_DEPLOYMENT_TARGET and kill MACOS_DEPLOYMENT_TARGET

We won't block the release on this bug but if reviews are received and the
patch is landed in time it might be able to make 1.1a2.

Comment 19

[Mark Mentovai]

We should also set LSMinimumSystemVersion = 10.2.0 in the various Info.plist
files.  That gives the friendliest message:

You cannot use the application "APP_NAME" with this version of Mac OS X.

Comment 20

[Mark Mentovai]

Attachment: LSMinimumSystemVersion = 10.2.0

This is supposed to mark the package as having a minimum system requirement of
10.2.0.  Unfortunately, neither this nor the adjustment to
MACOSX_DEPLOYMENT_TARGET is allowing for graceful failure on 10.1.0.

When attempting to launch DeerPark.app on 10.1.0, the system blindly forges
forward and fails due to undefined symbols without any UI feedback.  If you're
lucky, you get a quick dock bounce, but other than that, nothing.

If you really really really need UI feedback, I guess we could replace
firefox-bin with a stub that slaps up a dialog if the minima aren't met or
execs the real firefox-bin if they are.  My own opinion is that it's not worth
it.

The patches should still go in, but I don't really see any reason they should
block.	They can wait for the trunk after branching.

Comment 21

[Mark Mentovai]

Actually, if the 1.1/1.8 tree is to build on the Intel Macs out of the box, the
first patch to this bug really ought to be taken.  The NSPR version of this
patch is already in.

Comment 22

[Mark Mentovai]

Comment on attachment 187596 [details] [diff] [review]
Export MACOSX_DEPLOYMENT_TARGET and kill MACOS_DEPLOYMENT_TARGET

This was already approved for 18b3 but didn't make it.	Can we get it for 18b4?

Comment 23

[Josh Aas]

Checked in "Export MACOSX_DEPLOYMENT_TARGET and kill MACOS_DEPLOYMENT_TARGET"
for Mark since I'm doing Intel work today and I need that. Will leave bug open
for Mark to close out when he's ready.

Comment 24

[Mark Mentovai]

Thanks for the checkin and staying on tree patrol.

Closing this one out.  Note that the NSPR stuff isn't part of the client build
yet, but that should be OK.

I'm not going to push LSMinimumSystemVersion here because 10.1 doesn't care. 
We'll do it when we need it and it can help.  Open a new bug if you'd like to
handle the 10.1 situation more gracefully.