Closed
Bug 580407
Opened 13 years ago
Closed 13 years ago
Link js statically via --disable-shared-js
Categories
(Firefox Build System :: General, defect)
Tracking
(blocking2.0 beta4+)
RESOLVED
FIXED
mozilla2.0b4
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta4+ |
People
(Reporter: taras.mozilla, Assigned: taras.mozilla)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 9 obsolete files)
7.41 KB,
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•13 years ago
|
||
Assignee: nobody → tglek
Assignee | ||
Updated•13 years ago
|
Attachment #458806 -
Flags: review?(me)
Why are we making this a configure option?
Assignee | ||
Comment 3•13 years ago
|
||
Linux users might want separate js
/me sighs, ok, I'll try to review tomorrow.
Comment on attachment 458806 [details] [diff] [review] static mozjs I think this is ok, but it would be a lot easier to understand if --disable-shared-js defined JS_SHARED_LIBRARY so that my tiny mind didn't have to parse double negatives everywhere. Somebody else should look over the xpcconnect changes even though they're simple.
Attachment #458806 -
Flags: review?(me)
Attachment #458806 -
Flags: review+
Attachment #458806 -
Flags: feedback?(mrbkap)
Assignee | ||
Comment 6•13 years ago
|
||
This gets rid of doublenegatives. The behavior is that mozjs is linked statically for mozilla builds unless --enable-shared-js is specified. If one configures a standalone spidermonkey --enable-shared-js is the default.
Attachment #458806 -
Attachment is obsolete: true
Attachment #459097 -
Flags: review?(me)
Attachment #458806 -
Flags: feedback?(mrbkap)
Comment on attachment 459097 [details] [diff] [review] staticjs v2 ># HG changeset patch ># Parent e2346fa5faaa957a3689c1192584bedb63924a32 >Bug 580407 - Link js statically via --disable-shared-js > >diff --git a/config/autoconf.mk.in b/config/autoconf.mk.in >--- a/config/autoconf.mk.in >+++ b/config/autoconf.mk.in >@@ -669,3 +669,5 @@ ANDROID_TOOLCHAIN = @ANDROID_TOOLCHAIN@ > ANDROID_PLATFORM = @ANDROID_PLATFORM@ > ANDROID_SDK = @ANDROID_SDK@ > ANDROID_TOOLS = @ANDROID_TOOLS@ >+ >+JS_SHARED_LIBRARY = @JS_SHARED_LIBRARY@ >diff --git a/configure.in b/configure.in >--- a/configure.in >+++ b/configure.in >@@ -74,6 +74,16 @@ TARGET_OS="${target_os}" > MOZ_DEB_TIMESTAMP=`date +"%a, %d %b %Y %T %z" 2>&1` > AC_SUBST(MOZ_DEB_TIMESTAMP) > >+MOZ_ARG_ENABLE_BOOL(shared-js, >+[ --enable-shared-js >+ Create a shared JavaScript library.], >+ ENABLE_SHARED_JS=1, >+ ENABLE_SHARED_JS=0) >+ >+if test "$ENABLE_SHARED_JS" = "1" ; then >+ JS_SHARED_LIBRARY=1 >+fi >+AC_SUBST(JS_SHARED_LIBRARY) > > dnl ======================================================== > dnl = >@@ -1216,7 +1226,11 @@ MOZ_BZ2_LIBS='$(call EXPAND_LIBNAME_PATH > MOZ_PNG_CFLAGS= > MOZ_PNG_LIBS='$(call EXPAND_LIBNAME_PATH,mozpng,$(DEPTH)/modules/libimg/png)' > >+if test -z "$JS_SHARED_LIBRARY" ; then >+MOZ_JS_LIBS='-L$(LIBXUL_DIST)/bin -ljs_static' >+else > MOZ_JS_LIBS='-L$(LIBXUL_DIST)/bin -lmozjs' >+fi > DYNAMIC_XPCOM_LIBS='-L$(LIBXUL_DIST)/bin -lxpcom -lxpcom_core -lmozalloc' > MOZ_FIX_LINK_PATHS='-Wl,-rpath-link,$(LIBXUL_DIST)/bin -Wl,-rpath-link,$(prefix)/lib' > XPCOM_FROZEN_LDOPTS='-L$(LIBXUL_DIST)/bin -lxpcom -lmozalloc' >@@ -2269,7 +2283,11 @@ ia64*-hpux*) > MOZ_DEBUG_FLAGS='-Zi' > MOZ_DEBUG_LDFLAGS='-DEBUG -DEBUGTYPE:CV' > MOZ_FIX_LINK_PATHS= >- MOZ_JS_LIBS='$(LIBXUL_DIST)/lib/mozjs.lib' >+ if test -z "$JS_SHARED_LIBRARY" ; then >+ MOZ_JS_LIBS='$(LIBXUL_DIST)/lib/js_static.lib' >+ else >+ MOZ_JS_LIBS='$(LIBXUL_DIST)/lib/mozjs.lib' >+ fi > OBJ_SUFFIX=obj > RANLIB='echo not_ranlib' > STRIP='echo not_strip' >@@ -2329,7 +2347,11 @@ ia64*-hpux*) > RCFLAGS='-O coff --use-temp-file' > # mingw doesn't require kernel32, user32, and advapi32 explicitly > LIBS="$LIBS -luuid -lgdi32 -lwinmm -lwsock32" >- MOZ_JS_LIBS='-L$(LIBXUL_DIST)/lib -lmozjs' >+ if test -z "$JS_SHARED_LIBRARY" ; then >+ MOZ_JS_LIBS='-L$(LIBXUL_DIST)/bin -ljs_static' >+ else >+ MOZ_JS_LIBS='-L$(LIBXUL_DIST)/bin -lmozjs' >+ fi > MOZ_FIX_LINK_PATHS= > DYNAMIC_XPCOM_LIBS='-L$(LIBXUL_DIST)/lib -lxpcom -lxpcom_core -lmozalloc' > XPCOM_FROZEN_LDOPTS='-L$(LIBXUL_DIST)/lib -lxpcom -lmozalloc' >@@ -2375,7 +2397,11 @@ ia64*-hpux*) > MOZ_DEBUG_LDFLAGS='-DEBUG -DEBUGTYPE:CV' > WARNINGS_AS_ERRORS='-WX' > MOZ_OPTIMIZE_FLAGS='-O1' >- MOZ_JS_LIBS='$(LIBXUL_DIST)/lib/mozjs.lib' >+ if test -z "$JS_SHARED_LIBRARY" ; then >+ MOZ_JS_LIBS='$(LIBXUL_DIST)/lib/js_static.lib' >+ else >+ MOZ_JS_LIBS='$(LIBXUL_DIST)/lib/mozjs.lib' >+ fi > MOZ_FIX_LINK_PATHS= > DYNAMIC_XPCOM_LIBS='$(LIBXUL_DIST)/lib/xpcom.lib $(LIBXUL_DIST)/lib/xpcom_core.lib $(LIBXUL_DIST)/lib/mozalloc.lib' > XPCOM_FROZEN_LDOPTS='$(LIBXUL_DIST)/lib/xpcom.lib $(LIBXUL_DIST)/lib/mozalloc.lib' >@@ -9198,6 +9224,9 @@ if test "$BUILD_CTYPES"; then > # Build js-ctypes on the platforms we can. > ac_configure_args="$ac_configure_args --enable-ctypes" > fi >+if test -z "$JS_SHARED_LIBRARY" ; then >+ ac_configure_args="$ac_configure_args --disable-shared-js" >+fi > if test -z "$MOZ_NATIVE_NSPR"; then > ac_configure_args="$ac_configure_args --with-nspr-cflags='$NSPR_CFLAGS'" > ac_configure_args="$ac_configure_args --with-nspr-libs='$NSPR_LIBS'" >diff --git a/js/src/Makefile.in b/js/src/Makefile.in >--- a/js/src/Makefile.in >+++ b/js/src/Makefile.in >@@ -119,8 +119,11 @@ endif > # In fact, we now build both a static and a shared library, as the > # JS shell would like to link to the static library. > >+ifdef JS_SHARED_LIBRARY > FORCE_SHARED_LIB = 1 >+endif > FORCE_STATIC_LIB = 1 >+DIST_INSTALL = 1 > > VPATH = $(srcdir) > >diff --git a/js/src/config/autoconf.mk.in b/js/src/config/autoconf.mk.in >--- a/js/src/config/autoconf.mk.in >+++ b/js/src/config/autoconf.mk.in >@@ -338,3 +338,5 @@ WRAP_SYSTEM_INCLUDES = @WRAP_SYSTEM_INCL > ENABLE_TRACEJIT = @ENABLE_TRACEJIT@ > NANOJIT_ARCH = @NANOJIT_ARCH@ > HAVE_ARM_SIMD= @HAVE_ARM_SIMD@ >+ >+JS_SHARED_LIBRARY = @JS_SHARED_LIBRARY@ >diff --git a/js/src/configure.in b/js/src/configure.in >--- a/js/src/configure.in >+++ b/js/src/configure.in >@@ -206,6 +206,19 @@ MOZ_ARG_ENABLE_BOOL(compile-environment, > COMPILE_ENVIRONMENT=1, > COMPILE_ENVIRONMENT= ) > >+MOZ_ARG_ENABLE_BOOL(shared-js, >+[ --disable-shared-js >+ Do not create a shared library.], >+ DISABLE_SHARED_JS=0, >+ DISABLE_SHARED_JS=1) >+ >+if test "$DISABLE_SHARED_JS" = "1" ; then >+ AC_DEFINE(STATIC_JS_API) >+else >+ JS_SHARED_LIBRARY=1 >+fi >+AC_SUBST(JS_SHARED_LIBRARY) >+ > dnl ======================================================== > dnl = Android uses a very custom (hacky) toolchain; we need to do this > dnl = here, so that the compiler checks can succeed >diff --git a/js/src/xpconnect/src/xpcwrappednativejsops.cpp b/js/src/xpconnect/src/xpcwrappednativejsops.cpp >--- a/js/src/xpconnect/src/xpcwrappednativejsops.cpp >+++ b/js/src/xpconnect/src/xpcwrappednativejsops.cpp >@@ -1276,7 +1276,13 @@ XPC_WN_Helper_NewResolve(JSContext *cx, > > /***************************************************************************/ > >-extern "C" JS_IMPORT_DATA(JSObjectOps) js_ObjectOps; >+extern "C" >+#ifndef STATIC_JS_API >+JS_IMPORT_DATA(JSObjectOps) >+#else >+JSObjectOps >+#endif >+js_ObjectOps; > > static JSObjectOps XPC_WN_WithCall_JSOps; > static JSObjectOps XPC_WN_NoCall_JSOps;
Attachment #459097 -
Flags: review?(me) → review+
Gah I need to stop doing that.
Assignee | ||
Comment 9•13 years ago
|
||
Carrying over khuey's review
Attachment #459097 -
Attachment is obsolete: true
Attachment #459588 -
Flags: review+
Assignee | ||
Updated•13 years ago
|
blocking2.0: --- → ?
Assignee | ||
Comment 10•13 years ago
|
||
Nominated this as a blocker because it's one of the incremental steps on the way to a keeping our code in a single dll in bug 561842. This improves startup by causing fewer library page faults.
Updated•13 years ago
|
blocking2.0: ? → beta4+
Assignee | ||
Comment 11•13 years ago
|
||
checkin-needed on mozilla-central.
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Please avoid landing this until Bug 522770 is fixed. Making libxul bigger is scary for the moment.
Depends on: 522770
Keywords: checkin-needed
Bug 522770 has been fixed to the point where landing this no longer gives me nightmares.
Keywords: checkin-needed
Assignee | ||
Comment 14•13 years ago
|
||
Carrying over r+. Updated to apply cleanly on trunk. Got rid of the xpcwrappednativejsops.cpp as that code doesn't exist anymore.
Attachment #459588 -
Attachment is obsolete: true
Attachment #462185 -
Flags: review+
Assignee | ||
Comment 15•13 years ago
|
||
Last patch omitted the installer manifest.
Attachment #462185 -
Attachment is obsolete: true
Attachment #464485 -
Flags: review+
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Comment 16•13 years ago
|
||
Are we going to get a new patch before Friday? This needs to land by then. Also, when uploading the patch, please keep in mind the points raised in http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed (specifically, you aren't storing your username in the patch which makes more work for the person being kind enough to land for you).
Assignee | ||
Comment 17•13 years ago
|
||
(In reply to comment #16) > Are we going to get a new patch before Friday? This needs to land by then. > Also, when uploading the patch, please keep in mind the points raised in > http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed > (specifically, you aren't storing your username in the patch which makes more > work for the person being kind enough to land for you). hopefully. I found a weird regression in (Components.classes is not a function) in xpcshell-tests, no idea what's causing it yet.
Assignee | ||
Comment 18•13 years ago
|
||
Too bad I didn't catch this one earlier http://scotland.proximity.on.ca/dxr//mozilla-central/js/src/jstypes.h.html#l167 for some reason makes xpcshell unhappy. In retrospect, static js isn't going into a binary perhaps setting STATIC_JS_API wasn't the wisest choice. Instead I just added JS_SHARED_LIBRARY to guard Dllmain.
Attachment #464485 -
Attachment is obsolete: true
Attachment #465397 -
Flags: review?(me)
Assignee | ||
Comment 19•13 years ago
|
||
Comment on attachment 465397 [details] [diff] [review] Fixed patch Nevermind, this wont work.
Attachment #465397 -
Flags: review?(me)
Assignee | ||
Comment 20•13 years ago
|
||
Sorry it took so long. I finally realized that the problem is that xpcshell links to xul AND mozjs. That clearly doesn't work with a staticjs lib. It's possible to have staticjs + tests if we fold xpcshell code into libxul(conditionally on --enable-tests).
Attachment #465397 -
Attachment is obsolete: true
Attachment #465485 -
Flags: review?(me)
Comment on attachment 465485 [details] [diff] [review] Fixed patch(force shared js when tests are enabled) I didn't actually look at the patch, but we ship releases with --enable-tests, and I don't think folding xpcshell into code we ship is something we want to do.
Attachment #465485 -
Flags: review?(me) → review-
If mozjs is in xul why does xpcshell need to link to mozjs anymore?
So now that I read your patch it doesn't fold xpcshell into libxul at all. It forces js to be shared when --enable-tests is on. That still won't fly though because we ship releases with --enable-tests.
Assignee | ||
Comment 24•13 years ago
|
||
(In reply to comment #22) > If mozjs is in xul why does xpcshell need to link to mozjs anymore? Because currently JS gets folded in by a static-like link. Ie the JS symbols aren't exported. Changing the build to export JS symbols via libxul would involve including jsheaders with JS_EXPORT_API from xul code. That's the right way to fix this but I don't know how to do that(yet). (In reply to comment #23) > So now that I read your patch it doesn't fold xpcshell into libxul at all. It > forces js to be shared when --enable-tests is on. That still won't fly though > because we ship releases with --enable-tests. I didn't realize that's how we ship :( Thanks for the quick feedback.
(In reply to comment #24) > (In reply to comment #22) > > If mozjs is in xul why does xpcshell need to link to mozjs anymore? > > Because currently JS gets folded in by a static-like link. Ie the JS symbols > aren't exported. Changing the build to export JS symbols via libxul would > involve including jsheaders with JS_EXPORT_API from xul code. That's the right > way to fix this but I don't know how to do that(yet). AIUI that's what http://mxr.mozilla.org/mozilla-central/source/toolkit/library/dlldeps-xul.cpp is for. See the comments at the top of http://mxr.mozilla.org/mozilla-central/source/xpcom/build/dlldeps.cpp
Assignee | ||
Comment 26•13 years ago
|
||
I was talking about defining EXPORT_JS_API from xul code http://scotland.proximity.on.ca/dxr//mozilla-central/js/src/jstypes.h.html#l167 so dll exports could be defined correctly.
The solution here is probably to AC_DEFINE(EXPORT_JS_API) instead of STATIC_JS_API then.
(In reply to comment #27) > The solution here is probably to AC_DEFINE(EXPORT_JS_API) instead of > STATIC_JS_API then. Well, not quite. I think what we really want is to have EXPORT_JS_API defined whenever MOZILLA_INTERNAL_API is defined and IMPORT_JS_API defined anywhere else.
Comment 29•13 years ago
|
||
Could you just link static js into xpcshell? (I'm not sure how the interaction between two static copies of JS would work.)
Comment 30•13 years ago
|
||
Statically linking JS into xpcshell probably won't work. We really need to be exporting the JS symbols. I think this means: #if defined(STATIC_JS_API) && defined(MOZILLA_INTERNAL_API) # define JS_PUBLIC_API(t) JS_EXPORT_API(t) ... and keep the rest of the jstypes.h the same
Assignee | ||
Comment 31•13 years ago
|
||
This uses khuey's approach for exporting js symbols via libxul
Attachment #465485 -
Attachment is obsolete: true
Attachment #465764 -
Flags: review?(ted.mielczarek)
Updated•13 years ago
|
Attachment #465764 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 32•13 years ago
|
||
Carrying over r+. This patch is for checkin-ride-along https://wiki.mozilla.org/LandingQueue
Attachment #465764 -
Attachment is obsolete: true
Attachment #465769 -
Flags: review+
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 33•13 years ago
|
||
Minor omission in js/config
Attachment #465769 -
Attachment is obsolete: true
Attachment #465819 -
Flags: review+
Flags: in-testsuite-
Target Milestone: --- → mozilla2.0b4
Comment 35•13 years ago
|
||
This made x86-64 Linux builds with --disable-libxul crash: bug 587375
Depends on: 587375
We probably should document that all of the JS symbols are now exposed from libxul.
Keywords: dev-doc-needed
Comment 37•13 years ago
|
||
Looking at this patch, I'm slightly confused. Does this add --disable-shared-js or --enable-shared-js or both? It looks like it adds both, which seems odd. Need to get that sorted out to write this up properly for docs.
Comment 38•13 years ago
|
||
FWIW, assuming that --disable-shared-js is the correct flag, I've documented this here: https://developer.mozilla.org/En/SpiderMonkey/Build_Documentation#Building_SpiderMonkey_as_a_static_library
Assignee | ||
Comment 39•13 years ago
|
||
(In reply to comment #37) > Looking at this patch, I'm slightly confused. Does this add --disable-shared-js > or --enable-shared-js or both? It looks like it adds both, which seems odd. > Need to get that sorted out to write this up properly for docs. Your docs got it right. --disable-shared-js is a flag added to spidermonkey since by default it should build a shared library. Mozilla builds static js by default so it needed an opposite flag.
Updated•13 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•5 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•