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? > 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 Thanks for reviewing! /Daniel > > [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 >
