Support passing target to ki18n_wrap_ui macro
ClosedPublic

Authored by daandemeyer on Jul 23 2019, 7:11 PM.

Details

Summary

Modern CMake encourages using targets as much as possible instead of macros. This patch makes it possible to pass a target to ki18n_wrap_ui. Instead of adding the resulting header to the list of sources, the header is added directly to the target's sources using the target_sources command which was added in CMake 3.1. Specific usage of this functionality is as a result also guarded by a call to cmake_minimum_required. The change is backwards compatible and allows projects to move over incrementally to passing targets to ki18n_wrap_ui instead of source lists.

Test Plan

Tested by running Kate's cmake scripts with the new macro with a single usage replaced by a target instead of a source list. Both the source lists usages and the target usage succeeded without errors and the resulting header was successfully added to the target's source list.

Diff Detail

Repository
R249 KI18n
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
daandemeyer created this revision.Jul 23 2019, 7:11 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald TranscriptJul 23 2019, 7:11 PM
daandemeyer requested review of this revision.Jul 23 2019, 7:11 PM
cgiboudeaux added inline comments.
cmake/KF5I18nMacros.cmake.in
72

same thing as D22699. A macro must not change the minimum required version. (ki18n already requires CMake 3.5 anyway)

daandemeyer marked an inline comment as done.

Removed the cmake_minimum_required call

turbov added a subscriber: turbov.Jul 24 2019, 6:58 PM

@daandemeyer
Cool stuff! :))

one more proposal is to turn it into a function to avoid pollution of the outer scope w/ inner used (and still unset) variables %)
Using set(${RESULT_VAR} "blah-blah" PARENT_SCOPE) could help to do that %)

turbov added inline comments.Jul 24 2019, 7:00 PM
cmake/KF5I18nMacros.cmake.in
76

Its _Modern CMake_ :) no need to use that crap anymore %)
(I'm talking about matched end*(<BLAH-BLAH>) things %)

I want to keep the changes in KDE Frameworks focused on what's necessary to clean up Kate's CMake scripts. I'll leave the general cleanup for another revision.

@alexmerry I think I added the wrong reviewer for this. I noticed your name in the git blame of k18n_wrap_ui, perhaps you know who I should add as a reviewer for ki18n revisions?

@alexmerry I think I added the wrong reviewer for this. I noticed your name in the git blame of k18n_wrap_ui, perhaps you know who I should add as a reviewer for ki18n revisions?

The maintainer is already marked as reviewer. Please also note that the reviewer is not really important: all revision of Frameworks modules are copied to the kde-frameworks-devel@ list, which is a subscriber, and which is read by people interested in the development of Frameworks.

OK, thanks for the information. I'm still getting familiar with this whole process.

ilic added a comment.Aug 3 2019, 1:25 PM

(As a maintainer) I can just say, that I'm not qualified to judge if the added behavior and implementation fits the modern CMake practices. If someone else confirms this is indeed the case, I'm happy to accept it.

To give an example of how the addition would improve KDE CMake build scripts, let's look at part of Kate's CMake build script.

It currently looks like this:

# collect the needed source files
set (KATE_LIBRARY_SRCS
   kateappadaptor.cpp
   kateapp.cpp
   kateconfigdialog.cpp
   kateconfigplugindialogpage.cpp
   katedocmanager.cpp
   katefileactions.cpp
   katemainwindow.cpp
   katepluginmanager.cpp
   kateviewmanager.cpp
   kateviewspace.cpp
   katesavemodifieddialog.cpp
   katemwmodonhddialog.cpp
   katecolorschemechooser.cpp
   katequickopenmodel.cpp
   katetabbutton.cpp
   katetabbar.cpp

   # session
   session/katesessionsaction.cpp
   session/katesessionmanager.cpp
   session/katesessionmanagedialog.cpp
   session/katesession.cpp

   katemdi.cpp
   katerunninginstanceinfo.cpp
   katequickopen.cpp
   katewaiter.h
)

ki18n_wrap_ui(KATE_LIBRARY_SRCS
    ui/sessionconfigwidget.ui
    session/katesessionmanagedialog.ui
)

add_library(kdeinit_kate STATIC ${KATE_LIBRARY_SRCS})

If the proposed change is accepted, it could be refactored to the following:

add_library(kdeinit_kate STATIC "")

ki18n_wrap_ui(kdeinit_kate
    ui/sessionconfigwidget.ui
    session/katesessionmanagedialog.ui
)

target_sources(kdeinit_kate PRIVATE
   kateappadaptor.cpp
   kateapp.cpp
   kateconfigdialog.cpp
   kateconfigplugindialogpage.cpp
   katedocmanager.cpp
   katefileactions.cpp
   katemainwindow.cpp
   katepluginmanager.cpp
   kateviewmanager.cpp
   kateviewspace.cpp
   katesavemodifieddialog.cpp
   katemwmodonhddialog.cpp
   katecolorschemechooser.cpp
   katequickopenmodel.cpp
   katetabbutton.cpp
   katetabbar.cpp

   # session
   session/katesessionsaction.cpp
   session/katesessionmanager.cpp
   session/katesessionmanagedialog.cpp
   session/katesession.cpp

   katemdi.cpp
   katerunninginstanceinfo.cpp
   katequickopen.cpp
   katewaiter.h
)
  • We don't need a SRCS variable anymore. Instead, generated files are added directly to the library target using the target_sources command.
  • The order commands are called in becomes more intuitive. We can call add_library first, follow with the ki18n_wrap_ui command and end with specifying the long list of source files instead of having to start the script with specifying all source files.
alexmerry added a comment.EditedAug 5 2019, 7:44 PM

Since you tagged me in this, I'll say that (as a former maintainer of Extra-CMake-Modules as well as someone who still makes extensive use of CMake professionally despite not doing much KDE stuff these days) the change looks good (and follows modern conventions) from a CMake perspective.

The rest of KI18N_WRAP_UI is less nice (from a modern CMake perspective), but as a minimal change to support a feature that helps write cleaner CMake code downstream, I think this is a good patch.

ilic accepted this revision.Aug 5 2019, 7:52 PM
This revision is now accepted and ready to land.Aug 5 2019, 7:52 PM
This revision was automatically updated to reflect the committed changes.