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
