5.54 KB, patch
Josh Aas: review+
Benjamin Smedberg: approval1.8b4+
|Details | Diff | Splinter Review|
5.91 KB, patch
Mark Mentovai: review+
|Details | Diff | Splinter Review|
4.07 KB, patch
Josh Aas: review+
|Details | Diff | Splinter Review|
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.)
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.
Created attachment 187596 [details] [diff] [review] 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 x86* led to the configure script testing for x345686*. An additional patch to NSPR will be required.
Created attachment 187718 [details] [diff] [review] 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 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.
Created attachment 187725 [details] [diff] [review] 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 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.
Created attachment 187744 [details] [diff] [review] 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 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.
Created attachment 187899 [details] [diff] [review] NSPR companion, v4
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.
(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 on attachment 187899 [details] [diff] [review] NSPR companion, v4 r+, we've got a winner
(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.
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.
Yes, that's fine.
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.
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 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.
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.
Created attachment 189452 [details] [diff] [review] 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.
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 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?
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.
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.