Twisted 2.5 no longer needs win32 platform detection

[long.in.the.tooth]

Installing buildbot from CVS, using Python 2.4 and Twisted 2.5 (for Python 2.4)

Running

buildbot start .

Received this error: T

raceback (most recent call last):
  File "c:Python24Scriptsuildbot", line 4, in ?
    runner.run()
  File "C:Python24Libsite-packagesuildbotscripts
unner.py", line 725, in run
    start(so)
  File "C:Python24Libsite-packagesuildbotscriptsstartup.py", line 73, in start
    return launch(config)
  File "C:Python24Libsite-packagesuildbotscriptsstartup.py", line 110, in launch
    from twisted.scripts._twistw import run
ImportError: cannot import name run

Seems twisted 2.5 now does the platform detection itself, so we can simplify startup.py by taking out the platformType check on line 108.

Just import the 'run' function directly, as per the diff below:

Index: buildbot/scripts/startup.py
===================================================================
RCS file: /cvsroot/buildbot/buildbot/buildbot/scripts/startup.py,v
retrieving revision 1.4
diff -r1.4 startup.py
109,112c109
<         if platformType == "win32":
<             from twisted.scripts._twistw import run
<         else:
<             from twisted.scripts.twistd import run
---
>         from twisted.scripts.twistd import run

[long.in.the.tooth]

'course might want to check the twisted version first. Something a bit more complicated like this might be in order:

===================================================================
RCS file: /cvsroot/buildbot/buildbot/buildbot/scripts/startup.py,v
retrieving revision 1.4
diff -r1.4 startup.py
107a108
>         from twisted import version
109c110
<         if platformType == "win32":
---
>         if platformType == "win32" and ((version.major == 2) and (version.mino
r == 0)):

[warner]

this one is biting us here at work..

[warner]

zandr reports success on a windows-native system with the second patch, spelled as follows:

        # this is copied from bin/twistd. twisted-2.0.0 uses _twistw, later
        # versions do it for us.
        from twisted import version
        if (platformType == "win32"
            and (version.major == 2 and version.minor == 0)):
            from twisted.scripts import _twistw
            run = _twistw.run
        else:
            from twisted.scripts import twistd
            run = twistd.run
        run()

I want to look at the specific twisted versions involved a bit more closely, but I'll aim to commit this in the next few days.

[warner]

It looks like twisted-2.0 through 2.4 need _twistw, while 2.5 uses twistd. I updated the conditional to reflect this and committed the patch.

[bhearsum]

I think there's some fallout from this. I'm running Python 2.4 with Twisted 2.4.0 on Windows 2003 and I cannot start a buildslave. I'm getting a Twisted related traceback that cites startup.py:

File "d:/buildbot/python24/scripts/buildbot", line 4, in ?

runner.run()

File "d:uildbotpython24Libsite-packagesuildbotscripts

unner.py", line 960, in run

start(so)

File "d:uildbotpython24Libsite-packagesuildbotscriptsstartup.py", lin e 73, in start

return launch(config)

File "d:uildbotpython24Libsite-packagesuildbotscriptsstartup.py", lin e 121, in launch

run()

File "d:uildbotpython24libsite-packages wistedscripts\_twistw.py", line 51, in run

app.run(runApp, ServerOptions?)

File "d:uildbotpython24libsite-packages wistedapplicationapp.py", line 278, in run

runApp(config)

File "d:uildbotpython24libsite-packages wistedscripts\_twistw.py", line 34, in runApp

app.installReactor(configreactor?)

File "d:uildbotpython24libsite-packages wistedapplicationapp.py", line 34, in installReactor

reflect.namedModule(reactorTypes[reactor]).install()

File "d:uildbotpython24libsite-packages wistedinternetwin32eventreacto r.py", line 216, in install

main.installReactor(r)

File "d:uildbotpython24libsite-packages wistedinternetmain.py", line 2 3, in installReactor

assert not sys.modules.has_key('twisted.internet.reactor'),

AssertionError?: reactor already installed

[bhearsum]

I'm getting this with Python 2.5.1 + Twisted 2.5, too.

[bhearsum]

If I launch my slave with twistd directly, it works fine.

[trent_nelson]

Here's the patch I used for fixing this; note that it doesn't bother checking the twisted version, it just tries _twistd.run and if that fails, reverts to twistd.run:

Index: startup.py =================================================================== --- startup.py (revision 651) +++ startup.py (working copy) @@ -105,12 +105,16 @@

argv.append("--reactor=win32")

sys.argv = argv

• # this is copied from bin/twistd. twisted-2.0.0 uses _twistw.
• if platformType == "win32":
• from twisted.scripts import _twistw
• run = _twistw.run
• else:
• from twisted.scripts import twistd
• run = twistd.run
• run()

