-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Logging cleanup #34108
base: master
Are you sure you want to change the base?
Logging cleanup #34108
Conversation
Previously, it was placed in src/platform/logging/
Previously, it was placed in src/platform/logging/impl/android/
PR #34108: Size comparison from ee49ebd to 9a85ee5 Full report (8 builds for cc32xx, mbed, qpg, stm32, tizen)
|
Previously, it was placed in src/platform/logging/impl/stdio/
Its contents have been put into src/platform/BUILD.gn: - logging:stdio became platform:stdio_logging - logging:default became platform:default_logging
PR #34108: Size comparison from ee49ebd to 90f785a Full report (8 builds for cc32xx, mbed, qpg, stm32, tizen)
|
PR #34108: Size comparison from ee49ebd to abad890 Full report (96 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain in the PR summary why this change is desirable? The description says that we can remove logging
directory, however from a code organization perspective a directory named logging
would give targets of logging:<name>
and this change moves it to <name>-logging
so it seems like a noop but with moving thigs out of a dedicated folder. A folder named "logging" is obviously related to logging. A folder named "platform" does not seem so.
I would like to understand how this improves maintainability and structure. Naming-wise, platform:default-logging
seems slightly worse than platform/logging:default
(we could even remove the default and name it logging, so then you could use platform/logging
which could not be done if we move everything in platform.
@@ -92,7 +92,7 @@ static_library("chip-tool-utils") { | |||
sources += [ "commands/interactive/InteractiveCommands.cpp" ] | |||
deps += [ | |||
"${chip_root}/examples/common/websocket-server", | |||
"${chip_root}/src/platform/logging:headers", | |||
"${chip_root}/src/platform:logging-header", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This flattens the "logging" into "platform". We currently have this in src/app
and it is not good for maintainability - we end up getting a lot of separate libraries in one large build files, without them being related. We seem to start getting this into "core" as well as we figured out we have to fix deps. Flattening things out into "platform" seems undesirable.
Currently we have "platform/Linux", "platform/ESP32", "platform/Darwin" and so on. The first impression when looking at the directory structure is that every subdirectory is for a dedicated platform. There is also a "platform/fake" which somehow fits here. And then one discovers "platform/logging". Definitely there is no platform named "logging". So, the reasoning behind getting the logging out of the platform directory was to keep only real (except the "fake") platforms here. IMHO, that should improve readability of that directory level. However, I agree that this might make a mess on the upper level... On the other hand the "platform" directory level contains a lot of files related to platform which could be grouped into logical blocks (modules). Putting them into dedicated subdirectories will mix real platforms with platform modules... Currently the "platform/logging" contains logger implementation for STDIO (common for all platforms in case of forced STDIO logging), and implementation for Android. I think, that the Android one should be moved to the "platform/Android" anyway (it will follow pattern for other platforms where platform-specific implementation is kept in "platform/<name>" subdirectory. After this move, we will end up with only one file in the "platform/logging" - the common STDIO implementation. So, we thought that there is no reason to keep a dedicated directory for only one file in it. Basically that's the story behind this PR :) |
Currently, the
src/platform/logging
directory contains a handful of files:Each of them can be put in a more suitable place, which makes it possible to get rid of the
src/platform/logging
directory entirely.Changes
Move
src/platform/logging/LogV.h
tosrc/include/platform/
Move(this became a separate PR Move android's Logging.cpp to src/platform/android/ #34192)src/platform/logging/impl/android/Logging.cpp
tosrc/platform/android/
Move and rename
src/platform/logging/impl/stdio/Logging.cpp
tosrc/platform/StdioLoggingImpl.cpp
Merge content of
src/platform/logging/BUILD.gn
intosrc/platform/BUILD.gn
logging:stdio
becameplatform:stdio-logging
logging:default
becameplatform:default-logging