302212 - Mac OS X x86 builds can't target SDKs

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mark Mentovai]

uname -p is returning i386, but we need to refer to the system as i686 for the
purposes of arch-specific compiler dirs.  We need to work around in core, nspr,
and nss.  This causes build failures.  SDKless builds are unaffected.

Right now, an explicit SDK is required for x86 Camino builds because the
standard Camino mozconfig asks for the 10.2.8 SDK, which won't work on x86.  The
camino mozconfig should be updated to not use the 10.2.8 SDK on Intel Macs.

Comment 1

[Josh Aas]

Apple's inconsistent and non-standard uname behavior on x86 and PPC is radar bug
4193466.

Comment 2

[Mark Mentovai]

I'm strongly considering using -isysroot instead of the custom SDK location
logic when gcc 4 is the compiler.  I initially balked at -isysroot because it
wasn't really documented.  Apple still claims that it's "subject to change,"
although they are now recommending it for cross-targeting.  Xcode now uses
-isysroot also.

http://developer.apple.com/documentation/DeveloperTools/Conceptual/cross_development/Articles/cd_overview.html#//apple_ref/doc/uid/20002000-1114377-BABBACCD

Even though this is "subject to change" in the future, mapping all *86 `uname
-p` output to i686 is potentially worse.

This puts all of the SDK location intelligence into the compiler, where it
really belonged from the very beginning.

Comment 3

[Mark Mentovai]

Attachment: Core patch for -isysroot/-syslibroot, v1

Comment 4

[Mark Mentovai]

Attachment: NSPR patch for -isysroot/-syslibroot, v1

This patch is against the NSPR client branch, which is out of date compared to
the NSPR trunk.  Specifically, this patch includes the NSPR changes from bug
298543 which are already in the NSPR trunk.  This patch will apply cleanly to a
standard Mozilla client checkout.

This patch must land and be in the client branch before or at the same time as
the core patch.  The core patch eliminates NEXT_ROOT in cases where it is not
needed.  This patch makes NSPR use the --with-macos-sdk command-line configure
argument to locate the SDK.  This argument is passed to NSPR's configure in
current builds.

Comment 5

[Mark Mentovai]

Attachment: NSS patch for -isysroot/-syslibroot, v1

This patch must land and be in the NSS client branch at the same time as the
core patch.  The core patch eliminates NEXT_ROOT in cases where it is not
needed.  This patch makes NSPR use the MACOS_SDK_DIR environment variable to
locate the SDK.  This variable is passed to the NSS build by the Makefile in
security/manager when that Makefile is patched by the core patch here.

The SDK-locating trickery is still all present in NSS, so non-Mozilla users
need only set MACOS_SDK_DIR and have Darwin.mk figure out what to do.

Comment 6

[Mark Mentovai]

Attachment: Camino patch to select the proper target SDK, v1

Comment 7

[Mark Mentovai]

Comment on attachment 190717 [details] [diff] [review]
Camino patch to select the proper target SDK, v1

We agree it's probably better to just nuke the SDK from
mozilla/camino/config/mozconfig and set it on the tinderboxen directly instead.

Comment 8

[Mark Mentovai]

Attachment: Core patch for -isysroot/-syslibroot, v1, up to date

Changes to context in configure.in make the original core patch not apply. 
This is updated to the current trunk, but is otherwise identical.

Comment 9

[Wan-Teh Chang]

Comment on attachment 190715 [details] [diff] [review]
NSPR patch for -isysroot/-syslibroot, v1

r=wtc.	However, I'd like a new patch generated against
the tip of NSPR.  I will be responsible for getting the
previous change (removal of MACOS_DEPLOYMENT_TARGET) to
the NSPR client branch.  The reason is that your patch
doesn't have the fix I made on the NSPR tip
(removing the STR in _MACOSX_DEPLOYMENT_TARGET_STR).

I'm not qualified to really review this patch. Please
ask any two of Josh Aas, Simon Fraser, and Mike Pinkerton
to review the new patch.

Suggested changes:

>+dnl ========================================================
>+dnl = Mac OS X SDK support
>+dnl ========================================================
>+MACOS_SDK_DIR=
>+NEXT_ROOT=
>+AC_ARG_WITH(macos-sdk,
>+[  --with-macos-sdk=dir   Location of platform SDK to use (Mac OS X only)],
>+    MACOS_SDK_DIR=$withval)

Please move "MACOS_SDK_DIR=" and "NEXT_ROOT=" to the beginning
of this file, where we set many other variables to empty.

>-    if echo $OS_TEST | grep -c 86 2>/dev/null; then
>-        AC_DEFINE(i386)
>-        CPU_ARCH=i386
>-    else
>-        AC_DEFINE(ppc)
>-        CPU_ARCH=ppc
>-    fi
>+    case "${target_cpu}" in
>+        i*86*)
>+            AC_DEFINE(i386)
>+            CPU_ARCH=i386
>+            ;;
>+        *)
>+            AC_DEFINE(ppc)
>+            CPU_ARCH=ppc
>+            ;;
>+    esac

Is this change necessary?  It seems like just a rewrite.
Should we match "i*86" instead of "i*86*"?  Just wondering.

>+        dnl MACOS_SDK_DIR will be set to the SDK location whenever one is
>+        dnl in use.  NEXT_ROOT will be set and exported if it's needed for
>+        dnl ld.
>+        AC_SUBST(MACOS_SDK_DIR)
>+        AC_SUBST(NEXT_ROOT)
>+
>+        if test "$MACOS_SDK_DIR"; then

