Support passing target to ecm_qt_declare_logging_category
AbandonedPublic

Authored by daandemeyer on Jul 23 2019, 8:27 PM.

Details

Reviewers
alexmerry
Summary

Same as https://phabricator.kde.org/D22698 for ecm_qt_declare_logging_category.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
daandemeyer created this revision.Jul 23 2019, 8:27 PM
Restricted Application added a project: Build System. · View Herald TranscriptJul 23 2019, 8:27 PM
Restricted Application added subscribers: kde-buildsystem, kde-frameworks-devel. · View Herald Transcript
daandemeyer requested review of this revision.Jul 23 2019, 8:27 PM
apol added a subscriber: apol.Jul 23 2019, 9:31 PM

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.

daandemeyer added a comment.EditedJul 23 2019, 9:51 PM

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

cgiboudeaux added inline comments.
modules/ECMQtDeclareLoggingCategory.cmake
133

-1 for changing the minimum CMake version in a macro. The change must be compatible the project requirement.

cgiboudeaux added inline comments.Jul 24 2019, 7:45 AM
modules/ECMQtDeclareLoggingCategory.cmake
133

must be compatible with the project..

daandemeyer added inline comments.Jul 24 2019, 11:18 AM
modules/ECMQtDeclareLoggingCategory.cmake
133

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?

133

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.

cgiboudeaux added inline comments.Jul 24 2019, 11:35 AM
modules/ECMQtDeclareLoggingCategory.cmake
133

If the change cannot be made compatible with the minimum ECM version, I suggest waiting for KF6.

I

modules/ECMQtDeclareLoggingCategory.cmake
133

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?

Update CMake minimum required version to 3.1 (for target_sources).

Accidentally uploaded the wrong diff.

cgiboudeaux set the repository for this revision to R240 Extra CMake Modules.Aug 14 2019, 1:42 PM
daandemeyer abandoned this revision.Aug 22 2019, 7:53 AM

KF6 is a long time out so I'm abandoning this.