444485 - Make WinMobile Build work with Visual Studio 2008 (VS9)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[John Wolfe (:wolfe)]

Attachment: Patch to add Visual Studio 9 build support

Right now, the mozilla-central build for WinCE can only be built with Visual Studio 2005 (also known as Visual Studio 8, or VS8).

The build needs to be modified such that EITHER Visual Studio 2005 AND Visual Studio 2008 (also known as Visual Studio 9, or VS9).

This will involve changes within the build/wince directory, as well as changes to the MOZCONFIG file used to build WinMobile 6 XULRunner.

A patch provided changes the build procedure for the WinCE/Tools to place the compiled tools into a new build/wince/tools/bin subdirectory, which is created by the build/wince/tools Makefile.

The MOZCONFIG file will need to have the following lines:

CC=$topsrcdir/build/wince/tools/bin/arm-wince-gcc.exe
CXX=$topsrcdir/build/wince/tools/bin/arm-wince-gcc.exe
CPP=$topsrcdir/build/wince/tools/bin/arm-wince-gcc.exe
LD=$topsrcdir/build/wince/tools/bin/arm-wince-link.exe
AR=$topsrcdir/build/wince/tools/bin/arm-wince-lib.exe
AS=$topsrcdir/build/wince/tools/bin/arm-wince-as.exe

mk_add_options AR=@TOPSRCDIR@/build/wince/tools/bin/arm-wince-lib.exe
mk_add_options AS=@TOPSRCDIR@/build/wince/tools/bin/arm-wince-as.exe

