Opened 2 years ago

Closed 2 years ago

#5683 defect closed fixed (fixed)

Raise the minimum required version of zope.interface to 3.6.0

Reported by: itamar Owned by: itamar
Priority: normal Milestone: Python-3.x
Component: core Keywords:
Cc: vperic, thijs Branch: branches/zope-requirement-5683
(diff, github, buildbot, log)
Author: vperic Launchpad Bug:

Description (last modified by thijs)

As mentioned in http://pypi.python.org/pypi/zope.interface/4.0.0, newer versions of zope.interface give deprecation warning for implements(), which is used widely by Twisted. This was done in order to support Python 3; the newer version is a class decorator.

As part of Python 3 support we should probably switch to the new style. It was added in zope.interface 3.6, in 2010, so switching completely doesn't seem like an issue: http://pypi.python.org/pypi/zope.interface/3.6.0#id5

Using class decorators shouldn't be a problem post-12.1, since we'll have dropped Python 2.5 support by then (#5553).

We should ensure Twisted mentions it requires zope.interface 3.6 in the appropriate places (import time, dependency documentation).

Attachments (2)

zope.patch (1.1 KB) - added by vperic 2 years ago.
Update docs
zope2.patch (729 bytes) - added by vperic 2 years ago.
v2, without the change in setup.py

Download all attachments as: .zip

Change History (31)

comment:1 Changed 2 years ago by thijs

  • Cc thijs added

comment:2 Changed 2 years ago by mithrandi

This is still relevant for Python 3 support, but note that z.i 4.0.1 removed the DeprecationWarning again:

4.0.1 (2012-05-22)

Dropped explicit DeprecationWarnings for "class advice" APIS (these APIs are still deprecated under Python 2.x, and still raise an exception under Python 3.x, but no longer cause a warning to be emitted under Python 2.x).

comment:3 Changed 2 years ago by thijs

  • Description modified (diff)

comment:4 Changed 2 years ago by vperic

There is a package, zope.fixers, which has a 2to3 fixer to change this. It's very useful for the manual copy/pasting work (but obviously, all changes should have tests and this should be checked manually). I have written a short script that will run the fixer on all the files in a directory:

#!/usr/bin/python
import sys

from lib2to3.refactor import RefactoringTool, get_fixers_from_package
fixers = get_fixers_from_package('zope.fixers')

tool = RefactoringTool(fixers)
tool.refactor_dir(sys.argv[1], write=True)

I recommend running it under Python 3 (as it's much faster) and not running it on the whole twisted package at once, as it can be resource intensive.

Changed 2 years ago by vperic

Update docs

comment:5 Changed 2 years ago by vperic

I've attached a patch which notes in INSTALL and setup.py (the only places I found it mentioned) that the required zope.interface is now 3.6.0 or later. The topfile is marked as "feature" per itamar's suggestion on IRC (it's technically a .misc but then it wouldn't be displayed and it's an important change).

I suggest to change the scope of this ticket to cover just this, and then to open separate tickets for individual modules or packages; we could also open a new "meta" ticket for tracking this (basically, I'd like to get this small patch in so I can start fixing this).

comment:6 Changed 2 years ago by itamar

You can change the scope, sure, just change the subject of the ticket. Then open a ticket per package, say, for the API conversion.t

comment:7 Changed 2 years ago by vperic

  • Keywords review added
  • Summary changed from zope.interface 4.0 causes DeprecationWarning for implements() to Raise the minimum required version of zope.interface to 3.6.0

Ok, changing the scope of this to just raising the version requirement and submitting for review. Maybe the wording of the topfile can be improved? I'll open the new tickets once this gets in (since they don't make much sense otherwise).

comment:8 Changed 2 years ago by itamar

  • Keywords review removed
  • Owner set to vperic

Assuming the buildbots are happy running a branch, please merge.

comment:9 Changed 2 years ago by vperic

  • Author set to vperic
  • Branch set to branches/zope-requirement-5683

(In [35037]) Create branch zope-requirement-5683

comment:11 Changed 2 years ago by vperic

Ok, this isn't tested (the change in setup.py) so it cannot go in. I'm not sure exactly *how* to test it either, so if anyone has a suggestion...

(As exarkun pointed on chat, the "practical" issue is that some of the buildbots don't have zope.interface >= 3.6.0 and this wasn't caught by the run above)

comment:12 Changed 2 years ago by vperic

Phew! I've created tickets for individual modules now (tickets #5831 - #5846) and attached patches to most of them; I'll add the rest once at least this one gets in. I didn't submit them for review yet, since we want this to get merged first. I've manually checked (with coverage) that these are all executed - is this enough to consider them tested? In any case, this is something to be handled in each individual review.

About the issue here, I've found a Launchpad bug for zope.interface (There is no version attribute anywhere in zope.interface) and I see that exarkun and glyph both participated in it, but no solution was found (and current versions of zope.interface have no method of checking as far as I can see). As such, I'm not sure how this should be tested. I guess there should be at least one buildbot which actually installs Twisted (in a virtualenv I guess -- have we considered using Tox?) and that would help. The other solution is to drop the change from setup.py (leaving the dependency unqualified, like now) and just manually make sure all our buildbots are updated. Personally, I feel this is an inferior solution, because we lose one check somewhere down the line (even if we can't test it), but it does get around the "all code should be covered" requirement.

Whatever happens, our buildbots should be updated - should I open a separate ticket for this, or just mail the appropriate people, or.. ?

comment:13 Changed 2 years ago by itamar

You can check version, more or less, by checking for the existence of the decorator attribute ("implementor"?). So personally I think a unit test that checks for that would be fine. JP didn't like that idea for some reason, though.

comment:14 Changed 2 years ago by vperic

Right, I think that's a reasonable solution, checking for the exact feature we need. Where should such a test live, I don't think the setup.py is tested anyway? twisted.test.test_setup? Or folded into another test suite?

comment:15 Changed 2 years ago by exarkun

Sorry, I'm sure I'm just slow, but I don't see what purpose this unit test is going to serve. It's certainly not going to demonstrate that setup.py is declaring accurate dependencies. If we start making changes to make Twisted depend on zope.interface 3.6, plenty of existing unit tests will fail. So what good is a test that checks for the existence of implementor in zope.interface going to do?

comment:16 Changed 2 years ago by vperic

I'm not sure I understand what you mean by It's certainly not going to demonstrate that setup.py is declaring accurate dependencies. setup.py currently doesn't declare a minimum version, adding this is surely an improvement? If you don't consider it an improvement, I can still drop that change (remember, we still depend on zope.interface 3.3.0, it's just not tested anywhere). As for If we start making changes to make Twisted depend on zope.interface 3.6, plenty of existing unit tests will fail., I'm again not sure I follow? Why would updating a dependency make unit tests fail?

I agree with Itamar - we need a specific feature from zope.interface (well, @provides is also used in a few plces but they go together) and a reasonable way of testing we've got the right version is to check if we can get the specific feature we need.

comment:17 Changed 2 years ago by vperic

Of course, I would also be very happy if you could suggest an alternative that would be acceptable to you.

comment:18 Changed 2 years ago by mithrandi

If functionality in Twisted relies on zope.interface 3.6.0, then the unit tests that cover that functionality will fail when run against a version of zope.interface that is too low. Thus, there's no need for some arbitrary version-detection test, either code works or it doesn't.

However, this branch doesn't introduce any code that relies on functionality in zope.interface 3.6.0, it introduces a change in setup.py to declare a dependency on zope.interface 3.6.0; thus there should be a test that verifies the behaviour of setup.py, which has nothing to do with the behaviour of zope.interface at all.

Changed 2 years ago by vperic

v2, without the change in setup.py

comment:19 Changed 2 years ago by vperic

  • Keywords review added
  • Owner vperic deleted

Ok then, no changes in setup.py, since I couldn't figure out how to test it. The patch now only changes the INSTALL file. Here's a snippet from IRC where I talked about this with exarkun:

[13:16] <vperic> exarkun, idnar: Do you have a suggestion on *how* to test setup.py? If not, I'll just drop the change in setup.py and keep just the change in the docs (this for ticket 5683)
[13:18] <exarkun> vperic: nope, I have no suggestions on that subject.
[13:19] <vperic> exarkun: ok, would you then accept a patch which does not change setup.py?
[13:20] <exarkun> I dunno.  But I would not reject a patch that does not change setup.py just because it does not change setup.py.

Submitting for review again; ah, I forgot to change the topfile: just imagine it says "removal" instead of "feature".

comment:20 Changed 2 years ago by itamar

  • Keywords review removed
  • Owner set to itamar

I'm going to accept this as is... and open a separate ticket to add version requirements (ideally unconditionally rather than tied to setuptools) to setup.py. I have separately started looking into upgrading zope.interface on buildslaves so we can actually start using 3.6's functionality; the benefit of merging this now is that it warns users they should start upgrading.

comment:21 Changed 2 years ago by itamarst

  • Resolution set to fixed
  • Status changed from new to closed

(In [35161]) Minimum version of zope.interface is now 3.6.

Author: vperic
Review: itamar
Fixes: #5683
This is just a documentation change, noting to users that they should switch to zope.interface 3.6 or later.

comment:22 follow-up: Changed 2 years ago by itamar

Jean-Paul believes we should revert this, and only require 3.6 in release after next; this is viable only if we do a release in the very near future. Until we figure it out, I suggest holding off on merging any of of the @implementer tickets.

comment:23 in reply to: ↑ 22 Changed 2 years ago by glyph

Replying to itamar:

Jean-Paul believes we should revert this, and only require 3.6 in release after next;

Not that I necessarily disagree, but: would Jean-Paul (or you) care to give a specific reason for the revert?

comment:24 follow-up: Changed 2 years ago by itamar

We did not announce in advance that we're dropping support for older versions of zope.interface, so it would be good to have 12.2 to announce the soon-to-be-discontinued-support and the release after that to drop it.

comment:25 in reply to: ↑ 24 ; follow-up: Changed 2 years ago by glyph

Replying to itamar:

We did not announce in advance that we're dropping support for older versions of zope.interface, so it would be good to have 12.2 to announce the soon-to-be-discontinued-support and the release after that to drop it.

Perhaps we should add this grace period for dependency version compatibility to CompatibilityProcess?

comment:26 in reply to: ↑ 25 Changed 2 years ago by glyph

Replying to glyph:

Replying to itamar:

We did not announce in advance that we're dropping support for older versions of zope.interface, so it would be good to have 12.2 to announce the soon-to-be-discontinued-support and the release after that to drop it.

Perhaps we should add this grace period for dependency version compatibility to CompatibilityProcess?

Ahem. CompatibilityPolicy.

comment:27 Changed 2 years ago by itamarst

  • Resolution fixed deleted
  • Status changed from closed to reopened

(In [35278]) Revert #5683; we will require zope.interface 3.6 only after 12.2.

Reopens: #5683.

comment:28 Changed 2 years ago by exarkun

(In [35405]) Remove the zope.interface version requirement change

refs #5683

comment:29 Changed 2 years ago by exarkun

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [35416]) Merge zope-requirement-5683
Author: vperic
Reviewer: itamar
Fixes: #5683

Update the install document to indicate Zope Interface 3.6 or newer is now required.

Note: See TracTickets for help on using tickets.