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

make twisted.internet.endpoints importable on Windows when pywin32 is not installed #6032

Closed
twisted-trac opened this issue Sep 30, 2012 · 19 comments

Comments

@twisted-trac
Copy link

tray's avatar tray reported
Trac ID trac#6032
Type defect
Created 2012-09-30 18:42:12Z

Using Twisted 12.2, twisted.internet.endpoints cannot be imported on Windows unless pywin32 is available:

 Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "twisted\web\client.py", line 21, in <module>
    from twisted.internet.endpoints import TCP4ClientEndpoint, SSL4ClientEndpoint
  File "twisted\internet\endpoints.py", line 28, in <module>
    from twisted.internet import stdio
  File "twisted\internet\stdio.py", line 28, in <module>
    from twisted.internet import _win32stdio
  File "twisted\internet\_win32stdio.py", line 7, in <module>
    import win32api
ImportError: No module named win32api

As demonstrated by this traceback, this also affects any module that depends on endpoints. The pywin32 dependency is only necessary for stdio; other endpoints should be usable even if it is not installed.

This likely requires a Windows version of the no-modules builder to be set up first - see https://bugs.launchpad.net/twisted-buildbot-configuration/+bug/1059240.

Searchable metadata
trac-id__6032 6032
type__defect defect
reporter__tray tray
priority__high high
milestone__ 
branch__ 
branch_author__ 
status__closed closed
resolution__fixed fixed
component__core core
keywords__None None
time__1349030532000000 1349030532000000
changetime__1553542172752674 1553542172752674
version__None None
owner__exarkun exarkun
cc__davidsarah
@twisted-trac
Copy link
Author

davidsarah's avatar davidsarah commented

This would fix Tahoe-LAFS ticket https://tahoe-lafs.org/trac/tahoe-lafs/ticket/2028 .

@twisted-trac
Copy link
Author

zooko's avatar @zooko commented

For the upcoming Tahoe-LAFS v1.11 release, we're working-around this issue by requiring Twisted < 12.2.0:

tahoe-lafs/tahoe-lafs@d888b28

@twisted-trac
Copy link
Author

zooko's avatar @zooko commented

This interacts with twisted/nevow#43 (why does Nevow depend on Twisted >= 13.0?).

@twisted-trac
Copy link
Author

zooko's avatar @zooko commented

I suppose switching from pywin32 to new fork "pypiwin32" https://pypi.python.org/pypi/pypiwin32 (which exists because of this reason — http://sourceforge.net/p/pywin32/feature-requests/110/ , http://sourceforge.net/p/pywin32/bugs/669/ ) would solve this issue.

@twisted-trac
Copy link
Author

glyph's avatar @glyph commented

OK, I guess the Tahoe problem here is recently resolved, so the original reporters are probably not motivated to fix it; but, wow, this must be a terrible onboarding experience for windows users. We should definitely fix it.

@twisted-trac
Copy link
Author

glyph's avatar @glyph commented

Now that we test on appveyor, this (specifically, the no-modules builder) could be submitted as a regular Twisted ticket, without needing a buildbot configuration change.

@twisted-trac
Copy link
Author

oTree-org's avatar @oTree-org commented

Would be really great to have this fixed. I maintain a web app framework that is based on Django Channels. I can't upgrade to the latest Channels because it depends on twisted.internet.endpoints, which relies on pypiwin32. I have ruled out adding a dependency on pypiwin32 because it creates too many installation/deployment complications for my users.

Here is an issue someone logged in Django Channels that also references this issue:
django/channels#498

@twisted-trac
Copy link
Author

glyph's avatar @glyph commented

Thanks for the update. If you would like to contribute a fix, please have a look at the development process documentation.

Please just contribute a change to appveyor.yml that runs a tox environment without "alldeps" in its name so that we have a build tracking what happens without pywin32; at that point it should be relatively easy to add the try/except around the relevant import.

@twisted-trac
Copy link
Author

glyph's avatar @glyph commented

We would absolutely love to support Django Channels' use of Twisted, and I'm sorry I don't have the time personally to do this fix.

@twisted-trac
Copy link
Author

oTree-org's avatar @oTree-org commented

I wrapped the import in a simple try/except, and all Django Channels tests are passing.

In appveyor.yml, line 44, add this:

- PYTHON_HOME: C:\\PYTHON36-x64
  PYTHON_VERSION: "3.6"
  PYTHON_ARCH: "64"
  TOXENV: py36-nodeps-withcov-windows,codecov-publish
  TWISTED_REACTOR: "select"

In src/twisted/internet/endpoints.py, line 41, change this:

from twisted.internet.stdio import StandardIO, PipeAddress

To this:

try:
    from twisted.internet.stdio import StandardIO, PipeAddress
except ImportError:
    # fallback if pypiwin32 is not installed
    StandardIO = None
    PipeAddress = None

Also, this command passes (but it passed even before I made the change):

trial twisted.test.test_internet

Not sure how to really test or how to use AppVeyor. I'm totally new to Twisted. This is all the time I was able to spend on this for now. Leaving this here in case someone with more Twisted knowledge has a chance.

@twisted-trac
Copy link
Author

markrwilliams's avatar @markrwilliams commented

Replying to oTree-org:

Would be really great to have this fixed. I maintain a web app framework that is based on Django Channels. I can't upgrade to the latest Channels because it depends on twisted.internet.endpoints, which relies on pypiwin32. I have ruled out adding a dependency on pypiwin32 because it creates too many installation/deployment complications for my users.

Hi oTree-org! First, thanks for improving Twisted :)

I'm curious to know why pypiwin32 complicates installation and deployment. It's available as a wheel for Python 3.6. Would wheels for other Python versions make it less of an issue? Or is it just that it's a conditional transitive dependency - your framework depends on Django channels depends on Twisted, and now Windows users have remember to install pypiwin32 on Twisted's behalf?

I ask because we're considering moving to pywincffi for many reasons including ease of packaging, and if "making Twisted easier for Django channels" is one of those reasons, then I can prioritize that switch.

On the other hand, if any additional transitive dependency is a problem, then we need graceful degradation along the lines of what you've written in a later comment. We might need that anyway, because, as Glyph said, this is a really bad experience for Windows users.

@twisted-trac
Copy link
Author

oTree-org's avatar @oTree-org commented

Thanks for the reply! I'm fine with just supporting Python 3.6. Rather, the issue is about adding a complex dependency that is platform specific.

pywincffi definitely looks more manageable than pypiwin32, which is huge and doesn't install on non-Windows systems (which breaks the ability to "pip freeze" and push to Heroku, collaborate with others, etc; many of my framework's users are novice programmers so they have difficulty troubleshooting these things).

But it would be good to eliminate unnecessary dependencies. Django channels already installs a sizable set of dependencies:

asgiref
attrs
autobahn
Automat
channels
constantly
daphne
hyperlink
incremental
six
Twisted
txaio
zope.interface

With the standard Redis backend, add:

asgi-redis
msgpack-python
redis

I would like to avoid making the list longer, in order to keep my project lightweight, and to be able to understand what each part is doing. In particular, pywincffi+cffi are pretty complex, low-level, and platform-specific (which means I need to do more testing, and anticipate what happens if a Mac/Linux user accidentally installs it, etc).

At the end of the day, pywincffi would probably be OK, but the import cleanly avoids the dependency altogether, which is why it is more appealing to me :)

@twisted-trac
Copy link
Author

markrwilliams's avatar @markrwilliams commented

Replying to oTree-org:

Thanks for the reply! I'm fine with just supporting Python 3.6...
...In particular, pywincffi+cffi are pretty complex, low-level, and platform-specific (which means I need to do more testing, and anticipate what happens if a Mac/Linux user accidentally installs it, etc).

PEP 508 markers allow you to restrict dependencies to specific platforms:

(linux)$ pip install 'pywincffi ; platform_system == "Windows"'
Ignoring pywincffi: markers 'platform_system == "Windows"' don't match your environment

I did a little research with the PyPI BigQuery dataset to determine if people are using version of pip and setuptools that support PEP 508. Since you're OK supporting only 3.6, I limited the query to that version of Python:

SELECT
  details.installer.name,
  details.installer.version,
  COUNT(*) as total_downloads
FROM
  TABLE_DATE_RANGE(
    [the-psf:pypi.downloads],
    TIMESTAMP("20160114"),
    CURRENT_TIMESTAMP()
  )
WHERE
  REGEXP_MATCH(details.python, r"^3.6")
GROUP BY
  details.installer.name,
  details.installer.version
ORDER BY
  total_downloads DESC

Here were the results as of today:

Row details_installer_name details_installer_version total_downloads
1 pip 9.0.1 262450316
2 setuptools 28.8.0 3499929
3 setuptools 36.0.1 1606610
4 pip 8.1.2 1470281
5 setuptools 27.2.0 848139
6 setuptools 35.0.2 825094
7 pip 8.1.1 757826
8 setuptools 36.2.0 734394
9 setuptools 36.5.0 717401

All of these versions of pip and setuptools support PEP 508 markers. I think you'd be safe adding them to your project's dependencies so that pypiwin32 only gets installed on Windows.

At the end of the day, pywincffi would probably be OK, but the import cleanly avoids the dependency altogether, which is why it is more appealing to me :)

I'll look into pywincffi. But in the meantime, let us know if PEP 508 markers help the situation!

@twisted-trac
Copy link
Author

oTree-org's avatar @oTree-org commented

Pull request: #1088

@twisted-trac
Copy link
Author

oTree-org's avatar @oTree-org commented

Hi, is there anything I can do to move this pull request along?

@twisted-trac
Copy link
Author

glyph's avatar @glyph commented

Replying to oTree-org:

Hi, is there anything I can do to move this pull request along?

Hi oTree-org,

I'm sorry nobody has been by to code review your changes yet. Twisted does tend to go through seasons of waxing and waning interest, and things have been at a bit of a low ebb lately. But you can be the change you want to see in the world! :) There are lots of things you could potentially do to move things along.

  1. You can shorten the review queue so that reviewers have fewer choices when they do come along to review something - as a non-member / external / non-committer contributor to the project, this means you can look on https://twisted.reviews/ for pending reviews that have a "*" next to the author's name.
  2. You can advocate for your change, or offer to trade reviews, on the mailing list. You can subscribe and post here: https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
  3. You can do the same thing on IRC; #twisted-dev on freenode is the place to go.

I would suggest doing some combination of these things. Ultimately the real power move here is to get a commit bit, review every change but your own, then wait for any other reviewer to come along and notice there's only one review so they have to review yours :).

Reminding folks of the existence of https://twistedmatrix.com/highscores/ is sometimes helpful - it hasn't been super competitive lately, but then again, nobody's been talking about it recently.

Thanks for contributing to Twisted, and I hope that someone (maybe even me) will have time to get this reviewed and integrated soon!

@twisted-trac
Copy link
Author

exarkun's avatar @exarkun set owner to @exarkun
@exarkun set status to assigned

@twisted-trac
Copy link
Author

exarkun's avatar @exarkun set status to new

Approved

@twisted-trac
Copy link
Author

exarkun's avatar @exarkun set status to closed

In changeset 8292869

#!CommitTicketReference repository="" revision="8292869715d9471681ffff32296bc73b946868a3"
Merge pull request #1088 from oTree-org/6032-oTree-org-no-pywin32

Make twisted.internet.endpoints importable on Windows without pywin32

Author: oTree-org
Reviewer: exarkun
Fixes: ticket:6032

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

No branches or pull requests

2 participants