D21695: Add FindTaglib.cmake

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  Certainly a good idea to have this in ECM, so that this mess can be sorted out once and for all...

INLINE COMMENTS

> FindTaglib.cmake:56
> +# The minimum version of Taglib we require
> +if(NOT Taglib_FIND_VERSION)
> +    set(TAGLIB_FIND_VERSION "1.4")

Here it says Taglib camelcase...

> FindTaglib.cmake:57
> +if(NOT Taglib_FIND_VERSION)
> +    set(TAGLIB_FIND_VERSION "1.4")
> +endif()

And here it says TAGLIB uppercase. This can't be right, one of those needs to be adjusted.

> FindTaglib.cmake:80
> +    set(Taglib_CFLAGS ${TC_Taglib_CFLAGS})
> +    string(REGEX REPLACE " *-I" ";" Taglib_INCLUDE_DIRS "${Taglib_CFLAGS}")
> +endif()

This will set the include dir to <PREFIX/include/taglib....

> FindTaglib.cmake:84
> +find_path(Taglib_INCLUDE_DIRS
> +    NAMES tag.h taglib/tag.h
> +    HINTS ${TC_Taglib_INCLUDE_DIRS}

(.... this searches for it both ways, so it's unclear what the include dir will be --- with or without taglib at the end?)

> FindTaglib.cmake:94
> +if (Taglib_INCLUDE_DIRS AND NOT Taglib_VERSION)
> +    if(EXISTS "Taglib_INCLUDE_DIRS}/taglib/taglib.h")
> +        file(READ "${Taglib_INCLUDE_DIRS}/taglib/taglib.h" TAGLIB_H)

... and this then adds another /taglib/ to the path.

I've always had that problem with taglib. It's not clear to me if the C/C++ code should use #include <tag.h> or #include <taglib/tag.h> and therefore it's not clear whether the include path should contain the /taglib subdir or not.

> FindTaglib.cmake:129
> +        IMPORTED_LOCATION "${Taglib_LIBRARIES}"
> +        INTERFACE_COMPILE_OPTIONS "${Taglib_CFLAGS}"
> +        INTERFACE_INCLUDE_DIRECTORIES "${Taglib_INCLUDE_DIRS}"

Why set both the CFLAGS and the include dir? That means having the -I twice in there.

Line 80 assumes that the cflags will never contain other flags than -I.
If that's true, then using CFLAGS here is redundant.

If it's not true, then line 80 is wrong...

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D21695

To: heikobecker, kde-buildsystem, kde-frameworks-devel, dfaure
Cc: dfaure, LeGast00n, bencreasy, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-buildsystem/attachments/20190610/2fedc28b/attachment.html>