eliminate pywin32 dependency #1274

New issue

Closed

opened 2010-11-30 20:58:52 +00:00 by daira · 34 comments

daira commented

2010-11-30 20:58:52 +00:00

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 source: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.)

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 source: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.)

daira added the

labels 2010-11-30 20:58:52 +00:00

daira added this to the 1.9.0 milestone 2010-11-30 20:58:52 +00:00

daira commented

2010-12-31 21:07:25 +00:00

Author

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

daira self-assigned this 2010-12-31 21:07:25 +00:00

daira commented

2010-12-31 22:17:20 +00:00

Author

Attachment no-pywin32.darcs.patch (12696 bytes) added

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

**Attachment** no-pywin32.darcs.patch (12696 bytes) added Eliminate direct dependencies of Tahoe-LAFS on pywin32 (updated to trunk post-#1195). refs #1274

no-pywin32.darcs.patch

12 KiB

daira removed their assignment 2010-12-31 22:19:42 +00:00

daira modified the milestone from 1.9.0 to 1.8.2

2011-01-06 02:03:06 +00:00

daira commented

2011-01-06 02:03:52 +00:00

Author

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

daira commented

2011-01-15 05:14:17 +00:00

Author

In [4941/ticket1306]:

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

In [4941/ticket1306]: ``` Eliminate direct dependencies of Tahoe-LAFS on pywin32 (updated to trunk post-#1195). refs #1274 ```

zooko commented

2011-01-18 08:24:19 +00:00

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.

zooko self-assigned this 2011-01-18 08:24:19 +00:00

wadey commented

2011-01-19 04:53:46 +00:00

Owner

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

zooko removed their assignment 2011-01-19 04:59:09 +00:00

daira was assigned by zooko

2011-01-19 04:59:09 +00:00

daira commented

2011-01-19 08:46:59 +00:00

Author

In changeset:d21f4071c3229e18:

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

In changeset:d21f4071c3229e18: ``` Eliminate direct dependencies of Tahoe-LAFS on pywin32 (rebased to trunk). refs #1274 ```

zooko commented

2011-01-19 17:19:37 +00:00

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.)

We can now update [quickstart.html](http://tahoe-lafs.org/source/tahoe-lafs/trunk/docs/quickstart.html) to no longer mention `pywin32`! Hooray! :-) (Also [wiki/AdvancedInstall](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.)

daira commented

2011-01-20 00:51:04 +00:00

Author

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 source: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 source: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.

Replying to [zooko](/tahoe-lafs/trac/issues/1274#issuecomment-382466): > We can now update [quickstart.html](http://tahoe-lafs.org/source/tahoe-lafs/trunk/docs/quickstart.html) to no longer mention `pywin32`! Hooray! :-) Not so fast! Tahoe itself no longer directly depends on pywin32, but Twisted does. Specifically, in source:src/allmydata/test/test_runner.py, Tahoe uses [twisted.internet.utils.getProcessOutputAndValue](http://twistedmatrix.com/trac/browser/trunk/twisted/internet/utils.py?rev=24810#L161), which indirectly uses [_dumbwin32proc.py](http://twistedmatrix.com/trac/browser/trunk/twisted/internet/_dumbwin32proc.py) and [_pollingfile.py](http://twistedmatrix.com/trac/browser/trunk/twisted/internet/_pollingfile.py) from Twisted. Tahoe also has one call to `getProcessOutput` in source: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.

daira commented

2011-01-20 05:14:27 +00:00

Author

Attachment eliminate-pywin32-completely.darcs.patch (46007 bytes) added

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

**Attachment** eliminate-pywin32-completely.darcs.patch (46007 bytes) added Eliminate dependencies on pywin32, even via Twisted. refs #1274

eliminate-pywin32-completely.darcs.patch

45 KiB

daira removed their assignment 2011-01-20 05:15:18 +00:00

zooko commented

2011-01-20 05:43:11 +00:00

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

I reviewed [attachment:eliminate-pywin32-completely.darcs.patch](/tahoe-lafs/trac/attachments/000078ac-bf5c-e6bc-a183-21d1e11b5028) and it looks good!

daira commented

2011-01-20 06:18:02 +00:00

Author

In changeset:6dd8b6f47126fdc0:

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

In changeset:6dd8b6f47126fdc0: ``` Eliminate dependencies on pywin32, even via Twisted. refs #1274 ```

daira commented

2011-01-20 06:36:41 +00:00

Author

Attachment eliminate-pywin32-news-and-doc.darcs.patch (24134 bytes) added

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

**Attachment** eliminate-pywin32-news-and-doc.darcs.patch (24134 bytes) added 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

eliminate-pywin32-news-and-doc.darcs.patch

24 KiB

daira commented

2011-01-20 09:21:15 +00:00

Author

In changeset:d5138b3237d27d0a:

src/allmydata/util/iputil.py: loosen regexps and ensure that 'LANG=en_US.UTF-8' is set in the environment, to minimize problems with localized output of IP-address-finding tools. refs #1274

In changeset:d5138b3237d27d0a: ``` src/allmydata/util/iputil.py: loosen regexps and ensure that 'LANG=en_US.UTF-8' is set in the environment, to minimize problems with localized output of IP-address-finding tools. refs #1274 ```

zooko commented

2011-01-20 09:34:47 +00:00

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

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

warner was assigned by zooko

2011-01-20 09:34:47 +00:00

warner commented

2011-01-20 20:40:33 +00:00

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.

Replying to [davidsarah](/tahoe-lafs/trac/issues/1274#issuecomment-382467): > > * 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.

daira commented

2011-01-20 20:54:44 +00:00

Author

Replying to [warner]comment:20:

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 [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).

Replying to [warner]comment:20: > Replying to [davidsarah](/tahoe-lafs/trac/issues/1274#issuecomment-382467): > > > > * 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 [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).

daira commented

2011-01-21 07:54:32 +00:00

Author

In changeset:b6c2c9591d61a680:

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

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

warner commented

2011-01-21 17:20:39 +00:00

Replying to [davidsarah]comment:21:

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 [non-test code]in 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.

Replying to [davidsarah]comment:21: > 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 [non-test code]in 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 commented

2011-01-21 17:21:20 +00:00

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

daira commented

2011-01-21 22:32:50 +00:00

Author

Replying to [warner]comment:23:

Replying to [davidsarah]comment:21:

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 [non-test code]in 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)

Replying to [warner]comment:23: > Replying to [davidsarah]comment:21: > > > 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 [non-test code]in 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)

daira added the

r/fixed

label 2011-01-21 22:32:50 +00:00

daira closed this issue

2011-01-21 22:32:50 +00:00

warner commented

2011-01-21 23:34:10 +00:00

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.

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.

daira commented

2011-01-22 03:42:32 +00:00

Author

In changeset:3eadc8a05375656f:

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

In changeset:3eadc8a05375656f: ``` NEWS, docs/quickstart.html: pywin32 is no longer required on Windows. refs #1274 ```

daira commented

2011-06-09 18:51:01 +00:00

Author

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

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>

daira removed the

r/fixed

label 2011-06-09 18:51:01 +00:00

daira reopened this issue

2011-06-09 18:51:01 +00:00

daira commented

2011-06-09 19:06:17 +00:00

Author

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.

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.

daira commented

2011-06-11 01:23:56 +00:00

Author

Attachment raise-twisted-version-dep.darcs.patch (13235 bytes) added

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

**Attachment** raise-twisted-version-dep.darcs.patch (13235 bytes) added Raise Twisted version requirement to >= 9.0.0, in order to avoid an indirect dependency on pywin32 for Windows. refs #1274

raise-twisted-version-dep.darcs.patch

13 KiB

daira commented

2011-06-12 01:11:38 +00:00

Author

Attachment raise-twisted-version-dep-2.darcs.patch (13733 bytes) added

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

**Attachment** raise-twisted-version-dep-2.darcs.patch (13733 bytes) added 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

raise-twisted-version-dep-2.darcs.patch

13 KiB

zooko commented

2011-06-12 13:47:43 +00:00

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

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 ```

daira commented

2011-06-12 17:09:21 +00:00

Author

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'

Replying to [zooko](/tahoe-lafs/trac/issues/1274#issuecomment-382488): > 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](http://www.python.org/dev/peps/pep-0345/#environment-markers) like this: ``` Requires-Dist: Twisted (>=9.0.0); sys.platform == 'win32' Requires-Dist: Twisted (>=2.4.0); sys.platform != 'win32' ```

daira modified the milestone from 1.8.2 to 1.9.0

2011-06-13 00:45:39 +00:00

zooko commented

2011-06-14 05:30:45 +00:00

Replying to [davidsarah]comment:32:

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!

Replying to [davidsarah]comment:32: > > 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](http://tahoe-lafs.org/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!

daira commented

2011-07-17 23:33:12 +00:00

Author

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 commented

2011-07-18 10:23:09 +00:00

Owner

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.

daira commented

2011-07-19 00:55:42 +00:00

Author

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

(http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1435/dependency-updates.darcs.patch) replaces [attachment:raise-twisted-version-dep-2.darcs.patch](/tahoe-lafs/trac/attachments/000078ac-bf5c-e6bc-a183-016746507ac0).

daira commented

2011-07-22 05:26:54 +00:00

Author

In changeset: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

In changeset: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 ```

daira added the

r/fixed

label 2011-07-22 05:26:54 +00:00

daira closed this issue

2011-07-22 05:26:54 +00:00