Opened 7 years ago
Closed 7 years ago
#5935 defect closed fixed (fixed)
zope 3.6 now required but if you find that out at runtime you are in for a nasty surprise
Reported by: | Glyph | Owned by: | Jean-Paul Calderone |
---|---|---|---|
Priority: | normal | Milestone: | Python 3.3 Minimal |
Component: | core | Keywords: | |
Cc: | Thijs Triemstra | Branch: |
branches/check-zi-version-5935-2
branch-diff, diff-cov, branch-cov, buildbot |
Author: | exarkun |
Description
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.
Change History (22)
comment:1 Changed 7 years ago by
Owner: | set to Itamar Turner-Trauring |
---|---|
Type: | enhancement → defect |
comment:2 Changed 7 years ago by
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?)
comment:3 Changed 7 years ago by
Author: | → exarkun |
---|---|
Branch: | → branches/check-zi-version-5935 |
(In [35490]) Branching to 'check-zi-version-5935'
comment:4 Changed 7 years ago by
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.
comment:5 Changed 7 years ago by
Keywords: | review added |
---|
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 >>>
comment:6 follow-up: 7 Changed 7 years ago by
Unfortunately this introduces a failure into the MSI build process. Review of other considerations would still be appreciated though.
comment:7 Changed 7 years ago by
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.
comment:8 Changed 7 years ago by
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.
comment:9 Changed 7 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Itamar Turner-Trauring to Jean-Paul Calderone |
Great, thanks for turning this around so fast.
- 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 fordel implementor_only
. - Why
RuntimeError
? This ought to be just anImportError
with a more descriptive error message, no? - Consider catching
AttributeError
separately fromImportError
. 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 theImportError
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
comment:10 Changed 7 years ago by
The MSI issue is blocked on https://bugs.launchpad.net/twisted-buildbot-configuration/+bug/1045314
comment:11 Changed 7 years ago by
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...
comment:12 Changed 7 years ago by
Milestone: | → Python 3.3 Minimal |
---|
comment:13 Changed 7 years ago by
(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
comment:14 Changed 7 years ago by
Branch: | branches/check-zi-version-5935 → branches/check-zi-version-5935-2 |
---|
(In [35623]) Branching to 'check-zi-version-5935-2'
comment:15 Changed 7 years ago by
comment:16 Changed 7 years ago by
Python 3 needs to check for a newer version, 4.0, which is the first version with the __qualname__
fix.
comment:17 Changed 7 years ago by
Python 3 needs to check for a newer version, 4.0, which is the first version with the qualname fix.
grooooooaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaan
comment:18 Changed 7 years ago by
Keywords: | review added |
---|---|
Owner: | changed from Jean-Paul Calderone to Glyph |
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.
comment:19 follow-up: 20 Changed 7 years ago by
Cc: | Thijs Triemstra added |
---|
The INSTALL
documentation wasn't updated and maybe the Python3 porting howto should mention it as well.
comment:20 Changed 7 years ago by
Status: | new → assigned |
---|
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.
comment:21 Changed 7 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Glyph to Jean-Paul Calderone |
Status: | assigned → new |
Please Fix:
- "miscellaneous"
_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).
- Obviously you need to write a couple more docstrings for the test stuff; you already knew that though.
- 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:
- 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.
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.
- 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?
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()
- 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...
except ImportError:
is still quashing any error message that z.i might try to throw out.
- 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 :).
comment:22 Changed 7 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
(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.
Itamar promises me a fix this week :).