Please move "AC_SUBST(MACOS_SDK_DIR)" and "AC_SUBST(NEXT_ROOT)"
to the end of this file where we do AC_SUBST on many other variables.

Again, the new patch should be against the tip of NSPR.  Thanks a lot,
Mark.

Comment 10

[Wan-Teh Chang]

Comment on attachment 190716 [details] [diff] [review]
NSS patch for -isysroot/-syslibroot, v1

r=wtc.	This patch looks correct.

You can use = instead of := in most of the variable assignments.
In general := is only necessary when you have $(shell ...) on
the right hand side.  := evaluates the right hand side right
there, rather than when the variable is expanded.  So using
:= with $(shell ...) avoids evaluating the $(shell ...) function
repeatedly.

I will wait until the NSPR and Core patches have been reviewed
by the key Mac Mozilla developers to check this patch in.

Comment 11

[Mark Mentovai]

Attachment: NSPR patch for -isysroot/-syslibroot, v2

This addresses wtchang's comments and also fixes the FlatCarbon include path
used to compile prlink.c.  This patch was prepared against the nspr head, as
requested.  It does not apply against NSPRPUB_PRE_4_2_CLIENT_BRANCH, and
therefore, will not apply directly to trees checked out by client.mk.

Just to reiterate, this patch needs to be in the NSPR client branch before or
concurrent with the core patch landing.

Comment 12

[Mark Mentovai]

Attachment: NSS patch for -isysroot/-syslibroot, v2

I haven't tested this yet, but the only changes here are to switch := to = when
possible.  There are no recursive references in these assignments, so this
isn't a problem.  The patch was prepared against the NSS head this time. 
Although the NSS head's version is slightly newer than the client branch's, the
patch will still apply cleanly on the client branch.

Once again, this patch needs to land at the exact same time that the core patch
lands.

Comment 13

[Mark Mentovai]

> Is this change necessary?  It seems like just a rewrite.
> Should we match "i*86" instead of "i*86*"?  Just wondering.

It was done for a reason.  This:

if echo 86 | grep -c 86 2>/dev/null ; then

leaves stdout hooked up, so you get the output of grep -c.  Ug-lee.  It's
possible to fix it while leaving the grep, but I thought it'd be better to match
the logic up to how we do it elsewhere and eliminate a fork in the process.  I'm
using i*86* because the initial configure patch from Apple used it.  I don't
know whether they did it purposely because they expect to ship systems that call
themselves i686-something or because they copied the powerpc* template, but in
the interest of playing it safe, I continued the tradition.

Comment 14

[Wan-Teh Chang]

Comment on attachment 192616 [details] [diff] [review]
NSPR patch for -isysroot/-syslibroot, v2

r=wtc.

Mark: thanks a lot for creating this new patch.

I just brought the NSPRPUB_PRE_4_2_CLIENT_BRANCH up to
date, so any new NSPR patches can be created against the
NSPR client branch now.

I found some minor issues in mozilla/nsprpub/configure.in.

1. In this code fragment:

   # Add Mac OS X support for loading CFM & CFBundle plugins
   if test -f /System/Library/Frameworks/Carbon.framework/Carbon; then

Is that test still the best way to tell Mac OS X from a plain
Darwin system?

2. Your patch defines GCC_VERSION_MAJOR again.	You should remove
one of the definitions.

We probably should move the section from "changequote(,)" to
the first GCC_VERSION_MAJOR definition into the if test "$MACOS_SDK_DIR"
section.

3. I noticed a difference between the GCC < 4 and GCC >= 4.0 sections.
One defines OS_LIBS, but the other defines OS_LDFLAGS.	I am worried
that neither will be used by NSPR's build system.  The only Makefile
that actually uses this is mozilla/nsprpub/pr/src/Makefile.in.
Could you see whether the command line for linking libnspr4.dylib
is what you want?

4. If NSPR is pure Darwin code only, does MACOS_SDK_DIR/NEXT_ROOT
still matter?

Comment 15

[Wan-Teh Chang]

Comment on attachment 192616 [details] [diff] [review]
NSPR patch for -isysroot/-syslibroot, v2

Mark: in mozilla/nsprpub/pr/src/Makefile.in, this is the
relevant code fragment:

ifeq ($(OS_TARGET),MacOSX)
OS_LIBS 	= -framework CoreServices -framework CoreFoundation
endif

Two things to note:
1. The make variable we use is OS_LIBS.
2. The variable definition ignores anything you set in the
configure script.

Comment 16

[Wan-Teh Chang]

Comment on attachment 192617 [details] [diff] [review]
NSS patch for -isysroot/-syslibroot, v2

r=wtc.

Comment 17

[Wan-Teh Chang]

Comment on attachment 192616 [details] [diff] [review]
NSPR patch for -isysroot/-syslibroot, v2

Mark,

After looking at this patch again, I think it needs work.
The gcc >= 4.0 case should set OS_LIBS instead of OS_LDFLAGS,
and in mozilla/nsprpub/pr/src/Makefile.in, we should say
"OS_LIBS +=" instead of "OS_LIBS =" -framework CoreServices -framework
CoreFoundation.

Comment 18

[Simon Fraser [no longer active]]

Comment on attachment 192617 [details] [diff] [review]
NSS patch for -isysroot/-syslibroot, v2

rs=sfraser

Comment 19

[Wan-Teh Chang]

Comment on attachment 192617 [details] [diff] [review]
NSS patch for -isysroot/-syslibroot, v2

