On 8. 1. 26 22:11, Daniel Sahlberg wrote:
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
<http://configure.ac>: Raise required serf version to 1.3.4 (aka break
the buildbots)").
Guessing from the commit logs, it was a general removal of support for
old dependencies. It wasn't just Serf, it was also APR/-Util (we dropped
support for 0.9 and made 1.3 the new baseline).
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...
That should be thrown out. Anything that looks like compatibility for
Serf 0.x should have been ripped out ages ago.
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.
There's no "think" about this. The CMake build must impose the same
requirements as the autotools build.
- 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...).
I agree.
- 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.
I agree.
While you're at it, we may as well require Serf 1.3.10. That release is
almost three years old now, and it included some important fixes.
-- Brane
