On Sun, Feb 1, 2026 at 12:17 AM Timofei Zhakov <[email protected]> wrote:
> > > On Sat, Jan 31, 2026 at 9:44 PM Daniel Sahlberg < > [email protected]> wrote: > >> Den lör 31 jan. 2026 kl 20:39 skrev Timofei Zhakov <[email protected]>: >> >>> 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]. >>> >> >> Yes, I have tested with pkgconfig and it sets serf_VERSION. >> >> Good catch regarding the cmake find_package! >> >> >>> 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() >>> ]]] >>> >>> >> +1. Do you want to commit? >> > > Committed in r1931636. > > (it wasn't shown in the notification, but I updated the log message to > mark you as "suggested by" instead of original "reviewed by".) > > >> >> >>> 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 >> > > Committed in r1931635. > > -- > Timofei Zhakov > I lied. There is a better way to check versions. As it turns out, both find_package() and pkg_search_module() have this functionality out-of-the-box. Here is what I would propose: [[[ Index: CMakeLists.txt =================================================================== --- CMakeLists.txt (revision 1931642) +++ CMakeLists.txt (working copy) @@ -377,18 +377,12 @@ endif() ### Serf if (SVN_ENABLE_RA_SERF) if(SVN_USE_PKG_CONFIG) - pkg_search_module(serf IMPORTED_TARGET REQUIRED serf-2 serf-1) + pkg_search_module(serf IMPORTED_TARGET REQUIRED serf-2 serf-1>=1.3.4) add_library(external-serf ALIAS PkgConfig::serf) - set(DETECTED_SERF_VERSION "${serf_VERSION}") else() - find_package(Serf REQUIRED) + find_package(Serf REQUIRED 1.3.4) add_library(external-serf ALIAS Serf::Serf) - set(DETECTED_SERF_VERSION "${Serf_VERSION}") endif() - - if("${DETECTED_SERF_VERSION}" VERSION_LESS 1.3.4) - message(FATAL_ERROR "Serf 1.3.4 or later required, found ${DETECTED_SERF_VERSION}") - endif() endif() if (SVN_CHECKSUM_BACKEND STREQUAL "apr") ]]] I'm assuming that serf-2 with a lower version is impossible. -- Timofei Zhakov
