declare dependencies explicitly when we add them

[zooko]

Please start considering the addition of a dependency without the addition of an explicit declaration of that dependency in our Python packaging metadata to be a regression in Twisted. The Tahoe-LAFS v1.6 build worked when Twisted-8.2 was the most current version of Twisted, but when Twisted-9.0 came out with [27079] in it, then the Tahoe-LAFS v1.6 build broke. We can work around this in Tahoe-LAFS by adding pyasn1 to the list of dependencies of Tahoe-LAFS v1.7.

This is a suboptimal work-around:

Tahoe-LAFS itself doesn't depend on pyasn1. If the version of Twisted that is installed is < 9.0 then pyasn1 isn't needed at all, but there is no clean way to express that, so when Tahoe-LAFS v1.7 comes out it will have a hard dependency on pyasn1 even if that dependency isn't needed for the version of Twisted that is installed. Likewise, if Twisted 11.0 comes out next year using a different library instead of pyasn1, then unfortunately people who install Tahoe-LAFS v1.7 with Twisted 11.0 are still going to be getting the hard dependency on pyasn1 even though at that point it will no longer be needed.

The right way to manage this stuff is for Tahoe-LAFS to declare that it depends on Twisted and in particular on the extra "conch" feature of Twisted and for Twisted to declare that if you are going to use its extra "conch" feature then it depends on pycrypto and pyasn1.

Here is a patch (untested) to Twisted's setup.py that does this:

Index: setup.py
===================================================================
--- setup.py    (revision 26987)
+++ setup.py    (working copy)
@@ -78,6 +79,13 @@
     if 'setuptools' in sys.modules:
         from pkg_resources import parse_requirements
         requirements = ["zope.interface"]
+        extras_requirements = {}
+        if sys.platform == 'win32':
+            extras_requirements['process_management'] = ['pywin32']
+            extras_requirements['iocp_reactor'] = ['pywin32']
+            extras_requirements['conch'] = ['pycrypto', 'pyasn1']
+        else:
+            extras_requirements['process_management'] = []
+            extras_requirements['iocp_reactor'] = []
+            extras_requirements['conch'] = []
         try:
             list(parse_requirements(requirements))
         except:
