On Wednesday, November 02, 2011 4:25 PM, "Jonathan Nieder" <jrnieder_at_gmail.com> wrote:
> Hi again,
>
> Here's a hopefully saner patch. Thanks much for the quick feedback on
> the previous incarnation.
>
> I'm not very happy about putting -DSVN_SQLITE_COMPAT_VERSION in CFLAGS
> --- does subversion have a config.h somewhere?
>
http://s.apache.org/xy-problem --- Why do you think you need config.h?
Anyway: have a look at the other build/ac-macros/ files for precedents.
The makefile supports EXTRA_CFLAGS for the user to add his own CFLAGS at
'make' time.
> I can split this into three patches (one to expose
> SVN_SQLITE_VERNUM_PARSE from build/ac-macros/sqlite.m4, one
> introducing the SVN_SQLITE_MIN_VERSION_NUMBER handling in code, and
> another to wire it up into the configure script) if that's wanted.
> Let me know.
>
Will do. Thanks.
> Thanks a lot,
> Jonathan
>
> [[[
> If libsvn is built against one version of sqlite3 and then run using
> an older version, currently svn will error out:
>
> svn: Couldn't perform atomic initialization
> svn: SQLite compiled for 3.7.4, but running with 3.7.3
>
> Not all sqlite3 updates include ABI changes that are relevant to
> Subversion, though. This can be annoying when building on a modern
> system in order to deploy on a less modern one.
>
> This patch introduces a new --enable-sqlite-compatibility-version=X.Y.Z
The above line should be the first line of the log message: describe
the change first and the historical reasons for it either later or in
code comments.
> option for ./configure to allow people building Subversion to
> specify how old the system being deployed on might be, to address
> that. Setting the compatibility version to an older version turns
> on code in subversion that works around infelicities in those
> older versions and relaxes the version check at initialization
> time.
>
> If the compat version is set to a version before the minimum supported
> version (3.6.18), the build will fail early so the person building can
> adjust her expectations.
>
> The default for the compat version is the currently installed version,
> so there should be no change in behavior for users not passing this
> option to the configure script.
>
Sounds sane.
> * subversion/include/private/svn_dep_compat.h (SQLITE_VERSION_AT_LEAST):
> Check SVN_SQLITE_MIN_VERSION_NUMBER instead of SQLITE_VERSION_NUMBER.
>
> * subversion/libsvn_subr/sqlite.c
> (SVN_SQLITE_MIN_VERSION_NUMBER): Set to SQLITE_VERSION_NUMBER if
> undefined.
> (init_sqlite): Check sqlite version against SVN_SQLITE_MIN_VERSION_NUMBER
> instead of SQLITE_VERSION_NUMBER.
>
> * configure.ac: Provide a --enable-sqlite-compatibility-version switch
> that sets SVN_SQLITE_MIN_VERSION_NUMBER.
>
> * build/ac-macros/sqlite.m4
> (SVN_SQLITE_VERNUM_PARSE): Make it reusable (in particular for
> configure.ac), by taking a version string and a variable to store the
> corresponding version number as arguments.
> (SVN_SQLITE_MIN_VERNUM_PARSE): Simplify by calling
> SVN_SQLITE_VERNUM_PARSE.
> (SVN_SQLITE_PKG_CONFIG): Adapt SVN_SQLITE_VERNUM_PARSE call to the new
> calling convention.
>
> Found by: Joao Palhoto Matos <joao.palhoto_at_gmail.com>
> http://bugs.debian.org/608925
> ]]]
>
> Index: subversion/include/private/svn_dep_compat.h
> ===================================================================
> --- subversion/include/private/svn_dep_compat.h (revision 1196775)
> +++ subversion/include/private/svn_dep_compat.h (working copy)
> @@ -120,7 +120,7 @@ typedef apr_uint32_t apr_uintptr_t;
> */
> #ifndef SQLITE_VERSION_AT_LEAST
> #define SQLITE_VERSION_AT_LEAST(major,minor,patch) \
> -((major*1000000 + minor*1000 + patch) <= SQLITE_VERSION_NUMBER)
> +((major*1000000 + minor*1000 + patch) <= SVN_SQLITE_MIN_VERSION_NUMBER)
> #endif /* SQLITE_VERSION_AT_LEAST */
>
What if SVN_SQLITE_MIN_VERSION_NUMBER isn't #define'd yet? Below you
have an #ifndef guard, so presumably you need one here too.
> #ifdef __cplusplus
> Index: subversion/libsvn_subr/sqlite.c
> ===================================================================
> --- subversion/libsvn_subr/sqlite.c (revision 1196775)
> +++ subversion/libsvn_subr/sqlite.c (working copy)
> @@ -50,6 +50,10 @@
> #include <sqlite3.h>
> #endif
>
> +#ifndef SVN_SQLITE_MIN_VERSION_NUMBER
> + #define SVN_SQLITE_MIN_VERSION_NUMBER SQLITE_VERSION_NUMBER
> +#endif
> +
> #if !SQLITE_VERSION_AT_LEAST(3,6,18)
> #error SQLite is too old -- version 3.6.18 is the minimum required version
> #endif
> @@ -606,7 +610,7 @@ static volatile svn_atomic_t sqlite_init_state = 0
> static svn_error_t *
> init_sqlite(void *baton, apr_pool_t *pool)
> {
> - if (sqlite3_libversion_number() < SQLITE_VERSION_NUMBER)
> + if (sqlite3_libversion_number() < SVN_SQLITE_MIN_VERSION_NUMBER)
I don't have a working copy handy. Are there instance of SQLITE_VERSION_NUMBER
that _didn't_ get changed to SVN_SQLITE_MIN_VERSION_NUMBER?
> {
> return svn_error_createf(
> SVN_ERR_SQLITE_ERROR, NULL,
> Index: configure.ac
> ===================================================================
> --- configure.ac (revision 1196775)
> +++ configure.ac (working copy)
> @@ -172,6 +172,17 @@ SQLITE_URL="http://www.sqlite.org/sqlite-amalgamat
> SVN_LIB_SQLITE(${SQLITE_MINIMUM_VER}, ${SQLITE_RECOMMENDED_VER},
> ${SQLITE_URL})
>
> +AC_ARG_ENABLE(sqlite-compatibility-version,
> + AS_HELP_STRING([--enable-sqlite-compatibility-version=X.Y.Z],
> + [Allow binary to run against older SQLite]),
> + [sqlite_compat_ver=$enableval],[sqlite_compat_ver=no])
> +
> +if test -n "$sqlite_compat_ver" && test "$sqlite_compat_ver" != no; then
> + SVN_SQLITE_VERNUM_PARSE([$sqlite_compat_ver],
> + [sqlite_compat_ver_num])
> + CFLAGS="-DSVN_SQLITE_MIN_VERSION_NUMBER=$sqlite_compat_ver_num $CFLAGS"
> +fi
> +
Don't you have the SQLite version number here too? (The one we're building
against or linking to.) If so, you could simplify the semantics of
SVN_SQLITE_MIN_VERSION_NUMBER by declaring that configure will _always_
define it (to the installed version if --enable-sqlite-compatibility-version was supplied).
(I'm assuming this should indeed be --enable rather than --with, but not
sure enough about this to not mention it here for people to disagree if
I'm wrong.)
> dnl Set up a number of directories ---------------------
>
> dnl Create SVN_BINDIR for proper substitution
> Index: build/ac-macros/sqlite.m4
> ===================================================================
> --- build/ac-macros/sqlite.m4 (revision 1196775)
> +++ build/ac-macros/sqlite.m4 (working copy)
> @@ -106,7 +106,7 @@ AC_DEFUN(SVN_SQLITE_PKG_CONFIG,
> sqlite_version=`$PKG_CONFIG $SQLITE_PKGNAME --modversion --silence-errors`
>
> if test -n "$sqlite_version"; then
> - SVN_SQLITE_VERNUM_PARSE
> + SVN_SQLITE_VERNUM_PARSE([$sqlite_version], [sqlite_ver_num])
>
> if test "$sqlite_ver_num" -ge "$sqlite_min_ver_num"; then
> AC_MSG_RESULT([$sqlite_version])
> @@ -198,20 +198,22 @@ SQLITE_VERSION_OKAY
> fi
> ])
>
> -dnl SVN_SQLITE_VERNUM_PARSE()
> +dnl SVN_SQLITE_VERNUM_PARSE(version_string, result_var)
> dnl
> -dnl Parse a x.y[.z] version string sqlite_version into a number sqlite_ver_num.
> +dnl Parse a x.y[.z] version string version_string into a number result_var.
> AC_DEFUN(SVN_SQLITE_VERNUM_PARSE,
> [
> - sqlite_major=`expr $sqlite_version : '\([[0-9]]*\)'`
> - sqlite_minor=`expr $sqlite_version : '[[0-9]]*\.\([[0-9]]*\)'`
> - sqlite_micro=`expr $sqlite_version : '[[0-9]]*\.[[0-9]]*\.\([[0-9]]*\)'`
> - if test -z "$sqlite_micro"; then
> - sqlite_micro=0
> + version_string="$1"
> +
> + major=`expr $version_string : '\([[0-9]]*\)'`
> + minor=`expr $version_string : '[[0-9]]*\.\([[0-9]]*\)'`
> + micro=`expr $version_string : '[[0-9]]*\.[[0-9]]*\.\([[0-9]]*\)'`
> + if test -z "$micro"; then
> + micro=0
> fi
> - sqlite_ver_num=`expr $sqlite_major \* 1000000 \
> - \+ $sqlite_minor \* 1000 \
> - \+ $sqlite_micro`
> + $2=`expr $major \* 1000000 \
> + \+ $minor \* 1000 \
> + \+ $micro`
Assignment to $2 directly strikes me as bad style, can we avoid it?
> ])
>
> dnl SVN_SQLITE_MIN_VERNUM_PARSE()
> @@ -220,12 +222,7 @@ dnl Parse a x.y.z version string SQLITE_MINIMUM_VE
> dnl sqlite_min_ver_num.
> AC_DEFUN(SVN_SQLITE_MIN_VERNUM_PARSE,
> [
> - sqlite_min_major=`expr $SQLITE_MINIMUM_VER : '\([[0-9]]*\)'`
> - sqlite_min_minor=`expr $SQLITE_MINIMUM_VER : '[[0-9]]*\.\([[0-9]]*\)'`
> - sqlite_min_micro=`expr $SQLITE_MINIMUM_VER : '[[0-9]]*\.[[0-9]]*\.\([[0-9]]*\)'`
> - sqlite_min_ver_num=`expr $sqlite_min_major \* 1000000 \
> - \+ $sqlite_min_minor \* 1000 \
> - \+ $sqlite_min_micro`
> + SVN_SQLITE_VERNUM_PARSE([$SQLITE_MINIMUM_VER], [sqlite_min_ver_num])
> ])
>
> dnl SVN_DOWNLOAD_SQLITE()
>
Overall looks good, though I'd want to audit the uses of
SQLITE_VERSION_NUMBER in our code prior to applying it.
Received on 2011-11-03 10:48:03 CET