eliminate pywin32 dependency #1274
Labels
No Label
0.2.0
0.3.0
0.4.0
0.5.0
0.5.1
0.6.0
0.6.1
0.7.0
0.8.0
0.9.0
1.0.0
1.1.0
1.10.0
1.10.1
1.10.2
1.10a2
1.11.0
1.12.0
1.12.1
1.13.0
1.14.0
1.15.0
1.15.1
1.2.0
1.3.0
1.4.1
1.5.0
1.6.0
1.6.1
1.7.0
1.7.1
1.7β
1.8.0
1.8.1
1.8.2
1.8.3
1.8β
1.9.0
1.9.0-s3branch
1.9.0a1
1.9.0a2
1.9.0b1
1.9.1
1.9.2
1.9.2a1
LeastAuthority.com automation
blocker
cannot reproduce
cloud-branch
code
code-dirnodes
code-encoding
code-frontend
code-frontend-cli
code-frontend-ftp-sftp
code-frontend-magic-folder
code-frontend-web
code-mutable
code-network
code-nodeadmin
code-peerselection
code-storage
contrib
critical
defect
dev-infrastructure
documentation
duplicate
enhancement
fixed
invalid
major
minor
n/a
normal
operational
packaging
somebody else's problem
supercritical
task
trivial
unknown
was already fixed
website
wontfix
worksforme
No Milestone
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Reference: tahoe-lafs/trac-2024-07-25#1274
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.)
This patch needs to be rebased due to the changes in #1195.
Attachment no-pywin32.darcs.patch (12696 bytes) added
Eliminate direct dependencies of Tahoe-LAFS on pywin32 (updated to trunk post-#1195). refs #1274
(The 1.8.2 milestone is for eliminating the direct dependency.)
In [4941/ticket1306]:
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.
I just reviewed this patch and it looks good to me. +1
In changeset:d21f4071c3229e18:
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.)
Replying to zooko:
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:
test_runner.py
andiputil.py
to use thesubprocess
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.
Attachment eliminate-pywin32-completely.darcs.patch (46007 bytes) added
Eliminate dependencies on pywin32, even via Twisted. refs #1274
I reviewed eliminate-pywin32-completely.darcs.patch and it looks good!
In changeset:6dd8b6f47126fdc0:
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
In changeset:d5138b3237d27d0a:
Nothing left but
docs-needed
andnews-needed
. Assigning to Brian.Replying to davidsarah:
The usual problem with using
subprocess
isSIGCHLD
: Twisted'sreactor and
subprocess.py
fight over who gets to register a handler.The reactor isn't strictly running during a
trial
unit test context. Itsort of is, but
reactor.run()
isn't called, and I've had problems usingreactor.spawnProcess
orutils.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 ifsubprocess.py
takes over the SIGCHLD handler, we need to look carefullyand make sure that Twisted gets it back again afterwards. I don't know if we
use
getProcessOutputAndValue
anywhere outside ofiputil.py
, so aproblem 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 thatuncertainty.
So maybe a quick manual test which does a
reactor.spawnProcess
inresponse to a WUI button press is in order.
Replying to [warner]comment:20:
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 usedgetProcessOutput
and now usesubprocess.Popen
. Note that it is done in adeferToThread
(I'm not sure whether that helps, since the SIGCHLD handler is process-global).In changeset:b6c2c9591d61a680:
Replying to [davidsarah]comment:21:
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
isblocking, of course (when you use
p.wait
or I thinkcommunicate
), so using it foriputil.py
at startup issomewhat 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.
Oh, and is changeset:b6c2c9591d61a680 the last code patch for this ticket, modulo the SIGCHLD issue?
Replying to [warner]comment:23:
Yes, that's why iputil now uses
deferToThread
.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)
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 usereactor.deferToThread
and a blockingsubprocess
call, because of windows. Oh well.In changeset:3eadc8a05375656f:
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.htmlThat dependency (actually in
twisted.python.lockfile
, whichtwisted.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.
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-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
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§ion=all
http://packages.ubuntu.com/search?keywords=twisted&searchon=names&suite=all§ion=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.)Replying to zooko:
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.
A bit off-topic, but distutils2's approach fixes that problem; in
setup.cfg
you can use environment markers like this:Replying to [davidsarah]comment:32:
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
, and10.0.0
. So claiming that Tahoe-LAFS works correctly withTwisted 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!
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.
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.
(http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1435/dependency-updates.darcs.patch) replaces raise-twisted-version-dep-2.darcs.patch.
In changeset:8b4082677477daf1: