Why are we making this a configure option?

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.

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.

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;

Gah I need to stop doing that.

Carrying over khuey's review

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.

checkin-needed on mozilla-central.

Please avoid landing this until Bug 522770 is fixed. Making libxul bigger is scary for the moment.

Bug 522770 has been fixed to the point where landing this no longer gives me nightmares.

Carrying over r+. Updated to apply cleanly on trunk. Got rid of the xpcwrappednativejsops.cpp as that code doesn't exist anymore.

Last patch omitted the installer manifest.

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).

(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.

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.

Comment on attachment 465397 [details] [diff] [review] Fixed patch Nevermind, this wont work.

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).

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.

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.

(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

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.

Could you just link static js into xpcshell? (I'm not sure how the interaction between two static copies of JS would work.)

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

This uses khuey's approach for exporting js symbols via libxul

Carrying over r+. This patch is for checkin-ride-along https://wiki.mozilla.org/LandingQueue

Minor omission in js/config

This made x86-64 Linux builds with --disable-libxul crash: bug 587375

We probably should document that all of the JS symbols are now exposed from libxul.

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.

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

(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.