Do not force STDIO output for Linux example apps #34232
Closed
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
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
axelnxp,
bauerschwan,
bzbarsky-apple,
carol-apple,
cecille,
chapongatien,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
doru91,
fessehaeve,
harimau-qirex,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple and
kiel-apple
pullapprove
bot
requested review from
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang and
yunhanw-google
|
PR #34232: Size comparison from 6e31453 to f1c716b Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
approved these changes
Jul 8, 2024
yufengwangca
approved these changes
Jul 8, 2024
kiel-apple
approved these changes
Jul 8, 2024
|
PR #34232: Size comparison from 9306418 to c8c4486 Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #34232: Size comparison from 9306418 to 145a4ea Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bzbarsky-apple
requested changes
Jul 12, 2024
There was a problem hiding this comment.
Test failures are:
raise Exception('Timeout while waiting for %s' % waitForString)
because the tests are waiting for some stdio output from the example apps that never comes.
I expect this is actually quite common in workflows involving the example apps (e.g. it's very common for people to find the relevant pairing code in the stdio output). And certainly we have a bunch of tests that log things from those apps to files in CI and expect those logs to be available later if the test fails.
So I think we need to:
- Decide whether we actually want to change the default behavior, given the significant pain this will impose on people who currently use the example apps.
- Decide whether this should be an across-the-board change or whether some commonly-used example apps where this could be a particular problem should default to stdio (for now, or in general).
- Fix all our build documentation to describe the options for logging and how to select the one you want (no matter what the default is).
- If we change the default behavior for some apps, ensure that all existing testing that uses those apps is audited to see whether it's OK with the behavior change, and if not update the relevant build args.
But long story short: as-is this will break a ton of people's workflows. A bunch of in-tree ones (some of them leading to CI failures), but also lots of out-of-tree ones.
Yes, I'm aware of that. Now, I'm trying to figure how much work would it be to add support for changing logging to stdio for CI (and potentially other out of tree cases) while still allowing to build example apps with proper platform logging. The case is, that I would like to use the example app on a real device without mich hassle. For Linux apps it's not an issue since the platform logging happens to be stdout as well. But, Darwin and Tizen (Tizen is my real case) by default use dedicated logger. However, the example apps are both based on Linux example platform code, so that's why I'd like the platform common code not to force stdio logging. I'll convert this PR to draft for now. |
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.
Problem
Example apps should be designed as real applications. Hence, they should integrate with platform logger instead of logging to stdio.
Changes
Testing
CI will verify potential build breaks.