Opened 6 years ago

Last modified 3 years ago

#3238 enhancement new

patch: declare that twisted requires pywin32 if it is to offer process management or iocp reactor on Windows

Reported by: zooko Owned by:
Priority: normal Milestone:
Component: release management Keywords:
Cc: therve, zooko Branch:
Author: Launchpad Bug:

Description (last modified by exarkun)

See also distutils-sig discussion:

http://mail.python.org/pipermail/distutils-sig/2008-May/009501.html

Index: setup.py
===================================================================
--- setup.py    (revision 23632)
+++ setup.py    (working copy)
@@ -75,6 +75,8 @@
     if 'setuptools' in sys.modules:
         from pkg_resources import parse_requirements
         requirements = ["zope.interface"]
+        if sys.platform == 'win32':
+            requirements.append('pywin32')
         try:
             list(parse_requirements(requirements))
         except:

Attachments (6)

twistedrequirespywin32.patch.txt (458 bytes) - added by zooko 6 years ago.
patch against svn trunk to setup.py
svndiff.txt (987 bytes) - added by zooko 6 years ago.
twisted requires pywin32 if on win32 to offer iocp-reactor or process-management
svndiff.2.txt (987 bytes) - added by zooko 6 years ago.
if using setuptools and installing on Windows, declare that Twisted needs pywin32 in order to provide process management or iocp reactor
twisted-master.cfg (26.3 KB) - added by exarkun 5 years ago.
Twisted's master.cfg
divmod-master.cfg (29.5 KB) - added by exarkun 5 years ago.
Divmod's master.cfg
twisted_factories.py (28.5 KB) - added by exarkun 3 years ago.

Download all attachments as: .zip

Change History (36)

Changed 6 years ago by zooko

patch against svn trunk to setup.py

comment:1 Changed 6 years ago by exarkun

  • Description modified (diff)

fixing description markup

comment:2 Changed 6 years ago by therve

  • Cc therve added
  • Owner changed from radix to zooko

Actually, pywin32 is not required, or should not be. I think it's required by the iocp reactor, and process management, but you should be able to run a twisted application without that.

What make you think it's required?

comment:3 Changed 6 years ago by zooko

Hm. Thinking about it, I guess I must have thought that it was required because the various projects that I know of that use Twisted on Windows all say that pywin32 is required. Perhaps that is because they all use the iocp reactor by default? I believe that the non-iocp reactor has limitations on the number of simultaneous open sockets which make it unusable for many projects.

comment:4 Changed 6 years ago by therve

It's probably more linked with process management than reactor. The default select reactor handles 1024 sockets, so it's enough for lots of application. And IOCP is still a movable target.

So I'd say it's more a requirement to put on applications that uses Twisted AND iocp/process than in Twisted itself. -1 for me.

comment:5 Changed 6 years ago by zooko

It does? I thought the default reactor failed after 32 fds.

I agree that this is an application requirement -- that twisted itself on Windows does not require pywin32.

comment:6 Changed 6 years ago by zooko

We could use the "extras" keyword to declare that Twisted requires pywin32 if it is running on Windows and the user wants to use process management or iocp reactor, something like this:

Index: setup.py
===================================================================
--- setup.py    (revision 23635)
+++ setup.py    (working copy)
@@ -75,6 +75,10 @@
     if 'setuptools' in sys.modules:
         from pkg_resources import parse_requirements
         requirements = ["zope.interface"]
+        extras_requirements = {}
+        if sys.platform == 'win32':
+            extras_requirements['process_management'] = ['pywin32']
+            extras_requirements['iocp_reactor'] = ['pywin32']
         try:
             list(parse_requirements(requirements))
         except:
@@ -84,6 +88,7 @@
 """
         else:
             setup_args['install_requires'] = requirements
+            setup_args['extras_require'] = extras_requirements
         setup_args['include_package_data'] = True
         setup_args['zip_safe'] = False
     setup(**setup_args)

Now someone who does "easy_install Twisted", or whose requirements list "Twisted", will not get Twisted installed, but if their requirements list a dependency on "Twisted [iocp_reactor]", then they *will* get pywin32 if they install on Windows. Here is the documentation about this feature:

http://peak.telecommunity.com/DevCenter/setuptools#declaring-extras-optional-features-with-their-own-dependencies

I've tested this patch on my machine and it didn't cause any problem.

comment:7 Changed 6 years ago by zooko

I wrote "Now someone who does "easy_install Twisted", or whose requirements list "Twisted", will not get Twisted installed" but I meant that they would not get pywin32 installed. Of course they would get Twisted installed.

comment:8 Changed 6 years ago by zooko

Okay, it turns out that a feature like this will be necessary if Tahoe is going to be able to automatically install Twisted on Windows and get iocp reactor and process management. If Twisted would please apply a patch like the one in the message above, then Tahoe could express the fact that it requires "Twisted with the option of using iocp reactor" or "Twisted with process management", by putting something like the following in the Tahoe metadata:

install_requires['Twisted[iocp_reactor,process_management]']

Then users of Tahoe who install Tahoe on Windows will, in addition to automatically getting Twisted installed so that Tahoe can use it, also automatically get pywin32 installed so that Twisted can use that.

Thanks!

Changed 6 years ago by zooko

twisted requires pywin32 if on win32 to offer iocp-reactor or process-management

comment:9 Changed 6 years ago by therve

Note that easy-installing twisted on windows now needs a C compiler to work. Maybe we could provide some eggs, but we don't for now, so I'm not sure setting the dependency on pywin32 is a high priority. Why not simply declare it in you setup.py?

comment:10 follow-up: Changed 6 years ago by zooko

I don't understand the part about requiring a C compiler. What requires a C compiler?

"Why not simply declare it in you setup.py?"

That would work, but I think it is better for Twisted to declare it because it is actually a requirement of Twisted (when you want iocp reactor or process management behavior from Twisted) rather than of Tahoe. For example, perhaps a future version of Twisted will be released that provides iocp reactor without pywin32. Then instead of all packages which use Twisted with iocp reactor changing their setup.py's to say something like "If Twisted >= 8.6 then we don't require pywin32 else we do.", Twisted can show that it supplies the iocp reactor to its users while no longer requiring pywin32. Basically the whole issue of whether or not you require pywin32 should be up to Twisted to configure properly instead of up to every package which uses Twisted.

Thank you for your time.

comment:11 in reply to: ↑ 10 Changed 6 years ago by therve

Replying to zooko:

I don't understand the part about requiring a C compiler. What requires a C compiler?

iocp needs a pyrex extension.

comment:12 Changed 6 years ago by zooko

I see. So, having Twisted declare that, if the user wants iocp reactor, then Twisted will require pyrex at build time and pywin32 at installation, does not make it harder for people to install Twisted.

It does not make it so that they require a C compiler when previously they didn't. Rather it formalizes the information of whether or not a package which requires Twisted also requires that stuff. Make sense?

comment:13 Changed 6 years ago by zooko

  • Summary changed from patch: declare requirement of pywin32 if installing on Windows to patch: declare requirement of pywin32 if requiring process management or iocp reactor on Windows

Here's the port of this patch to the current trunk -- [25344]. (It is the exact same patch but shifted by a couple of lines.)

Please apply.

HACM yukyuk:~/playground/twisted/trunk$ svn diff
Index: setup.py
===================================================================
--- setup.py    (revision 25344)
+++ setup.py    (working copy)
@@ -78,6 +78,13 @@
     if 'setuptools' in sys.modules:
         from pkg_resources import parse_requirements
         requirements = ["zope.interface"]
+        extras_requirements = {}
+        if sys.platform == 'win32':
+            extras_requirements['process management'] = ['pywin32']
+            extras_requirements['iocp reactor'] = ['pywin32']
+        else:
+            extras_requirements['process management'] = []
+            extras_requirements['iocp reactor'] = []
         try:
             list(parse_requirements(requirements))
         except:
@@ -87,6 +94,7 @@
 """
         else:
             setup_args['install_requires'] = requirements
+            setup_args['extras_require'] = extras_requirements
         setup_args['include_package_data'] = True
         setup_args['zip_safe'] = False
     setup(**setup_args)

Changed 6 years ago by zooko

if using setuptools and installing on Windows, declare that Twisted needs pywin32 in order to provide process management or iocp reactor

comment:14 Changed 6 years ago by zooko

  • Summary changed from patch: declare requirement of pywin32 if requiring process management or iocp reactor on Windows to patch: declare that twisted requires pywin32 if it is to offer process management or iocp reactor on Windows

clarify Summary:

comment:15 Changed 6 years ago by exarkun

Any thoughts on testing strategy for this change?

comment:16 Changed 6 years ago by zooko

Well, we want to check that:

(a) if your project requires iocp_reactor or process_management functionality from Twisted on Windows that your projects setup realizes that this means it requires pywin32.

(b) the patch doesn't break anything else.

Here is a minimal setup.py:

from setuptools import setup

setup(name="minimal_project_to_test_twisted_extra",
    install_requires=["Twisted[iocp_reactor"]
)

If you execute this with mkdir -p instdir/lib/python2.5/site-packages ; PYTHONPATH=instdir/lib/python2.5/site-packages python ./setup.py install --prefix=instdir, then it will tell you that it can't install because Twisted doesn't offer the "iocp_reactor" functionality. If you apply the patch to Twisted to declare that it does offer the iocp_reactor functionality then invoking this setup.py script should succeed.

