Den mån 30 juni 2025 kl 08:46 skrev Timofei Zhakov <t...@chemodax.net>:
> 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).
>
Thanks for the review! I've committed this as r1926870 (pkg_search_module)
r1926871 (prioritize 2 over 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…
>
>
I borrowed that code from the best, see r1926351. :-)
Cheers,
Daniel
