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

FIX: guess_stream_needs_encoding must return False when fileobj is an instance of io.TextIOBase #185

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ninousf
Copy link

@ninousf ninousf commented Dec 18, 2023

FIX: guess_stream_needs_encoding must return False when fileobj is an instance of io.TextIOBase
because fileobj.write needs a str as parameter, not bytes

… instance of io.TextIOBase, because fileobj.write needs a str as parameter, not bytes
@oberstet
Copy link
Contributor

rgd

https://github.com/crossbario/txaio/actions/runs/7251555425/job/19755043215?pr=185#step:4:202

it would make sense to upgrade CI

python-version: ['3.7', '3.11', 'pypy-3.7', 'pypy-3.9']

for testing against latest PyPy 3.9 and 3.10 instead of 3.7 / 3.9

https://www.pypy.org/download.html

@oberstet
Copy link
Contributor

thanks for bumping pypy!

now it runs into https://github.com/crossbario/txaio/actions/runs/7254104155/job/19763863038?pr=185#step:6:307

I'm not sure what's going on .. this seems to indeed change some deep-buried behavior

@meejah any clues or hints or caveats you see?

@ninousf
Copy link
Author

ninousf commented Dec 19, 2023

the test failure appears also on main branch

git checkout master
micromamba env create -n txaio python=3.11 tox
micromamba activate txaio
make

@ninousf
Copy link
Author

ninousf commented Dec 21, 2023

it works with last twisted version 23.10.0, it only fails with the Twisted trunk
py311-tw2210: OK (0.73=setup[0.36]+cmd[0.37] seconds) py311-tw238: OK (0.72=setup[0.35]+cmd[0.37] seconds) py311-tw2310: OK (0.73=setup[0.37]+cmd[0.36] seconds) py311-twtrunk: FAIL code 1 (0.75=setup[0.35]+cmd[0.40] seconds)

@oberstet
Copy link
Contributor

works with last twisted version 23.10.0, it only fails with the Twisted trunk

this package does work as is

https://github.com/crossbario/txaio/actions?query=workflow%3Amain

what's not verified if it works with only moving to newer cpython/pypy still, and with current twisted trunk

so if you want this going forward, best would be 2 PRs: one with only the cpy/pypy bumping. I also assume setup.py needs adjustment to account for those new minimal version

if that works (on 1st PR), then I would relook into your actual change on a 2nd PR

if that 2nd PR does work, but does not work on twisted trunk, this must be investigated first, because this is quite unusual - but of course not impossible

sorry, I don't have time to work on this, hope above helps nevertheless

@meejah
Copy link
Contributor

meejah commented Jan 4, 2024

Sorry for the delay, rolled off my radar (ahem, github notifications ;)

Just looking at errors + code, it seems like possibly the BytesIO interface / API maybe changed, somehow? The messages override is "interesting" and it seems like that's what is failling in the test_logging...

Bigger picture, a bunch of this seems related to Python2 support -- if that's dropped (it is, right?), I think it could be simplified significantly in all likelihood.

@oberstet
Copy link
Contributor

thanks @meejah for chiming in and for your analysis & comments!

this seems related to Python2 support -- if that's dropped (it is, right?), I think it could be simplified significantly in all likelihood.

yes, Python 2 is dead and yes, if it would simplify things, we could definitely rip out anything that originally was only to make Python 2 fly from txaio (or autobahn or crossbar for that matter)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants