Skip to content
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

gh-116622: Android logging fixes #122698

Merged
merged 8 commits into from
Aug 6, 2024
Merged

Conversation

mhsmith
Copy link
Member

@mhsmith mhsmith commented Aug 5, 2024

This PR fixes several issues with Android's redirection of stdout and stderr to the Logcat, which was added in #118063:

  • The buffering behavior of TextIOWrapper was changed in #119507, which broke the technique we were previously using to stay within Logcat's line length limit. Since this behavior is not part of TextIOWrapper's public API, it's safer to bypass it completely and do the buffering ourselves.

  • When running in --verbose3 mode, I noticed that failure logs of large test modules like test_pathlib were truncated because they were being written faster than the test script could consume them. Fixed by adding a rate limit using the token bucket algorithm.

  • The Android stdout and stderr tests themselves failed in --verbose3 mode because stdout and stderr were captured by a StringIO. Fixed by detecting this mode and using some temporary streams with the same properties.

@mhsmith mhsmith requested a review from freakboy3742 August 5, 2024 18:10
@mhsmith mhsmith marked this pull request as draft August 5, 2024 18:39
@mhsmith mhsmith marked this pull request as ready for review August 5, 2024 22:16
Copy link
Contributor

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes all make sense to me; but there's a hypothesis test that is currently failing. I want to understand what's going on there before I formally approve/merge.

Copy link
Contributor

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the hypothesis failure is due to #122686, so I guess that makes this good to go!

@freakboy3742 freakboy3742 merged commit b0c48b8 into python:main Aug 6, 2024
34 checks passed
@miss-islington-app
Copy link

Thanks @mhsmith for the PR, and @freakboy3742 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 6, 2024
Modifies the handling of stdout/stderr redirection on Android to accomodate
the rate and buffer size limits imposed by Android's logging infrastructure.
(cherry picked from commit b0c48b8)

Co-authored-by: Malcolm Smith <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Aug 6, 2024

GH-122719 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Aug 6, 2024
brandtbucher pushed a commit to brandtbucher/cpython that referenced this pull request Aug 7, 2024
Modifies the handling of stdout/stderr redirection on Android to accomodate 
the rate and buffer size limits imposed by Android's logging infrastructure.
@hugovk
Copy link
Member

hugovk commented Aug 9, 2024

Hello! For next time, please could you update your blurb CLI? Thanks!

https://discuss.python.org/t/new-blurb-1-2-please-upgrade/59159

freakboy3742 pushed a commit that referenced this pull request Aug 16, 2024
gh-116622: Android logging fixes (GH-122698)

Modifies the handling of stdout/stderr redirection on Android to accomodate
the rate and buffer size limits imposed by Android's logging infrastructure.
(cherry picked from commit b0c48b8)

Co-authored-by: Malcolm Smith <[email protected]>
blhsing pushed a commit to blhsing/cpython that referenced this pull request Aug 22, 2024
Modifies the handling of stdout/stderr redirection on Android to accomodate 
the rate and buffer size limits imposed by Android's logging infrastructure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants