On Mon, Jun 30, 2025 at 1:37 PM Daniel Sahlberg <daniel.l.sahlb...@gmail.com>
wrote:
> 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).
>
> Great!
>
>> 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. :-)
>
I mean, I'm stupid. It's okay.
> Cheers,
> Daniel
>
>
As I understand it, some package distributions provide APR's package config
files with names like apr1 and aprutil1 (without spaces). Since we are
already using a function that supports search for multiple names, we could
handle it simply. See the patch below.
[[[
Index: CMakeLists.txt
===================================================================
--- CMakeLists.txt (revision 1926871)
+++ CMakeLists.txt (working copy)
@@ -263,12 +263,12 @@
### APR and APR-Util
if(SVN_USE_PKG_CONFIG)
- pkg_search_module(apr REQUIRED IMPORTED_TARGET apr-2 apr-1)
+ pkg_search_module(apr REQUIRED IMPORTED_TARGET apr-2 apr2 apr-1 apr1)
add_library(external-apr ALIAS PkgConfig::apr)
if(APR_VERSION VERSION_LESS 2.0.0)
# apr-1
- pkg_check_modules(aprutil-1 REQUIRED IMPORTED_TARGET apr-util-1)
+ pkg_search_module(aprutil-1 REQUIRED IMPORTED_TARGET aprutil1)
add_library(external-aprutil ALIAS PkgConfig::aprutil-1)
else()
# apr-2
]]]
Thoughts?
@Jun Omae <jun6...@gmail.com> I think you were running into issues with
that. Am I right?
--
Timofei Zhakov
