incubator-couchdb-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benoit Chesneau <bchesn...@gmail.com>
Subject Re: git commit: fix build with custom path - close #COUCHDB-1426
Date Thu, 01 Mar 2012 21:07:44 GMT
On Thu, Mar 1, 2012 at 7:14 PM, Paul Davis <paul.joseph.davis@gmail.com> wrote:
> A few hunks in here concern me:
>
> -    CPPFLAGS="$CPPFLAGS -I/opt/local/include/js"
>    CPPFLAGS="$CPPFLAGS -I/usr/local/include"
> -    CPPFLAGS="$CPPFLAGS -I/usr/local/include/js"
>    CPPFLAGS="$CPPFLAGS -I/usr/include"
> -    CPPFLAGS="$CPPFLAGS -I/usr/include/js"
>
> I remember having to add some of these for extremely specific
> scenarios. Are we absolutely sure this isn't going to lead to a
> regression for some people?

It should be handled in the js tests part not here.


>
> +AC_MSG_RESULT(js flags $CPPFLAGS)
> +
>
> This syntax looks weird to me. Its probably right but does anyone know
> the rules on when strings need square brackets?
>
> +AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
> +                   #include <jsconfig.h>
> +                   ]],
> +                   [[
> +                    #if JS_VERSION == 170
> +                    // Use spidermonkey 1.7.0
> +                    #else
> +                    # Not Spidermonkey 1.7.0
> +                    #endif
> +                    ]])],
> +                    [use_js170=yes],
> +                    [])
>
> Please don't invent a new indentation scheme for random hunks in a
> patch. Also, what's actually being tested here? I'm not seeing how
> this would fail unless we're depending on jsconfig.h only existing in
> 1.7.0 builds, in which case a test compile is not the way to run this
> check.
>

I don't invent a new indentation scheme actually. I just use the
defult provided by vim.

What it test is getting the version ==  170. If it's not equal it
fails. It actually works as expected.

> Everything after that hunk looks quite weird to me. I'm fairly certain
> we just had a commit that fixes this issue. I don't remember the
> details off the top of my head but this is getting into the land of
> silly walks with all the bending over backwards to find SpiderMonkey.
> At a certain point we might want to just ixnay a lot of this and just
> guess at the most common cases and if we can't find a version then we
> should just add a configure option.


The thing is that the way versions are tested is actually wrong. We
don't assume there could be more than one version in the patch we are
looking and e don't assume that what we want actually is an inferior
version of mozilla 185.

Reproducing the steps I gave allows to reproduce that.