How's that for a start?

comment:17 Changed 6 years ago by zooko

Hi, I was just testing out the process of installing Tahoe on Windows. It currently fails unit tests with this error message:

[Failure instance: Traceback: <type 'exceptions.NotImplementedError'>: spawnProcess not available since pywin32 is not installed.

If you accept the patch in this ticket, then I can move that issue from run-time to install-time, express it in a programmable and consistent way with all of the other Tahoe dependencies (see http://allmydata.org/trac/tahoe/browser/_auto_deps.py ), and even have the dependency on pywin32 automatically resolved by our install process.

Thanks!

comment:18 Changed 5 years ago by zooko

I just tried this patch against current trunk and it still works for me. I guess it is a bad idea to use spaces in names of packagey things like extras, so this patch changes the names from "process management" and "iocp reactor" to "process_management" and "iocp_reactor".

Index: setup.py
===================================================================
--- setup.py    (revision 26987)
+++ setup.py    (working copy)
@@ -78,6 +79,13 @@
     if 'setuptools' in sys.modules:
         from pkg_resources import parse_requirements
         requirements = ["zope.interface"]
+        extras_requirements = {}
+        if sys.platform == 'win32':
+            extras_requirements['process_management'] = ['pywin32']
+            extras_requirements['iocp_reactor'] = ['pywin32']
+        else:
+            extras_requirements['process_management'] = []
+            extras_requirements['iocp_reactor'] = []
         try:
             list(parse_requirements(requirements))
         except:
@@ -87,8 +95,18 @@
 """
         else:
             setup_args['install_requires'] = requirements
+            setup_args['extras_require'] = extras_requirements
         setup_args['include_package_data'] = True
         setup_args['zip_safe'] = False

comment:19 Changed 5 years ago by zooko

I noticed this ticket: #3397 which ends with this note from Glyph:

"""
Since Twisted is functioning properly here I'm closing this ticket. However, perhaps you can suggest a way to make this less confusing for other users and open a new ticket for better documentation.
"""

I propose this ticket (#3238) as a way to make this less confusing for other users and even to serve as a kind of documentation for Python-literate users.

comment:20 Changed 5 years ago by zooko

So I think that this ticket is blocked on someone implementing a test for it. comment:17 is my suggestion for how such a test would work. Should I submit some Python code that might work as a buildmaster config to execute such a test? Could someone give me the buildmaster config for a similar test from http://www.divmod.org/trac/ticket/2630 ? It is the builder named "q-nevowinstall".

comment:21 Changed 5 years ago by zooko

Sorry, I meant comment:16 is my suggestion for a test for this ticket.

Changed 5 years ago by exarkun

Twisted's master.cfg

Changed 5 years ago by exarkun

Divmod's master.cfg

comment:22 Changed 5 years ago by exarkun

Hopefully those two files will help.

comment:23 Changed 5 years ago by zooko

#3696 (Add extras_require to top-level setup.py to list Twisted optional dependancies) seems to be about implementing a superset of this ticket.

comment:24 Changed 5 years ago by zooko

  • Cc zooko added
  • Owner changed from zooko to exarkun

twisted-master.cfg says:

from twisted_factories import (
    TwistedDocumentationBuildFactory, FullTwistedBuildFactory,
    GoodTwistedBuildFactory, TwistedReactorsBuildFactory, Win32RemovePYCs,
    LinuxPyOpenSSLBuildFactory, DebianPyOpenSSLBuildFactory,
    OSXPyOpenSSLBuildFactory, Win32PyOpenSSLBuildFactory,
    TwistedEasyInstallFactory, TwistedPyPyBuildFactory,
    PyPyTranslationFactory, TwistedIronPythonBuildFactory,
    TwistedScheduler)

So I think I need a twisted_factories.py file in order to understand how TwistedEasyInstallFactory works and propose a patch to it or a new factory modelled on it which implements the test described in comment:16.

comment:25 Changed 4 years ago by zooko

Opened #4438 which emphasizes that if Twisted fails to declare an existing dependency then it is a bug, but if Twisted fails to declare a new dependency then it is a regression.

comment:26 Changed 4 years ago by zooko

Opened #4438 which emphasizes that if Twisted fails to declare an existing dependency then it is a bug, but if Twisted fails to declare a new dependency then it is a regression.

comment:27 Changed 4 years ago by zooko

sorry about the accidental repetition--i'm juggling a baby with my other hand.

comment:28 Changed 3 years ago by <automation>

  • Owner exarkun deleted

comment:29 Changed 3 years ago by zooko

Could someone who has access to the Twisted buildmaster config please share twisted_factories with me?

Changed 3 years ago by exarkun

comment:30 Changed 3 years ago by exarkun

There you go.

Note: See TracTickets for help on using tickets.