- + # Earlier versions of 2.x twisted on Windows used _twistw; later + # versions use twistd. + run = None + if platformType == "win32": + from twisted.scripts import _twistw + try: + run = _twistw.run + except AttributeError?: + pass + if not run: + from twisted.scripts import twistd + run = twistd.run + run()

(Patch also at  http://dark.teleri.net/~tnelson/scripts.py.patch)

[trent_nelson]

Replying to trent_nelson:

Here's the patch I used for fixing this; note that it doesn't bother checking the twisted version, it just tries _twistd.run and if that fails, reverts to twistd.run:

And here's a version of the patch that's actually readable:

Index: startup.py
===================================================================
--- startup.py	(revision 651)
+++ startup.py	(working copy)
@@ -105,12 +105,16 @@
             argv.append("--reactor=win32")
         sys.argv = argv
 
-        # this is copied from bin/twistd. twisted-2.0.0 uses _twistw.
-        if platformType == "win32":
-            from twisted.scripts import _twistw
-            run = _twistw.run
-        else:
-            from twisted.scripts import twistd
-            run = twistd.run
-        run()
-
+        # Earlier versions of 2.x twisted on Windows used _twistw; later
+        # versions use twistd.
+        run = None
+        if platformType == "win32":
+            from twisted.scripts import _twistw
+            try:
+                run = _twistw.run
+            except AttributeError:
+                pass
+        if not run:
+            from twisted.scripts import twistd
+            run = twistd.run
+        run()

[warner]

bhearsum: it looks like something was importing the reactor before our call to twisted.scripts.twistd.run() was able to install the win32-specific reactor. The rule is that if you want to use a non-default reactor, you must install the one of your choice before anything imports the reactor: if none has been installed before import, then the default one will be installed at that time.

But I'm not sure what might be importing the reactor early. Take a look in the start() function in startup.py: there are three different places where it might call launch(). Running on win32 ought to take the second call. The third call should only be used if we can do os.fork(), and the call to 'rc = Follower().follow()' that happens just before the third launch() point *will* import the reactor.

I can't tell (from the line number of your traceback) which of these three paths is being taken. Line number 73 doesn't correspond to a launch() call in my tree, so I suspect there is some amount of skew between your tree and mine.

Could you experiment a bit and see if you can find out which launch() call is being used? If you're feeling ambitious, patch the Twisted that you're using (either by unpacking a source tree and adding a PYTHONPATH= to use your unpacked tree instead of the /usr/lib one, or by editing the /usr/lib one directly) to print out a stack trace in twisted/internet/selectreactor.py (in the install() function). Our goal is to find out where the reactor is being imported early.

trent_nelson: did your patch specifically fix the assertion error that bhearsum observed? I don't yet understand how it could have, so I just wanted to doublecheck. I like the idea of catching AttributeError, so I'm inclined to switch to your approach, but I want to make sure I understand this assertion problem first.

[bhearsum]

I'm still investigating, but for what it's worth, line #73 is the second launch() (our tree is missing changeset 529, that's why the line numbers are off).

[bhearsum]

Alright, I think I've found something useful. I put a traceback in twisted.internet..selectreactor.install(), here's what it prints out:

File "d:/buildbot/python24/scripts/buildbot", line 3, in ?

from buildbot.scripts import runner

File "d:uildbotpython24libsite-packagesuildbotscripts

unner.py", line 816, in ?

from buildbot import master

File "d:uildbotpython24libsite-packagesuildbotmaster.py", line 14, in ?

from twisted.internet import defer, reactor

File "d:uildbotpython24libsite-packages wistedinternet

eactor.py", line 12, in ?

selectreactor.install()

File "d:uildbotpython24libsite-packages wistedinternetselectreactor.py", line 183, in install

traceback.print_stack()

It will print this when I run 'buildbot start ...' and even if I simply run 'buildbot'.

Now, when I run 'buildbot start ...' I also get the traceback that's in my above comment. As best I can tell, runner.py is installing the SelectReactor?, and then startup.py is attempting to install the Win32EventReactor. I'm not a Twisted buff, though I'm not sure =.

Does this yield anything useful to you, Brian? I'm happy to do more investigation if it doesn't.

Also, I tested out trent_nelson's patch and it did not fix anything.

[warner]

What is the context of that buildbot/scripts/runner.py:816 "from buildbot import master" call? That's the problem: the buildmaster imports the reactor. So the rule for runner.py and startup.py is that this code must *not* import the buildmaster.

I don't see any instances of "from buildbot import master" in the trunk's runner.py . Could this be something that used to be in the tree which got moved out? Or perhaps something that got added locally?

[bhearsum]

Ahh...here we go. Line 816 is where the config file tester landed -- which subclasses BuildMaster? to test the ConfigFile?. Looks like it's my fault, sorry for all the noise here. I'll try and get this fixed over in ticket #37