Closed Bug 580407 Opened 9 years ago Closed 9 years ago

Link js statically via --disable-shared-js

Categories

(Firefox Build System :: General, defect)

x86
Linux
defect
Not set

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)

No description provided.
Attached patch static mozjs (obsolete) — Splinter Review
Assignee: nobody → tglek
Attachment #458806 - Flags: review?(me)
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.
Attachment #458806 - Flags: review?(me)
Attachment #458806 - Flags: review+
Attachment #458806 - Flags: feedback?(mrbkap)
Attached patch staticjs v2 (obsolete) — Splinter Review
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+
Attached patch Static js v2.5 (obsolete) — Splinter Review
Carrying over khuey's review
Attachment #459097 - Attachment is obsolete: true
Attachment #459588 - Flags: review+
blocking2.0: --- → ?
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.
blocking2.0: ? → beta4+
checkin-needed on mozilla-central.
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
Attached patch 459588: Static js (obsolete) — Splinter Review
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+
Attached patch Static js (obsolete) — Splinter Review
Last patch omitted the installer manifest.
Attachment #462185 - Attachment is obsolete: true
Attachment #464485 - Flags: review+
Keywords: checkin-needed
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.
Attached patch Fixed patch (obsolete) — Splinter Review
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)
Comment on attachment 465397 [details] [diff] [review]
Fixed patch

Nevermind, this wont work.
Attachment #465397 - Flags: review?(me)
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.
(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
Attachment #465485 - Attachment is obsolete: true
Attachment #465764 - Flags: review?(ted.mielczarek)
Attachment #465764 - Flags: review?(ted.mielczarek) → review+
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+
Keywords: checkin-needed
Minor omission in js/config
Attachment #465769 - Attachment is obsolete: true
Attachment #465819 - Flags: review+
http://hg.mozilla.org/projects/electrolysis/rev/b6c7ed5a2922
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Flags: in-testsuite-
Target Milestone: --- → mozilla2.0b4
Depends on: 587350
Depends on: 587389
Blocks: 587470
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
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.
Depends on: 594338
Blocks: 601128
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.