GNOME Bugzilla – Bug 644971
Cleanup mozjs detection using js-config and mozilla-js.pc
Last modified: 2011-05-18 00:21:45 UTC
Currently, if js-config is installed in $PATH (usually by spidermonkey), the following configure.ac snippet: --- # Look for Spidermonkey. If js-config exists, use that; # otherwise we try some pkgconfig files from various distributions. AC_ARG_VAR([JS_CONFIG], [The js-config program to use]) if test "$ac_cv_env_JS_CONFIG_set" != "set"; then AC_PATH_PROG([JS_CONFIG], [js-config], []) fi if test -n "$JS_CONFIG"; then JS_CFLAGS="$($JS_CONFIG --cflags)" JS_LIBS="$($JS_CONFIG --libs)" FIREFOX_JS_LIBDIR="$($JS_CONFIG --libdir)" JS_PACKAGE= --- Will lead to JS_PACKAGE being set to ''. This will error out a few lines later at: --- AC_MSG_CHECKING([for mozilla-js >= 1.9.2 ]) if `$PKG_CONFIG --exists $JS_PACKAGE '>=' 1.9.2`; then AC_MSG_RESULT([yes]) else AC_MSG_ERROR([$JS_PACKAGE >= 1.9.2 is required]) fi --- And later again for mozilla-js >= 2 . Spidermonkey (which provides js-config) is installed by untarring the firefox source and doing the following: cd js/src && ./configure --prefix=/usr && make && make install With this, libmozjs.so is installed inside /usr/lib/, and no pkg-config file is installed. The pkg-config files in the firefox source are under `xulrunner/installer`, and are only installed if xulrunner (or firefox with inbuilt libxul) is installed. Hence, js-config is the only way to query mozjs for spidermonkey. The easiest solution would be to only use xulrunner via the mozilla-js.pc pkg-config file, and use JS_LIBS JS_CFLAGS etc from there. However, if spidermonkey and xulrunner are both installed, this will cause problems with AC_CHECK_LIB([mozjs]...) since that will pick up /usr/lib/libmozjs.so . The include and library paths should always be taken from the pkg-config file, and AC_CHECK_LIB doesn't obey those paths unless we hack it like so: -AC_CHECK_LIB([mozjs], [JS_GetStringBytes], if-found,, [$JS_LIBS]) +AC_CHECK_LIB([c], [JS_GetStringBytes], if-found,, [$JS_LIBS]) That way, it will *only* use the libraries in JS_LIBS to check if that function exists. I'm going to attach a patch to support both xulrunner and spidermonkey soon, because it's very easy for users to install spidermonkey, so we should try to support that too.
Created attachment 183587 [details] [review] Drop js-config support, and cleanup old xulrunner cruft This patch removes support for js-config (and hence spidermonkey), and only uses mozilla-js.pc to get libs and include information. It also removes xulrunner-1.8/1.9/1.9.1 cruft, and adds $JS_LIBS to gjs_console_LDADD in Makefile.am so that the mozjs path provided by pkg-config is used instead of libmozjs.so (installed by spidermonkey) in the path included in GOBJECT_LIBS. This also helps with compilation failures when using a local install of xulrunner and a system-wide install of spidermonkey.
Created attachment 183589 [details] [review] Fix js-config support, and remove xulrunner-1.8/1.9/1.9.1 cruft Instead of dropping support for js-config, this patch tries to fix support for it instead. Note that I couldn't actually get it to build against spidermonkey-1.9.2.13, so I don't know how correct this is. This patch also does the same gjs_console_LDADD stuff and the same cruft-removal as the previous patch.
Hmm, sorry about not getting to these patches earlier. So in the meantime, spidermonkey 1.8.5 was released, and I reworked the configure logic a lot for it here: https://bugzilla.gnome.org/show_bug.cgi?id=646369 I think we should probably drop "js-config" script support too. Can you look at rebasing these patches atop bug 646369 after it lands?
Created attachment 187328 [details] [review] Drop js-config support, remove old xulrunner cruft, etc. Now that there's a proper pkg-config file for spidermonkey, there's no point keeping js-config support around. Hence, this patch obsoletes the two older ones. This should work with mozilla-js >=1.9.2, and mozjs185.
Review of attachment 187328 [details] [review]: This is a good patch overall. I'd like to merge this with bug 636977. Do you want to take a stab at rebasing this patch on top of current gjs git master again, and fixing my comments here, and start implmenting 636977? ::: Makefile.am @@ +122,3 @@ $(GOBJECT_CFLAGS) gjs_console_LDADD = \ + $(JS_LIBS) \ I'd like this as a separate patch. We removed it before...i need to look at the history here first. ::: configure.ac @@ +150,3 @@ +AC_CHECK_FUNC([JS_StrictPropertyStub], AC_DEFINE([HAVE_JS_STRICTPROPERTYSTUB], [1], [Define if we have JS_StrictPropertyStub]),) +AC_CHECK_FUNC([JS_GetGlobalForScopeChain], AC_DEFINE([HAVE_JS_GETGLOBALFORSCOPECHAIN], [1], [Define if we have JS_GetGlobalForScopeChain]),) +AC_CHECK_FUNC([JS_GetStringBytes], AC_DEFINE([HAVE_JS_GETSTRINGBYTES], [1], [Define if we still have JS_GetStringBytes]),) This bit is broken; I fixed it in eced173716e2b6a4f213.
(In reply to comment #5) > Review of attachment 187328 [details] [review]: > > This is a good patch overall. I'd like to merge this with bug 636977. > > Do you want to take a stab at rebasing this patch on top of current gjs git > master again, and fixing my comments here, and start implmenting 636977? > Alright, I'm on it. Do you want a single patch for both this bug and 636977 or two patches?
(In reply to comment #6) > (In reply to comment #5) > > Review of attachment 187328 [details] [review] [details]: > > > > This is a good patch overall. I'd like to merge this with bug 636977. > > > > Do you want to take a stab at rebasing this patch on top of current gjs git > > master again, and fixing my comments here, and start implmenting 636977? > > > > Alright, I'm on it. Do you want a single patch for both this bug and 636977 or > two patches? Well, there are multiple phases to 636977 like: 1) Killing off the basic support from configure.ac 2) Unwinding the compat machinery in gjs/compat.h 3) Removing calls to JS_AddRoot* etc. ... If you want to merge phase 1) with this patch here that's fine. But we should do stuff like 2) 3) etc. as separate patches.
Created attachment 187934 [details] [review] Drop js-config support, remove old xulrunner cruft, etc. Patch rebased against master, JS_LIBS hunk split out. --- (In reply to comment #7) > Well, there are multiple phases to 636977 like: > 1) Killing off the basic support from configure.ac > 2) Unwinding the compat machinery in gjs/compat.h > 3) Removing calls to JS_AddRoot* etc. > ... > I see what you mean, I forgot about all that :) > If you want to merge phase 1) with this patch here that's fine. But we should > do stuff like 2) 3) etc. as separate patches. I think it's best to commit this patch separately from the only-fx-4 patches. That's what this patch does now. I'll attach preliminary patches to 636977 in a while.
Created attachment 187935 [details] [review] pick up the JS library from pkg-config instead of the system one This patch should be useful for people who compile their own spidermonkey, but also have a distribution spidermonkey in /usr/lib*/
Review of attachment 187934 [details] [review]: Looks good. Do you have a GNOME account? ::: configure.ac @@ +130,3 @@ + if ! test -d "$FIREFOX_JS_LIBDIR"; then + FIREFOX_JS_LIBDIR= + ## The library is in the non-devel directory also. I'm wondering if we still need this - let's not drop it now though.
Review of attachment 187935 [details] [review]: So, gjs-console.c does not use any mozjs symbols. If the issue is just finding the correct libmozjs, why isn't the line: gjs_console_LDFLAGS = -R $(FIREFOX_JS_LIBDIR) -rdynamic sufficient? Is something in your build system stripping the rpath?
(In reply to comment #10) > Review of attachment 187934 [details] [review]: > > Looks good. Do you have a GNOME account? > Not at the moment no. (In reply to comment #11) > Review of attachment 187935 [details] [review]: > > So, gjs-console.c does not use any mozjs symbols. If the issue is just finding > the correct libmozjs, why isn't the line: > > gjs_console_LDFLAGS = -R $(FIREFOX_JS_LIBDIR) -rdynamic > > sufficient? Is something in your build system stripping the rpath? But that's the runtime path, right? Won't compile-time path still be autodetected unless we add the correct path? At least that's what I found in my testing.
(In reply to comment #12) > (In reply to comment #10) > > Review of attachment 187934 [details] [review] [details]: > > > > Looks good. Do you have a GNOME account? > > > > Not at the moment no. Ok, I've pushed the first patch to git master; thanks! It's a good cleanup. > But that's the runtime path, right? Won't compile-time path still be > autodetected unless we add the correct path? At least that's what I found in my > testing. Yes; but how is the compile time relevant here? I changed gjs-console explicitly to not use Spidermonkey API directly (it's a demo for the gjs.pc vs gjs-internals.pc split).
(In reply to comment #13) > > But that's the runtime path, right? Won't compile-time path still be > > autodetected unless we add the correct path? At least that's what I found in my > > testing. > > Yes; but how is the compile time relevant here? I changed gjs-console > explicitly to not use Spidermonkey API directly (it's a demo for the gjs.pc vs > gjs-internals.pc split). But gjs and hence -lmozjs are still getting passed to libtool while linking because of libgjs.la in LDADD, which causes ld to pick up libmozjs in /usr/lib/ My setup is as follows: spidermonkey-1.8.0 in /usr/lib64/, xulrunner-2 in /usr/lib64/xulrunner-2.0/ The compile log is: ------- /bin/sh ./libtool --tag=CC --mode=link gcc -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -Wnested-externs -Wmissing-prototypes -Wsign-compare -Wcast-align -Wpointer-arith -Wmissing-declarations -Wchar-subscripts -Wall -g -O2 -R /usr/lib64/xulrunner-2.0 -rdynamic -Wl,-O1,--as-needed -o gjs-console gjs_console-console.o -pthread -lgobject-2.0 -lgthread-2.0 -lrt -lglib-2.0 libgjs.la libtool: link: gcc -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -Wnested-externs -Wmissing-prototypes -Wsign-compare -Wcast-align -Wpointer-arith -Wmissing-declarations -Wchar-subscripts -Wall -g -O2 -rdynamic -Wl,-O1 -Wl,--as-needed -o .libs/gjs-console gjs_console-console.o -pthread -L/usr/lib64 ./.libs/libgjs.so -L/usr/lib64/xulrunner-devel-2.0/lib /usr/lib64/libgmodule-2.0.so -ldl /usr/lib64/libgobject-2.0.so /usr/lib64/libgthread-2.0.so /usr/lib64/libglib-2.0.so -lrt -lmozjs -lplds4 -lplc4 -lnspr4 -lpthread -pthread -Wl,-rpath -Wl,/usr/lib64/xulrunner-2.0 ./.libs/libgjs.so: undefined reference to `JS_RemoveObjectRoot' ./.libs/libgjs.so: undefined reference to `JS_StrictPropertyStub' ./.libs/libgjs.so: undefined reference to `JS_EncodeStringToBuffer' ./.libs/libgjs.so: undefined reference to `JS_AddObjectRoot' ./.libs/libgjs.so: undefined reference to `JS_GetStringEncodingLength' ./.libs/libgjs.so: undefined reference to `JS_GetGlobalForScopeChain' ./.libs/libgjs.so: undefined reference to `JS_NewCompartmentAndGlobalObject' ./.libs/libgjs.so: undefined reference to `JS_GetStringCharsAndLength' ./.libs/libgjs.so: undefined reference to `JS_NewObjectForConstructor' ./.libs/libgjs.so: undefined reference to `JS_IsScriptFrame' ./.libs/libgjs.so: undefined reference to `JS_RemoveValueRoot' ./.libs/libgjs.so: undefined reference to `JS_AddValueRoot' collect2: ld returned 1 exit status
(In reply to comment #14) > > But gjs and hence -lmozjs are still getting passed to libtool while linking > because of libgjs.la in LDADD, which causes ld to pick up libmozjs in /usr/lib/ Ah, ok, yeah libtool is screwing us over here =/ Oh well, your patch doesn't hurt.
(In reply to comment #15) > (In reply to comment #14) > > > > But gjs and hence -lmozjs are still getting passed to libtool while linking > > because of libgjs.la in LDADD, which causes ld to pick up libmozjs in /usr/lib/ > > Ah, ok, yeah libtool is screwing us over here =/ Oh well, your patch doesn't > hurt. Thanks for reviewing and committing both patches! :)