On Fri, Jan 9, 2026 at 3:47 PM Daniel Sahlberg <[email protected]> wrote:
> 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? > > It's not as dirty of a hack though. It's not a hack at all IMO. The VERSION_LESS statement is the suggested way to.. well compare versions in cmake. But it's also important to keep in mind that here serf can be found using either pkgconfig or cmake module which means that they both might export package versions into different variables. In the case of pkgconfig it's <YYY>_VERSION [1] (where YYY is <prefix>_<module-name> -> serf_serf-1, assuming that I correctly understood the documentation (?) ). Were you testing with pkgconfig? If it worked, I guess we could stick with serf_VERSION here. Because I don't think the variable will be serf_serf-1_VERSION. It's just weird. On the other hand, cmake will export it as Serf_VERSION [2]. I would suggest adding checks that collect the correct version into a variable with canonical name. Below is a patch of what I'm talking about. [[[ Index: CMakeLists.txt =================================================================== --- CMakeLists.txt (revision 1931402) +++ CMakeLists.txt (working copy) @@ -378,11 +378,18 @@ if (SVN_ENABLE_RA_SERF) if(serf_FOUND) add_library(external-serf ALIAS PkgConfig::serf) + set(SVN_SERF_VERSION serf_VERSION) + # TODO: figure out how pkg_search_module() actually exports version endif() else() find_package(Serf REQUIRED) add_library(external-serf ALIAS Serf::Serf) + set(SVN_SERF_VERSION Serf_VERSION) endif() + + if(SVN_SERF_VERSION VERSION_LESS 1.3.4) + message(FATAL_ERROR "Serf 1.3.4 or later required, found ${SVN_SERF_VERSION}") + endif() endif() ]]] I just realised that when we're attempting to find a serf via pkgconfig but it can't be found, the configuration won't fail right here to missing dependency, but probably fail to external-serf not existing later on. I would suggest the following fix (that also simplifies it a little bit): [[[ @@ -374,11 +374,8 @@ endif() ### Serf if (SVN_ENABLE_RA_SERF) if(SVN_USE_PKG_CONFIG) - pkg_search_module(serf IMPORTED_TARGET serf-2 serf-1) - - if(serf_FOUND) - add_library(external-serf ALIAS PkgConfig::serf) - endif() + pkg_search_module(serf IMPORTED_TARGET REQUIRED serf-2 serf-1) + add_library(external-serf ALIAS PkgConfig::serf) else() find_package(Serf REQUIRED) add_library(external-serf ALIAS Serf::Serf) ]]] [1] https://cmake.org/cmake/help/latest/module/FindPkgConfig.html [2] https://svn.apache.org/viewvc/subversion/trunk/build/cmake/FindSerf.cmake?revision=1930447&view=markup#l58 > > >> >> - 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 > > -- Timofei Zhakov
