Generate all kdebugsettings .categories files automatically
ClosedPublic

Authored by kossebau on Oct 7 2018, 10:41 PM.

Details

Summary

Adds two wrapping variants of the macros
declare_qt_logging_category() & install_qt_logging_categories()
which have an argument TYPE to control specific behaviour that
otherwise is generalized in the wrapping macros to not have to repeat
any things with every macro call and to ensure consistency, e.g. in
the used description texts.

The wrapper macros also handle linking things by the matching EXPORT ids,
so the caller does not have to care for this.

No perfect solution yet, but at least a first working approach to automatic
generation of the kdebugsettings files.

Test Plan

Generated categories files contain same ids with same descriptions as
before.

Diff Detail

Repository
R32 KDevelop
Branch
morecategoriesgeneration
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 3791
Build 3809: arc lint + arc unit
kossebau created this revision.Oct 7 2018, 10:41 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptOct 7 2018, 10:41 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
kossebau requested review of this revision.Oct 7 2018, 10:41 PM
apol added a subscriber: apol.Oct 8 2018, 11:02 AM
apol added inline comments.
plugins/grepview/CMakeLists.txt
5–6

I find it odd that we're creating a macro to reduce the redundant parts but in practice not end up reducing much.

I would suggest also having the target as the argument rather than the output variable. It will make it easier to see the 1:1 relationship and also make it possible to use the target name as the basename.

Initial commit message also had:
"The EXPORT argument is kept explicit for now, though it could be considered to
hardcode the related install_qt_logging_categories() calls into the two
macros."

So much for quick thoughts in tired mode while writing the commit message. Actually this will not work.
Reason is that the macros are all run at cmake/configure time, and . And depend on the order they are executed. So first all declare_plugin_qt_logging_category calls have to be made to add the data to the target object, before the macro install_qt_logging_categories is run, as that one will generate the final file and use only the data existing with the target at that time.

plugins/grepview/CMakeLists.txt
5–6

One cannot see all the reduction directly from the diff for the plugins, as here we are jumping directly to generated installed categories file.
But see the diffs for the kdevplatforms libs, there it should be more clear to see how much there is reduced (and as wanted side-effect consistency is ensured).

Not sure what you mean by "having the target as the argument"? And "use the target name as the basename"?

The main purpose of the macros "declare*_qt_logging_category" is still to generate the debug source files. Collecting the data so that we can autogenerate the installed categories file for those who want to use kdebugsettings is a 2nd-order purpose.

kossebau edited the summary of this revision. (Show Details)Oct 8 2018, 11:47 AM
kossebau updated this revision to Diff 43224.Oct 9 2018, 3:20 PM
kossebau edited the summary of this revision. (Show Details)

update to latest master

Also simplify call interface even more, by now adding pairs of custom declare &
install macros:

  • for kdevplatform:
    • declare_platformlib_qt_logging_category
    • install_platformlib_qt_logging_categories
  • for plugins & app:
    • declare_plugin_qt_logging_category
    • declare_app_qt_logging_category
    • install_app_plugin_qt_logging_categories

Those pairs avoid the need for the manual linking between declare & install
by the EXPORT id, which now is handled by the custom macros internally.

declare_platformlib_qt_logging_category(KDevPlatformShell_LIB_SRCS
    CATEGORY_BASENAME "shell"
)

and

declare_plugin_qt_logging_category(kdevclazy_core_SRCS
    IDENTIFIER KDEV_CLAZY
    CATEGORY_BASENAME "clazy"
)

with the respective categories file creation and installation calls being:

install_platformlib_qt_logging_categories()
install_app_plugin_qt_logging_categories()
kossebau edited the summary of this revision. (Show Details)Oct 9 2018, 3:26 PM
apol added a comment.Oct 10 2018, 2:17 PM

Considering we're doing our own macro, we can be also more succint. I don't see why we can't have declare_plugin_qt_logging_category(targetname)

You can use target_sources() instead of adding it to the variable.

cmake/modules/KDevelopMacrosInternal.cmake
287

Wouldn't it be better to just add the install call in the declare_*_qt_logging_category calls? This way we wouldn't need a magical function call in the root CMakeLists.txt file.

In D16032#340668, @apol wrote:

Considering we're doing our own macro, we can be also more succint. I don't see why we can't have declare_plugin_qt_logging_category(targetname)

You can use target_sources() instead of adding it to the variable.

You mentioned something about target in an earlier comment. But I still have no idea what you mean here and what should be generated how, could you please give an explicit example, to help aligning my brain with yours? :)

cmake/modules/KDevelopMacrosInternal.cmake
287

Sadly does not work. As I told myself earlier before here:
"Reason is that the macros are all run at cmake/configure time, and . And depend on the order they are executed. So first all declare_plugin_qt_logging_category calls have to be made to add the data to the target object, before the macro install_qt_logging_categories is run, as that one will generate the final file and use only the data existing with the target at that time."

