See r1931194 with comments below...
Den fre 9 jan. 2026 kl 00:36 skrev Branko Čibej <[email protected]>:
> 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: 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 did not address this since I'm not too happy about my dirty hack in
CMakeLists.txt. Could someone (Timofei or Brane being our CMake experts?)
suggest a more elegant solution?
>
> - 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.
>
Removed...
>
> - 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.
>
Removed...
>
> 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.
>
Anyone else have an opinion here? If you want to run a recent Subversion
you can probably also run a recent Serf.
/Daniel
