[cmake-developers] [PATCH] Wrap ARGV1 and ARGV2 in quotes to avoid errors when they are empty

[Aurélien Gâteau]

Stephen Kelly wrote:

> Aurélien Gâteau wrote:
> 
>>> Here is an updated patch.
> 
> Hi,
> 
> Without your patch with this code:
> 
>  find_dependency(Foo "")
> 
> I get:
> 
>  CMake Error at .*/Modules/CMakeFindDependencyMacro.cmake:34 (if):
>    if given arguments:
> 
>      "STREQUAL" "EXACT"
> 
>    Unknown arguments specified
>  Call Stack (most recent call first):
>    CMakeLists.txt:20 (find_dependency)
> 
> 
> and I can use that information and backtrace to find the bug in my code.
> 
> If I write this code:
> 
>  find_dependency(Foo problem)
> 
> I get
> 
>  CMake Error at .*/Modules/CMakeFindDependencyMacro.cmake:62
>  (find_package):
>    find_package called with invalid argument "problem"
>  Call Stack (most recent call first):
>    CMakeLists.txt:20 (find_dependency)

I am not sure what you are trying to show here. With the later output you 
can also backtrace to find the bug and you get a hint that grepping for 
"problem" might bring you to the error. The first output is much harder to 
make sense of if you are not used to CMake.

For the record, I wrote this patch because a developer on #kde-devel was 
confused by the error and did not know how to investigate.

> Does that case need to be handled in the find_dependency macro too? Does
> it need to validate that the version is a number?

I would say no. Especially since developers are known to be creative with 
version numbers, so this is best left to find_package().
> 
> How strongly do you feel about this? Macros are already very hard to write
> correctly, and I am concerned that making the implementation more
> complicated where not needed is likely to lead to more bugs.
> 
> For me, letting the existing APIs (like if() and find_package()) do the
> error checking is better than attempting to wrap and validate everything
> in a macro.

I agree macros should do simple checks only, but I consider checking if an 
argument is empty to be simple enough.

Aurélien