Same as https://phabricator.kde.org/D22698 for ecm_qt_declare_logging_category.
Unit Tests Skipped
How about pass it as a TARGET argument using CMakeParseArguments.
You can see documentation about it here: https://cmake.org/cmake/help/latest/command/cmake_parse_arguments.html
+1 to the change, I really dislike using variables for this.
I'm not sure if its beneficial to use cmake_parse_arguments here. It seems like it would make the macro harder to understand compared to the current change. I'm also not entirely sure how it would work? I haven't worked with cmake_parse_arguments before.
One thing that might cause issues with the current change is that passing a target that doesn't exist will not fail but instead just create a new list variable (although that's also the case without the change since you can pass wrong list variables to the macro and it wouldn't notice).
What's the alternative for extra-cmake-modules? Can I just remove the cmake_minimum_required call or do I need to update the version elsewhere?
I found that the CMake script of extra-cmake-modules itself specifies 2.8.12 as the minimum so I'm assuming that's the minimum version for the modules as well. The advantage of specifying cmake_minimum_required inline here is that cmake-extra-modules will continue to work with versions of CMake older than 3.1 and give a better error message when the new functionality is used with an older CMake version. Of course, if its not a problem to update the minimum required version to 3.1 that's the preferred solution compared to specifying cmake_minimum_required inline.
Personally, I don't see the harm in adding a completely backwards compatible optional feature that requires a higher CMake version (that's already considered very old) but of course it's up to you guys to make the call.
If it is not accepted, can I just leave the revision open until it can be merged for KF6?