Logging cleanup #34108
Draft
Logging cleanup #34108
+300
−311
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, it was placed in src/platform/logging/
Previously, it was placed in src/platform/logging/impl/android/
github-actions
bot
added
tools
examples
platform
darwin
esp32
linux
nrf connect
zephyr
lib
telink
support
tizen
|
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
pullapprove
bot
requested review from
pidarped,
plauric,
rcasallas-silabs,
ReneJosefsen,
saurabhst,
selissia,
sharadb-amazon,
tcarmelveilleux,
tecimovic,
tehampson,
tima-q,
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang and
yunhanw-google
arkq
reviewed
Jun 27, 2024
|
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)
|
arkq
approved these changes
Jun 27, 2024
andy31415
requested changes
Jul 2, 2024
There was a problem hiding this comment.
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.
andy31415
reviewed
Jul 2, 2024
| @@ -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.
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 :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, the
src/platform/loggingdirectory 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/loggingdirectory entirely.Changes
Move
src/platform/logging/LogV.htosrc/include/platform/Move(this became a separate PR Move android's Logging.cpp to src/platform/android/ #34192)src/platform/logging/impl/android/Logging.cpptosrc/platform/android/Move and rename
src/platform/logging/impl/stdio/Logging.cpptosrc/platform/StdioLoggingImpl.cppMerge content of
src/platform/logging/BUILD.gnintosrc/platform/BUILD.gnlogging:stdiobecameplatform:stdio-logginglogging:defaultbecameplatform:default-logging