On Mon, Feb 2, 2026 at 10:49 AM Timofei Zhakov <[email protected]> wrote:
> 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. > > Committed in r1931690. -- Timofei Zhakov
