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

LogFile: rename .buffer (to fix newer flask/click) #2264

Merged
merged 1 commit into from
Aug 11, 2020

Conversation

obfusk
Copy link
Contributor

@obfusk obfusk commented Jul 17, 2020

Using (custom recipes for) the latest click (7.1.2) and flask (1.1.2) I got an AttributeError: 'str' object has no attribute 'write' because it tries using stdout.buffer.

@obfusk obfusk mentioned this pull request Aug 3, 2020
@obfusk
Copy link
Contributor Author

obfusk commented Aug 5, 2020

Without this change, recent versions of click (which is a dependency of flask) don't work.

@obfusk obfusk mentioned this pull request Aug 5, 2020
@AndreMiras
Copy link
Member

I'm concerned it could introduce a regression on sdl2 apps. Or was it backward compatible for a long while?

@obfusk
Copy link
Contributor Author

obfusk commented Aug 5, 2020

I'm concerned it could introduce a regression on sdl2 apps.

I'm not that familiar with those. Why would it introduce a regression?

Or was it backward compatible for a long while?

Was what backward compatible? I'm missing some context here 😅

The problem is caused by the fact that sys.stdout.buffer is normally an _io.BufferedWriter, so the current code is not conforming to the expected interface for sys.stdout (as used by e.g. recent versions of click):

>>> sys.stdout.buffer
<_io.BufferedWriter name='<stdout>'>

According to https://docs.python.org/3/library/sys.html#sys.stderr:

To write or read binary data from/to the standard streams, use the underlying binary buffer object. For example, to write bytes to stdout, use sys.stdout.buffer.write(b'abc').

This has been the case since python 3.0.

@AndreMiras
Copy link
Member

It's just that I'm wondering why we see this only now? So I assumed it was a change of behaviour, like new API or something.
If that's the case then updating to the new interface may work for new code but break for old code. It's just suspicion, but I haven't investigated

@obfusk
Copy link
Contributor Author

obfusk commented Aug 6, 2020

It's just that I'm wondering why we see this only now? So I assumed it was a change of behaviour, like new API or something.
If that's the case then updating to the new interface may work for new code but break for old code. It's just suspicion, but I haven't investigated

I investigated a bit. As far as I can tell, click:

  • checks for .buffer and assumes it is a binary stream if not None (and uses the original text stream if there's no .buffer)
  • detects a LogFile as both "compatible" and "misconfigured" (because it has no .encoding)
  • changed the logic (somewhere since version 7.0) and no longer accepts a text stream that is "compatible" but "misconfigured"
  • and thus now tries to use the .buffer binary stream because the LogFile is considered "misconfigured"

So I'm not expecting a regression. Not having a .buffer should be the correct fix for old and new versions both.

@AndreMiras
Copy link
Member

Thanks for the investigation. Let's merge and we can simply sue you if it breaks something 😄

Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

Thanks

@AndreMiras AndreMiras merged commit 0c7efcd into kivy:develop Aug 11, 2020
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.

2 participants