Hi,
While reviewing Subversion's usage of Serf I stumbled across a few things I
don't quite understand.
When building using autoconf, configure seems to require Serf 1.3.4 or
later:
[[[
configure: WARNING: Serf version too old: need 1.3.4
checking was serf enabled... no
An appropriate version of serf could not be found, so libsvn_ra_serf
will not be built. If you want to build libsvn_ra_serf, please
install serf 1.3.4 or newer.
]]]
I don't see a similar version check in CMake build. I did a very simple
check like this:
[[[
+ if(serf_VERSION VERSION_LESS 1.3.4)
+ message(FATAL_ERROR "Serf 1.3.4 or later required, found
${serf_VERSION}")
+ endif()
]]]
But I'm sure there are more CMake-ish ways of doing it.
When searching some more through the source, I found the following check in
ra_serf.h:
[[[
/* Enforce the minimum version of serf. */
#if !SERF_VERSION_AT_LEAST(1, 2, 1)
#error Please update your version of serf to at least 1.2.1.
#endif
]]]
It seems Subversion started requiring Serf 1.2.1 in r1489114 due to issues
#4274 and #4371, however I can't really find a good reason why this was
later bumped to 1.3.4 in r1572253 ("configure.ac: Raise required serf
version to 1.3.4 (aka break the buildbots)").
Lastly, I saw that include/private/svn_dep_compat.h check
if SERF_VERSION_AT_LEAST isn't defined (with a comment "/* Introduced in
Serf 0.1.1 */) and if so defines it same as later Serfs. However the define
isn't enough to build Subversion since ra_serf.h don't include
svn_dep_compat.h...
To summarise, I propose a few changes to remove some inconsistencies and
unnecessary code:
- I think CMake should check for a minimum Serf version just as the
autoconf buildsystem does. I could commit my suggestion above but I'm sure
someone else can do it more elegantly.
- I'm proposing to remove the minimum version check from ra_serf.h, since
we already check it in the build system. Better to have the check at one
place than two. Obviously it was forgotten during r1572253 and currently it
has no effect (well, in the CMake build it does...).
- Can we delete the definition of SERF_VERSION_AT_LEAST from
svn_dep_compat.h? Since it is in the private subfolder I assume it isn't
part of the public API and since we require at least 1.3.4 it sould be
defined in all supported versions of Serf.
Cheers,
Daniel
