Add proper logging using ECMQtDeclareLoggingCategory
ClosedPublic

Authored by meven on Jun 28 2019, 2:07 PM.

Details

Test Plan

Build
kdeinit5
kdebugsettings

Diff Detail

Repository
R159 KActivities Statistics
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
meven created this revision.Jun 28 2019, 2:07 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJun 28 2019, 2:07 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.Jun 28 2019, 2:07 PM
kossebau added inline comments.
autotests/CMakeLists.txt
21

Instead of relying on an undocumented cpp file name generated by ecm_qt_declare_logging_category, you rather want to have a separate SRC variable which carries the source files generated by the macro, and add its content here by instead adding this to the list:

${KActivitiesStats_LOG_SRCS}

That variable will also make it easier to track where this file is actually coming from, instead having to guess it.

src/CMakeLists.txt
19

As we want to reuse the source files also with the tests, better store in a separate var here, e.g. named KActivitiesStats_LOG_SRCS.
And add this manually to KActivitiesStats_LIB_SRCS and for the test as commented above:

ecm_qt_declare_logging_category(KActivitiesStats_LOG_SRCS
                                HEADER kactivities-stat-logsettings.h
                                IDENTIFIER KACTIVITY_STAT_LOG
CATEGORY_NAME kf5.kactivity.stat)
list(APPEND KActivitiesStats_LIB_SRCS ${KActivitiesStats_LOG_SRCS})
meven added inline comments.Jun 28 2019, 5:09 PM
src/CMakeLists.txt
19

Do you have a suggestion how to share the variable between the two CMake files ?
I tried moving the ecm_qt_declare_logging_category to the top level CMake but then I don't know how to share them with the children dirs.
I read about PARENT_SCOPE but the problem still stands scope-wise.
Or should I convert the add_subdirectory to include ones ? This would need other changes.

meven added inline comments.Jun 29 2019, 8:17 AM
autotests/CMakeLists.txt
21

While it is true, the solution here is not ideal, it is quite commonly used in kde projects and anyone opening the CMakeLists.txt should come up pretty quickly that this file is being generated elsewhere.
I just lack a viable alternative. I had a hard time to uncover this solution in the first place.

meven added inline comments.Jul 17 2019, 9:49 AM
autotests/CMakeLists.txt
21
meven updated this revision to Diff 62526.Jul 25 2019, 9:05 AM
meven marked 5 inline comments as done.

Better category name

meven added a comment.Jul 25 2019, 9:05 AM

@ivan What are you thoughts about this ?

ivan added a comment.Jul 25 2019, 5:21 PM

If @kossebau is satisfied, go for it

Nope, current solution does not satisfy my personal standards, sorry :)
Relying on undocumented names of generated sources files does not get my +1. That needs someone else to take responsibility :)

If transfering the name via variables s not possible, I would go for regenerating the respective logging source files in the autotest dir then again instead.

meven added a comment.Jul 25 2019, 6:03 PM
Relying on undocumented names of generated sources files does not get my +1. That needs someone else to take responsibility :)

Well this kind of file generation is common and is indirectly documented through the ecm_qt_declare_logging_category macro.

I don't think this is an important issue.

For what it is worth, the same technique is used in other repos :

kdebugsettings/autotests/CMakeLists.txt
22: ${CMAKE_BINARY_DIR}/src/kdebugsettings_debug.cpp

plasma-thunderbolt/autotests/kded/CMakeLists.txt
5: ${CMAKE_BINARY_DIR}/src/kded
11: ${CMAKE_BINARY_DIR}/src/kded/kded_bolt_debug.cpp

pulseaudio-qt/autotests/CMakeLists.txt
8: ${CMAKE_BINARY_DIR}/src/debug.cpp

This does not justify it but makes the points that other KDE devs did not see an issue with it.

I would gladly use an alternative if presented with one.

Relying on undocumented names of generated sources files does not get my +1. That needs someone else to take responsibility :)

Well this kind of file generation is common and is indirectly documented through the ecm_qt_declare_logging_category macro.

The current implementation of the macro is an internal detail. It is not part of the API contract.
Binding one's code to current internal implementation has two disadvantages: it makes it harder for the macro developers to enhance the macro, because they would break your code, Or they do not know someone is relying on internal details, and your code breaks one day.

I don't think this is an important issue.

For what it is worth, the same technique is used in other repos :

IMHO all cases which are bad code :)

This does not justify it but makes the points that other KDE devs did not see an issue with it.

All us KDE devs have different backgrounds, so not surprising there are different opinions :)

I would gladly use an alternative if presented with one.

I just tried to give one in my previous comment: regenerating the respective logging source files in the autotest dir again, by calling the macro there once more.
Or extend the documentation of the macro, to specify the cpp name. Then its part of the API contract officially :)

meven added a comment.Jul 26 2019, 6:59 AM
Relying on undocumented names of generated sources files does not get my +1. That needs someone else to take responsibility :)

Well this kind of file generation is common and is indirectly documented through the ecm_qt_declare_logging_category macro.

The current implementation of the macro is an internal detail. It is not part of the API contract.

I am quoting the macro documentation :

A header file, <filename>, will be generated along with a corresponding source file, ...

https://api.kde.org/ecm/module/ECMQtDeclareLoggingCategory.html

So here, It seems to me this generated cpp file is part of the macro documented behavior.

Binding one's code to current internal implementation has two disadvantages: it makes it harder for the macro developers to enhance the macro, because they would break your code, Or they do not know someone is relying on internal details, and your code breaks one day.

Fair point, in general, not relying on internal details

I just tried to give one in my previous comment: regenerating the respective logging source files in the autotest dir again, by calling the macro there once more.

I might do just do that, thanks.

meven updated this revision to Diff 62695.Jul 28 2019, 8:01 PM

Add comment regarding the origin of a generated cpp file

meven added a comment.Aug 7 2019, 7:56 AM
Relying on undocumented names of generated sources files does not get my +1. That needs someone else to take responsibility :)

Well this kind of file generation is common and is indirectly documented through the ecm_qt_declare_logging_category macro.

The current implementation of the macro is an internal detail. It is not part of the API contract.

I am quoting the macro documentation :

A header file, <filename>, will be generated along with a corresponding source file, ...

https://api.kde.org/ecm/module/ECMQtDeclareLoggingCategory.html

So here, It seems to me this generated cpp file is part of the macro documented behavior.

With this and my added comment on line 21 of autotests/CMakeLists.txt
What do you think @kossebau ?

meven added a comment.Aug 27 2019, 6:00 PM

@kossebau what about my last changes ?

@ivan I feel this is ok to have ${CMAKE_BINARY_DIR}/src/kactivities-stat-logsettings.cpp in the test CMakeLists.txt file with the documentation associated with ECMQtDeclareLoggingCategory and my comment, no-one can miss where this file is coming from.

ivan requested changes to this revision.Aug 27 2019, 6:21 PM

Change everything to plural.

I agree with @kossebau, though I don't think it will lead to problems in future, so I could say it is bearable dirtyness.

autotests/CMakeLists.txt
22

plural stats

kactivities-stats.categories
2

You mixed singulars and plurals - go for plurals everywhere:

s/KACTIVITY_STAT_LOG/KACTIVITIES_STATS_LOG/g

This revision now requires changes to proceed.Aug 27 2019, 6:21 PM
meven updated this revision to Diff 64784.Aug 28 2019, 5:14 AM

kactivities-stat-logsettings.h -> kactivities-stats-logsettings.h, KACTIVITY_STAT_LOG -> KACTIVITIE_STATS_LOG

meven updated this revision to Diff 64787.Aug 28 2019, 5:57 AM

Rebase on master

ivan added inline comments.Aug 28 2019, 6:25 AM
kactivities-stats.categories
2

Missed one here

src/CMakeLists.txt
21

KACTIVITIE -> KACTIVITIES

meven updated this revision to Diff 64789.Aug 28 2019, 6:58 AM

Fix plural

ivan accepted this revision.Aug 28 2019, 7:35 AM
This revision is now accepted and ready to land.Aug 28 2019, 7:35 AM
This revision was automatically updated to reflect the committed changes.

This is installing BOTH /etc/xdg/kactivities-stats.categories AND/usr/share/qlogging-categories5/kactivities-stats.categories which is surely wrong?

This is installing BOTH /etc/xdg/kactivities-stats.categories AND/usr/share/qlogging-categories5/kactivities-stats.categories which is surely wrong?

Indeed. As it depends on ECM >= 5.61, it has no reason to keep on installing it in KDE_INSTALL_CONFDIR