On Mon, 30 Jun 2025 at 1:23 AM, Daniel Sahlberg <daniel.l.sahlb...@gmail.com>
wrote:
> Den fre 27 juni 2025 kl 18:33 skrev Timofei Zhakov <t...@chemodax.net>:
>
>>
>>> Also, the searching libraries using pkg-config doesn't work on my
>>> environment. I think it is caused by the prefix passed to
>>> pkg_check_modules is wrong.
>>>
>>> [[[
>>> diff --git a/CMakeLists.txt b/CMakeLists.txt
>>> index e60809b1e..d94c30b14 100644
>>> --- a/CMakeLists.txt
>>> +++ b/CMakeLists.txt
>>> @@ -269,8 +269,8 @@ if(SVN_USE_PKG_CONFIG)
>>> # apr-1
>>> add_library(external-apr ALIAS PkgConfig::apr1)
>>>
>>> - pkg_check_modules(aprutil-1 REQUIRED IMPORTED_TARGET apr-util-1)
>>> - add_library(external-aprutil ALIAS PkgConfig::aprutil-1)
>>> + pkg_check_modules(aprutil1 REQUIRED IMPORTED_TARGET apr-util-1)
>>> + add_library(external-aprutil ALIAS PkgConfig::aprutil1)
>>> else()
>>> # apr-2
>>> pkg_check_modules(apr2 REQUIRED IMPORTED_TARGET apr-2)
>>> ]]]
>>
>>
>> The FindPkgConfig module provides a function called pkg_search_module()
>> that simplifies the code by searching through all names one by one. I
>> initially chose not to use it due to its less detailed logging. However, I
>> now believe it's reasonable to use it to support these names as well.
>>
>> --
>> Timofei Zhakov
>>
>
> I think Brane's argument for prioritising apr-2/serf-2 over apr-1/serf-1
> when available sounds reasonable.
>
+1. I agree that we should keep autoconf behavior here. Also now the aspect
that these libraries are usually available on the machine only if the
developer wants to use them (like to test against trunk versions) sounds
reasonable.
>
> Something like this?
>
> [[[
> Index: CMakeLists.txt
> ===================================================================
> --- CMakeLists.txt (revision 1926861)
> +++ CMakeLists.txt (working copy)
> @@ -263,18 +263,15 @@
> ### APR and APR-Util
>
> if(SVN_USE_PKG_CONFIG)
> - pkg_check_modules(apr1 IMPORTED_TARGET apr-1)
> + pkg_search_module(apr REQUIRED IMPORTED_TARGET apr-2 apr-1)
> + add_library(external-apr ALIAS PkgConfig::apr)
>
> - if(apr1_FOUND)
> + if(APR_VERSION VERSION_LESS 2.0.0)
> # apr-1
> - add_library(external-apr ALIAS PkgConfig::apr1)
> -
> pkg_check_modules(aprutil-1 REQUIRED IMPORTED_TARGET apr-util-1)
> add_library(external-aprutil ALIAS PkgConfig::aprutil-1)
> else()
> # apr-2
> - pkg_check_modules(apr2 REQUIRED IMPORTED_TARGET apr-2)
> - add_library(external-apr ALIAS PkgConfig::apr2)
> add_library(external-aprutil ALIAS PkgConfig::apr2)
> endif()
> else()
> @@ -375,15 +372,10 @@
> ### Serf
> if (SVN_ENABLE_RA_SERF)
> if(SVN_USE_PKG_CONFIG)
> - pkg_check_modules(serf1 IMPORTED_TARGET serf-1)
> + pkg_search_module(serf IMPORTED_TARGET serf-2 serf-1)
>
> - if(serf1_FOUND)
> - # serf-1
> - add_library(external-serf ALIAS PkgConfig::serf1)
> - else()
> - # serf-2
> - pkg_check_modules(serf2 REQUIRED IMPORTED_TARGET serf-2)
> - add_library(external-serf ALIAS PkgConfig::serf2)
> + if(serf_FOUND)
> + add_library(external-serf ALIAS PkgConfig::serf)
> endif()
> else()
> find_package(Serf REQUIRED)
> ]]]
>
> For APR:
> [[[
> Index: build/cmake/FindAPR.cmake
> ===================================================================
> --- build/cmake/FindAPR.cmake (revision 1926861)
> +++ build/cmake/FindAPR.cmake (working copy)
> @@ -23,10 +23,10 @@
> NAMES apr.h
> PATH_SUFFIXES
> include
> + include/apr-2
> + include/apr-2.0
> include/apr-1
> include/apr-1.0
> - include/apr-2
> - include/apr-2.0
> )
>
> if (APR_INCLUDE_DIR AND EXISTS ${APR_INCLUDE_DIR}/apr_version.h)
> ]]]
>
> Cheers,
> Daniel
>
Looks good to me, but I would propose separating this patch onto two
separate commits (first one with migration to pkg_search_module and the
second would prioritize apr-2 and serf-2 over apr-1 and serf-1).
Also the VERSION_LESS check of APR is beautiful. I’m surprised that it
works. I thought we’d have to do weird checks of filename or something like
that…
