zope 3.6 now required but if you find that out at runtime you are in for a nasty surprise

[glyph]

If you happen to have Zope Interface 3.5.1 - which, I should note, is the version included with just about every release of OS X people are now using, you get this error:

File ".../twisted/application/reactors.py", line 12, in <module>
    from twisted.plugin import IPlugin, getPlugins
  File ".../twisted/plugin.py", line 34, in <module>
    from twisted.python.modules import getModule
  File ".../twisted/python/modules.py", line 68, in <module>
    from twisted.python.filepath import FilePath, UnlistableError
  File ".../twisted/python/filepath.py", line 555, in <module>
    class FilePath(AbstractFilePath):
  File ".../zope/interface/declarations.py", line 495, in __call__
    raise TypeError("Can't use implementer with classes.  Use one of "
TypeError: Can't use implementer with classes.  Use one of the class-declaration functions instead.

It should instead say something like "You need Zope Interface at least 3.6 (but ideally earlier than 4.0 unless you want to see warnings...)" so the user knows what to fix.

[glyph]

Itamar promises me a fix this week :).

[glyph]

Trying to be a bit more pro-active than we usually are about documenting issues like this,  I put something on the Twisted Matrix Labs blog that we can direct contributors to in the meanwhile. (But they're all subscribed to the blog's RSS already so they don't need us to point them to it, right?)

[exarkun]

(In [35490]) Branching to 'check-zi-version-5935'

[exarkun]

Well, this sucks. There's no obvious better approach though. There's still no version attribute on zope.interface and pkg_resources is only sometimes available. Neither is there any obvious way to actually unit test this behavior. One approach could be to mock all of all recent versions of zope.interface and then exercise the code against each of them (somehow - execfile/sys.modules hacking? or with copies of init.py to create new importable packages?) and verify the behavior is as desired against each. That sounds like a pretty big mess. We could mock a minimal subset instead, but that seems like a pretty low quality test to me.

[exarkun]

Results of manual testing:

exarkun@top:~/Scratch/Source/zope.interface-3.5.0/build/lib.linux-x86_64-2.7$ python
Python 2.7.3 (default, Aug  1 2012, 05:14:39) 
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import twisted
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/exarkun/Projects/Twisted/branches/check-zi-version-5935/twisted/__init__.py", line 22, in <module>
    raise RuntimeError("Twisted requires zope.interface 3.6 or later.")
RuntimeError: Twisted requires zope.interface 3.6 or later.
>>> 
exarkun@top:~/Scratch/Source/zope.interface-3.5.0/build/lib.linux-x86_64-2.7$ cd ..
exarkun@top:~/Scratch/Source/zope.interface-3.5.0/build$ cd ..
exarkun@top:~/Scratch/Source/zope.interface-3.5.0$ cd ..
exarkun@top:~/Scratch/Source$ cd zope.interface-3.6.0/build/lib.linux-x86_64-2.7/
exarkun@top:~/Scratch/Source/zope.interface-3.6.0/build/lib.linux-x86_64-2.7$ python
Python 2.7.3 (default, Aug  1 2012, 05:14:39) 
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import twisted
>>> 

 Partial buildbot run results

[exarkun]

Unfortunately this introduces a failure into the MSI build process. Review of other considerations would still be appreciated though.

[glyph]

Replying to exarkun:

Unfortunately this introduces a failure into the MSI build process. Review of other considerations would still be appreciated though.

You may already know this, but the type of error presented on the MSI buildbot can be fixed by doing 'python setup.py egg_info' before attempting to programmatically discover the version number. It might be good to add that builder step if the misconfiguration is specific to the MSI builder.

[glyph]

Oddly, http://buildbot.twistedmatrix.com/builders/windows7-64-py2.7-msi/builds/1124/steps/setproperty/logs/stdio looks like things are properly separated out into stdout/stderr, and a trivial capturing of stdout as the version would not introduce this bug. So I guess the buildbot configuration is in fact to blame.

[glyph]

Great, thanks for turning this around so fast.

1. Obviously this needs some test coverage. Maybe just an execfile in an environment with an artificially broken zope.interface. Alternately, put the whole thing into a private function which is called at module level, which obviates the need for del implementor_only.
2. Why RuntimeError? This ought to be just an ImportError with a more descriptive error message, no?
3. Consider catching AttributeError separately from ImportError. It would be helpful to have separate messages for "you've got zope.interface installed and it's the wrong version" versus "you don't have zope.interface installed". I say this because when considering my own point 2, I realize that the ImportError from Zope might have its own informative message in there that we don't want to quash.

I don't see the need for much review beyond this though, the approach is fine and if you can fix the MSI issue one way or another it can be merged. (Hooray for module-scope code which is always implicitly covered.)

-glyph

[exarkun]

The MSI issue is blocked on  https://bugs.launchpad.net/twisted-buildbot-configuration/+bug/1045314

[glyph]

Alternately, since this warning is always, always, always, always, 100% of the time completely spurious and useless, you could add a filter to the warnings module right before doing the import...

[exarkun]

(In [35622]) Address review comments

• Factor requirements checks into a function where they can more easily be tested (though this does not help demonstrate that init.py actually runs the checks)
• Add unit tests for the checks (with some grossness probably not easily avoided)
• Add the test module to the Python 3 ported list too

refs #5935

[exarkun]

(In [35623]) Branching to 'check-zi-version-5935-2'

[exarkun]

(In [35625]) try to make the error messages even nicer?

refs #5935

[itamar]

Python 3 needs to check for a newer version, 4.0, which is the first version with the __qualname__ fix.

[exarkun]

Python 3 needs to check for a newer version, 4.0, which is the first version with the qualname fix.

grooooooaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaan

[exarkun]

Sorry, this is horrible. It's also missing some obvious bits of documentation. If the rest looks good, then of course I'll fill those in. If the rest is too appalling then I'd really prefer not to bother.

[thijs]

The INSTALL documentation wasn't updated and maybe the Python3 porting howto should mention it as well.

[glyph]

Replying to thijs:

The INSTALL documentation wasn't updated and maybe the Python3 porting howto should mention it as well.

This is a good catch but it can be done separately. We don't need to overload tedious little chores like this with a million extra requirements. We could easily get lost in the weeds of doing environment-detecting versioning requirements with setuptools.

Reviewing.

[glyph]

Please Fix:

1. "miscellaneous"

2. _AssertRaisesContext is never going to go away on its own. On the other hand, who cares, maybe. Consider filing a ticket to delete it that someone might find in the tracker (along with appropriate warnings in the description explaining when it can be fixed).

3. Obviously you need to write a couple more docstrings for the test stuff; you already knew that though.

4. Deal with thijs's suggestion about mentioning py3 in an appropriate timeframe, probably not on this ticket. I don't know if there's already a ticket for py3 adjustments to installation documentation.

Some thoughts:

1. Woah, this is horrible. It strikes me that this is stuff that Python or distutils or setuptools or something ought to be doing for us. This is a topic we should keep in mind for discussion at PyCon.

2. SetAsideModule is a pretty interesting little utility for testing. There are other tests within Twisted that could benefit from it; at the very least, for example, twisted.internet.test.test_glibbase. (Probably something in the plugin tests too, and probably somewhere within Conch's tests...) I am a little surprised that there isn't anything like it already. It always worries me a little when a potentially generally-useful test utility like this is squirreled away in a specific test module. We need a place where not-fully-tested-themselves testing utilities can go to be staged so that at least we can prevent our fixture-maintenance efforts from being diffused into uselessly rewriting a million different versions of everything.

3. Similarly, _makePackages - you went so far as to make a test for this private testing utility in a test module, but it's still sitting around by itself?

4. SetAsideModule and _makePackages are obviously intended to go along with each other but their implementations aren't helpfully related. Plus, all the module-level attribute package description dictionaries don't have an immediately clear purpose. I can't help but bikeshed here:

   _zope35 = MockPackage({
       'zope': {
           'interface': {
               'Interface': _SuccessInterface,
               'implementer': _functionOnlyImplementer,
               },
           },
       })
   # ...
   def test_something(self):
       with _zope38:
           with self.assertRaises(ImportError) as raised:
               _checkRequirements()
   

5.  I forced some builds. They mostly look good, but it looks like one of the supported builders is failing and needs a dependency upgrade. At least the error message is sensible now! Maybe Tom Prince is already on the case...

6. except ImportError: is still quashing any error message that z.i might try to throw out.

7. Also, except: should always print the original traceback, shouldn't it?

Finally: I felt I should give all the feedback I discovered while reading this branch, but please don't feel obligated to address everything - or, in fact, anything - in the second section. The purpose of the review process is simply to make sure that changes are all improvements; this change (modulo the lack of docstrings for a few things that might make it a little trickier for maintainers) already meets that criterion. At this point I am sorry I didn't just give it a passing review right away :).

[exarkun]

(In [35648]) Merge check-zi-version-5935-2

Author: exarkun Reviewer: glyph Fixes: #5935

Add a check for the required version of zope.interface to twisted/__init__.py and present a meaningful exception when the requirement is not satisfied. This improves on the previous behavior where a strange and often confusing exception would be raised when a too-old version of zope.interface is installed.