Mark,

Since this patch requires mozilla/security/manager/Makefile.in
to pass MACOS_SDK_DIR to the NSS coreconf build system, I can't
check in this patch before the Core patch.

Is there anything I can do to enable me to check this patch in
first?	I am wondering if I can add something like this:

ifdef NEXT_ROOT
MACOS_SDK_DIR = $(NEXT_ROOT)
endif

before the ifneq (,$(MACOS_SDK_DIR)) test.

This is just temporary and will be removed when the Core patch
is checked in.

Another possibility is to use NEXT_ROOT instead of MACOS_SDK_SDR.

Comment 20

[Mark Mentovai]

wtc, if you don't mind the extra overhead of fixing it up now and re-fixing it
again later, that's OK with me - but I also don't mind waiting until the core
patch is reviewed and the NSPR patch is refactored.  For a temporary fix-up in
NSS, I'd like for it to only redefine MACOS_SDK_DIR if it's not already defined,
so that once the core patch does land, the value will be taken from the proper
variable right off the bat and not be overridden by NEXT_ROOT.

(Which, of course, should be identical to MACOS_SDK_DIR anyway if it's defined,
but I think you see what I'm swinging at here.)

As for your remaining NSPR concerns, there was a compelling reason I had used
LIBS in one path and LDFLAGS in another in mozilla.  I need to go back and look
at my notes to see if it's also a problem in NSPR, and fix up accordingly if so.

Comment 21

[Wan-Teh Chang]

Mark: ok, I will wait.  Let's check in all three
patches at the same time.

Comment 22

[Mark Mentovai]

Attachment: NSPR patch for -isysroot/-syslibroot, v3

wtchang, I've implemented your suggestions from comment 17.  Here, I am using
both OS_LIBS and LDFLAGS (which is OS_LDFLAGS in makefiles) for NSPR. 
OS_LDFLAGS is used when building the small utilities like nsinstall and now;
since they're compiled with the SDK in the include path, they should be linked
with the SDK in the library path.  (Alternatively, they can be compiled and
linked without the SDK at all, which would eventually be better in a cross
environment.)

I also noticed that libraries in NSPR dependent on libnspr, such as libplds4,
were getting the system libraries and not the SDK during their final link. 
I've set RESOLVE_LINK_SYMBOLS=1 for Darwin to account for this.

Comment 23

[Mark Mentovai]

Comment on attachment 191279 [details] [diff] [review]
Core patch for -isysroot/-syslibroot, v1, up to date

This core patch still applies and still needs review.

Comment 24

[Mark Mentovai]

Attachment: NSPR patch for -isysroot/-syslibroot, v4

The configure.in diff in NSPR patch v3 included a piece of the patch from bug
296878.  This version is better.

Comment 25

[Mark Mentovai]

Attachment: NSPR patch for -isysroot/-syslibroot, v5

OK.  There's a very good reason to not use LIBS/OS_LIBS in NSPR: it causes the
SDK to appear in the output of nspr-config --libs.  This is fine when building
NSPR standalone, but it causes the SDK to be placed into NSPR_LIBS in the
Mozilla build.	Things that link against libnspr, such as js, then get two SDK
inclusions.  That's bad, the OS X libtool/ld can't handle -syslibroot specified
more than once.

So, here's another cut.  This time, I'm using LDFLAGS (for the tools) and
MKSHLIB (for the libraries).

Comment 26

[Mark Mentovai]

The story here is that if we want to support building on Intel Macs in the tree
(and I think we do), this patch needs to be taken.

Comment 27

[Scott MacGregor]

This patch would have a better chance of making the cut if the reviews were done
and it was already in the trunk. Every day that passes when that hasn't
happened, the less chance this has of making it :)

Comment 28

[Asa Dotzler [:asa]]

If you guys get all this together in the next day or two, we'll evaluate the
patches for inclusion in RC1. If not, we'll look to take these changes after the
release but before 1.5.1 (or whatever the next release is called.)

Comment 29

[Wan-Teh Chang]

Attachment: NSPR patch for -isysroot/-syslibroot, v6

Mark: sorry about the late review.  Your NSPR patch v5
is basically good.  I fixed three minor problems in it.
Please review and test it.

1. I use DSO_LDOPTS instead of MKSHLIB because DSO_LDOPTS
is more analogous to LDFLAGS (options and flags) than
MKSHLIB (a command) is.

2. Your patch v5 has a redundant GCC_VERSION_MAJOR assignment.
I also found that the code that defines GCC_VERSION_FULL, etc.
can be moved inside the if test "$MACOS_SDK_DIR" block.  So
I made this change.

3. The change to mozilla/nsprpub/pr/src/Makefile.in is no
longer necessary because patch v5 doesn't set OS_LIBS. So
I removed that change.

Comment 30

[Wan-Teh Chang]

Mark:

In NSPR patch v3 (comment 22), you added this:

    dnl RESOLVE_LINK_SYMBOLS causes libraries dependent on libnspr to
    dnl be linked against the selected SDK, if any.
    RESOLVE_LINK_SYMBOLS=1

This is removed in your NSPR patch v5.  Why?

LDFLAGS is used not only to link the small utlilties
(now and nsinstall, which are run on the host) but also
to link the test programs in mozilla/nsprpub/pr/tests
(which are run on the target).  So in a cross compilation,
now/nsinstall and the test programs need to be handled
differently.  We do that by using the INTERNAL_TOOLS
make variable in mozilla/nsprpub/config/{Makefile.in,rules.mk}.
We don't need to worry about cross compilation now.
Just wanted to respond to your comment about it in
comment 22.

