D18952: new find module for Canberra

[Harald Sitter]

This revision was automatically updated to reflect the committed changes.
Closed by commit R240:50e8dd7b2d00: new find module for Canberra (authored by 
sitter).

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18952?vs=52042=52043

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

AFFECTED FILES
  docs/find-module/FindCanberra.rst
  find-modules/FindCanberra.cmake

To: sitter, cgiboudeaux
Cc: aacid, apol, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18952: new find module for Canberra

[Harald Sitter]

sitter marked an inline comment as done.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

To: sitter, cgiboudeaux
Cc: aacid, apol, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18952: new find module for Canberra

[Harald Sitter]

sitter updated this revision to Diff 52042.
sitter added a comment.


  explicitly set FOUND_VAR so it is camelcase too

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18952?vs=51976=52042

BRANCH
  master

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

AFFECTED FILES
  docs/find-module/FindCanberra.rst
  find-modules/FindCanberra.cmake

To: sitter, cgiboudeaux
Cc: aacid, apol, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18952: new find module for Canberra

[Christophe Giboudeaux]

cgiboudeaux accepted this revision.
cgiboudeaux added a comment.
This revision is now accepted and ready to land.


  Thanks! just a little thing to fix before pushing

INLINE COMMENTS

> FindCanberra.cmake:75
> +find_package_handle_standard_args(Canberra
> +REQUIRED_VARS
> +Canberra_LIBRARIES

Also add

  FOUND_VAR Canberra_FOUND

The default for CMake 2.8.12 which ECM requires is '_FOUND'.

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

To: sitter, cgiboudeaux
Cc: aacid, apol, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18952: new find module for Canberra

[Harald Sitter]

sitter updated this revision to Diff 51976.
sitter added a comment.


  fix bad copy paste in compat setup

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18952?vs=51952=51976

BRANCH
  master

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

AFFECTED FILES
  docs/find-module/FindCanberra.rst
  find-modules/FindCanberra.cmake

To: sitter, cgiboudeaux
Cc: aacid, apol, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18952: new find module for Canberra

[Harald Sitter]

sitter marked 5 inline comments as done.

REPOSITORY
  R240 Extra CMake Modules

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

To: sitter, cgiboudeaux
Cc: aacid, apol, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18952: new find module for Canberra

[Harald Sitter]

sitter updated this revision to Diff 51952.
sitter added a comment.


  - pkgconfig is now quiet
  - variables are now camelcase
- old variables are still set for compat
  - new imported target (also sets pkgconfig's cflags, which I presume is the 
sane thing to do)
  - set package description url & description

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18952?vs=51576=51952

BRANCH
  master

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

AFFECTED FILES
  docs/find-module/FindCanberra.rst
  find-modules/FindCanberra.cmake

To: sitter, cgiboudeaux
Cc: aacid, apol, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18952: new find module for Canberra

[Christophe Giboudeaux]

cgiboudeaux added a comment.


  In D18952#413454 , @aacid wrote:
  
  > Yes, imported targets are the future/present :)
  >
  > Canberra::Canberra sounds good to me as target name
  
  
  +1
  You may also add a set_package_properties() call. see eg: D18947 
. This way, the users only have to use the 
VERSION and TYPE in their CMakeLists.txt.

INLINE COMMENTS

> FindCanberra.cmake:9-16
> +# ``CANBERRA_FOUND``
> +# True if (the requested version of) Canberra is available
> +# ``CANBERRA_VERSION``
> +# The version of Canberra
> +# ``CANBERRA_LIBRARIES``
> +# The libraries of Canberra for use with target_link_libraries()
> +# ``CANBERRA_INCLUDE_DIRS``

The variables shall be renamed "Canberra_XXX".

If needed, the uppercase ones shall be added at the bottom of the file with a 
comment to indicate they only exist for compatibility.

> FindCanberra.cmake:48-49
> +
> +find_package(PkgConfig)
> +pkg_check_modules(PC_CANBERRA libcanberra)
> +

Use the 'QUIET' keyword for both lines

> FindCanberra.cmake:53
> +NAMES canberra
> +HINTS ${PC_CANBERRA_LIBRARY_DIRS} ${PC_CANBERRA_LIBDIR}
> +)

PC_CANBERRA_LIBDIR isn't needed

> FindCanberra.cmake:58
> +NAMES canberra.h
> +HINTS ${PC_CANBERRA_INCLUDE_DIRS} ${PC_CANBERRA_INCLUDEDIR}
> +)

Same thing for PC_CANBERRA_INCLUDEDIR

REPOSITORY
  R240 Extra CMake Modules

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

To: sitter, cgiboudeaux
Cc: aacid, apol, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18952: new find module for Canberra

[Albert Astals Cid]

aacid added a comment.


  Yes, imported targets are the future/present :)
  
  Canberra::Canberra sounds good to me as target name

REPOSITORY
  R240 Extra CMake Modules

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

To: sitter, cgiboudeaux
Cc: aacid, apol, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18952: new find module for Canberra

[Harald Sitter]

sitter added a comment.


  Good point, I've also noticed that (all?) our finders now also create an 
IMPORTED target. Should we maybe add that too? If so I guess 
`Canberra::Canberra` would be the preferred target name?

REPOSITORY
  R240 Extra CMake Modules

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

To: sitter, cgiboudeaux
Cc: apol, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18952: new find module for Canberra

[Harald Sitter]

sitter updated this revision to Diff 51576.
sitter added a comment.


  document in rst syntax

REPOSITORY
  R240 Extra CMake Modules

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18952?vs=51491=51576

BRANCH
  master

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

AFFECTED FILES
  docs/find-module/FindCanberra.rst
  find-modules/FindCanberra.cmake

To: sitter, cgiboudeaux
Cc: apol, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18952: new find module for Canberra

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> FindCanberra.cmake:4
> +#
> +#  CANBERRA_FOUND- true if libcanberra was found
> +#  CANBERRA_LIBRARIES- libcanberra libraries to link against

Maybe you can follow the style (rst syntax?) that Volker used here D18944 
.

REPOSITORY
  R240 Extra CMake Modules

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

To: sitter, cgiboudeaux
Cc: apol, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18952: new find module for Canberra

[Luca Beltrame]

lbeltrame added a reviewer: cgiboudeaux.

REPOSITORY
  R240 Extra CMake Modules

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

To: sitter, cgiboudeaux
Cc: kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns

D18952: new find module for Canberra

[Harald Sitter]

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

REVISION SUMMARY
  used by:
  
  - knotification
  - (possibly also knotifyconfig at some point)
  - plasma-pa
  - kmix

REPOSITORY
  R240 Extra CMake Modules

BRANCH
  master

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

AFFECTED FILES
  docs/find-module/FindCanberra.rst
  find-modules/FindCanberra.cmake

To: sitter
Cc: kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, bruns