@@ -87,8 +95,18 @@
 """
         else:
             setup_args['install_requires'] = requirements
+            setup_args['extras_require'] = extras_requirements
         setup_args['include_package_data'] = True
         setup_args['zip_safe'] = False

See also #3696 and #3238.

The current blocker for #3238 is how to write an automated test of this behavior. I understand how to do that in principle, and we have automated tests of this sort of thing in the Tahoe-LAFS buildbot, but I don't have full access to the source code of the Twisted buildbot so it is hard for me to write tests of this functionality for Twisted: http://twistedmatrix.com/trac/ticket/3238#comment:24

[exarkun]

Please file a ticket for adding a declaration for the pyasn1 dependency (and apparently a pycrypto and pywin32 dependency - although I don't understand why all of these would only apply on Windows). Please engage people in discussion on IRC or the mailing list if you want to see a systematic change in development workflow (alternatively, if you prefer, consider this ticket as closed as fixed - I will have started to consider this).

[davidsarah]

Why has this been declared invalid? The issue isn't specific to the pyasn1 dependency; it is about declaring conditional dependencies in general.

Nitpick about the patch: extras_requirements['conch'] = ['pycrypto', 'pyasn1'] should be outside the sys.platform == 'win32' test, since that dependency is not conditional on the platform being Windows.

[exarkun]

Why has this been declared invalid?

It's invalid because there's nothing else I can do to resolve it.

The issue isn't specific to the pyasn1 dependency

But there is a specific pyasn1 dependency issue which can be resolved. I would be very happy to see a ticket for that, and for it to be resolved.

it is about declaring conditional dependencies in general.

Okay. I hereby proclaim that conditional dependencies in general should be declared. What else was expected to be outcome of this ticket?

[zooko]

Replying to exarkun:

it is about declaring conditional dependencies in general.

Okay. I hereby proclaim that conditional dependencies in general should be declared. What else was expected to be outcome of this ticket?

How about a unit test which goes red if someone commits a patch to trunk which adds a dependency without adding an explicit declaration of that dependency to the metadata?

[glyph]

Replying to zooko:

How about a unit test which goes red if someone commits a patch to trunk which adds a dependency without adding an explicit declaration of that dependency to the metadata?

That would be neat, but there's no clearly defined way to determine when someone adds a dependency. If there were such a method, why not just generate the metadata from that same inspection, rather than force us to type it twice?

[exarkun]

What if trial looked at sys.modules at the end of a test run, filtered out things from the stdlib, and presented a report of all the other top-level packages it found had been loaded? Maybe this information could be turned into something useful without much further massaging, and then compared against another list somewhere somehow. Or even feed directly into the release process (though I do like the idea of an explicit manual step for new dependencies).

[zooko]

I wonder if using a tool like virtualenv or tox we could configure a test to (a) exclude the system-wide Python packages from being importable, (b) pre-populate a virtualenv with a specific set of packages which are declared as being requirements, (c) prevent downloading packages from the network at build-time or test-time (or at least fail the code under test if it does that), (d) run build and tests in that environment. That seems like a test that would reliably be green unless we add a patch that imports some code which we didn't declare as a dependency, in which case the test would reliably go red. At the same time, if we add a patch that imports some code and also adds the package to the declared dependencies, then this test would reliably stay green (because step (b) pre-population of the environment would include that newly added dependency).

So could we at least re-open this ticket as "not invalid" since such a test is possible?

I intend to learn more about tox and virtualenv in the future...

[zooko]

Hm, now that I think about it, we have a test that does almost that over in Tahoe-LAFS:

 http://tahoe-lafs.org/buildbot/builders/clean

It is called "the desert island test" and the only way it isn't as cool as the test I envision in comment:7 is that Tahoe-LAFS's "desert island test" doesn't start with a controlled virtual environment and prevent the use of packages from outside that environment -- part (a). It does however do parts (b), (c) and (d), and it is useful to us.

[exarkun]

in which case the test would reliably go red

Why would this happen? Why wouldn't the tests just be skipped?

[zooko]

Replying to exarkun:

in which case the test would reliably go red

Why would this happen? Why wouldn't the tests just be skipped?

I'm sorry, I don't understand the question. (And apparently my lack of understanding caused me to drop the ball until now, 7 months later.)

We have a test similar to this, already, on Tahoe-LAFS. It runs a build and gives a red flag if the build process tries to download a package. It is called "The Desert Island build test".

Now, the one there is quite fragile, because if someone installs a package on the buildslave machine, then the code under test (the build system) will use that system-installed package and thus it will manage to hide the fact that it depends on that package from the tester. See  Tahoe-LAFS #1346 (desert-island test can pass incorrectly because packages are installed). Now, I think that virtualenv is a tool whose raison d'etre is to isolate a Python environment from the operating-system-wide Python environment, and thus it might serve well for this purpose -- to isolate the code under test (which in this case is the setup.py script and affiliated Python code) from the operating system's Python environment (which in this case includes Buildbot and all of buildbot's dependencies, including Twisted and pywin32).

So, what I propose as a way to test this ticket is a test which runs Twisted unit tests in an environment where only packages that were explicitly listed in the machine-parseable metadata are importable. This test will naturally go red if Twisted attempts to import any other package, since it won't be present in that environment and it will get an ImportError exception.

Ideally this would be done with some sort of isolation a la virtualenv instead of by trying to construct, ad hoc, a buildslave which can run Buildbot but which doesn't have pywin32 importable to the process of the code-under-test.

[exarkun]

The builder won't go red. We skip tests that require functionality provided by unavailable dependencies.

[zooko]

Replying to exarkun:

The builder won't go red. We skip tests that require functionality provided by unavailable dependencies.

I see. So I would like a test that goes red if the, e.g. the Twisted conch unit tests don't PASS. So if the Twisted conch unit tests SKIP then I want this test to go red. This is testing what Tahoe-LAFS wants: we want Twisted to provide conch. If it doesn't provide conch, for any reason, then it isn't satisfying our requirements.

I can think of a few ways to rig up a test like that. One would be to have a configuration option to tell the unit tests whether they should treat absence of a dependency as a SKIP or an ERROR.

A more generalized and principled way to do this might be to have a way to express what features of Twisted are required. If you are using Twisted and you don't require conch, then the getting an ImportError from pyasn1 should be a SKIP. If you are using Twisted and you do require conch, then getting an ImportError from pyasn1 should be an ERROR.

What do you think?

[glyph]

Replying to zooko:

A more generalized and principled way to do this might be to have a way to express what features of Twisted are required. If you are using Twisted and you don't require conch, then the getting an ImportError from pyasn1 should be a SKIP. If you are using Twisted and you do require conch, then getting an ImportError from pyasn1 should be an ERROR. What do you think?

I have thought of a similar feature for trial several times. We have a similar issue with Calendar Server where we want our runs of the Twisted suite to always verify certain dependencies (although not Conch in our case).

[exarkun]

Something along the lines of...

from twisted.trial.util import requiredOrOptionalImport
from twisted.trial.unittest import TestCase

pyasn1, pyasn1Skip = requiredOrOptionalImport("pyasn1")

class SomeConchTests(TestCase):
    if pyasn1Skip:
        skip = pyasn1Skip

    ...

along with some command line options like...

trial --require-module=pyasn1 ...

?

(Although you probably couldn't quite implement that API since you wouldn't be able to reach the command line options. Actually I can't think of any object that has access to the command line options that the test code has access to.)