eliminate pywin32 dependency

[davidsarah]

Anything that can be done using pywin32 can also be done, a little more klunkily, using ctypes. Currently we depend on pywin32 directly to get disk stats at src/allmydata/storage/server.py@4595#L177, and indirectly via Twisted. Twisted depends on it only for spawnProcess in ​http://twistedmatrix.com/trac/browser/trunk/twisted/internet/posixbase.py?rev=29163#L331. These dependencies could be easily eliminated, simplifying installation on Windows.

(We ended up not needing a change to Twisted.)

[davidsarah]

This patch needs to be rebased due to the changes in #1195.

[davidsarah]

Eliminate direct dependencies of Tahoe-LAFS on pywin32 (updated to trunk post-#1195). refs #1274

[davidsarah]

(The 1.8.2 milestone is for eliminating the direct dependency.)

[david-sarah@…]

In [4941/ticket1306]:

Eliminate direct dependencies of Tahoe-LAFS on pywin32 (updated to trunk post-#1195). refs #1274

[zooko]

I could try to review this. Release Master Brian Warner says he'll accept it based on the strength of manual testing by David-Sarah and hopefully other Windows users during the imminent release-candidate phase.

[wadey]

I just reviewed this patch and it looks good to me. +1

[david-sarah@…]

In d21f4071c3229e18:

Eliminate direct dependencies of Tahoe-LAFS on pywin32 (rebased to trunk). refs #1274

[zooko]

We can now update ​quickstart.html to no longer mention pywin32! Hooray! :-)

(Also wiki:AdvancedInstall, but I do not take any responsibility for that page so if it is going to get updated to reflect this change it will have to be updated by someone else.)

[davidsarah]

Replying to zooko:

We can now update ​quickstart.html to no longer mention pywin32! Hooray! :-)

Not so fast! Tahoe itself no longer directly depends on pywin32, but Twisted does. Specifically, in src/allmydata/test/test_runner.py​, Tahoe uses ​twisted.internet.utils.getProcessOutputAndValue, which indirectly uses ​_dumbwin32proc.py and ​_pollingfile.py from Twisted. Tahoe also has one call to getProcessOutput in src/allmydata/util/iputil.py​.

There are two options for getting rid of this indirect dependency:

• change the Twisted code to use ctypes. The two files I mentioned use 12 Win32 API functions (WaitForSingleObject, GetExitCodeProcess, CreatePipe, SetNamedPipeHandleState, GetCurrentProcess, DuplicateHandle, CloseHandle, CreateProcess, TerminateProcess, PeekNamedPipe, ReadFile, WriteFile), and two structures (SECURITY_ATTRIBUTES, STARTUPINFO). This is probably a few hours work to translate to ctypes.
• change test_runner.py and iputil.py to use the subprocess module.

I was originally thinking of the first of these options, but the second seems easier, and maybe even feasible for 1.8.2. I'll try it after I'm done pushing the 1.8.2 patches that have already been reviewed.

[davidsarah]

Eliminate dependencies on pywin32, even via Twisted. refs #1274

[zooko]

I reviewed attachment:eliminate-pywin32-completely.darcs.patch​ and it looks good!

[david-sarah@…]

In 6dd8b6f47126fdc0:

Eliminate dependencies on pywin32, even via Twisted. refs #1274

[davidsarah]

NEWS, docs/quickstart.html: pywin32 is no longer required on Windows. Update the download section of quickstart.html to include the release candidate. Apply to trunk just before release. refs #1306, #1274

[david-sarah@…]

In d5138b3237d27d0a:

(The changeset message doesn't reference this ticket)

[zooko]

Nothing left but docs-needed and news-needed. Assigning to Brian.

[warner]

Replying to davidsarah:

• change test_runner.py and iputil.py to use the subprocess module.

The usual problem with using subprocess is SIGCHLD: Twisted's reactor and subprocess.py fight over who gets to register a handler. The reactor isn't strictly running during a trial unit test context. It sort of is, but reactor.run() isn't called, and I've had problems using reactor.spawnProcess or utils.getProcessOutputAndValue from tests (the "potential zombie child" warning), which I've addressed by explicitly invoking the twisted call that installs its SIGCHLD handler.

But it almost certainly is running when iputil.py gets used. So if subprocess.py takes over the SIGCHLD handler, we need to look carefully and make sure that Twisted gets it back again afterwards. I don't know if we use getProcessOutputAndValue anywhere outside of iputil.py, so a problem there may not bite us until we add a call like that later (like maybe a call to du -s to measure storage usage), and I'd like to avoid that uncertainty.

So maybe a quick manual test which does a reactor.spawnProcess in response to a WUI button press is in order.

[davidsarah]

Replying to warner:

Replying to davidsarah:

• change test_runner.py and iputil.py to use the subprocess module.

The usual problem with using subprocess is SIGCHLD: Twisted's reactor and subprocess.py fight over who gets to register a handler. The reactor isn't strictly running during a trial unit test context. It sort of is, but reactor.run() isn't called, and I've had problems using reactor.spawnProcess or utils.getProcessOutputAndValue from tests (the "potential zombie child" warning), which I've addressed by explicitly invoking the twisted call that installs its SIGCHLD handler.

But it almost certainly is running when iputil.py gets used. So if subprocess.py takes over the SIGCHLD handler, we need to look carefully and make sure that Twisted gets it back again afterwards. I don't know if we use getProcessOutputAndValue anywhere outside of iputil.py, so a problem there may not bite us until we add a call like that later (like maybe a call to du -s to measure storage usage), and I'd like to avoid that uncertainty.

With this patch, we no longer use anything that calls reactor.spawnProcess (either in test or non-test code). Does that resolve the problem?

iputil.py is the only place where we previously used getProcessOutput and now uses subprocess.Popen. Note that it is done in a deferToThread (I'm not sure whether that helps, since the SIGCHLD handler is process-global).

[david-sarah@…]

In b6c2c9591d61a680:

src/allmydata/util/iputil.py: correct an error in the address-matching regexps introduced by the previous patch to iputil. refs #1274

[warner]

Replying to davidsarah:

With this patch, we no longer use anything that calls reactor.spawnProcess (either in test or non-test code). Does that resolve the problem?

iputil.py is the only place [in non-test code] where we previously used getProcessOutput and now use subprocess.Popen. Note that it is done in a deferToThread (I'm not sure whether that helps, since the SIGCHLD handler is process-global).

I don't think that resolves the problem (although I need to stare at the code and test it to be sure). The issue was using subprocess.Popen in a twisted program, regardless of whether or not that same program is using any reactor.spawnProcess calls. subprocess is blocking, of course (when you use p.wait or I think communicate), so using it for iputil.py at startup is somewhat excusable but using it anywhere later would be bad.

But it may be the case that we won't see any damage from this right now. When/if we start using spawned children at runtime (again, du -s is the best I can imagine right now), we may have to undo this, or live without that feature.

I'll try to run some tests this week to see what happens.

[warner]

Oh, and is b6c2c9591d61a680 the last code patch for this ticket, modulo the SIGCHLD issue?

[davidsarah]

Replying to warner:

Replying to davidsarah:

With this patch, we no longer use anything that calls reactor.spawnProcess (either in test or non-test code). Does that resolve the problem?

iputil.py is the only place [in non-test code] where we previously used getProcessOutput and now use subprocess.Popen. Note that it is done in a deferToThread (I'm not sure whether that helps, since the SIGCHLD handler is process-global).

I don't think that resolves the problem (although I need to stare at the code and test it to be sure). The issue was using subprocess.Popen in a twisted program, regardless of whether or not that same program is using any reactor.spawnProcess calls. subprocess is blocking, of course (when you use p.wait or I think communicate), so using it for iputil.py at startup is somewhat excusable but using it anywhere later would be bad.

Yes, that's why iputil now uses deferToThread.

But it may be the case that we won't see any damage from this right now. When/if we start using spawned children at runtime (again, du -s is the best I can imagine right now), we may have to undo this, or live without that feature.

We would have to use subprocess for those in any case, to avoid reintroducing the dependency on pywin32 on Windows (besides the SIGCHLD issue).

This ticket is done, I've tested it on win32 (including starting a gateway node and using the WUI to the pubgrid), and Dcoder ran the testsuite successfully on win64. Unfortunately, it seems that buildbot depends on pywin32, so we can't set up the Windows buildbots without it installed. I'll open another ticket for that. (#1334)

[warner]

Sigh, so it sounds like the new rule is that we aren't allowed to use reactor.spawnProcess or related functions in tahoe; instead we must use reactor.deferToThread and a blocking subprocess call, because of windows. Oh well.

[david-sarah@…]

In 3eadc8a05375656f:

NEWS, docs/quickstart.html: pywin32 is no longer required on Windows. refs #1274

[davidsarah]

It appears that Twisted 8.2.0 depends on pywin32 when importing twisted.internet.defer (which we need, and nevow also needs). See this tahoe-dev thread: ​http://tahoe-lafs.org/pipermail/tahoe-dev/2011-June/006428.html

[davidsarah]

That dependency (actually in twisted.python.lockfile, which twisted.internet.defer imports), seems to have been removed in this changeset: ​http://twistedmatrix.com/trac/changeset/26634#file1 , which would have been released in Twisted 9.0.

Perhaps we should bump the Twisted version requirement to >= 9.0, either only on Windows, or on all platforms. That version was released on 2009-11-24.

[davidsarah]

Raise Twisted version requirement to >= 9.0.0, in order to avoid an indirect dependency on pywin32 for Windows. refs #1274

[davidsarah]

Raise Twisted version requirement to >= 9.0.0 (at both setup- and install-time), in order to avoid an indirect dependency on pywin32 for Windows. refs #1274

[zooko]

I looked at the patch itself and saw no bug in it. I wonder about raising the required version of Twisted, though. It is a shame to prevent someone using Debian Lenny, or Ubuntu Hardy from installing Tahoe-LAFS v1.9.0 using their pre-packaged version of Twisted:

​http://packages.debian.org/search?keywords=twisted&searchon=names&suite=all&section=all

​http://packages.ubuntu.com/search?keywords=twisted&searchon=names&suite=all&section=all

I'd like to know what Brian thinks.

Making it conditional on the platform might not be a good idea--although that works when Tahoe-LAFS is being built by having its source:setup.py​ file executed, there is no way to encode it into the resulting src/allmydata_tahoe.egg-info/requires.txt file, so if someone were to build a Tahoe-LAFS egg, that egg would come with metadata indicating a Twisted version requirement depending on what platform the egg was built on, although the egg itself would be platform-independent. This sort of thing could lead to confusion in the future, and we have enough confusion with regard to packaging as it is. (We already do a similar dependency version conditioned on platform for pycryptopp: ​http://tahoe-lafs.org/trac/tahoe-lafs/browser/trunk/src/allmydata/_auto_deps.py?annotate=blame&rev=4976#L64 . I suppose that too is potentially a source of confusion.)

$ cat src/allmydata_tahoe.egg-info/requires.txt 
setuptools >= 0.6c6
zfec >= 1.1.0
simplejson >= 1.4
zope.interface
Twisted >= 2.4.0
foolscap[secure_connections] >= 0.6.1
Nevow >= 0.6.0
pycrypto == 2.0.1, == 2.1.0, >= 2.3
pyasn1 >= 0.0.8a
mock
pycryptopp >= 0.5.20

[davidsarah]

Replying to zooko:

I looked at the patch itself and saw no bug in it. I wonder about raising the required version of Twisted, though. It is a shame to prevent someone using Debian Lenny, or Ubuntu Hardy from installing Tahoe-LAFS v1.9.0 using their pre-packaged version of Twisted:

​http://packages.debian.org/search?keywords=twisted&searchon=names&suite=all&section=all

​http://packages.ubuntu.com/search?keywords=twisted&searchon=names&suite=all&section=all

I have to say, I don't really understand what the problem is with having a Tahoe instance built by 'python setup.py build' use a local instance of Twisted, when the installed instance is < 9.0.0.

I can see that someone might want to use their OS packagement tools to make sure that all instances of libraries are up-to-date (in order to promptly patch security bugs etc.), but that argument doesn't really hold water when the result would be to use a library version that is ~2 years old.

Making it conditional on the platform might not be a good idea--although that works when Tahoe-LAFS is being built by having its source:setup.py​ file executed, there is no way to encode it into the resulting src/allmydata_tahoe.egg-info/requires.txt file, so if someone were to build a Tahoe-LAFS egg, that egg would come with metadata indicating a Twisted version requirement depending on what platform the egg was built on, although the egg itself would be platform-independent.

A bit off-topic, but distutils2's approach fixes that problem; in setup.cfg you can use ​environment markers like this:

Requires-Dist: Twisted (>=9.0.0); sys.platform == 'win32'
Requires-Dist: Twisted (>=2.4.0); sys.platform != 'win32'

[zooko]

Replying to davidsarah:

I have to say, I don't really understand what the problem is with having a Tahoe instance built by 'python setup.py build' use a local instance of Twisted, when the installed instance is < 9.0.0.

Yeah, I guess the cost really isn't that bad. Also, while I feel a sort of vague warm happiness about our longstanding compatibility with Twisted 2.4.0, it isn't really tested. A glance at ​our buildbot waterfall shows the following versions of Twisted: 8.1.0, 10.1.0, 11.0.0, and 10.0.0. So claiming that Tahoe-LAFS works correctly with Twisted 2.4.0 is best understood as "good intentions".

(Aside: I'm glad Brian hasn't yet gotten around to removing some of the tool versions information from the waterfall, so that I was able to gather that information by scanning a single page.)

The two Twisted 8.1.0 buildslaves are both Debian 5 "Lenny". As you say, if we raise this requirement then the Tahoe-LAFS build on those buildslaves should just use a local copy of a newer version of Twisted. Shall we try it?

I'd still like to hear from Brian on this ticket!

[davidsarah]

Note that the new drop-upload feature (#1429) requires Twisted 10.1 on Linux. That could be an optional or platform-specific dependency, but it might be simpler for it not to be. Raising the dependency to >= 10.1 unconditionally would fix this ticket as well.

[gdt]

FWIW, pkgsrc (and hence NetBSD) has had 10.1.0 since 2010-11-04. Given 10.1.0's release announcement on 2010-07-04, it seems systems without 10.1 are now out of date.

[davidsarah]

​http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1435/dependency-updates.darcs.patch replaces raise-twisted-version-dep-2.darcs.patch​.

[david-sarah@…]

In 8b4082677477daf1:

Update the dependency on Twisted to >= 10.1. This allows us to simplify some documentation: it's no longer necessary to install pywin32 on Windows, or apply a patch to Twisted in order to use the FTP frontend. fixes #1274, #1438. refs #1429