Closed
Bug 479585
Opened 15 years ago
Closed 15 years ago
Compile JS for Symbian
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: christian.bugzilla, Unassigned)
References
Details
(Keywords: fixed1.9.1, mobile)
Attachments
(1 file, 5 obsolete files)
1.25 KB,
patch
|
jimb
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•15 years ago
|
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Fennec → Core
QA Contact: general → general
this WIP patch is made based on this attachment 366591 [details] [diff] [review] https://bugzilla.mozilla.org/attachment.cgi?id=366591 in bug 432606 . currently js for symbian can be compiled with this patch, resulting "libjs_static.a"(should be .lib) without the "libmozjs.dll".
js for Symbian OS compiled as static library. Build finished successfully. No tests performed in this patch, and both EDITLINE and SHELL components are skipped because of the OS doesn't intended to support them. configure.in in the HG root is also modified as an update to attachment 366790 [details] [diff] [review] in bug 432606. configure.in is heavily modified, WINSCW target is removed cause I don't think we could support the not useful but time consuming emulator build. Also some earlier incorrect settings on Symbian cross compile are adjusted.
Attachment #367051 -
Attachment is obsolete: true
Attachment #367733 -
Flags: review?
Attachment #367733 -
Flags: review? → review?(ted.mielczarek)
Comment 3•15 years ago
|
||
Drive-by: +# if defined(SYMBIAN) + PROT_READ | PROT_WRITE, MAP_PRIVATE, -1, 0); +#else PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); +#endif Looks like this wants to be like so, based on prevailing style: +# ifdef SYMBIAN + PROT_READ | PROT_WRITE, MAP_PRIVATE, -1, 0); +# else PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); +# endif Does Symbian not like MAP_ANONYMOUS? Should this be based upon a configure test for MAP_ANONYMOUS, rather than #ifdef SYMBIAN? Does MAP_ANON work? I notice we specialize for that above, for Mac OS X. Maybe the right fix is to leave this code alone and add to that region (defining MAP_ANONYMOUS to 0, I guess?)
jsgc.cpp updated according to comment #3 and IRC talks.
Attachment #367733 -
Attachment is obsolete: true
Attachment #368179 -
Flags: review?(ted.mielczarek)
Attachment #367733 -
Flags: review?(ted.mielczarek)
Attachment #368179 -
Flags: review?(ted.mielczarek) → review?(crowder)
Updated•15 years ago
|
Attachment #368179 -
Flags: review?(crowder) → review?(jim)
Comment 5•15 years ago
|
||
Comment on attachment 368179 [details] [diff] [review] jsgc.cpp updated I've already looked at this, and I'm relatively happy with it. I'd prefer to leave the review to Jim Blandy. One other thing, though: +# if !defined(MAP_ANONYMOUS) && defined(SYMBIAN) +# define MAP_ANONYMOUS 0 +# endif Why bother with the defined(SYMBIAN) here? If we don't have MAP_ANONYMOUS, we're going to have to compensate, and not just on Symbian.
(In reply to comment #5) > (From update of attachment 368179 [details] [diff] [review]) > I've already looked at this, and I'm relatively happy with it. I'd prefer to > leave the review to Jim Blandy. One other thing, though: > > +# if !defined(MAP_ANONYMOUS) && defined(SYMBIAN) > +# define MAP_ANONYMOUS 0 > +# endif > > Why bother with the defined(SYMBIAN) here? If we don't have MAP_ANONYMOUS, > we're going to have to compensate, and not just on Symbian. I think remove the "&& defined(SYMBIAN)" will be OK. I don't have special means here, and the original intention is just to let other platforms know the absence of MAP_ANONYMOUS.
Comment 7•15 years ago
|
||
Why are we adding the 'case "$host"' at lines 217-226 in js/src/configure.in? That doesn't seem to have anything to do with Symbian, and it's not enclosed in any Symbian conditional.
Comment 8•15 years ago
|
||
What's the story behind the use of '$t' here? [ --enable-symbian-target=\$t
Comment 9•15 years ago
|
||
It's helpful to keep changes unrelated to Symbian support in a separate patch. Also, I think the output is clearer without this change, because 'host' indicates more about the script's expectations of the value it finds. @@ -207,13 +247,13 @@ _SAVE_CFLAGS="$CFLAGS" _SAVE_LDFLAGS="$LDFLAGS" - AC_MSG_CHECKING([for host c compiler]) + AC_MSG_CHECKING([for $host c compiler]) AC_CHECK_PROGS(HOST_CC, $HOST_CC gcc cc /usr/ucb/cc cl icc, "") if test -z "$HOST_CC"; then AC_MSG_ERROR([no acceptable c compiler found in \$PATH]) fi AC_MSG_RESULT([$HOST_CC]) - AC_MSG_CHECKING([for host c++ compiler]) + AC_MSG_CHECKING([for $host c++ compiler]) AC_CHECK_PROGS(HOST_CXX, $HOST_CXX $CCC c++ g++ gcc CC cxx cc++ cl icc, "") if test -z "$HOST_CXX"; then AC_MSG_ERROR([no acceptable c++ compiler found in \$PATH])
Comment 10•15 years ago
|
||
AC_ARG_ENABLE(symbian-target, [ --enable-symbian-target=\$t Specify symbian flavor. (WINSCW or GCCE)], OS_TARGET=`echo $enableval | tr a-z A-Z`, OS_TARGET=) It seems like we usually determine OS_TARGET just by looking at the final component of the target triplet (which the configure macros provide in ${target_os}). Is there some reason we can't do that here, just by using targets like *-*-symbian-winscw or *-*-symbian-gcce?
Comment 11•15 years ago
|
||
Here, it would be better for configure.in to set something more generic like DISABLE_JS_SHELL, and then test that in Makefile.in # editline needs to get built before the shell +ifneq ($(OS_ARCH),SYMBIAN) DIRS += shell +endif
Updated•15 years ago
|
Attachment #368179 -
Flags: review?(jim) → review-
Comment 12•15 years ago
|
||
All that said --- thanks very much for contributing this! I'm looking forward to seeing this in the tree.
Comment 13•15 years ago
|
||
Harry, you should add the following patch. Symbian path separator is "\\", not "/" --- a/js/src/jsfile.cpp Mon Mar 16 20:54:59 2009 +0900 +++ b/js/src/jsfile.cpp Sat Mar 21 10:16:52 2009 +0900 @@ -56,6 +56,18 @@ # define CURRENT_DIR "c:\\" # define POPEN _popen # define PCLOSE _pclose +#elif defined(__SYMBIAN32__) +# include <strings.h> +# include <stdio.h> +# include <stdlib.h> +# include <unistd.h> +# include <limits.h> +# define FILESEPARATOR '\\' +# define FILESEPARATOR2 '/' +# define CURRENT_DIR "c:\\" +# define POPEN popen +# define PCLOSE pclose +#else #elif defined(XP_UNIX) || defined(XP_BEOS) # include <strings.h> # include <stdio.h> @@ -263,7 +275,7 @@ static JSBool js_isAbsolute(const char *name) { -#if defined(XP_WIN) || defined(XP_OS2) +#if defined(XP_WIN) || defined(XP_OS2) || defined(__SYMBIAN32__) return *name && name[1] == ':'; #else return (name[0] Also, I believe that you need add the following to js.exe. When we can build js.exe, this patch is needed. diff -r 7bf7ade5886d js/src/shell/js.cpp --- a/js/src/shell/js.cpp Mon Mar 16 20:54:59 2009 +0900 +++ b/js/src/shell/js.cpp Sat Mar 21 10:16:52 2009 +0900 @@ -1079,7 +1079,7 @@ fprintf(gOutFile, "before %lu, after %lu, break %08lx\n", (unsigned long)preBytes, (unsigned long)rt->gcBytes, -#ifdef XP_UNIX +#if defined(XP_UNIX) && !defined(__SYMBIAN32__) (unsigned long)sbrk(0) #else 0
Comment 14•15 years ago
|
||
Makoto Kato, thanks for your suggestion, I have added the "jsfile.cpp" patch already in a new update. However, I think currently we are not ready to build the EXE target cause it needs linking DLLs and UID for each executable, then "shell/js.cpp" patching line is not included.
Comment 15•15 years ago
|
||
I will split source patch and configure scripts patch. Here is the updated sources patch, including contribution from Makoto Kato. configure patches will be submitted soon. And according to proposal made by WTC in bug 442706 comment #53 , I will not change lines about --enable-symbian-target until we make a proper decision on this. If you have other comment, please let me known.
Attachment #368179 -
Attachment is obsolete: true
Attachment #368655 -
Flags: review?
Attachment #368655 -
Flags: review? → review?(jim)
Comment 16•15 years ago
|
||
Comment #11 has been included in this patch. Comment #9 has been included. however. Maybe Christian will give explanation on this. Comment #7 has been included. As stated in comment #15 , --enable-symbian-target is not changed. Also supporting build scripts outside "js/src" are attached as a separate patch.
Attachment #368662 -
Flags: review?(jim)
Comment 17•15 years ago
|
||
this is only a supporting file which patches configure.in and related build files in HG root. Refer bug 432606 for how to build.
Comment 18•15 years ago
|
||
Comment on attachment 368655 [details] [diff] [review] [committed] updated jsfile.cpp and jsgc.cpp We shouldn't be handling portability this way, but for jsfile.cpp, it's fine.
Attachment #368655 -
Flags: review?(jim) → review+
Comment 19•15 years ago
|
||
(In reply to comment #15) > And according to proposal made by WTC > in bug 442706 comment #53 , I will not change lines about > --enable-symbian-target until we make a proper decision on this. If you have > other comment, please let me known. Harry, I've added a comment to that bug on the direction I think we should take there. (I noticed you're not CC'd on that bug; you might want to add yourself, so you can get mail automatically when people add more comments.)
Comment 20•15 years ago
|
||
Comment on attachment 368662 [details] [diff] [review] build scripts patch Looks good. Leaving 'r?', pending the conversation re: --enable-symbian-target.
Comment 21•15 years ago
|
||
(In reply to comment #18) > (From update of attachment 368655 [details] [diff] [review]) > We shouldn't be handling portability this way, but for jsfile.cpp, it's fine. In which way do you suggest we should do for portability in jsgc.cpp ?
Comment 22•15 years ago
|
||
(In reply to comment #21) > In which way do you suggest we should do for portability in jsgc.cpp ? That part of the patch seemed fine.
Attachment #368662 -
Attachment is obsolete: true
Attachment #368662 -
Flags: review?(jim)
Comment 23•15 years ago
|
||
Comment on attachment 368662 [details] [diff] [review] build scripts patch Patch for compiling script files will be submitted in bug 458141.
Attachment #368663 -
Attachment is obsolete: true
Comment 24•15 years ago
|
||
Hi Jim, You had already given r+ to attachment 368655 [details] [diff] [review] . Then would you give any other comments or get this checked into the trunk? And in bug 485141 there is also the attachment 371446 [details] [diff] [review] needs your attention. Thanks again for your time on this.
Comment 25•15 years ago
|
||
Hi, Harry. Sorry for the long silence. (The code freeze deadline for our beta is today.) Let me take a look at this.
Updated•15 years ago
|
Attachment #368655 -
Attachment description: updated jsfile.cpp and jsgc.cpp → [committed] updated jsfile.cpp and jsgc.cpp
Comment 26•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e644c385154b
Flags: wanted1.9.1+
Keywords: fixed1.9.1
Reporter | ||
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•