D23552: ECM: remove set_package_properties from FindCanberra
fsitter added a comment. You think sending your minions to insult me and then disabling my account will solve the issue? what will you do next? Disable registration so no one points out your hypocrisy? Which overlord made the decision to disable my account and for what? REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D23552 To: dfaure, cgiboudeaux, sitter Cc: fsitter, kossebau, kde-buildsystem, kde-frameworks-devel, apol, aacid, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns
D23552: ECM: remove set_package_properties from FindCanberra
dfaure abandoned this revision. dfaure added a comment. Ah, I didn't realize there was no warning if calling set_package_properties with different properties. Good idea. Done in https://commits.kde.org/knotifications/06f5a4a6f8622913cb9cecd89b98cf762a489823 REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D23552 To: dfaure, cgiboudeaux, sitter Cc: kossebau, kde-buildsystem, kde-frameworks-devel, apol, aacid, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns
D23552: ECM: remove set_package_properties from FindCanberra
cgiboudeaux added a comment. I'd do the opposite, remove `DESCRIPTION` and `URL` from knotification's CMakeLists.txt and only leave the `PURPOSE` line. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D23552 To: dfaure, cgiboudeaux, sitter Cc: kossebau, kde-buildsystem, kde-frameworks-devel, apol, aacid, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns
D23552: ECM: remove set_package_properties from FindCanberra
kossebau added a comment. Compare though all the > Ideally this is set already directly in the Find-module. for `DESCRIPTION` and `URL` properties in the docs https://cmake.org/cmake/help/latest/module/FeatureSummary.html Which makes sense to me, at least for FindModules, where one can assume the FindX.cmake file exists, so the properties are set, and one only has to add the values for the properties `PURPOSE` and `TYPE`, as their are usage dependent. So in this case perhaps rather KNotifications should be adapted. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D23552 To: dfaure, cgiboudeaux, sitter Cc: kossebau, kde-buildsystem, kde-frameworks-devel, apol, aacid, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns
D23552: ECM: remove set_package_properties from FindCanberra
sitter added a comment. No objections from me. This was explicitly suggested during review though, so I'd like @cgiboudeaux to approve this. INLINE COMMENTS > FindCanberra.cmake:95 > > include(FeatureSummary) > Can be removed as well. REPOSITORY R240 Extra CMake Modules REVISION DETAIL https://phabricator.kde.org/D23552 To: dfaure, cgiboudeaux, sitter Cc: kde-buildsystem, kde-frameworks-devel, apol, aacid, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns
D23552: ECM: remove set_package_properties from FindCanberra
dfaure created this revision. dfaure added reviewers: cgiboudeaux, sitter. Herald added projects: Frameworks, Build System. dfaure requested review of this revision. REVISION SUMMARY The user of this find module has to do it, so that they can specify the purpose of using canberra in their application. We can't do it twice, it leads to - Warning: Property DESCRIPTION for package Canberra already set to "Event sound library", overriding it with "Library for generating event sounds" The documentation for FeatureSummary always does find_package and then set_package_properties, i.e. it was never meant for the finder to do this. TEST PLAN knotifications no longer warns REPOSITORY R240 Extra CMake Modules BRANCH master REVISION DETAIL https://phabricator.kde.org/D23552 AFFECTED FILES find-modules/FindCanberra.cmake To: dfaure, cgiboudeaux, sitter Cc: kde-buildsystem, kde-frameworks-devel, apol, aacid, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns
