Hi,
You are right of course. Thanks for the review.
I've committed the following:
* 1923121: Timofei's patch
* 1923122: My find_package_handle_standard_args() fixes
I've also nominated for backport to 1.4.x in r1923123.
Cheers,
Daniel
Den tis 14 jan. 2025 kl 08:47 skrev Timofei Zhakov <t...@chemodax.net>:
> Hi Daniel,
>
> I didn’t notice this problem before…
>
> The resolution looks good to me.
>
> However, the fix seems to be a separate change, which reproduced after the
> version changed. I think that it’s better to commit it separately, before
> we change the version, since this was incorrect before, but the policy that
> checks for it didn’t exist in the version 3.0.
>
> What do you think?
>
> Timofei Zhakov
>
>
> On Sun, 12 Jan 2025 at 9:24 PM, Daniel Sahlberg <
> daniel.l.sahlb...@gmail.com> wrote:
>
>> Den fre 10 jan. 2025 kl 21:31 skrev Timofei Zhakov <t...@chemodax.net>:
>>
>>> Hi,
>>>
>>> Thanks for reviewing the idea.
>>>
>>> As we're agreed, I've made the patch for this and attached it to the
>>> email as 'serf-cmake-update-version.patch.txt'. Actually, it required
>>> making similar modifications in the other files, so APR and APRUtil modules
>>> will use the same version. The logmessage is also provided below.
>>>
>>> [[[
>>> Require CMake of the version 3.12 or later instead of 3.0.
>>>
>>> * CMakeLists.txt
>>> (cmake_minimum_required): Change the version.
>>> (CMP0074): Don't set this policy, since this is set by default in the
>>> new minimum required version.
>>> * build/FindAPR.cmake,
>>> build/FindAPRUtil.cmake
>>> (cmake_minimum_required): Change the version.
>>>
>>> Patch by: rinrab
>>> ]]]
>>>
>>
>> Thanks for the patch!
>>
>> I tried (under Ubuntu) it but got a warning (below). Reverting the patch
>> I see that is isn't related to these changes, but maybe we should fix them
>> at the same time?
>>
>> [[[
>> -- Found APR: -L/usr/lib/x86_64-linux-gnu;-lapr-1 (found version "1.7.2")
>> CMake Warning (dev) at
>> /usr/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:438
>> (message):
>> The package name passed to `find_package_handle_standard_args` (APRUTIL)
>> does not match the name of the calling package (APRUtil). This can
>> lead to
>> problems in calling code that expects `find_package` result variables
>> (e.g., `_FOUND`) to follow a certain pattern.
>> Call Stack (most recent call first):
>> build/FindAPRUtil.cmake:152 (find_package_handle_standard_args)
>> CMakeLists.txt:204 (find_package)
>> This warning is for project developers. Use -Wno-dev to suppress it.
>>
>> -- Found APRUTIL: -L/usr/lib/x86_64-linux-gnu;-laprutil-1 (found version
>> "1.6.3")
>> ]]]
>>
>> Changing the patch as follows removes the warning:
>> [[[
>> Index: build/FindAPRUtil.cmake
>> ===================================================================
>> --- build/FindAPRUtil.cmake (revision 1923094)
>> +++ build/FindAPRUtil.cmake (working copy)
>> @@ -17,7 +17,7 @@
>> # under the License.
>> # ===================================================================
>>
>> -cmake_minimum_required(VERSION 3.0)
>> +cmake_minimum_required(VERSION 3.12)
>>
>> #.rst:
>> # FindAPRUtil
>> @@ -67,7 +67,7 @@
>>
>> set(APRUTIL_VERSION ${APR_VERSION})
>> include(FindPackageHandleStandardArgs)
>> - find_package_handle_standard_args(APRUTIL
>> + find_package_handle_standard_args(APRUtil
>> REQUIRED_VARS APRUTIL_VERSION
>> VERSION_VAR APRUTIL_VERSION)
>>
>> @@ -149,7 +149,7 @@
>> endif() # NOT Windows
>>
>> include(FindPackageHandleStandardArgs)
>> - find_package_handle_standard_args(APRUTIL
>> + find_package_handle_standard_args(APRUtil
>> REQUIRED_VARS APRUTIL_LIBRARIES
>> APRUTIL_INCLUDES
>> VERSION_VAR APRUTIL_VERSION)
>> ]]]
>>
>> Does this seem alright? Is it OK if I commit it as "Patch by: rinrab
>> (tweaked by me)" (and with an appropriate update to the log message)?
>>
>> Cheers,
>> Daniel
>>
>>
