D21695: Add FindTaglib.cmake

[Elvis Angelaccio]

elvisangelaccio commandeered this revision.
elvisangelaccio added a reviewer: heikobecker.
elvisangelaccio added a comment.


  In D21695#675056 , 
@elvisangelaccio wrote:
  
  > New attempt at 
https://invent.kde.org/frameworks/extra-cmake-modules/-/merge_requests/6
  
  
  Merged, closing.

REPOSITORY
  R240 Extra CMake Modules

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

To: elvisangelaccio, kde-buildsystem, kde-frameworks-devel, dfaure, heikobecker
Cc: asturmlechner, ngraham, elvisangelaccio, cgiboudeaux, dfaure, LeGast00n, 
cblack, bencreasy, michaelh, bruns

D21695: Add FindTaglib.cmake

[Elvis Angelaccio]

elvisangelaccio abandoned this revision.

REPOSITORY
  R240 Extra CMake Modules

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

To: elvisangelaccio, kde-buildsystem, kde-frameworks-devel, dfaure, heikobecker
Cc: asturmlechner, ngraham, elvisangelaccio, cgiboudeaux, dfaure, LeGast00n, 
cblack, bencreasy, michaelh, bruns

D21695: Add FindTaglib.cmake

[Elvis Angelaccio]

elvisangelaccio added a comment.


  New attempt at 
https://invent.kde.org/frameworks/extra-cmake-modules/-/merge_requests/6

REPOSITORY
  R240 Extra CMake Modules

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

To: heikobecker, kde-buildsystem, kde-frameworks-devel, dfaure
Cc: asturmlechner, ngraham, elvisangelaccio, cgiboudeaux, dfaure, LeGast00n, 
cblack, bencreasy, michaelh, bruns

D21695: Add FindTaglib.cmake

[Elvis Angelaccio]

elvisangelaccio added a comment.


  @heikobecker are you still interested in this patch? I can take over 
otherwise.

REPOSITORY
  R240 Extra CMake Modules

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

To: heikobecker, kde-buildsystem, kde-frameworks-devel, dfaure
Cc: ngraham, elvisangelaccio, cgiboudeaux, dfaure, LeGast00n, GB_2, bencreasy, 
michaelh, bruns

D21695: Add FindTaglib.cmake

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> dfaure wrote in FindTaglib.cmake:84
> ( this searches for it both ways, so it's unclear what the include dir 
> will be --- with or without taglib at the end?)

BTW since then it was determined that applications should use , so you 
should NOT search for taglib/tag.h

> dfaure wrote in FindTaglib.cmake:94
> ... 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  or #include  and therefore 
> it's not clear whether the include path should contain the /taglib subdir or 
> not.

In light of the above, this should be ${Taglib_INCLUDE_DIRS}/taglib.h

(though it's weird to use a plural form variable in such a way; there should 
probably be a singular form variable in this file, and then the plural one 
exported for users)

REPOSITORY
  R240 Extra CMake Modules

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

To: heikobecker, kde-buildsystem, kde-frameworks-devel, dfaure
Cc: ngraham, elvisangelaccio, cgiboudeaux, dfaure, LeGast00n, GB_2, bencreasy, 
michaelh, bruns

D21695: Add FindTaglib.cmake

[Nathaniel Graham]

ngraham added a comment.


  Can we move forward on this?

REPOSITORY
  R240 Extra CMake Modules

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

To: heikobecker, kde-buildsystem, kde-frameworks-devel, dfaure
Cc: ngraham, elvisangelaccio, cgiboudeaux, dfaure, LeGast00n, GB_2, bencreasy, 
michaelh, bruns

D21695: Add FindTaglib.cmake

[Nathaniel Graham]

ngraham added a comment.


  @heikobecker ping!

REPOSITORY
  R240 Extra CMake Modules

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

