On Friday, November 04, 2011 1:24 AM, "Jonathan Nieder" <jrnieder_at_gmail.com> wrote:
> Daniel Shahaf wrote:
> > On Wednesday, November 02, 2011 4:25 PM, "Jonathan Nieder" <jrnieder_at_gmail.com> wrote:
>
> >> 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?
>
> Sorry, that was cryptic of me. The -DSVN_SQLITE_COMPAT_VERSION looked
> lonely on the command line not surrounded by settings like -DHAVE_INT
> and -DHAVE_PRINTF --- I was afraid there might be some other idiom I
> was missing.
>
> I'm happy keeping it in CFLAGS.
>
There is the svn_config_private.h idiom. There is also the
${target}_CFLAGS Makefile variable, generated by the build/*/*.py
infrastructure.
I don't feel that throwing it in CFLAGS is intuitively wrong, in our
build system.
> Your other comments looked reasonable, too. Details:
>
> - there were no unpatched instances of SQLITE_VERSION_NUMBER, but
> there was one unpatched instance of SQLITE_VERSION. So now I do
>
> -DSVN_SQLITE_MIN_VERSION_NUMBER=1002003
> -DSVN_SQLITE_MIN_VERSION="1.2.3"
>
Okay.
> - it would be simple to make the configure script take care of the
> default for SVN_SQLITE_MIN_VERSION_NUMBER instead of the header
> doing so, but I prefer the latter since it means you can use
> "gcc -c" directly to build without remembering the -D
> option and still have a chance of the result working. So I've
> left that alone for now.
>
+1
> - assignment to $2 in sqlite.m4 could be replaced by
>
> ver_num=`expr ...`
> $2=$ver_num
>
> or:
>
> result_var="$2"
> ...
> eval "$result_var=\$ver_num"
>
> Left untouched for now.
>
What I actually had in mind was:
f() {
echo foo
}
bar=`f`
That's primarily because I'm not used to the $1=* idiom (in its various
forms as you suggest above). If it's a standard idiom for configure
script writing I wouldn't feel strongly to change it.
> [[[
> Introduce a --enable-sqlite-compatibility-version=X.Y.Z option for
> ./configure to allow people building Subversion to specify how old the
> system being deployed on might be. Setting the compatibility version
> to an older version turns on code in subversion that works around
> infelicities in older versions and relaxes the version check ("SQLite
> compiled for 3.7.4, but running with 3.7.3") 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.
>
Looks good.
> 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.
>
I'll read/review the rest later.
Received on 2011-11-04 07:53:02 CET