>
> On Wed, Feb 29, 2012 at 6:01 PM,  <jhs@apache.org> wrote:
>> Updated Branches:
>>  refs/heads/COUCHDB-1426 [created] 56a00ede8
>>
>>
>> fix build with custom path - close #COUCHDB-1426
>>
>> This patch make sure that js libs and include given using the options
>> --with-js-lib and --with-js-include are used in priority, ie before the
>> detection of the version. Also if spidermonkey 1.7.0 is detected it
>> removes useless tests.
>>
>>
>> Project: http://git-wip-us.apache.org/repos/asf/couchdb/repo
>> Commit: http://git-wip-us.apache.org/repos/asf/couchdb/commit/56a00ede
>> Tree: http://git-wip-us.apache.org/repos/asf/couchdb/tree/56a00ede
>> Diff: http://git-wip-us.apache.org/repos/asf/couchdb/diff/56a00ede
>>
>> Branch: refs/heads/COUCHDB-1426
>> Commit: 56a00ede8ab237fc3b2c1a3ea23a688e5aa997c0
>> Parents: 766d461
>> Author: benoitc <benoitc@apache.org>
>> Authored: Thu Mar 1 00:16:14 2012 +0100
>> Committer: Jason Smith (air) <jhs@apache.org>
>> Committed: Wed Feb 29 23:59:55 2012 +0000
>>
>> ----------------------------------------------------------------------
>>  configure.ac |  128 +++++++++++++++++++++++++++++++++--------------------
>>  1 files changed, 80 insertions(+), 48 deletions(-)
>> ----------------------------------------------------------------------
>>
>>
>> http://git-wip-us.apache.org/repos/asf/couchdb/blob/56a00ede/configure.ac
>> ----------------------------------------------------------------------
>> diff --git a/configure.ac b/configure.ac
>> index 7ce4842..e526a18 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -137,13 +137,33 @@ AC_ARG_WITH([erlang], [AC_HELP_STRING([--with-erlang=PATH],
>>  ])
>>  AC_SUBST(ERLANG_FLAGS)
>>
>> -PKG_CHECK_MODULES([JS], [mozjs185], [
>> +AC_ARG_WITH([js-lib], [AC_HELP_STRING([--with-js-lib=PATH],
>> +            [set PATH to the SpiderMonkey library directory])],
>> +    [
>> +    JS_LIB_DIR=$withval
>> +    JS_LIBS="-L$JS_LIB_DIR"
>> +    AC_MSG_RESULT(yo $JS_LIBS)
>> +], [
>> +    PKG_CHECK_MODULES([JS], [mozjs185], [
>>     JS_LIB_DIR="$(${PKG_CONFIG} --variable=libdir mozjs185)"
>> +    ], [
>> +        PKG_CHECK_MODULES([JS], [mozilla-js >= 1.7], [
>> +            JS_LIB_DIR="$(${PKG_CONFIG} --variable=sdkdir mozilla-js)/lib"
>> +        ], [
>> +            JS_LIB_DIR="${libdir}"
>> +        ])
>> +    ])
>> +])
>> +
>> +
>> +AC_ARG_WITH([js-include], [AC_HELP_STRING([--with-js-include=PATH],
>> +    [set PATH to the SpiderMonkey include directory])], [
>> +    JS_INCLUDE="$withval"
>> +    JS_CFLAGS="-I$JS_INCLUDE"
>>  ], [
>>     PKG_CHECK_MODULES([JS], [mozilla-js >= 1.7], [
>> -        JS_LIB_DIR="$(${PKG_CONFIG} --variable=sdkdir mozilla-js)/lib"
>> +        JS_CFLAGS="$(${PKG_CONFIG} --variable=includedir mozilla-js)/lib"
>>     ], [
>> -        JS_LIB_DIR="${libdir}"
>>         JS_CFLAGS="-I/usr/include"
>>         JS_CFLAGS="$JS_CFLAGS -I/usr/include/js"
>>         JS_CFLAGS="$JS_CFLAGS -I/usr/include/mozjs"
>> @@ -152,19 +172,6 @@ PKG_CHECK_MODULES([JS], [mozjs185], [
>>     ])
>>  ])
>>
>> -AC_ARG_WITH([js-include], [AC_HELP_STRING([--with-js-include=PATH],
>> -    [set PATH to the SpiderMonkey include directory])], [
>> -    JS_INCLUDE="$withval"
>> -    JS_CFLAGS="-I$JS_INCLUDE"
>> -], [])
>> -
>> -AC_ARG_WITH([js-lib], [AC_HELP_STRING([--with-js-lib=PATH],
>> -    [set PATH to the SpiderMonkey library directory])],
>> -    [
>> -    JS_LIB_DIR=$withval
>> -    JS_LIBS="-L$withval"
>> -], [])
>> -
>>  use_js_trunk=no
>>  AC_ARG_ENABLE([js-trunk], [AC_HELP_STRING([--enable-js-trunk],
>>     [allow use of SpiderMonkey versions newer than js185-1.0.0])], [
>> @@ -177,11 +184,8 @@ AS_CASE([$(uname -s)],
>>     [CYGWIN*], [] ,
>>     [*], [
>>     CPPFLAGS="$CPPFLAGS -I/opt/local/include"
>> -    CPPFLAGS="$CPPFLAGS -I/opt/local/include/js"
>>     CPPFLAGS="$CPPFLAGS -I/usr/local/include"
>> -    CPPFLAGS="$CPPFLAGS -I/usr/local/include/js"
>>     CPPFLAGS="$CPPFLAGS -I/usr/include"
>> -    CPPFLAGS="$CPPFLAGS -I/usr/include/js"
>>     LDFLAGS="$LDFLAGS -L/opt/local/lib"
>>     LDFLAGS="$LDFLAGS -L/usr/local/lib"
>>  ])
>> @@ -211,6 +215,8 @@ LIBS="$JS_LIBS $LIBS"
>>  OLD_CPPFLAGS="$CPPFLAGS"
>>  CPPFLAGS="$JS_CFLAGS $CPPFLAGS"
>>
>> +AC_MSG_RESULT(js flags $CPPFLAGS)
>> +
>>  AC_CHECK_HEADER([jsapi.h], [], [
>>     AC_CHECK_HEADER([js/jsapi.h],
>>         [
>> @@ -222,39 +228,65 @@ AC_CHECK_HEADER([jsapi.h], [], [
>>  Are the Mozilla SpiderMonkey headers installed?])
>>         ])])
>>
>> -AC_CHECK_LIB([mozjs185], [JS_NewContext], [JS_LIB_BASE=mozjs185], [
>> -    AC_CHECK_LIB([mozjs185-1.0], [JS_NewContext], [JS_LIB_BASE=mozjs185-1.0],
[
>> -        AC_CHECK_LIB([mozjs], [JS_NewContext], [JS_LIB_BASE=mozjs], [
>> -            AC_CHECK_LIB([js], [JS_NewContext], [JS_LIB_BASE=js], [
>> -                AC_CHECK_LIB([js3250], [JS_NewContext], [JS_LIB_BASE=js3250],
[
>> -                    AC_CHECK_LIB([js32], [JS_NewContext], [JS_LIB_BASE=js32],
[
>> -                        AC_MSG_ERROR([Could not find the js library.
>> -
>> -Is the Mozilla SpiderMonkey library installed?])])])])])])])
>> -
>> -# Figure out what version of SpiderMonkey to use
>> -
>> -AC_CHECK_LIB([$JS_LIB_BASE], [JS_NewCompartmentAndGlobalObject],
>> -    # Prevent people from accidentally using SpiderMonkey's that are too new
>> +use_js170=no
>> +AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
>> +                   #include <jsconfig.h>
>> +                   ]],
>> +                   [[
>> +                    #if JS_VERSION == 170
>> +                    // Use spidermonkey 1.7.0
>> +                    #else
>> +                    # Not Spidermonkey 1.7.0
>> +                    #endif
>> +                    ]])],
>> +                    [use_js170=yes],
>> +                    [])
>> +
>> +if test "$use_js170" = "no"; then
>> +    AC_CHECK_LIB([mozjs185], [JS_NewContext], [JS_LIB_BASE=mozjs185], [
>> +        AC_CHECK_LIB([mozjs185-1.0], [JS_NewContext], [JS_LIB_BASE=mozjs185-1.0],
[
>> +            AC_CHECK_LIB([mozjs], [JS_NewContext], [JS_LIB_BASE=mozjs], [
>> +                AC_CHECK_LIB([js], [JS_NewContext], [JS_LIB_BASE=js], [
>> +                    AC_CHECK_LIB([js3250], [JS_NewContext], [JS_LIB_BASE=js3250],
[
>> +                        AC_CHECK_LIB([js32], [JS_NewContext], [JS_LIB_BASE=js32],
[
>> +                            AC_MSG_ERROR([Could not find the js library.
>> +
>> +    Is the Mozilla SpiderMonkey library installed?])])])])])])])
>> +
>> +    # Figure out what version of SpiderMonkey to use
>> +
>> +    AC_CHECK_LIB([$JS_LIB_BASE], [JS_NewCompartmentAndGlobalObject],
>> +        # Prevent people from accidentally using SpiderMonkey's that are too
new
>> +
>> +        if test "$use_js_trunk" = "no"; then
>> +            AC_CHECK_DECL([JSOPTION_ANONFUNFIX], [], [
>> +                AC_MSG_ERROR([Your SpiderMonkey library is too new.
>> +
>> +    Versions of SpiderMonkey after the js185-1.0.0 release remove the optional
>> +    enforcement of preventing anonymous functions in a statement context. This
>> +    will most likely break your existing JavaScript code as well as render all
>> +    example code invalid.
>> +
>> +    If you wish to ignore this error pass --enable-js-trunk to ./configure.])],
>> +            [[#include <jsapi.h>]])
>> +        fi
>> +        AC_DEFINE([SM185], [1],
>> +            [Use SpiderMonkey 1.8.5]))
>>
>> -    if test "$use_js_trunk" = "no"; then
>> -        AC_CHECK_DECL([JSOPTION_ANONFUNFIX], [], [
>> -            AC_MSG_ERROR([Your SpiderMonkey library is too new.
>> +    AC_CHECK_LIB([$JS_LIB_BASE], [JS_ThrowStopIteration],
>> +        AC_DEFINE([SM180], [1],
>> +            [Use SpiderMonkey 1.8.0]))
>>
>> -Versions of SpiderMonkey after the js185-1.0.0 release remove the optional
>> -enforcement of preventing anonymous functions in a statement context. This
>> -will most likely break your existing JavaScript code as well as render all
>> -example code invalid.
>> +else
>>
>> -If you wish to ignore this error pass --enable-js-trunk to ./configure.])],
>> -        [[#include <jsapi.h>]])
>> -    fi
>> -    AC_DEFINE([SM185], [1],
>> -        [Use SpiderMonkey 1.8.5]))
>> +    AC_CHECK_LIB([mozjs], [JS_NewContext], [JS_LIB_BASE=mozjs], [
>> +                AC_CHECK_LIB([js], [JS_NewContext], [JS_LIB_BASE=js], [
>> +                    AC_CHECK_LIB([js3250], [JS_NewContext], [JS_LIB_BASE=js3250],
[
>> +                        AC_CHECK_LIB([js32], [JS_NewContext], [JS_LIB_BASE=js32],
[
>> +                            AC_MSG_ERROR([Could not find the js library.
>>
>> -AC_CHECK_LIB([$JS_LIB_BASE], [JS_ThrowStopIteration],
>> -    AC_DEFINE([SM180], [1],
>> -        [Use SpiderMonkey 1.8.0]))
>> +    Is the Mozilla SpiderMonkey library installed?])])])])])
>> +fi
>>
>>  AC_CHECK_LIB([$JS_LIB_BASE], [JS_GetStringCharsAndLength],
>>     AC_DEFINE([HAVE_JS_GET_STRING_CHARS_AND_LENGTH], [1],
>>

Mime
View raw message