To: heikobecker, kde-buildsystem, kde-frameworks-devel, dfaure
Cc: ngraham, elvisangelaccio, cgiboudeaux, dfaure, LeGast00n, GB_2, bencreasy, 
michaelh, bruns

D21695: Add FindTaglib.cmake

[Elvis Angelaccio]

elvisangelaccio added a comment.


  Where does this FindTaglib.cmake come from?
  
  We should probably use the one from kio-extras, which got a bunch of fixes in 
the last months.

REPOSITORY
  R240 Extra CMake Modules

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

To: heikobecker, kde-buildsystem, kde-frameworks-devel, dfaure
Cc: elvisangelaccio, cgiboudeaux, dfaure, LeGast00n, bencreasy, michaelh, 
ngraham, bruns

D21695: Add FindTaglib.cmake

[Elvis Angelaccio]

elvisangelaccio added a comment.


  In D21695#476826 , @heikobecker 
wrote:
  
  > I'm not entirely sure about taglib-config on Windows
  
  
  See 
https://cgit.kde.org/kio-extras.git/commit/cmake/FindTaglib.cmake?id=548f525f4308810888c85f42a570139029c40618

REPOSITORY
  R240 Extra CMake Modules

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

To: heikobecker, kde-buildsystem, kde-frameworks-devel, dfaure
Cc: elvisangelaccio, cgiboudeaux, dfaure, LeGast00n, bencreasy, michaelh, 
ngraham, bruns

D21695: Add FindTaglib.cmake

[Christophe Giboudeaux]

cgiboudeaux added a comment.


  attic/modules/FindTaglib.cmake shall be deleted.

REPOSITORY
  R240 Extra CMake Modules

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

To: heikobecker, kde-buildsystem, kde-frameworks-devel, dfaure
Cc: cgiboudeaux, dfaure, LeGast00n, bencreasy, michaelh, ngraham, bruns

D21695: Add FindTaglib.cmake

[Christophe Giboudeaux]

cgiboudeaux added inline comments.

INLINE COMMENTS

> FindTaglib.cmake:19
> +#
> +# If ``Taglib_Found is TRUE, it will also define the following imported
> +# target:

If ``Taglib_FOUND`` ...

> FindTaglib.cmake:22
> +#
> +# ``Taglib::Talib``
> +#   The Taglib library

Typo

REPOSITORY
  R240 Extra CMake Modules

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

To: heikobecker, kde-buildsystem, kde-frameworks-devel, dfaure
Cc: cgiboudeaux, dfaure, LeGast00n, bencreasy, michaelh, ngraham, bruns

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  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  or #include  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

D21695: Add FindTaglib.cmake

[Heiko Becker]

heikobecker added a comment.


  I'm not entirely sure about taglib-config on Windows and Android (can't test 
there), but similar to pkg-config I omitted the special casing. Tried to test 
this by moving taglib-config out of the way on Linux and a taglib install in 
default locations, which worked fine.

REPOSITORY
  R240 Extra CMake Modules

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

To: heikobecker, kde-buildsystem, kde-frameworks-devel
Cc: LeGast00n, bencreasy, michaelh, ngraham, bruns

D21695: Add FindTaglib.cmake

[Heiko Becker]

heikobecker created this revision.
heikobecker added reviewers: kde-buildsystem, kde-frameworks-devel.
Herald added projects: Frameworks, Build System.
heikobecker requested review of this revision.

REVISION SUMMARY
  The old one from KDELibs4 times is used by several projects from
  Frameworks, KDE Applications and Extragear.
  Modernized according the current cmake coding style but it also
  keeps compatibility with the old find module for now and should be
  a drop-in replacement.

TEST PLAN
  Removed the bundled FindTaglib.cmake from kfilemetadata, kio-extras,
  juk, amarok and rename and built them successfully.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

AFFECTED FILES
  find-modules/FindTaglib.cmake

To: heikobecker, kde-buildsystem, kde-frameworks-devel
Cc: LeGast00n, bencreasy, michaelh, ngraham, bruns