Closed Bug 322578 Opened 19 years ago Closed 18 years ago

Support ppc<->x86 cross builds for Mac OS X

Categories

(Firefox Build System :: General, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mark, Assigned: mark)

References

Details

(Keywords: fixed1.8.0.2, fixed1.8.0.5, fixed1.8.1, Whiteboard: [camino-1.0])

Attachments

(10 files, 17 obsolete files)

2.40 KB, application/x-gzip
Details
7.01 KB, patch
jaas
: review+
mikepinkerton
: superreview+
Details | Diff | Splinter Review
383 bytes, text/plain
Details
26.22 KB, patch
Details | Diff | Splinter Review
12.93 KB, patch
Details | Diff | Splinter Review
2.25 KB, patch
mark
: review+
Details | Diff | Splinter Review
8.37 KB, patch
Details | Diff | Splinter Review
11.72 KB, patch
Details | Diff | Splinter Review
3.38 KB, patch
mark
: review+
Details | Diff | Splinter Review
1.28 KB, patch
mark
: review+
benjamin
: superreview+
Details | Diff | Splinter Review
We should be able to build for Mac OS X x86 and ppc targets using either architecture as the build host.  This is a step toward being able to produce universal binaries from a single tree.  (Before anyone throws a fit about that, yes, I'm aware of all of the issues.)
Assignee: nobody → mark
Attached patch v1 (obsolete) — Splinter Review
This allows ppc->x86 cross compiles.  In order to use it, the following needs to be added to the .mozconfig:

export CC=i686-apple-darwin8-gcc-4.0.1
export CXX=i686-apple-darwin8-g++-4.0.1
export CROSS_COMPILE=1
ac_add_options --target=i686-apple-darwin8.0.0
export RANLIB=ranlib AR=ar AS=gcc LD=ld STRIP="strip -x -S"

Replace i686 with powerpc.  Configured this way, it is possible to test a cross build where the host and target are both the same architecture.  The key is making sure that the target's identifier doesn't match the host's exactly.  The difference between darwin8.3.0 vs. darwin8.0.0 is sufficient for this purpose and is otherwise inconsequential.

The following file will be removed from the tree:
mozilla/config/asdecode.cpp

asdecode will no longer be used.  Instead of AppleSingle-encoded files, resources will be present in the tree within data forks.  This requires new versions of the following files:

mozilla/modules/plugin/samples/default/mac/NullPlugin.rsrc
mozilla/plugin/oji/MRJCarbon/plugin/Resources/Dialogs.rsrc
mozilla/plugin/oji/MRJCarbon/plugin/Resources/Strings.rsrc
mozilla/plugin/oji/MRJCarbon/plugin/Resources/Version.rsrc (can be removed)

We don't build the MRJCarbon stuff, the changes I made there are untested.  Removing asdecode fixes the remaining portion of bug 179863, and allows the whole tree to build on UFS and other forkless filesystems.

In order to do a ppc->x86 cross build, one more patch is needed, otherwise the target-compiler-works checks will fail because the SDK has not yet been set up and the target compiler by default uses the libraries in the root SDK, which are not currently fat on ppc.  This patch is sufficient to get over that hurdle.  I don't intend to check anything like this in:

Index: mozilla/configure.in
--- mozilla/configure.in        5 Jan 2006 18:07:04 -0000       1.1581
+++ mozilla/configure.in        6 Jan 2006 17:09:49 -0000
@@ -269,8 +269,14 @@
     AC_CHECK_PROGS(CC, $CC "${target_alias}-gcc" "${target}-gcc", :)
     unset ac_cv_prog_CC
+_TMP_CFLAGS=$CFLAGS
+CFLAGS="-isysroot /Developer/SDKs/MacOSX10.4u.sdk $CFLAGS"
     AC_PROG_CC
+CFLAGS="$_TMP_CFLAGS"
     AC_CHECK_PROGS(CXX, $CXX "${target_alias}-g++" "${target}-g++", :)
     unset ac_cv_prog_CXX
+_TMP_CXXFLAGS=$CXXFLAGS
+CXXFLAGS="-isysroot /Developer/SDKs/MacOSX10.4u.sdk $CXXFLAGS"
     AC_PROG_CXX
+CXXFLAGS="$_TMP_CXXFLAGS"
     AC_CHECK_PROGS(RANLIB, $RANLIB "${target_alias}-ranlib" "${target}-ranlib", :)
     AC_CHECK_PROGS(AR, $AR "${target_alias}-ar" "${target}-ar", :)
Index: mozilla/nsprpub/configure.in
--- mozilla/nsprpub/configure.in        24 Dec 2005 15:21:05 -0000      1.83.2.124
+++ mozilla/nsprpub/configure.in        6 Jan 2006 17:11:35 -0000
@@ -415,8 +415,14 @@
     AC_CHECK_PROGS(CC, $CC "${target_alias}-gcc" "${target}-gcc", echo)
     unset ac_cv_prog_CC
+_TMP_CFLAGS=$CFLAGS
+CFLAGS="-isysroot /Developer/SDKs/MacOSX10.4u.sdk $CFLAGS"
     AC_PROG_CC
+CFLAGS="$_TMP_CFLAGS"
     AC_CHECK_PROGS(CXX, $CXX "${target_alias}-g++" "${target}-g++", echo)
     unset ac_cv_prog_CXX
+_TMP_CXXFLAGS=$CXXFLAGS
+CXXFLAGS="-isysroot /Developer/SDKs/MacOSX10.4u.sdk $CXXFLAGS"
     AC_PROG_CXX
+CXXFLAGS="$_TMP_CXXFLAGS"
     AC_CHECK_PROGS(RANLIB, $RANLIB "${target_alias}-ranlib" "${target}-ranlib", echo)
     AC_CHECK_PROGS(AR, $AR "${target_alias}-ar" "${target}-ar", echo)

To build fully, you will need to have a fat libIDL.  Alternatively, when the build fails for lack of a fat libIDL, touch $OBJDIR/xpcom/typelib/xpidl/xpidl to skip building a real target xpidl.  This is not a problem, because you will have built host_xpidl which is used during the build process, and the target xpidl is not packaged anyway.
Comment on attachment 207732 [details] [diff] [review]
v1

>Index: mozilla/security/manager/Makefile.in
>+ifdef $(TARGET_CPU_ARCH)
>+DEFAULT_GMAKE_FLAGS += CPU_ARCH="$(TARGET_CPU_ARCH)"
>+else
>+DEFAULT_GMAKE_FLAGS += CPU_ARCH="$(TARGET_CPU)"
>+endif # TARGET_CPU_ARCH

Should be:

+ifdef TARGET_CPU_ARCH

This affects (and was intended to handle) cross builds -> ppc, where $(TARGET_CPU_ARCH) and $(TARGET_CPU) differ: ppc and powerpc, respectively.
Are you aware of the issue that the NSS build runs some code made during the build (shlibsign) as part of its own build ? Because of this, cross compiling would require that you have a build targetted for the local architecture first - and fix the NSS coreconf build system to use it.
Julien:
We ran into the shlibgen issue in the general cross-compiling case.  Since we do not build native versions of NSPR & DBM, we avoid running shlibgen when CROSS_COMPILE=1 is set.  Afaik, this behavior isn't going to change with the NSS 3.11 release.

Mark:
Since I'm commenting, what's the difference between TARGET_CPU_ARCH & CPU_ARCH? IIRC, almost all of the variables are assumed to be for the target system unless prefixed by HOST_.  In security/manager/Makefile.in, it seems like we should standardize on using either CPU_ARCH or TARGET_CPU and not have a special case.

I recommend splitting the resource changes into a separate patch.
(In reply to comment #4)
> Since I'm commenting, what's the difference between TARGET_CPU_ARCH & CPU_ARCH?

I'm only using CPU_ARCH inside NSS, because that's the name of the existing variable.  It is the same as TARGET_CPU_ARCH outside of NSS.

The valid values for TARGET_CPU_ARCH are ppc and i386.  TARGET_CPU_ARCH is intended to be used as the argument for Apple ld's -arch parameter and other similar uses.  CPU_ARCH (in the toplevel configure.in, set based on OS_TEST) can't be used because its values are ppc or x86.  TARGET_CPU (from the system triplet) can't be used because its values are powerpc and (nominally) i686.

CPU_ARCH (outside NSS) is only used to set TARGET_XPCOM_ABI.

I named it TARGET_CPU_ARCH because CPU_ARCH was already taken.  I'm open to suggestions for different names.

> IIRC, almost all of the variables are assumed to be for the target system
> unless prefixed by HOST_.  In security/manager/Makefile.in, it seems like we
> should standardize on using either CPU_ARCH or TARGET_CPU and not have a
> special case.

TARGET_CPU represents uname -p, so I can't make it be ppc/i386 instead of powerpc/i686.  At the same time, uname -p values are incorrect for what NSS needs in its CPU_ARCH spec.  NSS really wants ppc/i386.  I presume other cross users are satisfied with the values they're presently getting.
These are the asdecoded .rsrc files that replace the ones currently in the tree, in conjunction with the attached patch that removes asdecode.
This removes all dependencies on asdecode.  A cvs remove of asdecode.cpp will also be needed.

Attachment 208770 [details] contains replacement decoded resource-in-data-fork rsrc files which must be used with this patch.

I've only tested this on Tiger, it should at least be tested on 10.3.9/1.5.  (Volunteers?)

asdecode was only used in the sample DefaultPlugin.  The MRJCarbon plugin is not built, but the external JEP build does use that directory.  That's why I've cc'd smichaud and decoded those resources.
Attachment #209493 - Flags: superreview?(mikepinkerton)
Attachment #209493 - Flags: review?(joshmoz)
Attachment #207732 - Attachment is obsolete: true
Attachment #209497 - Flags: superreview?
Attachment #209497 - Flags: review?(cls)
Attachment #209497 - Flags: superreview? → superreview?(bryner)
Attachment #209498 - Flags: superreview?(cls)
Attachment #209498 - Flags: review?(wtchang)
Attachment #209499 - Flags: review?(wtchang)
Along with these patches, this .mozconfig will build for x86.  To build for ppc, change i386 to ppc and i686 to powerpc.
Attached patch Branch version (reference) (obsolete) — Splinter Review
This is a consolidation of the core, NSPR, and NSS patches.  It was generated against the 1.8.0 branch, where context changes prevent a clean application of the above patches, generated against the trunk.  It does not include the asdecode patch.
Comment on attachment 209497 [details] [diff] [review]
Core: support ppc<->x86 cross builds

Of special note: this patch does not build xpidl for the target host, only host_xpidl for the build host.  This is not intended to be a permanent solution.  Fat libraries (libIDL and glib) are needed to build xpidl for the target host.  glib won't build fat out of the box at the moment.  The easiest way out here might be to provide a tarball with fat prerequisites, since fat support in Fink and DarwinPorts doesn't even seem to be on the horizon.

This isn't expected to be a problem for the time being, because (target) xpidl is not currently packaged.
Comment on attachment 209498 [details] [diff] [review]
NSPR: support ppc<->x86 cross builds

+                    dnl NEXT_ROOT will be in the environment, but it
+                    dnl shouldn't be set for the build host.  NSPR doesn't
+                    dnl use either of these variables presently, but since
+                    dnl they're being set, they should be set to something
+                    dnl that works.

Now, why did I write that?  It's not true.  NSPR uses $HOST_CC (but not $HOST_CXX).
Attachment #209493 - Flags: review?(joshmoz) → review+
Blocks: 324651
Comment on attachment 209493 [details] [diff] [review]
Remove asdecode (checked in on trunk)

sr=pink
Attachment #209493 - Flags: superreview?(mikepinkerton) → superreview+
Attachment #209493 - Attachment description: Remove asdecode → Remove asdecode (checked in on trunk)
Attachment #208770 - Attachment description: Replacement .rsrc files → Replacement .rsrc files (checked in on trunk)
Comment on attachment 209493 [details] [diff] [review]
Remove asdecode (checked in on trunk)

I checked in "remove asdecode" and "replacement .rsrc files" on the trunk.  I will remove asdecode.cpp shortly.
Attachment #209493 - Flags: approval1.8.0.2?
blocking1.8.0.2, as this is a major part of the universal binary strategy for Macs, and 1.8.0.2 will be the first universal release as far as I'm aware.
Flags: blocking1.8.0.2?
Comment on attachment 209493 [details] [diff] [review]
Remove asdecode (checked in on trunk)

I had to back this out because it busted the SeaMonkey trunk tinderboxen.  They're still running Jaguar.  (Why?)

I don't have any Jaguar systems suitable for development any more, and I don't think any of the other Mac developers do either.
I am perfectly happy to check this in and leave Jaguar-based tinderboxen broken until they can be upgraded (bug 299214, which blocks a lot of other stuff anyway). We absolutely need to get this stuff tested on trunk Firefox ASAP.
(In reply to comment #19)
> I am perfectly happy to check this in and leave Jaguar-based tinderboxen broken
> until they can be upgraded (bug 299214, which blocks a lot of other stuff
> anyway).

Ditto.

> We absolutely need to get this stuff tested on trunk Firefox ASAP.

I have a Tiger Tinderbox at my desk.  I anticipate it can replace at least one or two of the Jaguar boxes and we can shuffle the rest of the applicable builds over to one of our Panther systems.
Someone's bound to complain about the loss of perf data from monkey, but it sure as hell ain't gonna be me.  The sooner we can get off Jaguar, the better.
Depends on: 299214
This includes one new hunk:

 if test -n "$CROSS_COMPILE"; then
      if test -z "$HOST_LIBIDL_CONFIG"; then
         HOST_LIBIDL_CONFIG="$LIBIDL_CONFIG"
     fi
-    if test -n "$HOST_LIBIDL_CONFIG"; then
+    if test -n "$HOST_LIBIDL_CONFIG" && test "$HOST_LIBIDL_CONFIG" != "no"; then
         HOST_LIBIDL_CFLAGS=`${HOST_LIBIDL_CONFIG} --cflags`
         HOST_LIBIDL_LIBS=`${HOST_LIBIDL_CONFIG} --libs`
     else
         HOST_LIBIDL_CFLAGS="$LIBIDL_CFLAGS"
         HOST_LIBIDL_LIBS="$LIBIDL_LIBS"
     fi
 fi

$LIBIDL_CONFIG is set to "no" on systems that don't have libidl-config installed, such as when libIDL-2 is installed and located via pkg-config.  Without this change, you'll see "no: command not found" at configure time, and xpidl will not compile because $HOST_LIBIDL_CFLAGS and $HOST_LIBIDL_LIBS will not be set.
Attachment #209497 - Attachment is obsolete: true
Attachment #209997 - Flags: superreview?(bryner)
Attachment #209997 - Flags: review?(cls)
Attachment #209497 - Flags: superreview?(bryner)
Attachment #209497 - Flags: review?(cls)
The only change is an updated comment (see comment 14).
Attachment #209498 - Attachment is obsolete: true
Attachment #209998 - Flags: superreview?(cls)
Attachment #209998 - Flags: review?(wtchang)
Attachment #209498 - Flags: superreview?(cls)
Attachment #209498 - Flags: review?(wtchang)
Attached patch Branch version (reference, v2) (obsolete) — Splinter Review
Includes updates from comment 22 and comment 23.
Attachment #209503 - Attachment is obsolete: true
Attachment #209997 - Flags: superreview?(bryner) → superreview+
Blocks: 323355
Only one change was needed to make these cross-build patches suitable for the universal build:

nsExtensionManager.js[.in] uses $OS_TARGET, which is set to `uname -s` = "Darwin" in a native build, but to $target_os = "darwin8.0.0" in a cross build.  This was causing the diff check to fail for this file when merging (bug 324855) the ppc and x86 bits.  See also bug 323330.

-+        darwin*)      OS_ARCH=Darwin ;;
++        darwin*)      OS_ARCH=Darwin OS_TARGET=Darwin ;;

No consumers of OS_TARGET were harmed in the production of this change.
Attachment #209997 - Attachment is obsolete: true
Attachment #210116 - Flags: review?(cls)
Attachment #209997 - Flags: review?(cls)
Attachment #209999 - Attachment is obsolete: true
Comment on attachment 210116 [details] [diff] [review]
Core: support ppc<->x86 cross builds (v3)

Oh, looks like Gavin took care of OS_TARGET in bug 323330.  Good.  I still think OS_TARGET should be set to Darwin in configure.in.
Branch v3, asdecode removal, and replacement .rsrc files have landed on CAMINO_1_0_BRANCH.  Here comes the first in-production test.
Whiteboard: [camino-1.0]
Flags: blocking1.8.0.2? → blocking1.8.0.2+
Attachment #209493 - Flags: branch-1.8.1?(joshmoz)
Attachment #209493 - Flags: branch-1.8.1?(joshmoz) → branch-1.8.1+
This needs review urgently, it's critical to shipping 1.5.0.2 for Mac as a universal binary.
Comment on attachment 210116 [details] [diff] [review]
Core: support ppc<->x86 cross builds (v3)

>Index: mozilla/xpcom/typelib/xpidl/Makefile.in

>+ifneq (($origin _STRIP_SDK)_($origin MACOS_SDK_DIR),undefined_undefined)

Let's avoid $(origin) and do ifneq (,$(_STRIP_SDK)$(MACOS_SDK_DIR)) here.

With that, r=bsmedberg to go ahead and get this landed... I'll leave r?cls because he knows cross-compile system better than I.
Attachment #210116 - Flags: review+
Comment on attachment 209499 [details] [diff] [review]
NSS: support ppc<->x86 cross builds

Mark,

Your NSS patch can be summarized as follows.

1. Handle CPU_ARCH in a cross compilation.

This is the only change in this patch that is
related to cross compilations.  Although I think
this change is fine, a more robust way to do this
would be the following (please add a comment):

ifndef CPU_ARCH
ifeq (86,$(findstring 86,$(OS_TEST)))
CPU_ARCH        = i386
else
CPU_ARCH        = ppc
endif
endif

OS_REL_CFLAGS   = -D$(CPU_ARCH)

The reason I think this might be more robust is
that it leaves the variable OS_TEST unchanged.

2. Rename DARWIN_SDK_DSOFLAGS to DARWIN_SDK_SHLIBFLAGS.

This change is not necessary.  I guess you renamed
this variable because of the next change.

3. Move $(DARWIN_SDK_DSOFLAGS) from DSO_LDOPTS to
MKSHLIB.

For NSS, this change is a no-op.  I guess this change
must be necessary for Mozilla, and you're trying to
make the three build systems (Mozilla, NSPR, and NSS)
as consistent as possible.

Please let me know if it's okay to do only #1 or
only #1 and #3.
Comment on attachment 209998 [details] [diff] [review]
NSPR: support ppc<->x86 cross builds (v2)

Mark,

I just reviewed your NSPR patch v2.

1. In mozilla/nsprpub/config/rules.mk, the proper
fix would be to use HOST_LDFLAGS instead.

I found that only Mac OS X, Windows (with MSVC as
opposed to GCC/MinGW), and OS/2 are setting the
LDFLAGS variable in mozilla/nsprpub/configure.in.
For Windows (with MSVC) and OS/2, CROSS_COMPILE
isn't an issue.  So setting LDFLAGS=$(HOST_LDFLAGS)
in mozilla/nsprpub/config/rules.mk would only
require testing on Mac OS X.  This would require
setting HOST_LDFLAGS for Mac OS X and adding
  AC_SUBST(HOST_LDFLAGS)
to mozilla/nsprpub/configure.in, and
adding
  HOST_LDFLAGS   = @HOST_LDFLAGS@
to mozilla/nsprpub/config/autoconf.mk.in.

2. In mozilla/nsprpub/configure.in, can you use
_SAVE_CFLAGS and _SAVE_CXXFLAGS instead of
_TMP_CFLAGS and _TMP_CXXFLAGS?

Also, it is okay to use the hardcoded value
MacOSX10.4u.sdk in the fake CFLAGS and CXXFLAGS
we use in the CC and CXX checks?

3. Why do you remove -arch $(CPU_ARCH) from MKSHLIB?
You didn't remove -arch $(CPU_ARCH) from MKSHLIB in
your NSS patch.
Wan-Teh, in response to your comments about the NSS patch (comment 31, attachment 209499 [details] [diff] [review]):

1. I set OS_TEST on the assumption that its value was significant in that an NSS Makefile might key off its value somewhere.  There are already two locations in the NSS Makefiles where OS_TEST is used, although both are HP-UX only.  If you share my concern about (ab)use of OS_TEST in NSS Makefiles, then my original patch is cheap insurance, otherwise, yours is clearer.

2. This change only makes sense in light of #3, although I do feel that it should be changed if #3 is used as-is, as it avoids imprecision in the names used in Darwin.mk.

3. This change is necessary because libnssckbi is currently being linked without the SDK.  lib/ckfw/builtins/config.mk resets DSO_LDOPTS to -bundle, obliterating -dynamiclib and related arguments (intended) and the SDK (unintended).  If DSO_LDOPTS is to be used as a switch between -bundle and -dynamiclib for Darwin, then the SDK must be set elsewhere.  MKSHLIB is as good a place as any.  This also a problem in lib/ckfw/capi/config.mk, but libnsscapi is not part of the Mozilla build.

I thought that I had caught all cases of linking being done without the SDK in previous patches, but I had missed this one.  The cross environment aggressively enforces the requirement to link against the proper SDK.
In response to Wan-Teh's comments about the NSPR patch (comment 33):

1. This change is fine.  No special treatment is needed for HOST_LDFLAGS in configure, as the only thing added to LDFLAGS in the Mac OS X case is the SDK.

2a. OK, this new patch includes this change.  I will also make this change in the core patch.

2b. It's something of a necessary evil just to get configure past the bootstrapping stage on a ppc build host, because the system libraries aren't fat and the only place x86 libraries exist are in that SDK.  This is only the case for ppc build hosts running Tiger cross-compiling for x86.  It's of course subject to change at the whims of Apple.  That's why it's wrapped in a conditional that only applies the fix-up where configure would fail with 100% certainty if it weren't applied, and is so limited that it won't ever be applied in far-off future cases.  Because the hard-coded SDK is the only way to make things work in the problematic ppc/10.4->x86 configuration, and because it's dropped out after the bootstrapping step (to be replaced by the user-supplied SDK), I don't think it's worth worrying about.  It's a small price to pay for life on the bleeding edge.

3. The recommended cross build environment sets CC="gcc-3.3 -arch ppc" or CC="gcc-4.0 -arch i386" and LD=$CC.  When MKSHLIB includes -arch, it therefore results in a double -arch.  I'm trying to avoid that situation.  When linking, zero -arch arguments are required (the linker knows what to do based on its input), so I'm removing the -arch from ld in straight native builds and allowing -arch to pass through in cross builds only because it is already present in the environment.  (LD=gcc-VER, no -arch, should also be functional for cross builds, but I haven't tested it and prefer to allow the ease of using LD=$CC.)  Multiple identical -arch arguments to ld don't presently pose a problem, but it's precisely the type of thing that's a target for breakage in the future.

Had I been in the same frame of mind when I wrote the NSS patch, I would have removed -arch from MKSHLIB there too.  Let's do that in the next version of the NSS patch.
Attachment #209998 - Attachment is obsolete: true
Attachment #211811 - Flags: superreview?(cls)
Attachment #211811 - Flags: review?(wtchang)
Attachment #209998 - Flags: superreview?(cls)
Attachment #209998 - Flags: review?(wtchang)
I have re-landed the asdecode removal patch (attachment 209493 [details] [diff] [review]) and replacement resources (attachment 208770 [details]) on the trunk.  Jaguar is no longer supported as a build platform.  Remaining Jaguar tinderboxen will soon catch fire.  See comment 18 et seq.
Identical to v3 + change from comment 30 + _SAVE_CFLAGS instead of _TMP_CFLAGS from comment 32.  This has r=bsmedberg sr=bryner.  I'd also like cls to look it over.
Attachment #210116 - Attachment is obsolete: true
Attachment #211813 - Flags: superreview+
Attachment #211813 - Flags: review?(cls)
Attachment #210116 - Flags: review?(cls)
The tree has been closed due to lack of Mac performance numbers, something I agree with (not sure who did it, though).  Holding the tree hostage to negotiate with the build team is not acceptable behavior; please back out until the tinderboxes have been upgraded or set up equivalent performance tests on other tinderboxes.
Backed out (again) due to Jaguar fire (again).
Attachment #211811 - Flags: superreview?(cls) → superreview+
Comment on attachment 211813 [details] [diff] [review]
Core: support ppc<->x86 cross builds (v4)

Need more coffee.
Attachment #211813 - Flags: superreview?(wtchang)
Attachment #211813 - Flags: superreview+
Attachment #211813 - Flags: review?(cls)
Attachment #211813 - Flags: review+
Comment on attachment 211813 [details] [diff] [review]
Core: support ppc<->x86 cross builds (v4)

Really, really need more coffee. 

I'm still not seeing the need for TARGET_CPU_ARCH.  

First, even if you pass 'powerpc' or 'i686' to NSS in CPU_ARCH, CPU_ARCH will be reset to a proper value due to your change to security/coreconf/Darwin.mk  in the NSS patch.

Second, OS_TEST is so close to what you want for TARGET_CPU_ARCH that it should be very simple to massage those values into the proper arguments.  OS_TEST is only used in a handful of places at the moment and it doesn't look like changing i686->i386 will make a difference in any of those places.

It looks like there's only 1 place that we absolutely need the ppc/i386 combo and that's passing ARCHS to pbxbuild.  Push comes to shove, you can add an explicit test there like you do in js/src/Makefile.in. The other checks can be done with the existing variables.

Why are you not redefining OS_TEST when cross-compiling?

In js/src/Makefile.in , -DCROSS_COMPILE=1 is already set when cross-compiling.
Attachment #211813 - Flags: superreview?(wtchang)
Attachment #211813 - Flags: superreview+
Attachment #211813 - Flags: review-
Attachment #211813 - Flags: review+
cls, thank you very much for your thorough review.  It looks like the best solution is to reuse OS_TEST for what I am using TARGET_CPU_ARCH for now.  I'll prepare a new patch with this change later: the values for OS_TEST will be ppc and i386 on Darwin whether cross-compiling or not.

For your first point, the reason I thought it important to pass ppc/i386 into NSS' OS_TEST is contained in comment 33 point 1.

> In js/src/Makefile.in , -DCROSS_COMPILE=1 is already set when cross-compiling.

CROSS_COMPILE is defined (as 1) in mozilla-config.h, but that's not included by jscpucfg.c.  CROSS_COMPILE must be defined in jscpucfg.c, or it will produce a bad jsautocfg.h.  Specficially in this case, endianness will be flipped.
Changes here:
- Set OS_TEST to ppc or x86 in both native and cross compilatons, eliminating
  TARGET_CPU_ARCH.
- OS_TEST is used as the ARCHS setting for xcodebuild and as the CPU_ARCH
  setting for NSS.  I don't want to pass a string into NSS that can't be
  directly used as its CPU_ARCH.  I am posting a revised NSS patch shortly.
- -DCROSS_COMPILE=1 is only set directly when compiling jscpucfg.c, otherwise
  it's expected to be in mozilla-config.h.
Attachment #211813 - Attachment is obsolete: true
Attachment #211910 - Flags: review?(cls)
This requires CPU_ARCH to be specified as ppc or i386 during cross-compilation.  It implements wtchang's suggestion from comment 31 point 1, but retains my implementation for points 2 and 3 (see comment 33).  I am fine with leaving OS_TEST set to the build host's CPU in NSS as long as you can guarantee that it will not be used in a way that harms the cross build.
Attachment #209499 - Attachment is obsolete: true
Attachment #211912 - Flags: review?(wtchang)
Attachment #209499 - Flags: review?(wtchang)
Comment on attachment 209493 [details] [diff] [review]
Remove asdecode (checked in on trunk)

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #209493 - Flags: approval1.8.0.2? → approval1.8.0.2+
Comment on attachment 211910 [details] [diff] [review]
Core: support ppc<->x86 cross builds (v5)

>Index: mozilla/security/manager/Makefile.in
> 	CPU_ARCH="$(TARGET_CPU)" \
> 	$(NULL)
>+ifeq ($(OS_ARCH),Darwin)
>+DEFAULT_GMAKE_FLAGS += CPU_ARCH="$(OS_TEST)"
>+else
>+DEFAULT_GMAKE_FLAGS += CPU_ARCH="$(TARGET_CPU)"
>+endif

The first line shown above, CPU_ARCH="$(TARGET_CPU)", should be removed for obvious reasons.
Attachment #211910 - Flags: review?(cls) → review+
Comment on attachment 209493 [details] [diff] [review]
Remove asdecode (checked in on trunk)

This asdecode patch and associated resources have been checked in on the trunk again, hopefully for the last time.
The asdecode patch and resources have landed on MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH.  Not marking fixed1.8.1 or fixed1.8.0.2 until the rest of these patches have landed.
I removed asdecode.cpp on the trunk, 1_8, and 1_8_0 branches.
Blocks: 327848
I am adding this section to configure.in:

+case "$build:$target" in
+    i?86-apple-darwin*:powerpc-apple-darwin*)
+        dnl cross_compiling will have erroneously been set to "no" in this
+        dnl case, because the x86 build host is able to run ppc code in a
+        dnl translated environment, making a cross compiler appear native.
+        cross_compiling=yes
+        ;;
+esac

In NSPR, the only place this is significant is where INTERNAL_TOOLS is set, in config, for building now and nsinstall.
Attachment #211811 - Attachment is obsolete: true
Attachment #212422 - Flags: review?(wtchang)
Attachment #211811 - Flags: review?(wtchang)
This portion has been checked into the trunk.
Attachment #211910 - Attachment is obsolete: true
I couldn't check in to js.  Someone with permission in js, please check this in.  It has r=bsmedberg r=cls sr=bryner.
Attachment #212537 - Flags: superreview+
Attachment #212537 - Flags: review+
Attachment #212537 - Flags: approval1.8.0.2?
Attachment #212537 - Flags: approval-branch-1.8.1?(shaver)
Attachment #212536 - Flags: approval1.8.0.2?
Attachment #212536 - Flags: approval-branch-1.8.1?(benjamin)
Attachment #212536 - Flags: approval-branch-1.8.1?(benjamin) → approval-branch-1.8.1+
Comment on attachment 212537 [details] [diff] [review]
JS: support ppc<->x86 cross builds (checked in)

mrbkap checked the JS patch in on the trunk.
Attachment #212537 - Attachment description: JS: support ppc<->x86 cross builds → JS: support ppc<->x86 cross builds (checked in)
Comment on attachment 211912 [details] [diff] [review]
NSS: support ppc<->x86 cross builds (v2)

Mark, the NSS patch v2 is the same as the v1 patch.
Are you going to remove -arch $(CPU_ARCH), too?
I accidentally attached the NSS v1 patch as v2 - the filename confirms.  Here's the real v2 patch, regenerated.
Attachment #211912 - Attachment is obsolete: true
Attachment #212651 - Flags: review?(wtchang)
Attachment #211912 - Flags: review?(wtchang)
Comment on attachment 212651 [details] [diff] [review]
NSS: support ppc<->x86 cross builds (v2, for real)

r=wtc.
Attachment #212651 - Flags: review?(wtchang) → review+
Comment on attachment 212536 [details] [diff] [review]
Core: support ppc<->x86 cross builds (checked in)

Mark, in mozilla/security/manager/Makefile.in, I
suggest adding a comment here:

>+ifeq ($(OS_ARCH),Darwin)
>+DEFAULT_GMAKE_FLAGS += CPU_ARCH="$(OS_TEST)"
>+else
>+DEFAULT_GMAKE_FLAGS += CPU_ARCH="$(TARGET_CPU)"
>+endif

because the special case for Darwin is not obvious
at all.

Looking at mozilla/security/coreconf/Linux.mk, I
think that if the cross compilation target is Linux,
we should override OS_TEST instead of CPU_ARCH.  Don't
you think?  On Linux, NSS only uses $(CPU_ARCH) in
CPU_TAG, which is a component of NSS's OBJDIR_NAME
and is just a name.
I found that NSS coreconf uses 'x86' as CPU_ARCH on almost
all other platforms.  Now that we no longer pass -arch $(CPU_ARCH)
to the compiler, we have more flexibility with CPU_ARCH.

So in this alternate patch (v2'), I set CPU_ARCH to x86
instead of i386.  This requires a simultaneous change to
mozilla/security/manager/Makefile.in.

If you like this patch, this will replace NSS patch
(v2, for real).
Attachment #212671 - Flags: review?(mark)
Comment on attachment 212422 [details] [diff] [review]
NSPR: support ppc<->x86 cross builds (v4)

r=wtc.  Nice clean patch.

One question: in mozilla/nsprpub/config/rules.mk,
we have

>+OS_LDFLAGS=$(HOST_LDFLAGS)

Does the following work?

  LDFLAGS=$(HOST_LDFLAGS)

LDFLAGS looks nicer there.

In mozilla/nsprpub/configure.in, it is better to put

>+AC_SUBST(HOST_LDFLAGS)

under AC_SUBST(HOST_CFLAGS) or AC_SUBST(LDFLAGS).
I can make this change when I check this patch in.
Attachment #212422 - Flags: review?(wtchang) → review+
Filed bug 328131 to deal with comment 57, which could potentially grow to be much larger than something that's properly addressed here.
Comment on attachment 212671 [details] [diff] [review]
NSS: support ppc<->x86 cross builds (v2')

+# When cross-compiling, CPU_ARCH should already be defined as the target
+# architecture, x86 or ppc.

That's not true (see below), so it renders this test erroneous:

+ifeq ($(CPU_ARCH),x86)

-DEFAULT_GMAKE_FLAGS += CPU_ARCH="$(OS_TEST)"
+# coreconf wants ppc or x86.
+DEFAULT_GMAKE_FLAGS += CPU_ARCH="$(CPU_ARCH)"
 else
 DEFAULT_GMAKE_FLAGS += CPU_ARCH="$(TARGET_CPU)"
 endif

$(CPU_ARCH) is not defined in this scope.  We need to keep $(OS_TEST) or $(TARGET_CPU).  At this point, the special-case for Darwin is no longer necessary, because -arch has been removed from the Darwin coreconf makefrag.  We can go back to passing $(TARGET_CPU) in and not worry that it will be set to powerpc instead of ppc.
Attachment #212671 - Flags: review?(mark) → review-
Testing this one now.
Attachment #212651 - Attachment is obsolete: true
Attachment #212671 - Attachment is obsolete: true
Attachment #212675 - Flags: review?(wtchang)
Comment on attachment 212675 [details] [diff] [review]
NSS: support ppc<->x86 cross builds (v2")

I think we have a winner.  The only way this could be made any cleaner would be to just assign |CPU_ARCH = $(OS_TEST)| if CPU_ARCH isn't defined in Darwin.mk, instead of forcing CPU_ARCH in native builds to match the values that would be used in cross builds (now equivalent to uname -p).  OS_TEST would give us "i386" or "Power Macintosh".  We have such a stupid `uname -m`.
Comment on attachment 212422 [details] [diff] [review]
NSPR: support ppc<->x86 cross builds (v4)

(In reply to comment 59:)

This change will work:

-+OS_LDFLAGS=$(HOST_LDFLAGS)
++LDFLAGS=$(HOST_LDFLAGS)

Your placement of HOST_LDFLAGS is a good idea.

This is ready for checkin!
Comment on attachment 212537 [details] [diff] [review]
JS: support ppc<->x86 cross builds (checked in)

approved for 1.8.0 branch pending shaver's approval for 1.8.1, a=dveditz
Attachment #212537 - Flags: approval1.8.0.2? → approval1.8.0.2+
Comment on attachment 212536 [details] [diff] [review]
Core: support ppc<->x86 cross builds (checked in)

approved for 1.8.0 branch, a=dveditz
Attachment #212536 - Flags: approval1.8.0.2? → approval1.8.0.2+
I checked in this patch on the NSPR trunk (NSPR 4.7) and
NSPRPUB_PRE_4_2_CLIENT_BRANCH (mozilla 1.9 alpha).
Attachment #212422 - Attachment is obsolete: true
Attachment #212680 - Flags: approval1.8.0.2?
Attachment #212680 - Flags: approval-branch-1.8.1?(wtchang)
This is the version of the core patch prepared against the 1.8.0 branch.  It's ready for checkin, but it's too late for me to check in any more patches and watch the tree tonight.  I'll do it tomorrow.

This is the same as the checked-in trunk core patch, but with context adjustments in configure.in and xpidl/Makefile.in.  I've also omitted the security/manager/Makefile.in diff in this patch, see comment 62 and the NSS v2" patch.

For 1.8 checkin, the configure diff from this patch file will be used, but the xpidl diff from the trunk patch will be used.
Comment on attachment 212680 [details] [diff] [review]
NSPR: support ppc<->x86 cross builds (v5, checked in)

approved for 1.8.0 branch, a=dveditz
Attachment #212680 - Flags: approval1.8.0.2? → approval1.8.0.2+
Comment on attachment 212694 [details] [diff] [review]
Core: support ppc<->x86 cross builds (v6, for 1.8.0 branch, checked in on branches)

Core patch checked in on branches.
Attachment #212694 - Attachment description: Core: support ppc<->x86 cross builds (v6, for 1.8.0 branch) → Core: support ppc<->x86 cross builds (v6, for 1.8.0 branch, checked in on branches)
This implements a variant of Mark's suggestion in comment 3:

    CPU_ARCH = $(OS_TEST)

Instead of that, I set

    CPU_ARCH := $(shell uname -p)

I found that coreconf already does this for NetBSD and OpenBSD.
Perhaps we should just define OS_TEST as $(shell uname -p) on
these platforms, but I don't want to do that in this bug.

Note that I use the GNU make filter-out function with the pattern
i%86, to be as close to the i*86* pattern we use in configure.in as
possible.
Attachment #212675 - Attachment is obsolete: true
Attachment #212808 - Flags: review?(mark)
Attachment #212675 - Flags: review?(wtchang)
My previous comment has a typo.  "comment 3" should be "comment 63".
Comment on attachment 212808 [details] [diff] [review]
NSS: support ppc<->x86 cross builds (v3)

This is good.  Copy me on a new bug if you open one to clean up the OS_TEST confusion.
Attachment #212808 - Flags: review?(mark)
Attachment #212808 - Flags: review+
Attachment #212808 - Flags: approval1.8.0.2?
Attachment #212808 - Flags: approval-branch-1.8.1?(wtchang)
Comment on attachment 212808 [details] [diff] [review]
NSS: support ppc<->x86 cross builds (v3)

I checked in the mozilla/security/manager/Makefile.in
change in this patch on the Mozilla trunk (Mozilla 1.9
alpha).

I checked in the NSS change in this patch on the NSS
trunk (NSS 3.12), NSS_3_11_BRANCH (NSS 3.11.1),
NSS_3_10_BRANCH (NSS 3.10.3), and NSS_CLIENT_TAG
(Mozilla 1.9 alpha).

Mark, I will check in this patch on the MOZILLA_1_8_0_BRANCH
because I need to backport another change.
Attachment #212808 - Flags: approval-branch-1.8.1?(wtchang) → approval-branch-1.8.1+
Comment on attachment 212680 [details] [diff] [review]
NSPR: support ppc<->x86 cross builds (v5, checked in)

I checked in this patch on the NSPR_4_6_BRANCH (NSPR 4.6.2),
MOZILLA_1_8_BRANCH (Mozilla 1.8.1), and MOZILLA_1_8_0_BRANCH
(Mozilla 1.8.0.2).
Attachment #212680 - Flags: approval-branch-1.8.1?(wtchang) → approval-branch-1.8.1+
Is there anything left to do for this bug on the MOZILLA_1_8_0_BRANCH?  if no, please add "fixed1.8.0.2" to the keywords.  Thanks
Comment on attachment 212537 [details] [diff] [review]
JS: support ppc<->x86 cross builds (checked in)

b181=shaver
Attachment #212537 - Flags: approval-branch-1.8.1?(shaver) → approval-branch-1.8.1+
davel, we're still waiting on 1.8.0 approval for the NSS patch, attachment 212808 [details] [diff] [review].
Checked in the JS patch on the 1.8 and 1.8.0 branches.

By my math, the only thing left to be checked in is the NSS patch on the 1.8.0 branch, pending approval.
Status: NEW → RESOLVED
Closed: 18 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Comment on attachment 212808 [details] [diff] [review]
NSS: support ppc<->x86 cross builds (v3)

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #212808 - Flags: approval1.8.0.2? → approval1.8.0.2+
Comment on attachment 212808 [details] [diff] [review]
NSS: support ppc<->x86 cross builds (v3)

Wan-Teh will check this in per comment 74.
Comment on attachment 212808 [details] [diff] [review]
NSS: support ppc<->x86 cross builds (v3)

I checked in this patch on the MOZILLA_1_8_0_BRANCH.

Nelson, here is a summary of this patch.
1. security/coreconf/Darwin.mk
- Support cross-compiling on PowerPC Mac for Intel Mac
  and vice versa.  In cross compilation, CPU_ARCH is
  set to either powerpc or i386, which is the same as
  the return value of 'uname -p'.  We can't use
  OS_TEST (uname -m) because its return value on
  PowerPC Mac is not suitable ("PowerPC Macintosh").
- Rename DARWIN_SDK_DSOFLAGS to DARWIN_SDK_SHLIBFLAGS.
- Move $(DARWIN_SDK_DSOFLAGS) from DSO_LDOPTS to MKSHLIB
  because we override DSO_LDOPTS when we build libnssckbi.
- Remove the unnecessary -arch $(CPU_ARCH) flag to $(CC).
2. security/manager/Makefile.in
Back out an earlier change made unnecessary by the first
change above.

Feel free to cancel the review request if you don't
need to review this patch.
Attachment #212808 - Flags: review?(nelson)
Keywords: fixed1.8.0.2
Comment on attachment 212808 [details] [diff] [review]
NSS: support ppc<->x86 cross builds (v3)

Considering the number of people who have collaborated on and reviewed this patch already, I don't think my additional review is necessary.  Thanks.
Attachment #212808 - Flags: review?(nelson)
Depends on: 327092
Not sure how I missed it but these changes broke the cross-mingw build of jscpucfg.  Turns out that the cross-mingw build was depending upon broken behavior of CROSS_COMPILE not being defined.
* Remove the unused uid_t/gid_t configure check that causes a conflict in the cross-mingw build of jscpucfg
* Use ACDEFINES instead of explicitly defining CROSS_COMPILE in JSCPUCFG_DEFINES
* Remove -DXP_* defines from HOST_CFLAGS to avoid conflict when building jscpufg
Attachment #215687 - Flags: superreview?(benjamin)
Attachment #215687 - Flags: review?(mark)
Attachment #215687 - Flags: superreview?(benjamin) → superreview+
Comment on attachment 215687 [details] [diff] [review]
Fix jscpucfg for mingw (checked in)

OK, as long as it's generally understood that XP_* is out of the question in js.  (The XP_* change affects the jskwgen compilation too, but it's not used there.)

While you're here, could you make one more change that I should have made (if you approve):

Change DEFINES += -Ui386 / DEFINES += -Di386=1 to append to JSCPUCFG_DEFINES instead.

These define fixups are only needed for jscpucfg, because nothing else brings in NSPR_CFLAGS.
Attachment #215687 - Flags: review?(mark) → review+
Comment on attachment 215687 [details] [diff] [review]
Fix jscpucfg for mingw (checked in)

The mingw patch has been checked in on the trunk with the requested JSCPUCFG_DEFINES change.
Attachment #215687 - Attachment description: Fix jscpucfg for mingw → Fix jscpucfg for mingw (checked in on trunk)
Blocks: 332050
Attachment #215687 - Flags: approval1.8.0.4?
Attachment #215687 - Flags: approval-branch-1.8.1?
Comment on attachment 215687 [details] [diff] [review]
Fix jscpucfg for mingw (checked in)

Too close to the end of this cycle for this kind of change, get it in early in 1.8.0.5
Attachment #215687 - Flags: approval1.8.0.5?
Attachment #215687 - Flags: approval1.8.0.4?
Attachment #215687 - Flags: approval1.8.0.4-
Attachment #215687 - Flags: approval-branch-1.8.1? → approval-branch-1.8.1?(benjamin)
Attachment #215687 - Flags: approval-branch-1.8.1?(benjamin) → approval-branch-1.8.1+
Comment on attachment 215687 [details] [diff] [review]
Fix jscpucfg for mingw (checked in)

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #215687 - Flags: approval1.8.0.5? → approval1.8.0.5+
Attachment #215687 - Attachment description: Fix jscpucfg for mingw (checked in on trunk) → Fix jscpucfg for mingw (checked in)
Keywords: fixed1.8.0.5
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: