twisted.plugin should use twisted.python.modules

[glyph]

Currently it has its own ad-hoc path-walking stuff too, and that should be removed.

[glyph]

There is actually quite a serious bug in the ad-hoc crap.

If you have 2 entries on your sys.path, [".../development-install", ".../system-install"] in this configuration:

.../development-install/
.../development-install/twisted/
.../development-install/twisted/__init__.py
.../development-install/twisted/plugin/
.../development-install/twisted/plugin/__init__.py
.../development-install/twisted/plugin/twisted_stuff.py
.../development-install/twisted/plugin/dropin.cache
.../system-install/
.../system-install/twisted/
.../system-install/twisted/__init__.py
.../system-install/twisted/plugin/
.../system-install/twisted/plugin/__init__.py
.../system-install/twisted/plugin/twisted_stuff.py
.../system-install/twisted/plugin/dropin.cache

Both dropin.cache files will be built and updated as appropriate for your permissions, but there is a problem! When scanning the cache, first the place where the code is actually imported from (your development install) is scanned and its cache loaded. Then, thanks to Twisted's path magic, the system install's cache is also loaded - clobbering your cache!

The effect of this is that only plugins with names described in your system install will be loaded. They will be imported from your development install, so you can change their implementation in your development install and those changes will be reflected, but you can't remove them or add new plugins to your development install and have them show up.

The solution to this issue is to stop loading plugin packages from clobbered Twisted directories: if the directory exists, and is not the main path entry, but does contain an __init__.py itself, that means it can't possibly be valid. I think this is a substantial modification to the way that the plugin loader works, and given that, I am going to take this opportunity to use t.p.m., since that should also make it easier to answer questions like "is this the primary path entry" without ugly side-effects.

[glyph]

(In [19199]) switch twisted.plugin to use twisted.python.modules re #1445 re #1951

[glyph]

Ready for review in plugmod-1951

This comes along with a few new features on FilePath which I used to implement the new plugin loading thing: FilePath instances can now be used as dictionary keys, they set stat_float_times (because who wouldn't want that?), and there is now a documented and consistent exception from children(). I also cleaned up a few pyflakes warnings.

[exarkun]

• filepath.py
  • The ancestry of UnlistableError looks pointless to me. Just make it subclass Exception. Can _any_ good possibly come of variable inheritence?
  • I don't understand the XXX comment above UnlistableError.__init__. That code isn't being selective at all, it's just initializing an instance.
  • __dict__.update(__dict__)? Blech. Half the problem with WindowsError is that its attributes suck. And since UnlistableError can be constructed with either a WindowsError or an OSError, you'll have to look at originalException to figure out what attributes to examine. So having them on UnlistableError isn't useful. Also, I notice there are no tests for any of UnlistableError's attributes, nor any documentation.
  • Hooray, FilePath.children docstring.
    • (or a subclass thereof) is implied, I think. The epytext markup is incorrect, too. Exceptions go before the :: @raise OSError: ....
    • It is explained when a subclass of OSError is raised, but not when OSError itself is raised.
  • FilePath.__hash__ should probably at least hash a tuple of the class and the path. As is, 'twisted.python.filepathFilePath:::/foo/bar' hashes the same as FilePath('/foo/bar'). Not disasterous, but avoiding it would be preferable.
  • Was ignoring alwaysCreate in __hash__ intentional? The difficulty I have always found when thinking about making FilePath hashable is whether FilePaths with the same path but different alwaysCreate values should hash the same or not. It is surprising if they don't, but probably equally surprising if they do.
  • It occurs to me that the usage of stat_float_times in restat (previously the custom implementation of getmtime) isn't thread-safe. It should at least be documented that concurrent manipulation of this flag will probably result in something breaking.
• zippath.py
  • WindowsError didn't subclass Exception previously intentionally. We never raise it, we never should raise it. It's just a name, not an exception.

  • ose = OSError("Leaf zip entry listed")
    ose.errno = errno.ENOENT
    raise ose
    

    is surely spelled more simply as

    raise OSError(errno.ENOENT, "Leaf zip entry listed")
    

• getCache
  • Hooray for docstrings. Maybe you could write some more about what module is for though.
  • Why the getModule import inside getCache instead of at module scope?

I have some reservations about the unit tests added, but maybe they're okay. Mostly the amount of setup code required for the number of files involved in these tests is unpleasant. A memory-backed filesystem would probably simplify the code significantly. OTOH, a different API for creating a large number of files in the filesystem might help just as much. I'm going to hold off on looking at the tests closely though, until the entire Twisted test suite passes with this branch merged. Right now, several twisted.web tests fail (only when running the full suite for me). I can't tell why, mostly because of #2367.

[glyph]

I'll try to address as much of the simple stuff as possible. Some points of contention:

• FilePath
  • The ancestry of UnlistableError is there for compatibility. Changing those exceptions to be a new type will break any old code which is attempting to catch this type of error. After all the recent discussion of compatibility and deprecation, I thought it would be bad to break backward compatibility, especially since I didn't have to. Similarly with the gross attribute copying. Unless you comment otherwise, I am mentally going to transform your comments into a note that if I'm doing something for compatibility there ought to be tests that verify that it is compatible, since there were no tests for the previous error handling.
  • For the purposes of using them as dictionary keys, the value of alwaysCreate should probably be ignored in __hash__. I can't think of a use-case where I'd want it honored. (And for that matter it's really a crummy API. I can never remember why it isn't an argument to open().)
  • I am sorely tempted to just stat_float_times(True) at the module level and screw anybody who really wants broken integer values there. Is there any real reason not to do that? I suppose on some level it's incompatible, but the default Python API here is so messed up that it has long been declared that the current integer mode of operation will eventually be eliminated. Otherwise I can go ahead and make a thread-safe version of restat().
  • I was planning to make WindowsError raiseable for compatibility testing that would work cross-platform, and then forgot to do any testing with it. Should I make it unraiseable and do platform-dependent testing, or add some tests which raise it? (If it's really supposed to be unraiseable, perhaps it should subclass "object".)

Finally, a memory-backed filesystem would be nice, but I don't think it would really help in this particular case. sys.path is not a fixture which is factored for testability, nor are sys.modules et. al.. Maybe we could test t.p.modules a bit more with in-memory filesystem objects but I think these tests actually have to look like they have to look to expose the bug they were trying to expose. After all, I needed a test which would fail in trunk and pass in the branch; passing in the branch was easy, but failing clean in trunk contorted them into their current shape. Personally, I'm really happy that they don't work with the twisted.plugins package any longer, and at least use a temporary scratch area. (Still if somebody contributed MemoryPath I'd be pretty happy about that, not least of which for having a reference implementation for how error handling is supposed to work.)

[glyph]

No twisted.web tests fail for me, either with the branch itself or merged to trunk. I've tried on Linux, Windows, and Mac.

On the mac, twisted.conch.test.test_manhole.ManholeLoopbackStdio.testControlBackslash fails intermittently for me, but that happens in trunk as well.

On Windows, however, a spate of test_paths, trial.test_loader, and test_modules tests fail. Looks like I am going to need to fix those first.

[glyph]

OK, Windows tests are fixed. I have no idea if this will fix your twisted.web issue, but I doubt it.

[glyph]

A correction about my comments on conditional inheritance: compatibility was the reason, but conditional inheritance wasn't the right way to go about it. There should be Unlistable as well as two Unlistable subclasses, which mix in WindowsError and OSError respectively, and are raised conditionally depending on the type of exception that comes out of listdir. Otherwise it isn't possible to use the Exception-ized WindowsError to do any compatibility testing.

[exarkun]

I've attached trial trailer from a failing test run. I verified that this doesn't happen with current trunk@HEAD. For completeness, here is the output of whbranch as well:

Divmod: focus-872-4
Twisted: plugmod-1951
Daylife: trunk
Settings: trunk
StressQ: trunk
pypy: trunk
evil: trunk
python: release24-maint
ccSatellite: trunk
DivmodSites: trunk
CalculatorClock: trunk
busradio: trunk
WebSite: trunk
gaslight: trunk
pyglet: trunk
parrot: trunk
merit: trunk
Lupy: trunk
vector: trunk
factory: trunk
ctypeslib: trunk
market: trunk
af: trunk
iGmail: trunk
docutils: trunk

[jknight]

Please don't put stat_float_times(True) in restat. If you need to set it, do so in t.p.compat. (it is default in 2.5, afterall).

[glyph]

Replying to jknight:

Please don't put stat_float_times(True) in restat. If you need to set it, do so in t.p.compat. (it is default in 2.5, afterall).

Uhh... no? Please give some reasons why you think setting it in restat is a worse place than compat.

[jknight]

Changing global behavior of your program in some random library call? How can that be a good thing?

Compat is where we change things that are wrong with old pythons to make them look more like new ones. We don't do that scattered around the codebase.

Let's say you have a program that somehow manages to avoid using FilePath for part of its execution, and then later calls it. Suddenly your stat calls change from not having floating point timestamps to having them. It would be *really* annoying to track down failures caused by that.

[glyph]

Python's brokenness makes it impossible to do this really correctly. We considered a number of different approaches and considered this one the least bad.

The goal here is that FilePath should, unlike os.*, guarantee its return type as best it can.

If you have a program that requires stat_float_times(False) for some portion of its execution, but also uses FilePath, that program can do one of two things:

• Assume that things are fine until you use FilePath, or import compat in your case. In this case, anything that sets stat_float_times will break your code.
• Always set stat_float_times(False) around the offending code.

If people take the first option, nothing can be done to help them. FilePath can try to put the global state back where it found it, but - what about thread safety? The previous attempt at doing so made FilePath unusable by itself in a thread, even if no other code was using stat_float_times. It might be possible to make FilePath threadsafe by itself, but what about other code? With horrible reference-grabbing trickery (t.p.rebuild style) you might be able to force the exposed stat_float_times to have a lock, but there's no way you'll convince the legacy code to grab the lock around the full critical section.

If the code takes road number two, and we put the re-set into twisted.python.compat, then if any other code does the only reasonable thing to restore compatibility behavior, which is to say, set the flag before calling legacy code, then FilePath's guarantees will be broken. Of course there is a tiny race condition here as well, but like I said, this is the best out of a number of shockingly bad options.

Given the choice between preserving FilePath's guarantees and simplicity and the behavior of incredibly broken, ancient code which I have never seen the demonstrated existence of, I'll have to take the former. I tried very hard not to break compatibility in this branch any more than necessary, but it really just is not possible to have a cooperative invocation of stat_float_times.

[glyph]

If all you are suggesting is also setting stat_float_times in compat, I might be amenable to that. But I think that is really an unrelated issue :)

[jknight]

I'm not asking for a cooperative invocation of stat_float_times. I'm asking for break the other thing early, if it's going to be broken. Breaking it only after using FilePath is annoying to figure out the cause of, and unnecessary.

I don't know of anything calling stat_float_times(False) (and neither does google code search, fwiw), but there is plenty of code that doesn't call stat_float_times at all. It's the latter that really concerns me.

But let's pretend there is something that *really does* require stat_float_times(False), and forces it so upon startup. You are saying you want to force it back to True every time FilePath gets used? If the program author really does want to explicitly change it back to False, I assert that you *shouldn't* attempt to prevent them from doing so. Let them shoot themself in the foot. You can't actually prevent foot-shooting in python, in any case. What if someone overrides os.path.abspath to be return "haha". Ono, FilePath is broken!

[glyph]

Ready for review in plugmod-1951.

Tests pass for me on Windows, Mac, Linux.

(Sorry jknight, I still think this is the best of a bad situation as far as using stat_float_times is concerned. If it's any consolation, FilePath is used very early in the invocation of pretty much any Twisted program - any time you load plugins, which includes the startup of both twistd and trial.)

[jerub]

Oh gosh. That's the first time every single module that's touched by a branch has ever been pyflakes-clean. Bravo! Bravo! Encore!

I don't like this style:

+        self.savedPath = sys.path[:]
+        self.savedModules = sys.modules.copy()

I much prefer using:

        self.savedPath = list(sys.path)
        self.savedModules = dict(sys.modules)

But please use whatever you're comfortable with.

This makes sense to me only because I know that getPlugins is an iterator:

+        list(plugin.getPlugins(plugin.ITestPlugin, plugindummy.plugins))

Please add a comment to that line.

uncached and cached would be 0 and 1. What's 2 for? Good measure?

+        for x in range(3):      # both cached and uncached

Please use, and state, your own judgement about the above issues, and then assign the ticket back to me for review again.

[glyph]

list() and dict() strike me as implicit, whereas copy() and [:] strike me as explicit. Similarly, I agree with you that list(getPlugins(...)) is too magical and I wish there were a nicer way to spell that.

I've sprinkled a few comments in there to explain what was going on.

[jerub]

Please test this using gtkreactor, then merge if it's all good.

[jknight]

(Sorry jknight, I still think this is the best of a bad situation as far as using stat_float_times is concerned. If it's any consolation, FilePath is used very early in the invocation of pretty much any Twisted program - any time you load plugins, which includes the startup of both twistd and trial.)

As you noticed, apparently some code really *does* break with float file times (e.g. web2.dav), and so you are breaking it. Now the breakage has been worked around for anyone using FilePath (e.g. web2.dav), but some people *do* use non-filepath python operations, too. I'm actually going to change my recommendation to not call stat_float_times(True) *ever*, because there doesn't actually seem to be a reason to do so here. You haven't given one yet, at least.

Having a consistent return type isn't a reason: if the stat.st_mtime is an int, convert it to a float for getModificationTime's return value. You don't get the extra resolution, but you also don't break other code.

Additionally, getStatusChangeTime: this is different enough name from "st_ctime" that you really ought to mention that that is what it is returning in the docstring. Also, you might be interested to know that "ctime" is actually creation time on windows, not "status change time". Thus, "statusChangeTime" may not actually be a good name for this value. getCTime, maybe, I dunno...

Please also mention the reason for the original functions being deprecated, so that people don't just think they can simply replace one use with the other without thinking, and wonder why they were deprecated in the first place. Right now there's no indication that they're deprecated because they return ints.

[glyph]

(In [19271]) Convert Twisted's plugin system to use twisted.python.modules for code discovery

This fixes several bugs in the traversal and caching logic of the plugins system, and simplifies it by factoring the code such that the representation of the Python path and import system is now delegated entirely to an API designed to do it. The other major bug fixed here made plugin modules from a development and system installation of Twisted conflict, even if one path entry clearly "won" from the perspective of sys.path, PYTHONPATH, __import__, et. al.

In addition to the main fix here, several new features were introduced into the twisted.python.filepath, twisted.python.zippath and twisted.python.modules modules.

• twisted.python.filepath
  • New methods: getStatusChangeTime, getModificationTime, and getAccessTime, to access higher precision timestamps as floats.
  • New exception-handling behavior: FilePath.children will now raise UnlistableError, allowing users to catch non-fatal reasons why a directory might not be listable. More importantly this allows for portable error handling between UNIX and Windows.
  • FilePath instances are now usable as dictionary keys.
• twisted.python.zippath
  • Acquired error-handling similar to FilePath
  • Acquired new methods for inspecting timestamps, both the old deprecated get(m|c|a)time form and the newer form. This should allow both older and newer code to work with ZipPath instances. The modification stamp is actually pulled from the zipfile metadata.
• twisted.python.modules
  • Gratuitous Windows error-handling logic was removed and replaced with a simple except UnlistableError thanks to FilePath's new features.

Finally, test coverage was improved, and a few gratuitous pyflakes warnings and bits of trailing whitespace were eliminated.

Fixes #1951

Author: glyph

Reviewers: jerub, exarkun

[jknight]

Reopening due to unresponded to comments.

[glyph]

Even though it's closed, I figure I should respond to some of this:

Replying to jknight:

(Sorry jknight, I still think this is the best of a bad situation as far as using stat_float_times is concerned. If it's any consolation, FilePath is used very early in the invocation of pretty much any Twisted program - any time you load plugins, which includes the startup of both twistd and trial.)

As you noticed, apparently some code really *does* break with float file times (e.g. web2.dav), and so you are breaking it. Now the breakage has been worked around for anyone using FilePath (e.g. web2.dav), but some people *do* use non-filepath python operations, too. I'm actually going to change my recommendation to not call stat_float_times(True) *ever*, because there doesn't actually seem to be a reason to do so here. You haven't given one yet, at least.

It is impossible to avoid breaking code that needs a non-cooperative global flag to define its functionality. We have avoided breaking compatibility with the objects that we control, and that's as well as we can do. Python has already broken this compatibility with itself in Python 2.5.

As far as the resolution on the timestamps: we had numerous problems in the early days of the plugin system. Some involved the tests, and I vaguely recall some involving rounding problems with the cache generation time. I'm not interested in repeating that experience just because I can't remember what that code was for at the moment.

Additionally, getStatusChangeTime: this is different enough name from "st_ctime" that you really ought to mention that that is what it is returning in the docstring. Also, you might be interested to know that "ctime" is actually creation time on windows, not "status change time". Thus, "statusChangeTime" may not actually be a good name for this value. getCTime, maybe, I dunno...

We should probably have some tickets to flesh out how FilePath deals with time in general. There should probably be a few more methods to provide the widest common base, and document the fallbacks.

Please also mention the reason for the original functions being deprecated, so that people don't just think they can simply replace one use with the other without thinking, and wonder why they were deprecated in the first place. Right now there's no indication that they're deprecated because they return ints.

No. That was quite intentional. The functions were never documented in the first place, and now they are deprecated; don't use them. If you really want to use them then you already know what they do. They're not deprecated because they return ints; I would vastly prefer high-precision integers like C gives you, but that's not how Python does time.

Finally, I was in the middle of responding to your comments when you reopened it. Please don't do that. You are free to do the review yourself next time, but we aren't going to adjust the process so that everybody is happy with every feature. Next time, feel free to step in and do the official review :).

[glyph]

Welll... maybe you'll have a chance at that after all. Reverting due to a 2.5 test failure. New-style exceptions have a different dir() than new-style/mixed classes! Grr!

[jknight]

It is impossible to avoid breaking code that needs a non-cooperative global flag to define its functionality. We have avoided breaking compatibility with the objects that we control, and that's as well as we can do. Python has already broken this compatibility with itself in Python 2.5.

The only sure way to avoid breaking code is to not touch the flag at all, and function with it set both ways. FilePath could and should do so.

As far as the resolution on the timestamps: we had numerous problems in the early days of the plugin system. Some involved the tests, and I vaguely recall some involving rounding problems with the cache generation time. I'm not interested in repeating that experience just because I can't remember what that code was for at the moment.

There must not be tests that depend on mtimes at resolution of more than 1s, as that is never the case on native windows python builds < python 2.5. No matter whether stat_float_times is set or not.

But even if it is the case that it's determined that Twisted ought to "strongly suggest" that os.stat_float_times(True) is set, the place to do so is upon import in compat. Do it once, and if someone needs to set it back, allow them. Setting it every time is rude and not demonstrably necessary, vague accusations of possible test failures notwithstanding. I don't think this is a good idea, but it's surely better than what you're doing here.

Also, please don't modify trac tickets while I'm writing a comment either, it's a pain in the ass! ;0

[jknight]

Speaking directly to the possible failure in plugins.py, I don't believe that is the case with the current code, as it (correctly) uses a >= comparison, so that if the cache file has the same mtime as the source file, the cache is treated as dirty.

[glyph]

I was referring to the reopening, not the comment-writing - not much we can do about trac's... deficiencies.

Do you want to do a real review on this now that it's back up?

[jknight]

Replying to jknight:

Speaking directly to the possible failure in plugins.py, I don't believe that is the case with the current code, as it (correctly) uses a >= comparison, so that if the cache file has the same mtime as the source file, the cache is treated as dirty.

I'm not sure whether your changes broke this code, or whether we just got *extraordinarily* lucky and it just so happens to have the precise failure case being discussed, and it happened only on your branch by accident. I'm going to rerun this test on windows to see.

 http://twistedmatrix.com/buildbot/win32-py2.4-select/builds/406/step-default/2

[jknight]

Okay, definitely a regression. Running this one test with --until-failure on trunk, it never shows. On the dir-1951 branch (on windows), shows within 5 repetitions on python2.4 and seemingly immediately on python2.5. Going to bed now, but I'd be happy to rereview tomorrow or help track down this failure.

[glyph]

The test is really just buggy. It probably passes in trunk due to a series of timing accidents; the real solution here, I think, is to rewrite it to use os.utime in the test instead of hoping that the file will look different by the time the plugin system gets to it.

A few more aggressive tests on Windows seem to indicate that the filesystem metadata doesn't update until a few seconds after the data's been written.

[glyph]

jknight had implied to me on IRC that closing the file in the middle of the test in question didn't help, so I spent a while trying to figure out something else. I must have misunderstood though, since I just tried that and it seems to have worked like a charm.

I discovered that the difference between trunk and the branch was the difference between the (correct) use of setContent in the branch, and the ad-hoc cache-file-replacement code in trunk, which would pretty much just give up on Windows.

This branch is ready for review once more, in dir-1951

[jknight]

Taking the review, even though i'm not going to look at it until tonight.

[jknight]

filepath.py:

Missing verbs:

memory), OSError or a platform-specific variant be raised.

...variant will be raised.

@raise UnlistableError: If the inability to list the directory due to

...directory is due to...

    @param reraise: a boolean.  If true, re-raise exceptions from
    L{os.stat}; otherwise, simply swallow them and set this path's current
    stat value to 0. 

stat value being 0 is an implementation detail, not something the user ever sees. It should say something more like "mark this file as not existing, and remove any cached stat information."

---

zippath.py:

Copy&paste-o:

getModificationTime and getStatusChangeTime say "Return the file representing this archive's last access time."

---

plugins.py:

This part of getCache:

        try:
            dropinDotCache = pickle.load(dropinPath.open('rb'))
        except:
            dropinDotCache = {}
            lastCached = 0
        else:
            lastCached = dropinPath.getModificationTime()

is newly subject to a race condition, if someone else modifies dropinPath after you open it, before you get its mtime. The checks should be reversed: get the mtime before opening the file.

There's also another (not new) race condition: modifying a plugin between loading the module and writing the cache file. It should probably be fixed by setting the mtime of the cache file to the time at which the code *started* scanning for new plugins.

There's also a regression here (although, apparently untested): errors in writing the dropin.cache file are no longer caught and logged, but now cause twisted to die a horrible death upon startup.

One final minor note: I think all the changes such as:

-try:
-    import cPickle as pickle
-except ImportError:
-    import pickle
+def _determinePickleModule():
+    """
+    Determine which 'pickle' API module to use.
+    """
+    try:
+        import cPickle
+        return cPickle
+    except ImportError:
+        import pickle
+        return pickle

seem to obfuscare the code more than make it clearer. Just an IMO thing there, though.

[jknight]

Ah, one more thing: some of the errors I was seeing before were not in fact due to errors (now fixed) on this branch, but due simply to there being a leftover copy of the larger pluginextra.py file in twisted/plugins when starting the test. Since it doesn't unimport the module before starting, this makes the test fail.

[glyph]

Replying to jknight:

I believe I've dealt with everything, except:

There's also another (not new) race condition: modifying a plugin between loading the module and writing the cache file. It should probably be fixed by setting the mtime of the cache file to the time at which the code *started* scanning for new plugins.

I think it's pointless to attempt to fix this particular issue. Unless your Python files are being written atomically (and there's a good chance they aren't, especially since it's impossible to do that on Windows), there's the additional race condition of attempting to import a partial file.

There's also a regression here (although, apparently untested): errors in writing the dropin.cache file are no longer caught and logged, but now cause twisted to die a horrible death upon startup.

This was a really good catch, and I've added a new test for it.

One final minor note: I think all the changes such as:

-try:
-    import cPickle as pickle
-except ImportError:
-    import pickle
+def _determinePickleModule():
+    """
+    Determine which 'pickle' API module to use.
+    """
+    try:
+        import cPickle
+        return cPickle
+    except ImportError:
+        import pickle
+        return pickle

seem to obfuscare the code more than make it clearer. Just an IMO thing there, though.

I could go either way on that. On the one hand it makes it possible to factor out this selection into some other place in Twisted. On the other hand it is more lines of code.

The tiebreaker is that pyflakes currently complains about the former style, and I'd prefer that the entire codebase be pyflakes-clean. So I think it should stay.

(Also: "obfuscare"? Was that a typo? I hope not, that's an awesome word.)

It's ready for its (hopefully final) review.

[exarkun]

Fixing Pyflakes seems preferable to adjusting our coding standards solely on the basis of spurious Pyflakes warnings.

[jknight]

FYI: Windows does have MoveFile(...., MOVEFILE_REPLACE_EXISTING); that mscrt's rename doesn't use it is unfortunate, but not very surprising (given that mscrt is basically designed to piss of people trying to write portable programs)

The issue with the race condition is that if you run Twisted once, while writing out a file, not only will it die once, future invocations will be screwed up as well. I'll file a new bug for this one, since it's not a regression.

Please fix the docstrings in zipfile.ZipArchive for real:

Besides it still saying "access time" for getStatusChangeTime, I also just noticed that "Return the file representing this archive's last access time." doesn't make much sense given ambiguous order of operations of English.

Instead, I'd suggest:

"Return the archive file's last access time."

"Return the archive file's modification time."

"Return the archive file's status change time."

Then, approved for commit. :)

[jknight]

Replying to jknight:

The issue with the race condition is that if you run Twisted once, while writing out a file, not only will it die once, future invocations will be screwed up as well. I'll file a new bug for this one, since it's not a regression.

Done: #2395.

[glyph]

Re-docstringing now.

[glyph]

(In [19305]) Convert Twisted's plugin system to use twisted.python.modules for code discovery

This fixes several bugs in the traversal and caching logic of the plugins system, and simplifies it by factoring the code such that the representation of the Python path and import system is now delegated entirely to an API designed to do it. The other major bug fixed here made plugin modules from a development and system installation of Twisted conflict, even if one path entry clearly "won" from the perspective of sys.path, PYTHONPATH, __import__, et. al.

In addition to the main fix here, several new features were introduced into the twisted.python.filepath, twisted.python.zippath and twisted.python.modules modules.

• twisted.python.filepath
  • New methods: getStatusChangeTime, getModificationTime, and getAccessTime, to access higher precision timestamps as floats.
  • New exception-handling behavior: FilePath.children will now raise UnlistableError, allowing users to catch non-fatal reasons why a directory might not be listable. More importantly this allows for portable error handling between UNIX and Windows.
  • FilePath instances are now usable as dictionary keys.
• twisted.python.zippath
  • Acquired error-handling similar to FilePath
  • Acquired new methods for inspecting timestamps, both the old deprecated get(m|c|a)time form and the newer form. This should allow both older and newer code to work with ZipPath instances. The modification stamp is actually pulled from the zipfile metadata.
• twisted.python.modules
  • Gratuitous Windows error-handling logic was removed and replaced with a simple except UnlistableError thanks to FilePath's new features.

Finally, test coverage was improved, and a few gratuitous pyflakes warnings and bits of trailing whitespace were eliminated.

Fixes #1951

Author: glyph

Reviewers: jerub, exarkun