485141 - configure.in and build scripts for Symbian OS

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect)

Description

[Harry Li]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3 GTB5
Build Identifier: 

At present, the method for obtaining $OS_TARGET on Symbian platform is still not confirmed by related peers. Discussion happened at the end of bug 442706, but that is a resolved bug now. We could continue the discussion and patching in this new bug.

Reproducible: Always

Comment 1

[Jim Blandy :jimb]

For what it's worth, simply omitting the hunk that adds the --enable-symbian-target option from the patch in bug 479585, attachment 368662 [details] [diff] [review] lets you set OS_TARGET to whatever you like: doing 'configure --target=CPU-COMPANY-symbianSYSTEM' sets OS_TARGET to 'symbianSYSTEM'.  So I'm not sure what the option allows you to do that you can't do already.

Comment 2

[Harry Li]

Now I am working on patching three configure.in files (located in "/","/nsprpub","/js/src") with only --target parameter to set $OS_TARGET.
And I changed the summary of this bug to make it not only discussing $OS_TARGET but also addressing build system issues.

Comment 3

[Harry Li]

Attachment: xulrunner-symbian.apr05

--enable-symbian-target is no longer needed. Before building, $CROSS_COMPILE environment variable should be set to 1. Then build with arguments :

../hg/src/configure --target=arm-none-symbianelf --disable-debug --with-symbian-sdk=/c/Symbian/9.2/S60_3rd_FP1 --enable-application=xulrunner --enable-default-toolkit=cairo-symbian --disable-javaxpcom --disable-jit

NSPRPUB/ and JS/SRC/ building have been tested.

Comment 4

[Harry Li]

Attachment: nsprpub-symbian.apr05

Comment 5

[Harry Li]

Attachment: js-symbian.apr05

Comment 6

[Harry Li]

Attachment: xulrunner-symbian-build.apr07

Comment 7

[Harry Li]

Attachment: js-symbian-build.apr07

Comment 8

[Harry Li]

with these three patches, now we could build NSPR, JS, and parse/expat successfully.
Make sure there is the environment variable $CROSS_COMPILE=1
invoking configure.in in the obj dir: 
../hg/src/configure --target=arm-none-symbianelf --disable-debug
--with-symbian-sdk=/c/Symbian/9.2/S60_3rd_FP1 --enable-application=xulrunner
--enable-default-toolkit=cairo-symbian --disable-javaxpcom --disable-jit

Comment 9

[Jim Blandy :jimb]

Harry, do you have permission to land patches on the trunk yourself, or do you need me to land this one for you?

Comment 10

[Harry Li]

