On Wednesday, November 02, 2011 4:25 PM, "Jonathan Nieder" <jrnie...@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.palh...@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.
