crossbario / txaio

Utilities to support code that runs unmodified on Twisted and asyncio
MIT License
58 stars 30 forks source link

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

Open ninousf opened 10 months ago

ninousf commented 10 months ago

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

oberstet commented 10 months ago

rgd

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

it would make sense to upgrade CI

https://github.com/crossbario/txaio/blob/50a111a95b19168127681429bebd3827e1e2f6e5/.github/workflows/main.yml#L39

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

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

oberstet commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 9 months ago

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)