(In reply to comment #9)
> Harry, do you have permission to land patches on the trunk yourself, or do you
> need me to land this one for you?

I don't have permission to land patches, so I'd appreciated you do this for me. Thanks.

Comment 11

[Christian Sejersen]

Harry, I think you should set nelson for review for the nspr patch and ted for the xulrunner one.

Comment 12

[Jim Blandy :jimb]

Comment on attachment 371446 [details] [diff] [review]
js-symbian-build.apr07

Actually, I screwed up here:

 ifneq (,$(CROSS_COMPILE)$(filter-out WINNT OS2,$(OS_ARCH)))
-ifneq ($(OS_ARCH), WINCE)
+ifeq (,$(filter-out SYMBIAN WINCE,$(OS_ARCH)))
 # nsinstall doesn't get built until we enter config/ in the exports phase,
 # so we'll have to manually ensure it gets built here if we want to use
 # $(EXPORTS)

This can't be right: the new condition will run the enclosed code only if OS_ARCH *is* SYMBIAN or WINCE, whereas I think the intention of the patch was to exclude it on SYMBIAN as well as WINCE --- ifneq.

I would have gone ahead and fixed this and landed it... but how can this patch be working for you as written?

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 371112 [details] [diff] [review]
nsprpub-symbian.apr05

Harry, please answer these questions about your patch.

It appears that you are removing support for one of the two Symbian 
"flavors", namely WINSCW.  From contextual comments, I gather that this
flavor is for developing code to run on a Symbian simulator on Windows.

Q1: Why do you want to remove that?
Q2: Is there agreement among the Symbian developers that that should be 
removed?

It appears that, in the case of a symbian OS, the makefile relies on two 
different but similarly named make variables: OS_TARGET and target_os.
Please explain the difference between them.

Comment 14

[Harry Li]

Sorry for the inactivity of recent weeks cause I was in a work city relocation.

A1: Because I think supporting for emulator flavor will take us some time, and even with a working emulator build, the speed of test cases execution is too slow to accept. So this become impractical in my opinion. While the build scripts were changing and I haven't tested these changes on the emulator to ensure they are working, so I removed support for this branch.
A2: I haven't had agreement with other Symbian developers. However, Christian suggested leaving choice for other people to complete this in future. You could see bug 483816 was created with this purpose.

I should state that I am a very beginner on GNU autoconf and make. In the beginning of porting, I used --enable-symbian-target=GCCE/WINSCW to specify $OS_TARGET , and in latter parts of configure script and Makefile I detect build flavor by this variable. When these build scripts patches got reviewed, Mozilla peers suggests taking build flavors from --target argument, then we should set --target=arm-none-symbianelf for device build and --target=arm-none-symbianwinscw for emulator build. So I could only get build flavor from $target_os variable in configure script, then this is used to set $OS_TARGET in autoconf.mk to help Makefile to know which flavor should be built.

Because my work city relocation is still not needed, maybe I couldn't do code related works until 1st week in May, sorry for the inconvenience.

Comment 15

[Harry Li]

(In reply to comment #12)
> (From update of attachment 371446 [details] [diff] [review])
> Actually, I screwed up here:
> 
>  ifneq (,$(CROSS_COMPILE)$(filter-out WINNT OS2,$(OS_ARCH)))
> -ifneq ($(OS_ARCH), WINCE)
> +ifeq (,$(filter-out SYMBIAN WINCE,$(OS_ARCH)))
>  # nsinstall doesn't get built until we enter config/ in the exports phase,
>  # so we'll have to manually ensure it gets built here if we want to use
>  # $(EXPORTS)
> 
> This can't be right: the new condition will run the enclosed code only if
> OS_ARCH *is* SYMBIAN or WINCE, whereas I think the intention of the patch was
> to exclude it on SYMBIAN as well as WINCE --- ifneq.
> 
> I would have gone ahead and fixed this and landed it... but how can this patch
> be working for you as written?

Hi Jim,
Sorry for the late response.

After re-check the patch and run the build scripts again, I think this line is correct. To my understanding, "$(filter-out SYMBIAN WINCE,$(OS_ARCH)" will result "SYMBIAN" if $(OS_ARCH) is "SYMBIAN", "WINCE" if $(OS_ARCH) is "WINCE" or
"" if $(OS_ARCH) is "MAC". Then we take this expression into "$ifeq (,$(filter-out SYMBIAN WINCE,$(OS_ARCH))". Cause the first argument is "", a null, so if $(OS_ARCH) is SYMBIAN or WINCE, the null and the second non-null express will result a false, then it will be lead to not compiling nsinstall.

Comment 16

[Jim Blandy :jimb]

(In reply to comment #15)
> To my understanding, "$(filter-out SYMBIAN WINCE,$(OS_ARCH)" will
> result "SYMBIAN" if $(OS_ARCH) is "SYMBIAN", "WINCE" if $(OS_ARCH) is "WINCE"
> or
> "" if $(OS_ARCH) is "MAC".

Actually, filter-out does the opposite: it removes all occurrences of SYMBIAN and WINCE from the list of words in OS_ARCH.  Since OS_ARCH is always a single word, the whole filter-out call expands to the empty string when OS_ARCH is SYMBIAN or WINCE, or something non-empty otherwise.  I believe the function you're looking for is 'filter'.

For debugging makefiles, you can add calls to make's $(info) function to the 'ifeq' body to see whether it's being triggered or not.  I also find it very helpful to run 'make -qp' and search through the output, which shows the rules, variable values, and source locations that set them.  See the descriptions of 'filter' and 'filter-out' here:
http://www.gnu.org/software/make/manual/html_node/Text-Functions.html#Text-Functions

I'm at a loss to explain why your build works, but I'm pretty sure that 'make' conditional is not correct.

Comment 17

[Jim Blandy :jimb]

(In reply to comment #14)
> I should state that I am a very beginner on GNU autoconf and make.

If you join the #jsapi IRC channel on irc.mozilla.org, I'd be happy to answer your beginner questions on autoconf and make.

Comment 18

[Harry Li]

Attachment: js_src-symbian-build.may08

After double recheck, I found the previous configure gave "OS_ARCH=symbianelf" in autoconf.mk file, that's the reason why I got a valid build. Now I move this line (and OS_RELEASE, CPU_ARCH) to the place where configure will actually put it into "autoconf.mk". With this patch I could build js successfully.

Comment 19

[Harry Li]

Attachment: xulrunner-symbian-build.may08

This patch contains the same fix as stated in comment #18 .

Comment 20

[Jim Blandy :jimb]

(In reply to comment #18)
> After double recheck, I found the previous configure gave "OS_ARCH=symbianelf"
> in autoconf.mk file, that's the reason why I got a valid build. Now I move this
> line (and OS_RELEASE, CPU_ARCH) to the place where configure will actually put
> it into "autoconf.mk". With this patch I could build js successfully.

Thanks for sorting this out.  Should I land this now?

Comment 21

[Harry Li]

(In reply to comment #20)
> (In reply to comment #18)
> > After double recheck, I found the previous configure gave "OS_ARCH=symbianelf"
> > in autoconf.mk file, that's the reason why I got a valid build. Now I move this
> > line (and OS_RELEASE, CPU_ARCH) to the place where configure will actually put
> > it into "autoconf.mk". With this patch I could build js successfully.
> 
> Thanks for sorting this out.  Should I land this now?

This is ready for landing.
Thanks :)

Comment 22

[Jim Blandy :jimb]

js_src-symbian-build.may08 passes the try server; will push tomorrow morning, when I'm able to stay and watch the tree for consequences.

Comment 23

[Jim Blandy :jimb]

Got distracted Friday; plan to land today.

Comment 24

[Jim Blandy :jimb]

Landed in mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/9703fd69e126

Comment 25

[(not currently active) Ted Mielczarek]

Comment on attachment 376367 [details] [diff] [review]
xulrunner-symbian-build.may08

+    HOST_CC=gcc
+    HOST_CXX=g++

You shouldn't need to set these, configure should automatically look for gcc for the host compiler:
http://mxr.mozilla.org/mozilla-central/source/configure.in#297

+    HOST_CFLAGS=-D_X86_
+    HOST_CXXFLAGS=-D_X86_

Why is this necessary?

+    CFLAGS="-MD -nostdinc"

You should always do: CFLAGS="$CFLAGS ..." in configure, so the user can pass in their own CFLAGS on the commandline.

+        CFLAGS="$CFLAGS -c -Wall -Wno-unknown-pragmas -fexceptions -march=armv5t -mapcs -pipe -msoft-float"

I don't think you want -c in there.

Also, -Wall should get set in this block here, unless the symbian compiler doesn't trip the GNU_CC variable:
http://mxr.mozilla.org/mozilla-central/source/configure.in#1450

Any reason you set CFLAGS/CXXFLAGS in two steps, instead of just all at once?

+    *-symbianwinscw)
+        dnl TODO: add emulator build code
+        OS_TARGET=WINSCW


If this isn't going to work yet, could you put in something like AC_MSG_ERROR([Symbian emulator build is not yet supported]) ?

+    AC_DEFINE(NDEBUG)

I don't think you should be defining this unilaterally, we set this when you --disable-debug:
http://mxr.mozilla.org/mozilla-central/source/configure.in#6423

+    AC_DEFINE(HAVE_FCNTL_FILE_LOCKING)
+    AC_DEFINE(HAVE_SOCKLEN_T)

It looks like these defines are only used in NSPR, so you shouldn't need them here.

+cairo-symbian)
+    dnl TODO: add section for cairo port of symbian
+    ;;
+    

Do you have a cairo port yet? I don't think I'm happy leaving this as a TODO in configure.

 if test "$result" = "no"; then
-    AC_MSG_ERROR([Your compiler does not follow the C++ specification for temporary object destruction order.])
+    AC_MSG_WARN([Your compiler does not follow the C++ specification for temporary object destruction order.])
 fi

I don't think this is going to work. That check is there for a reason, the comment references bug 235381 which talks about crashes on Solaris.

r-, need some answers to the questions I've posed here.

Comment 26

[Jim Blandy :jimb]

(In reply to comment #25)

Hmm, some of Ted's comments here apply also to the patch for js/src.  Could you put together a follow-up patch for that, too?  In particular:

        CFLAGS="-MD -nostdinc"

You should add to CFLAGS, not replace its value,

        CFLAGS="$CFLAGS -c -Wall -Wno-unknown-pragmas -fexceptions -march=armv5t -mapcs -pipe -msoft-float"

This shouldn't include -c.

            CXXFLAGS="$CFLAGS -Wno-ctor-dtor-privacy"

This should be extending CXXFLAGS, not CFLAGS.  If you need to repeat those flags, perhaps you could put them in a variable first, and then add them to CFLAGS and CXXFLAGS.

            CFLAGS="$CFLAGS ${GCCE_INCLUDE} -x c"
            CXXFLAGS="$CXXFLAGS ${GCCE_INCLUDE} -x c++"

Why do you need to specify '-x c' and '-x c++' here?  Are you compiling files that have non-standard filename extensions?  If so, could we get the -x flags for those targets someplace more specific?

And --- my apologies.  I took the attitude that whatever was in a Symbian conditional was the Symbian developers' concern, and didn't look too critically, but that's bogus.  Anyone trying to work on those scripts or makefiles will be trying to avoid breaking other targets, and odd stuff will cause them a lot of confusion and headache.  For example, we don't want people wondering, "Gee, is CFLAGS something I should be able to set or not?  It's usually preserved, but then in some cases (say, this Symbian code here) the inherited value is just wiped out..."  Thanks to Ted for keeping standards high.