So the generation of the file has to be explicitly triggered, as we do not know which declare call is the last one. Hm, unless we just append a new line every time to the file, and do the principal creation on the first call. Will give that a try tonight,

In D16032#340668, @apol wrote:

Considering we're doing our own macro,

In the long term, if we get to some sane solution, we should see to push this in a generalized way upstream to ECM for sure, to extend or subtitute what had been drafted with D9446

On some quick experiments I could not find a nice way to merge the generation of the .categories file into the declare macros. So when it comes to invested resources, I would rather prefer to keep the current explicit install_* macro calls for now, until someone has a better idea.

Maybe it's also not that bad to have some place in the actual CMakeLists.txt where one can see how actually the generation and installation of the categories file is triggered, instead of having them appear magically and be installed magically?

kfunk added a subscriber: kfunk.Oct 11 2018, 5:28 AM

I'm torn about this review; I'm with Aleix in the sense that this adds lots and lots of extra CMake code to maintain without a /lot/ of gain.

CMakeLists.txt
185

I think it's over the top to factor that out into a reusable function if it's only going to be used once. Would revert to the previous version.

app/CMakeLists.txt
14

Dito: I think it's over the top to factor that out into a reusable function if it's only going to be used once. Would revert to the previous version.

cmake/modules/KDevelopMacrosInternal.cmake
138

Way too much code duplication in all those declare_*_qt_logging_category calls, IMO... But I realize that if you want to keep _declare_qt_logging_category KDevelop-agnostic (for possible future upstreaming to ECM...) there's no way around this.

If you don't want to keep _declare_qt_logging_category KDevelop-agnostic you could add a TYPE argument to it and set some defaults for the different ARGS_* based on TYPE instead. Thinking this out further, we wouldn't even need different declare_*_qt_logging_categoryfunctions, but just use a declare_qt_logging_category function with a TYPE parameter directly. Just specifiy TYPE ... at the call-site in the individual CMakeLists.txt files...

138

Any reason this is macro()? Could be a function(), no?

138

Style (sth for a future commit maybe): _sources -> sources or sources_var (to indicate it's an out var)

apol added a comment.Oct 11 2018, 3:04 PM
In D16032#340668, @apol wrote:

Considering we're doing our own macro, we can be also more succint. I don't see why we can't have declare_plugin_qt_logging_category(targetname)

You can use target_sources() instead of adding it to the variable.

You mentioned something about target in an earlier comment. But I still have no idea what you mean here and what should be generated how, could you please give an explicit example, to help aligning my brain with yours? :)

Does that make sense to you?

function(declare_plugin_qt_logging_category targetname)
    set(SRCS)
    _declare_qt_logging_category(SRCS
        HEADER debug.h
        IDENTIFIER ${targetname}
        CATEGORY_NAME "kdevelop.plugins.${targetname}"
        EXPORT ${_app_plugin_qt_logging_categories_export_name}
        DESCRIPTION "KDevelop plugin: ${targetname}"
    )

    target_sources(${targetname} PRIVATE ${SRCS})
endfunction()

Could possibly be called from kdevplatform_add_plugin.

kossebau added a comment.EditedOct 11 2018, 4:11 PM
In D16032#341315, @apol wrote:
In D16032#340668, @apol wrote:

Considering we're doing our own macro, we can be also more succint. I don't see why we can't have declare_plugin_qt_logging_category(targetname)

You can use target_sources() instead of adding it to the variable.

You mentioned something about target in an earlier comment. But I still have no idea what you mean here and what should be generated how, could you please give an explicit example, to help aligning my brain with yours? :)

Does that make sense to you?

<snip sample>

Ah , thanks, now I see, you meant the plugin target :)
There is one blocker for this: some plugins share the generated debug.h and debug.cpp files between the actual plugin and some tests. In those cases adding the generated sources to the plugin target does not work (grep for "_LOG_" to find those places). I very much agree though that ideally all plugins should be made consistent here.

Could possibly be called from kdevplatform_add_plugin.

Yes, that should make things more simple & consistent.

Please bear with this patch: its whole idea was to add the automatic generation of the kdebugsettings .categories files, not to fix the complete current logging category situation :)
Let's fix things step-by-step.

cmake/modules/KDevelopMacrosInternal.cmake
138

Macro, because we want to change a variable from the calling scope (the one whose name we get by _sources, which is passed on to ecm_qt_declare_logging_category which is the one actually going to change the very variable.)

138

The TYPE seems a nice idea to save on the implementation side, will give a try.

kossebau updated this revision to Diff 43451.Oct 12 2018, 10:12 AM

Reduce to only two public macros, using TYPE argument to define details

kfunk accepted this revision.Oct 23 2018, 6:37 PM

Hm, still not entirely convinced of all the added CMake code..., but at least the uses of the macros look better now.

Please push to master branch.

This revision is now accepted and ready to land.Oct 23 2018, 6:37 PM
kossebau edited the summary of this revision. (Show Details)Oct 23 2018, 7:02 PM
This revision was automatically updated to reflect the committed changes.