Opened 6 years ago

Closed 4 years ago

#8009 enhancement closed fixed (fixed)

twisted.web.twcgi should be ported to Python 3

Reported by: hawkowl Owned by: Craig Rodrigues
Priority: normal Milestone: Python-3.x
Component: web Keywords:
Cc: Branch: 8009-rodrigc-cgi-py3
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban

Attachments (3)

twcgi-py3-8009-1.patch (12.4 KB) - added by Unit03 5 years ago.
Work in progress
twcgi-py3-8009-2.patch (12.8 KB) - added by Unit03 5 years ago.
twcgi-py3-8009-3.patch (13.0 KB) - added by Unit03 5 years ago.

Download all attachments as: .zip

Change History (15)

Changed 5 years ago by Unit03

Attachment: twcgi-py3-8009-1.patch added

Work in progress

comment:1 Changed 5 years ago by Unit03

Keywords: review added

I've ported the module but due to t.spread.pb.Viewable dependency (and using hint from hawkowl) I'm replacing it with object in 3.x (where importing throws SyntaxError):

    from twisted.spread.pb import Viewable
except SyntaxError:
    Viewable = object


class CGIProcessProtocol(protocol.ProcessProtocol, Viewable):

Initial patch attached.

Thing is, even then all t.w.test.test_cgi tests still pass - looks like no test actually tests that dependency.

What do I do:

  • add such test(s)?
  • del CGIProcessProtocol in 3.x and skip all tests in t.w.test.test_cgi if 3.x?
  • or just leave it like that?

And, based on that, what to write in news file? That it's ported? Ported, but won't work with perspective broker?

comment:2 Changed 5 years ago by Glyph

I think this is Viewable so that it will work across a connection via twisted.web.distrib. So I think it's fine to say that it's ported but doesn't work with distrib yet. Small changes are best, so if you could add those tests in a separate ticket that would be great.

comment:3 Changed 5 years ago by Unit03

Then I'm re-submitting the patch with only one change (news entry) and I'll try to find time for writing these tests.

Changed 5 years ago by Unit03

Attachment: twcgi-py3-8009-2.patch added

comment:4 Changed 5 years ago by Adi Roiban

Author: adiroiban
Branch: branches/twcgi-py3-8009

(In [46303]) Branching to twcgi-py3-8009.

comment:5 Changed 5 years ago by Adi Roiban

Keywords: review removed
Owner: set to Unit03

Many thanks for your changes. They look good.

The import hack needs at least one comment. If it needs more work, please create a ticket and add the ticket url in the comment.

I took the liberty to format the news file fragment into a more narrative way. Please check that all is ok. Feel free to revert my changes.

I have also updated the code to use the open context manager and try to encode the url into a way which is much easier to read.

We don't have tests for the test code so it is very important for the test code to be easy to read and understand and avoid any obfuscation

in CGIScript.render

The current code uses both str and bytes for the env key names. I think that you should choose a single.

If possible, please submit a new patch on top of the latest branch.

Many thanks again!

comment:6 Changed 5 years ago by Adi Roiban

All tests passed on your patch. I have re-run the test with my changes

Changed 5 years ago by Unit03

Attachment: twcgi-py3-8009-3.patch added

comment:7 Changed 5 years ago by Unit03

Keywords: review added
Owner: changed from Unit03 to Adi Roiban

One eternity later: third version of a patch, using changes by adiroiban (thank you!) and with:

  • comment for import hack
  • reworded news entry a little
  • env key names in CGIScript.render changed to strs, decoded with 'iso-8859-1' (like headers in t.w.http_headers)

comment:8 Changed 5 years ago by Adi Roiban

Branch: branches/twcgi-py3-8009branches/twcgi-py3-8009-3

(In [47337]) Branching to twcgi-py3-8009-3.

comment:9 Changed 5 years ago by Adi Roiban

Keywords: review removed
Owner: changed from Adi Roiban to Unit03

Thanks for the update.

  1. I have applied your latest patch and sent it to buildbot.

It is green but test_cgi tests are enabled for this code.

The new code needs to coverted by py3 tests.

In the future please consider updating the twisted/python/ file so that all code related to this functionality is part of the py3 distribution and is covered by tests.

I have updated your patch to include test_cgi and send it again to buildbot.

It looks like twisted.web.twcgi module itself was already included... I think for the wscgi part.

  1. t.spread.pb.Viewable hack should have it's own ticket to fix this hack and once t.spread.pb.Viewable is ported or fix, this comment can be removed.

I am not sure if this hacke is acceptable. I have never used the twcgi module, so you will have to get in touch with glyph or hawkowl to see if they are happy with this hack.

I would say that we need to port t.spread.pb.Viewable first, and after that to merge this.

If t.spread.pb.Viewable should not be ported, it should be deprecated.

  1. The patch also needs a NEWS file fragment (release notes fragment)

as described here

Please check my comments and submit new patch.

Please also check the buildbot results after test_cgi was enabled


comment:10 Changed 4 years ago by Craig Rodrigues

Branch: branches/twcgi-py3-8009-38009-rodrigc-cgi-py3
Description: modified (diff)
Keywords: review added

comment:11 Changed 4 years ago by Glyph

Keywords: review removed
Owner: changed from Unit03 to Craig Rodrigues

Please address and land as per the github review. Thanks!

comment:12 Changed 4 years ago by Craig Rodrigues <rodrigc@…>

Resolution: fixed
Status: newclosed

In 09f3eeb:

Merge pull request #736 from twisted/8009-rodrigc-cgi-py3

Author: rodrigc
Reviewer: glyph
Fixes: ticket:8009

Port twisted.web.twcgi to Python 3

Note: See TracTickets for help on using tickets.