Comment 31

[Wan-Teh Chang]

Mark: I understand why you removed RESOLVE_LINK_SYMBOLS=1
now (it's because you are no longer using OS_LIBS; see comment
25).

Comment 32

[Mark Mentovai]

Comment on attachment 199226 [details] [diff] [review]
NSPR patch for -isysroot/-syslibroot, v6

v6 works, and the diff-to-diff diff looks good.  I have two concerns:

Regarding your point (1), in the gcc < 4 branch, please prepend the library
search path to $DSO_LDOPTS as is done for $LDFLAGS instead of appending it as I
had done for $MKSHLIB.	The path should come "early" in this branch, I had only
placed it "late" in $MKSHLIB because prepending there was impossible.

Regarding your point (3), it seems to me that += is still better than =, but I
don't really care either way and this can be fixed if it's ever necessary in
the future.

Thanks for bearing with me through multiple revisions!

Comment 33

[Wan-Teh Chang]

Attachment: NSPR patch for -isysroot/-syslibroot, v6.1

Mark: I implemented your suggested change (prepending
instead of appending in the gcc < 4 branch).  I assume
that in the gcc >= 4 branch the position of
-Wl,-syslibroot,${MACOS_SDK_DIR} doesn't matter.

Regarding using = or += with OS_LIBS in
mozilla/nsprpub/pr/src/Makefile.in, I use =
with OS_LIBS in that makefile for all platforms.
That was intentional.  It's a (tedious) way to
make sure each NSPR shared library is only linked
with the system (OS) libraries they need.

Comment 34

[Mark Mentovai]

Comment on attachment 199321 [details] [diff] [review]
NSPR patch for -isysroot/-syslibroot, v6.1

Right, the order doesn't matter in the gcc >= 4 branch because you can only
have one syslibroot.

Comment 35

[Josh Aas]

These patches don't work on Intel Macs with GCC 4.0.1. Bad SDK and compiler
compat errors with the current patches.

Comment 36

[Mark Mentovai]

(In reply to comment #35)
> These patches don't work on Intel Macs with GCC 4.0.1. Bad SDK and compiler
> compat errors with the current patches.

Apple changed the gcc front end to pass -syslibroot to the linker when -isysroot
is used.  This results in double -syslibroot arguments.  ld can't handle that.

Comment 37

[Mark Mentovai]

These patches need to be adjusted to not pass -Wl,-syslibroot when gcc >= 4.0.1.

Comment 38

[Mark Mentovai]

Attachment: Core patch for -isysroot, v3, supports GCC 4.0.1

I have a new set of patches ready that understand GCC 4.0.1.  I'd like Josh to
test these out in an SDK build on an Intel.  These are identical to the
existing r+ set of patches, with slight changes in the GCC 4.0 case.

The change in the core patch:
only add -Wl,-syslibroot to LDFLAGS if gcc == 4.0.0.  -isysroot is always used
at link time, so no additional consideration is necessary for gcc > 4.0.0

Comment 39

[Mark Mentovai]

Attachment: NSPR patch for -isysroot, v7, supports GCC 4.0.1

The change in the NSPR patch:
add -Wl,-syslibroot to LDFLAGS and DSO_LDOPTS if gcc == 4.0.0, and add
-isysroot if gcc > 4.0.0.

Comment 40

[Mark Mentovai]

Attachment: NSS patch for -isysroot, v3, supports GCC 4.0.1

The change in nss:
add -Wl,-syslibroot to LDFAGS and DSO_LDOPTS if gcc == 4.0.0.  For gcc > 4.0.0,
only add -isysroot to DSO_LDOPTS, it's already present where LDFLAGS is useddue
to its inclusion in CFLAGS.

Comment 41

[Wan-Teh Chang]

Comment on attachment 200356 [details] [diff] [review]
NSS patch for -isysroot, v3, supports GCC 4.0.1

>cvs diff -u8 mozilla/security/coreconf/Darwin.mk
>
>Index: mozilla/security/coreconf/Darwin.mk
>===================================================================
>RCS file: /cvsroot/mozilla/security/coreconf/Darwin.mk,v
>retrieving revision 1.10.2.3
>diff -u -8 -r1.10.2.3 Darwin.mk
>--- mozilla/security/coreconf/Darwin.mk	25 Aug 2005 23:21:31 -0000	1.10.2.3
>+++ mozilla/security/coreconf/Darwin.mk	21 Oct 2005 16:09:32 -0000
>@@ -46,40 +46,45 @@
> ifeq (86,$(findstring 86,$(OS_TEST)))
> OS_REL_CFLAGS	= -Di386
> CPU_ARCH	= i386
> else
> OS_REL_CFLAGS	= -Dppc
> CPU_ARCH	= ppc
> endif
> 
>-ifneq (,$(NEXT_ROOT))
>+ifneq (,$(MACOS_SDK_DIR))
>     GCC_VERSION_FULL := $(shell $(CC) -v 2>&1 | grep "gcc version" | sed -e "s/^.*gcc version[  ]*//" | awk '{ print $$1 }')
>     GCC_VERSION_MAJOR := $(shell echo $(GCC_VERSION_FULL) | awk -F. '{ print $$1 }')
>     GCC_VERSION_MINOR := $(shell echo $(GCC_VERSION_FULL) | awk -F. '{ print $$2 }')
>-    GCC_VERSION := $(GCC_VERSION_MAJOR).$(GCC_VERSION_MINOR)
>-
>-    DARWIN_SDK_CFLAGS := -nostdinc
>+    GCC_VERSION = $(GCC_VERSION_MAJOR).$(GCC_VERSION_MINOR)
> 
>     ifeq (,$(filter-out 2 3,$(GCC_VERSION_MAJOR)))
>         # GCC <= 3
>-        DARWIN_TARGET_ARCH_LIB := darwin
>-        DARWIN_SDK_CFLAGS += -isystem $(NEXT_ROOT)/usr/include/gcc/darwin/$(GCC_VERSION)
>+        DARWIN_SDK_CFLAGS = -nostdinc -isystem $(MACOS_SDK_DIR)/usr/include/gcc/darwin/$(GCC_VERSION) -isystem $(MACOS_SDK_DIR)/usr/include -F$(MACOS_SDK_DIR)/System/Library/Frameworks
>+        ifneq (,$(shell find $(MACOS_SDK_DIR)/Library/Frameworks -maxdepth 0))
>+            DARWIN_SDK_CFLAGS += -F$(MACOS_SDK_DIR)/Library/Frameworks
>+        endif
>+        DARWIN_SDK_LDFLAGS = -L$(MACOS_SDK_DIR)/usr/lib/gcc/darwin -L$(MACOS_SDK_DIR)/usr/lib/gcc/darwin/$(GCC_VERSION_FULL) -L$(MACOS_SDK_DIR)/usr/lib
>+        NEXT_ROOT = $(MACOS_SDK_DIR)
>+        export NEXT_ROOT
>     else
>         # GCC >= 4
>-        CPU_ARCH_LONG := $(shell uname -p)
>-        DARWIN_TARGET_ARCH_LIB := $(CPU_ARCH_LONG)-apple-darwin$(shell echo $NEXT_ROOT | perl -pe 's/MacOSX10\.([\d]*)//;if ($$1) {$$_=$$1+4;} else {$$_="'${OS_RELEASE}'";s/(\d+)//;$$_=$$1;}')
>-        DARWIN_SDK_CFLAGS += -isystem $(NEXT_ROOT)/usr/lib/gcc/$(DARWIN_TARGET_ARCH_LIB)/$(GCC_VERSION_FULL)/include
>-    endif
>-
>-    DARWIN_SDK_CFLAGS += -isystem $(NEXT_ROOT)/usr/include -F$(NEXT_ROOT)/System/Library/Frameworks
>-    DARWIN_SDK_LDFLAGS := -L$(NEXT_ROOT)/usr/lib/gcc/$(DARWIN_TARGET_ARCH_LIB) -L$(NEXT_ROOT)/usr/lib/gcc/$(DARWIN_TARGET_ARCH_LIB)/$(GCC_VERSION_FULL) -L$(NEXT_ROOT)/usr/lib
>-
>-    ifneq (,$(shell find $(NEXT_ROOT)/Library/Frameworks -maxdepth 0))
>-        DARWIN_SDK_CFLAGS += -F$(NEXT_ROOT)/Library/Frameworks
>+        DARWIN_SDK_CFLAGS = -isysroot $(MACOS_SDK_DIR)
>+        ifneq (4.0.0,$(GCC_VERSION_FULL))
>+            # gcc > 4.0.0 passes -syslibroot to ld based on -isysroot.
>+            # Don't add -isysroot to DARWIN_SDK_LDFLAGS, because the programs
>+            # that are linked with those flags also get DARWIN_SDK_CFLAGS.
>+            DARWIN_SDK_DSOFLAGS = -isysroot $(MACOS_SDK_DIR)
>+        else
>+            # gcc 4.0.0 doesn't pass -syslibroot to ld, it needs to be
>+            # explicit.
>+            DARWIN_SDK_LDFLAGS = -Wl,-syslibroot,$(MACOS_SDK_DIR)
>+            DARWIN_SDK_DSOFLAGS = -Wl,-syslibroot,$(MACOS_SDK_DIR)
>+        endif
>     endif
> 
>     LDFLAGS += $(DARWIN_SDK_LDFLAGS)
> endif
> 
> # "Commons" are tentative definitions in a global scope, like this:
> #     int x;
> # The meaning of a common is ambiguous.  It may be a true definition:
>@@ -94,16 +99,16 @@
> 
> ifdef BUILD_OPT
> OPTIMIZER	= -O2
> endif
> 
> ARCH		= darwin
> 
> # May override this with -bundle to create a loadable module.
>-DSO_LDOPTS	= -dynamiclib -compatibility_version 1 -current_version 1 -install_name @executable_path/$(notdir $@) -headerpad_max_install_names $(DARWIN_SDK_LDFLAGS)
>+DSO_LDOPTS	= -dynamiclib -compatibility_version 1 -current_version 1 -install_name @executable_path/$(notdir $@) -headerpad_max_install_names $(DARWIN_SDK_DSOFLAGS)
> 
> MKSHLIB		= $(CC) -arch $(CPU_ARCH) $(DSO_LDOPTS)
> DLL_SUFFIX	= dylib
> PROCESS_MAP_FILE = grep -v ';+' $(LIBRARY_NAME).def | grep -v ';-' | \
>                 sed -e 's; DATA ;;' -e 's,;;,,' -e 's,;.*,,' -e 's,^,_,' > $@
> 
> G++INCLUDES	= -I/usr/include/g++

Comment 42

[Josh Aas]

Comment on attachment 200356 [details] [diff] [review]
NSS patch for -isysroot, v3, supports GCC 4.0.1

WFM - I was able to build Camino on Intel Mac OS X 8F1099 with this patch.

Comment 43

[Josh Aas]

Comment on attachment 200355 [details] [diff] [review]
NSPR patch for -isysroot, v7, supports GCC 4.0.1

WFM - I was able to build Camino on Intel Mac OS X 8F1099 with this patch.

Comment 44

[Josh Aas]

Comment on attachment 200354 [details] [diff] [review]
Core patch for -isysroot, v3, supports GCC 4.0.1

WFM - I was able to build Camino on Intel Mac OS X 8F1099 with this patch.

Comment 45

[Mark Mentovai]

Comment on attachment 200354 [details] [diff] [review]
Core patch for -isysroot, v3, supports GCC 4.0.1

bryner, this is slightly updated to catch some late-breaking changes from
Apple.

Comment 46

[Mark Mentovai]

wtchang, it looks like we're set to run with these on the trunk.  I'll leave it to you to check these in since, as discussed, they're interdependent.

Comment 47

[Mark Mentovai]

Comment on attachment 190717 [details] [diff] [review]
Camino patch to select the proper target SDK, v1

(Camino-only portion)

Deobsoleting, we'll go this way, and revisit if and when we're ready for cross builds.

Comment 48

[Mark Mentovai]

Camino portion checked in.  wtchang, the core, NSPR, and NSS portions are all yours.

Comment 49

[Wan-Teh Chang]

Comment on attachment 200354 [details] [diff] [review]
Core patch for -isysroot, v3, supports GCC 4.0.1

I checked in this Core patch on the trunk (1.9a).

Checking in configure.in;
/cvsroot/mozilla/configure.in,v  <--  configure.in
new revision: 1.1534; previous revision: 1.1533
done
Checking in config/autoconf.mk.in;
/cvsroot/mozilla/config/autoconf.mk.in,v  <--  autoconf.mk.in
new revision: 3.369; previous revision: 3.368
done
Checking in config/config.mk;
/cvsroot/mozilla/config/config.mk,v  <--  config.mk
new revision: 3.339; previous revision: 3.338
done
Checking in extensions/java/xpcom/tools/xpidl/Makefile.in;
/cvsroot/mozilla/extensions/java/xpcom/tools/xpidl/Makefile.in,v  <--  Makefile.
in
new revision: 1.5; previous revision: 1.4
done
Checking in security/manager/Makefile.in;
/cvsroot/mozilla/security/manager/Makefile.in,v  <--  Makefile.in
new revision: 1.58; previous revision: 1.57
done
Checking in xpcom/typelib/xpidl/Makefile.in;
/cvsroot/mozilla/xpcom/typelib/xpidl/Makefile.in,v  <--  Makefile.in
new revision: 1.39; previous revision: 1.38
done

Comment 50

[Wan-Teh Chang]

Comment on attachment 200355 [details] [diff] [review]
NSPR patch for -isysroot, v7, supports GCC 4.0.1

I checked in this NSPR patch on the NSPRPUB_PRE_4_2_CLIENT_BRANCH
(Gecko 1.9a).

Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.78.2.118; previous revision: 1.78.2.117
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.83.2.116; previous revision: 1.83.2.115
done
Checking in config/autoconf.mk.in;
/cvsroot/mozilla/nsprpub/config/autoconf.mk.in,v  <--  autoconf.mk.in
new revision: 1.25.2.10; previous revision: 1.25.2.9
done
Checking in pr/src/linking/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/src/linking/Makefile.in,v  <--  Makefile.in
new revision: 1.10.2.5; previous revision: 1.10.2.4
done

Comment 51

[Wan-Teh Chang]

Comment on attachment 200356 [details] [diff] [review]
NSS patch for -isysroot, v3, supports GCC 4.0.1

I checked this NSS patch into the NSS_CLIENT_TAG (Gecko 1.9a).

Comment 52

[Wan-Teh Chang]

Comment on attachment 200356 [details] [diff] [review]
NSS patch for -isysroot, v3, supports GCC 4.0.1

Mark:

I looked at this NSS patch closer, and saw something I
missed.

In the GCC <= 3 case, you only set DARWIN_SDK_LDFLAGS
and don't set DARWIN_SDK_DSOFLAGS, so nothing SDK related
is added to DSO_LDOPTS.  Is this correct?

Comment 53

[Wan-Teh Chang]

Comment on attachment 200355 [details] [diff] [review]
NSPR patch for -isysroot, v7, supports GCC 4.0.1

Mark:

I have a related question about this NSPR patch.

In the gcc > 4.0.0 case, you added -isysroot ${MACOS_SDK_DIR}
to both CFLAGS and LDFLAGS.  You had to avoid doing the
equivalent thing in NSS, so I wonder why this is OK in NSPR.
Is this because the NSPR makefile rule to build the 'now' and
'nsinstall' tools doesn't have both $(CFLAGS) and $(LDFLAGS)
on the command line?  The rule is reproduced here:

$(OBJDIR)/%$(PROG_SUFFIX): $(OBJDIR)/%.$(OBJ_SUFFIX)
        @$(MAKE_OBJDIR)
ifeq ($(MOZ_OS2_TOOLS),VACPP)
        $(LD) $(EXEFLAGS) $<
else
        $(CC) $(XCFLAGS) $< $(LDFLAGS) $(XLDOPTS) $(OUTOPTION)$@
endif

Comment 54

[Mark Mentovai]

(In reply to comment #52)
> (From update of attachment 200356 [details] [diff] [review] [edit])
> In the GCC <= 3 case, you only set DARWIN_SDK_LDFLAGS
> and don't set DARWIN_SDK_DSOFLAGS, so nothing SDK related
> is added to DSO_LDOPTS.  Is this correct?

No, it was an oversight.  Both LDFLAGS and DSOFLAGS should be set.

Comment 55

[Mark Mentovai]

(In reply to comment #53)
> (From update of attachment 200355 [details] [diff] [review] [edit])
> In the gcc > 4.0.0 case, you added -isysroot ${MACOS_SDK_DIR}
> to both CFLAGS and LDFLAGS.  You had to avoid doing the
> equivalent thing in NSS, so I wonder why this is OK in NSPR.
> Is this because the NSPR makefile rule to build the 'now' and
> 'nsinstall' tools doesn't have both $(CFLAGS) and $(LDFLAGS)
> on the command line?  The rule is reproduced here:

Precisely.  I wanted to avoid situations that would give multiple -isysroot arguments.  The compiler seems to accept them, but because the linker rejects multiple -syslibroot arguments, this strikes me as an area that Apple might eventually tighten up parameter checking.

Comment 56

[Wan-Teh Chang]

Attachment: NSS patch: set DARWIN_SDK_DSOFLAGS for GCC <= 3

Mark, please review and test this patch.  Thanks.

Comment 57

[Mark Mentovai]

Attachment: NSS patch: set DARWIN_SDK_DSOFLAGS for GCC <= 3, v2

wtchang, your last patch was good, but it highlighted another error in the GCC <= 3 branch for both NSS and NSPR.  The -F arguments, which specify the locations of frameworks, should be passed along to the linker.

Comment 58

[Mark Mentovai]

Attachment: NSPR patch: pass -F arguments to ld when gcc <= 3

Comment 59

[Mark Mentovai]

Comment on attachment 201031 [details] [diff] [review]
NSS patch: set DARWIN_SDK_DSOFLAGS for GCC <= 3, v2

Correct erroneous spacing here:

-        DARWIN_SDK_CFLAGS = -nostdinc -isystem $(MACOS_SDK_DIR)/usr/include/gcc/darwin/$(GCC_VERSION) -isystem $(MACOS_SDK_DIR)/usr/include -F$(MACOS_SDK_DIR)/System/Library/Frameworks
+	DARWIN_SDK_FRAMEWORKS = -F$(MACOS_SDK_DIR)/System/Library/Frameworks

I've tested both of these patches with gcc 3.3 builds.

Comment 60

[Wan-Teh Chang]

Comment on attachment 201031 [details] [diff] [review]
NSS patch: set DARWIN_SDK_DSOFLAGS for GCC <= 3, v2

Mark: why don't we need the -F arguments in
DARWIN_SDK_LDFLAGS?  DARWIN_SDK_LDFLAGS is also
passed to the linker (via LDFLAGS) when building programs.
Is this because NSS uses both $(CFLAGS) and $(LDFLAGS)
when building a program, and $(CFLAGS) already has
the -F arguments?

I am amazed that we can find a solution even though
it is fragile.

I will fix the erroneous spacing that you mentioned.

Comment 61

[Wan-Teh Chang]

Comment on attachment 201032 [details] [diff] [review]
NSPR patch: pass -F arguments to ld when gcc <= 3

We should use a variable, say MACOS_SDK_LIBS, to represent the linker flags
so that we don't need to repeat the long string of flags in LDFLAGS and
DSO_LDOPTS.

Don't you need to make the same change to mozilla/configure.in?

Comment 62

[Mark Mentovai]

(In reply to comment #60)
> (From update of attachment 201031 [details] [diff] [review] [edit])
> Mark: why don't we need the -F arguments in
> DARWIN_SDK_LDFLAGS?  DARWIN_SDK_LDFLAGS is also
> passed to the linker (via LDFLAGS) when building programs.
> Is this because NSS uses both $(CFLAGS) and $(LDFLAGS)
> when building a program, and $(CFLAGS) already has
> the -F arguments?

Yes, this is the same reason we don't set DARWIN_SDK_LDFLAGS to include an -isysroot argument for gcc > 4.0.0.  When LDFLAGS is used (to build programs), so is CFLAGS.

> I am amazed that we can find a solution even though
> it is fragile.

More by luck than anything else, we're not pushing the wrong buttons.  (Or the right buttons, depending on your perspective.)  SDK mismatches between headers and libraries are far more likely to run interference with C++ than with C, although the potential for trouble definitely exists, especially when you begin using some of the more exotic frameworks.  Since neither NSPR nor NSS use C++, and NSS at least doesn't rely on esoteric Mac-isms, we're able to avoid trouble there more easily.

(In reply to comment #61)
> We should use a variable, say MACOS_SDK_LIBS, to represent the linker flags
> so that we don't need to repeat the long string of flags in LDFLAGS and
> DSO_LDOPTS.

Like this?

-                LDFLAGS="-L${MACOS_SDK_DIR}/usr/lib/gcc/darwin -L${MACOS_SDK_DIR}/usr/lib/gcc/darwin/${GCC_VERSION_FULL} -L${MACOS_SDK_DIR}/usr/lib $LDFLAGS"
-                DSO_LDOPTS="-L${MACOS_SDK_DIR}/usr/lib/gcc/darwin -L${MACOS_SDK_DIR}/usr/lib/gcc/darwin/${GCC_VERSION_FULL} -L${MACOS_SDK_DIR}/usr/lib $DSO_LDOPTS"
+                MACOS_SDK_LIBS="-L${MACOS_SDK_DIR}/usr/lib/gcc/darwin -L${MACOS_SDK_DIR}/usr/lib/gcc/darwin/${GCC_VERSION_FULL} -L${MACOS_SDK_DIR}/usr/lib ${SDK_C_FRAMEWORK}"
+                LDFLAGS="${MACOS_SDK_LIBS} $LDFLAGS"
+                DSO_LDOPTS="${MACOS_SDK_LIBS} $DSO_LDOPTS"

That's good with me.

> Don't you need to make the same change to mozilla/configure.in?

Nope.  The Mozilla core build passes CFLAGS or CXXFLAGS to the linker.  To be sure, I grepped a recent 3.3 build log and found that the only compiler (linker) invocations to use -L but not -F were in NSPR and NSS.

I think that should just about wrap it up.  We shouldn't need to tinker with this again until we're ready to do cross-architecture Mac builds.

(Or until Apple releases another compiler update.)

Comment 63

[Wan-Teh Chang]

Attachment: NSPR patch: pass -F arguments to ld when gcc <= 3, v2

Mark, yes, that's exactly what I mean.  Here it is in
the form of a patch.  I've checked this patch and your
NSS patch in on tag/branch used by the Mozilla trunk.
So you can mark this bug fixed now.

Comment 64

[Mark Mentovai]

Thanks to everyone on this, especially Wan-Teh.

We'll probably need to revisit this at some future point on the 1.8 branch in order to do x86 Camino builds without additional patches.

Comment 65

[Mark Mentovai]

Nominating for 1.8.1 to properly support Mac OS X x86 when that's shipping.  This has been on the trunk for a couple of weeks.

Comment 66

[Mark Mentovai]

Moving my stability bugs to blocking1.8.0.1?

Comment 67

[Mark Mentovai]

Comment on attachment 200354 [details] [diff] [review]
Core patch for -isysroot, v3, supports GCC 4.0.1

Requesting approval1.8.0.1, these patches are needed for proper x86 Mac support.  These patches most likely won't apply directly due to trunk-branch context changes.  I can prepare a new set if preferred.

Comment 68

[[e-mail]]

Attachment: 302212.core.4.patch

(In reply to comment #38)
> Created an attachment (id=200354) [edit]
> Core patch for -isysroot, v3, supports GCC 4.0.1

I took Mark's patch and updated it to apply cleanly on the Camino 1.0b2 source code. It failed in "mozilla/xpcom/typelib/xpidl/Makefile.in" and "mozilla/extensions/java/xpcom/tools/xpidl/Makefile.in"
After this I ran 'autoconf' in the mozilla root directory.

Then I successfully compiled Camino 1.0b2 with GCC 4.0.1 and the 10.4u SDK.

Comment 69

[Mark Mentovai]

(In reply to comment #68)
> I took Mark's patch and updated it to apply cleanly on the Camino 1.0b2 source
> code. It failed in "mozilla/xpcom/typelib/xpidl/Makefile.in" and
> "mozilla/extensions/java/xpcom/tools/xpidl/Makefile.in"

I'm curious about this.  I just put my money where my mouth is and tried applying these patches to the 1.8.0 branch, and they apply cleanly without fuzz.  For the two files you mentioned, there's not even an offset.

Diffing my core.3 patch and your regenerated version, I can see that they're completely identical, with the exception of modification times, version numbers, and line number offsets.  There's zero movement in context and deltas.

I don't see why one patch would fail to apply completely and the other would succeed.

Comment 70

[[e-mail]]

(In reply to comment #69)
Mark. I got FAILED on those two parts when I applied your original patch, which I was prepared for. So I did not look any further into the cause. I just manually edited the files.
But after you replied I tested again and this time it applies fine. Go figure!

At least we know the patch applies and seems to fix the GCC 4.0.1 and SDK problems on the branch.

Comment 71

[Mark Mentovai]

Comment on attachment 207284 [details] [diff] [review]
302212.core.4.patch

Identical to core.3, attachment 200354 [details] [diff] [review].

Comment 72

[Benjamin Smedberg]

Comment on attachment 201070 [details] [diff] [review]
NSPR patch: pass -F arguments to ld when gcc <= 3, v2

Please mark "fixed1.8.1" and "fixed1.8.0.1" when this is completely landed on the appropriate branches.

Comment 73

[Mark Mentovai]

As before, these patches all need to be checked in simultaneously by someone with access to NSPR and NSS, for the versions used by the 1_8_0 and 1_8 branches: core v3, NSPR v7, NSS v3, NSS DARWIN_SDK_DSOFLAGS, NSPR -F.  At last check, these patches all applied cleanly, some with minor offset corrections.

Comment 74

[Wan-Teh Chang]

Mark, the NSPR and NSS used by MOZILLA_1_8_0_BRANCH and
MOZILLA_1_8_BRANCH are on the same CVS branches, and are
not in Restricted CVS commit access mode.  So you can
check in all of the patches and you just need to do that
on the MOZILLA_1_8_0_BRANCH and MOZILLA_1_8_BRANCH.

Comment 75

[Mark Mentovai]

Excellent.  Thanks.

Comment 76

[Mark Mentovai]

All portions have been checked in on MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH.