Build
kdeinit5
kdebugsettings
Details
- Reviewers
ivan kossebau - Group Reviewers
Frameworks - Commits
- R159:2bb035ff67fd: Add proper logging using ECMQtDeclareLoggingCategory
Diff Detail
- Repository
- R159 KActivities Statistics
- Branch
- arcpatch-D22143
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 14505 Build 14523: arc lint + arc unit
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. 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}) |
src/CMakeLists.txt | ||
---|---|---|
19 | Do you have a suggestion how to share the variable between the two CMake files ? |
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. |
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.
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.
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 :)
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.
With this and my added comment on line 21 of autotests/CMakeLists.txt
What do you think @kossebau ?
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 | ||
1 | You mixed singulars and plurals - go for plurals everywhere: s/KACTIVITY_STAT_LOG/KACTIVITIES_STATS_LOG/g |
kactivities-stat-logsettings.h -> kactivities-stats-logsettings.h, KACTIVITY_STAT_LOG -> KACTIVITIE_STATS_LOG
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