Add FindTaglib.cmake
AbandonedPublic

Authored by elvisangelaccio on Jun 9 2019, 3:34 PM.

Details

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.

Diff Detail

Repository
R240 Extra CMake Modules
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12604
Build 12622: arc lint + arc unit
heikobecker created this revision.Jun 9 2019, 3:34 PM
Restricted Application added projects: Frameworks, Build System. · View Herald TranscriptJun 9 2019, 3:34 PM
heikobecker requested review of this revision.Jun 9 2019, 3:34 PM

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.

dfaure requested changes to this revision.Jun 10 2019, 11:04 AM
dfaure added a subscriber: dfaure.

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

find-modules/FindTaglib.cmake
56

Here it says Taglib camelcase...

57

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

80

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

84

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

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 <tag.h> or #include <taglib/tag.h> and therefore it's not clear whether the include path should contain the /taglib subdir or not.

129

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...

This revision now requires changes to proceed.Jun 10 2019, 11:04 AM
krop added a subscriber: krop.Jun 10 2019, 11:41 AM
krop added inline comments.
find-modules/FindTaglib.cmake
19

If `Taglib_FOUND` ...

22

Typo

krop added a comment.Jun 10 2019, 11:44 AM

attic/modules/FindTaglib.cmake shall be deleted.

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.

Can we move forward on this?

dfaure added inline comments.Sep 13 2019, 1:01 PM
find-modules/FindTaglib.cmake
84

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

94

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)

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

elvisangelaccio abandoned this revision.Jun 21 2020, 9:17 PM