(replacing the currently existing "vs8ppc2003arm" subdirectory with the new generic "bin" subdirectory.

Comment 1

[John Wolfe (:wolfe)]

Attachment: Patch done up in Unified DIFF instead of context DIFF.

Same code, formatting of DIFF changed to unified.

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 329594 [details] [diff] [review]
Patch done up in Unified DIFF instead of context DIFF.

diff -r a17cd4b80fef -r a5ef59c98a73 build/wince/tools/Makefile
+ifeq ($(MSYSTEM),MINGW32)
+DEVENV_FLAG=//
+else
+DEVENV_FLAG=/
+endif


Does devenv not respect - in place of / in options? The previous contents of this file seem to indicate otherwise. That should remove the need for this variable.

+ifeq ($(VSINSTALLDIR),C:\Program Files\Microsoft Visual Studio 9.0)

This is pretty awful, and guarantees that this will break if someone installs into a different dir. If you're ok with relying on MozillaBuild-set variables, you can switch on MOZ_MSVCVERSION, it's set to 8 or 9.

+	if [ -d bin ]; then echo "bin/ exists"; else mkdir bin; fi

Seems a little verbose. Why not just use |mkdir -p dir|?

diff -r a17cd4b80fef -r a5ef59c98a73 build/wince/tools/vs9ppc2003arm/build_tools.bat

Any particular reason you have both a Makefile and a batch file to do the same thing?

diff -r a17cd4b80fef -r a5ef59c98a73 build/wince/tools/vs9ppc2003arm/toolspath.h
+#define WCE_BIN   "c:\\Program Files\\Microsoft Visual Studio 9.0\\VC\\ce\\bin\\x86_arm\\"

Again, this seems like it's going to cause people headaches.

diff -r a17cd4b80fef -r a5ef59c98a73 xpcom/reflect/xptcall/src/md/win32/Makefile.in
+ifeq ($(VSINSTALLDIR),C:\Program Files\Microsoft Visual Studio 9.0)

This is unacceptable in this makefile. You could do some selection in configure.in somewhere here:
http://mxr.mozilla.org/mozilla-central/source/configure.in

In addition, could we set AS in configure.in instead of hardcoding this value here?

I didn't really look at the little helper C++ programs, hopefully that's ok.

Comment 3

[John Wolfe (:wolfe)]

Most of the changes outlined by Ted are easily done, and ready to put into a new patch.

The reason a batch file (build/wince/tools/vs9ppc2003arm/build_tools.bat) is around that does the same thing as the Makefile is historical - VS8's shunt code had the same batch file, so I duplicated it in the VS9 tools source code directory.


The hard-coded paths to executables are due to the shifting locations into which Microsoft places their binaries, and the need to exactly specify which version of Visual Studio executables are to be run.

We take great pains at highlighting the need for modifying toolpath.h if a developer installs their Visual Studio or SDKs into places besides the default installation directories.  

That is not an excuse, just an explanation of why the toolpath.h file has not been implemented differently.  There are more than enough things to do to get XULRunner.exe running on WinCE/WinMobile -- then we could come back to how the build environment's shunt code is configured.


For xpcom/reflect/xptcall/src/md/win32/Makefile.in:

TOPSRCDIR/configure.in is hard-wiring AS=$(CC), causing my mozconfig setting for AS to be obliterated.  Here is the source code line for the configure.in problem area:

http://mxr.mozilla.org/mozilla-central/source/configure.in#940


There are two possible solutions to this problem:

(1) Make AS only be set to $(CC) if AS is unset:

-AS='$(CC)'
+if test -z "$AS"; then
+  AS='$(CC)'
+fi


or 

(2) in a WINCE-specific area of configure.in, check for a non-null AS_BIN (as set on line 935) and set AS=$(AS_BIN) if AS_BIN is non-null.


PRO (1): Fixes the real problem of artificially "forcing" AS to $(CC)
CON (1): Might break builds with mozconfig setting AS to something else
CON (1): No good way to test this change without running against all tinderbox builds

PRO (2): Least change to builds other than WINCE
CON (2): Adds complexity to CONFIGURE.IN
CON (2): Does not fix the underlying bug

I would like to land these changes soon, so that XPCOM for WinCE can at least be built.  I am just hesitant to make a change to configure.in that may effect other OS' builds.

What do people think?

Comment 4

[John Wolfe (:wolfe)]

Attachment: Updated patch - adjusted for Ted's comments

Ted, I have modified the patch as requested - except for the 

I also added a mozce_dbg.c module for outputting shunt debug information to a log file.  I do nothing more than compile the module and link it into the shunt in this patch.  In a later patch I will modify the logging calls to use macros that can turn on/off API logging and to-file logging.  I put the new module here because the visual studio project would need to be redone if I didn't.

Since no one but WINCE development is using the WinCE shunt, I think the mozce_dbg.c module is a very low-risk addition to this patch.

Removed the redundant build_tools.bat batch file for VS9, but left the VS8 build_tools.bat batch file alone.

Replaced the Makefile.in IF statement with a hard-coded TOPSRCDIR path.  Not beautiful, but the hard-coded line can be removed once the TOPSRCDIR/configure.in changes for WINCE can be landed.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 337357 [details] [diff] [review]
Updated patch - adjusted for Ted's comments

Your license headers are wrong. The original developer is certainly not Netscape. Please see http://www.mozilla.org/MPL/boilerplate-1.1/ .

Comment 6

[John Wolfe (:wolfe)]

Attachment: Updated Patch with revised license block in build/wince/tools/Makefile

Exact same patch as 337357, except for the license block of build/wince/tools/Makefile.

The original code is is now Mozilla Central code, released April 2008.  

The initial developer of the code is now Mozilla Corporation, and portions created by the initial developer are copyright 2008.

Is that more kosher for a new file such as this Makefile?

Comment 7

[John Wolfe (:wolfe)]

Attachment: Revised as per Doug's Helpful Eye.

Doug looks over my shoulder and makes me do right via copyright block.

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 339838 [details] [diff] [review]
Revised as per Doug's Helpful Eye.

need the same thing on mozce_dbg.c

Comment 9

[John Wolfe (:wolfe)]

Attachment: One last copyright block, this time in mozce_dbg.c

Missed one of the copyright blocks - corrected in this patch.

Comment 10

[Doug Turner (:dougt)]

copyright looks fine.  not sure about the change to:

xpcom/reflect/xptcall/src/md/win32/Makefile.in

rest is okay.

Can you drop that change, and put a new patch up?

Comment 11

[John Wolfe (:wolfe)]

Attachment: Removed change to XPCOM Reflect XPTCall SRC MD Win32 Makefile.in

Removed change to Makefile.in.

Requires arm-wince-asm.exe to be on your mingw32 path in order to correctly compile the assembly.

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 339843 [details] [diff] [review]
Removed change to XPCOM Reflect XPTCall SRC MD Win32 Makefile.in

look fine.

Comment 13

[Doug Turner (:dougt)]

$ hg head
changeset:   19521:aa9fd87b58cd
tag:         tip
user:        Doug Turner <[e-mail]>
date:        Mon Sep 22 22:02:44 2008 +0100
summary:     Bug 444485 - Make Windows CE Mobile Build work with Visual Studio 2008 (VS9).  Patch by Wolfe, r=dougt+ted. npodb.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

verified with alpha2