Clean up Kate addons CMake scripts.
ClosedPublic

Authored by daandemeyer on Jul 24 2019, 5:14 PM.

Details

Summary

This is the first in a series of revisions modernizing the Kate CMake scripts.
I'm submitting the changes to the addons directory first as they form a
nicely separated list of changes.

Note that if https://phabricator.kde.org/D22698 and
https://phabricator.kde.org/D22699 are merged, most if not all of the
remaining source lists in the addons directory can be removed in favor of
working with targets.

List of changes made:

  • Standardize style on two spaces and no whitespace between commands and arguments.
  • Remove directory commands in favor of target-based commands as recommended by modern CMake.
  • Remove usage of qt5_add_resources in favor of CMAKE_AUTORCC and adding .qrc files directly to a target's sources.
  • Remove usage of source lists (where possible) in favor of CMake 3.1's target_sources command.
  • Remove the CMake binary directory as an include directory from most addons.
  • Remove unnecessary comments, CMake project calls, config files, etc from addon CMake scripts.
  • Remove HAVE_CTERMID from config.h and move the check and addition to compile definitions to the project addon CMake script as its only used in the project plugin sources.
Test Plan

Apply changes and verify Kate still builds and works exactly as before.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
daandemeyer created this revision.Jul 24 2019, 5:14 PM
Restricted Application added a subscriber: kwrite-devel. · View Herald TranscriptJul 24 2019, 5:14 PM
daandemeyer requested review of this revision.Jul 24 2019, 5:14 PM
daandemeyer edited the summary of this revision. (Show Details)
daandemeyer edited the summary of this revision. (Show Details)
asturmlechner added inline comments.
addons/CMakeLists.txt
1–12 ↗(On Diff #62596)

It would be a good opportunity to keep them sorted.

For maintainer to decide: Although I rather have optional dependencies be found all separately as it does not lead to KF5 be set to not found if one of them is being disabled or not found (and other packages had build failures caused by that).

daandemeyer added inline comments.Jul 24 2019, 5:32 PM
addons/CMakeLists.txt
1–12 ↗(On Diff #62596)

Doesn't the KF5 find module take care of that? I found this comment in its documentation:

If all the required components (those given in the COMPONENTS argument, but not those given in the OPTIONAL_COMPONENTS argument) are found, `KF5_FOUND`will be set to true. Otherwise, it will be set to false.

turbov added a subscriber: turbov.Jul 24 2019, 5:42 PM
turbov added inline comments.
CMakeLists.txt
67 ↗(On Diff #62596)

No need to use CMAKE_CURRENT_BINARY_DIR here -- it's the default value.

addons/backtracebrowser/CMakeLists.txt
16 ↗(On Diff #62596)

Raw string literals probably could help to avoid awkward escapes:

target_compile_definitions(
    katebacktracebrowserplugin PRIVATE 
    [[TRANSLATION_DOMAIN="katebacktracebrowserplugin"]]
)

need to test...

22 ↗(On Diff #62596)

IMHO this formatting style is much clean:

target_link_libraries(
  katebacktracebrowserplugin 
  PRIVATE
    KF5::TextEditor
    KF5::I18n
)
  • add the indentation after named keywords with multiple arguments
  • leave the open parentheses alone
daandemeyer added inline comments.Jul 24 2019, 5:45 PM
addons/backtracebrowser/CMakeLists.txt
16 ↗(On Diff #62596)

I'm actually not sure if the escapes are necessary anymore when using target_compile_definitions. I'll do some tests.

22 ↗(On Diff #62596)

Agreed, I'll update to this style.

  • Switched to new recommended style for multi-line commands.
  • Removed escaping from TRANSLATION_DOMAIN definitions as this is done automatically by CMake.
  • Added a few missing PRIVATE's and removed some extra whitespace where needed.
daandemeyer marked 5 inline comments as done.Jul 24 2019, 6:12 PM

This may be a stupid question but is the TRANSLATION_DOMAIN stuff actually used somewhere? I'm not familiar with how it works. However, I find it strange that its not defined for kwrite and kate but is defined for all plugins.

turbov added a comment.EditedJul 24 2019, 6:34 PM

Modern CMake has a recommendation to avoid variables when working w/ targets (google "Variables are fragile").
With a small modification to ki18n_wrap_ui (yeah, another project), it could be possible to replace this:

set(katebacktracebrowserplugin_PART_SRCS
  katebacktracebrowser.cpp
  btparser.cpp
  btfileindexer.cpp
  btdatabase.cpp
)

set(katebacktracebrowserplugin_PART_UI
  btbrowserwidget.ui
  btconfigwidget.ui
)

ki18n_wrap_ui(katebacktracebrowserplugin_PART_SRCS ${katebacktracebrowserplugin_PART_UI})


add_library(katebacktracebrowserplugin MODULE ${katebacktracebrowserplugin_PART_SRCS})

with this:

add_library(
  katebacktracebrowserplugin MODULE 
  katebacktracebrowser.cpp
  btparser.cpp
  btfileindexer.cpp
  btdatabase.cpp
)

ki18n_wrap_ui(
  katebacktracebrowserplugin # NOTE passing the target name!
  btbrowserwidget.ui
  btconfigwidget.ui
)

the main point is to avoid variables (some of them just one time used) and "make the target declaration the only source of truth" %)

ki18n_wrap_ui should learn to accept a target name as the first argument and call target_sources(<tgt>...) to update target sources directly instead of storing 'em to a variable. For the backward compatibility, it's possible to check the first argument with if(TARGET...) and do the direct update or fallback to the old behavior (assuming that was a variable name to update) on else()...

If you pending some other CMake related patches and have some spare time please add this feature too %)))

asturmlechner added inline comments.Jul 24 2019, 6:38 PM
addons/CMakeLists.txt
1–12 ↗(On Diff #62596)

Yes, it seems to work fine now that I tried. It may have been a bug with an older cmake version.

kossebau added a subscriber: kossebau.EditedJul 24 2019, 6:46 PM

This may be a stupid question but is the TRANSLATION_DOMAIN stuff actually used somewhere? I'm not familiar with how it works. However, I find it strange that its not defined for kwrite and kate but is defined for all plugins.

Short story: yes, it is used, silently by all the *i18n* calls which take this definition as first argument (hidden behind a macro that any *i18n* calls is). For kate & kwrite you do not see that, as there is a default catalog defined per application, and both kate & kwrite set that using KLocalizedString::setApplicationDomain("fooapp");.And for libs & plugins TRANSLATION_DOMAIN allows to overwrite the default catalog in that macro-magic way in each translation look-up call, to make sure their own catalog as defined by the domain is used.

If you want to get more familiar, please see https://api.kde.org/frameworks/ki18n/html/prg_guide.html#link_cat

daandemeyer marked 3 inline comments as done.Jul 24 2019, 6:50 PM

@turbov This is exactly what I've already proposed, See https://phabricator.kde.org/D22698. I've also proposed similar patches for ecm_add_app_icon and ecm_qt_declare_logging_category (https://phabricator.kde.org/D22709, https://phabricator.kde.org/D22699)

@kossebau Thanks for the explanation.

I think the overall direction of these changes is good, some cleanup and modernization seems in order.
@turbov provides useful hints to improve, once you reached a fixed-point, just ping me again, I see no reason to not merge this then.

@cullmann I think this revision is good to merge now. I was thinking of making a second revision with the changes to the global, kwrite and kate CMake scripts to keep things a bit more manageable. However, those changes depend on this revision so ideally this revision would be merged before I make the second one. Does that sound good to you or should I add the other changes to this revision as well?

Is there also a tutorial on building plugins somewhere that needs to be updated in tandem with these changes?

cullmann accepted this revision.Jul 25 2019, 8:27 AM

There is no real tutorial, normally one just copies a small plugin as template ;=)
I am fine with merging this.

This revision is now accepted and ready to land.Jul 25 2019, 8:27 AM
This revision was automatically updated to reflect the committed changes.
cullmann reopened this revision.Jul 26 2019, 11:44 AM
This revision is now accepted and ready to land.Jul 26 2019, 11:44 AM

My bad, I forgot to replace a source list variable with the empty string.
This does not cause errors on the latest CMake versions because it is now
allowed to pass no sources to an add_executable call so I didn't have any
problems locally. The issue is solved by replacing ${ProjectPluginSrc} in
addons/project/autotests/CMakeLists.txt by the empty string ("")

Do I need to make a revision for this on phabricator or it is faster to fix
it locally?

Regards,

Daan

Replace leftover source list variable with an empty string.

The new revision should fix the compile error

Hmm, could you attach just a diff to latest master?
arc is too dumb to handle this ;=)

Diff to latest master as requested.

This revision was automatically updated to reflect the committed changes.