Opened 5 years ago

Last modified 2 weeks ago

#3696 enhancement new

Add extras_require to top-level to list Twisted optional dependancies

Reported by: thijs Owned by:
Priority: normal Milestone:
Component: core Keywords: review
Cc: zooko Branch:
Author: Launchpad Bug:


Setuptools provides a feature for listing optional dependancies: extras_require. It seems like this functionality should be in trunk/twisted/python/ or trunk/twisted/python/ or something, especially given that trunk/twisted/conch/topfiles/ needs to know about the pycrypto dependency somehow too.

Attachments (2)

extra_requires-3696.diff (2.0 KB) - added by herrwolfe 3 weeks ago.,, news file
extras_require-3696-2.diff (3.4 KB) - added by herrwolfe 2 weeks ago.
extra_requires implementation, tests, news file

Download all attachments as: .zip

Change History (25)

comment:1 Changed 5 years ago by glyph

  • Owner changed from glyph to radix

For previous discussion, see #976, which spawned this ticket.

comment:2 Changed 5 years ago by glyph

Oops I meant #967.

comment:3 Changed 5 years ago by zooko

See also #3238 (patch: declare that twisted requires pywin32 if it is to offer process management or iocp reactor on Windows).

comment:4 Changed 5 years ago by zooko

See also (pycrypto package is required for manhole). We're going to declare that Tahoe-LAFS depends on pycrypto. But that isn't really right. Really it is that Tahoe-LAFS depends on Twisted's "manhole" feature, which depends on pycrypto. So for example, if a future release of Twisted implemented manhole without using pycrypto, then Tahoe-LAFS would not require pycrypto anymore. So the "best" way to solve this is for Tahoe-LAFS to specify that it depends on Twisted plus the "extra" of manhole, and for Twisted to declare that if you want the "extra" of manhole then you need pycrypto.

comment:5 Changed 5 years ago by zooko

  • Cc zooko added

comment:6 Changed 4 years ago by davidsarah

Also, twisted.conch depends on pycrypto and pyasn1.

comment:7 Changed 4 years ago by zooko

Opened #4438 which emphasizes that if Twisted fails to declare an existing dependency then it is a bug, but if Twisted fails to declare a new dependency then it is a regression.

comment:8 Changed 4 years ago by <automation>

  • Owner radix deleted

comment:9 Changed 4 months ago by glyph

#7092 is a duplicate of this.

comment:10 Changed 4 months ago by glyph

#6767 is related to this.

Changed 3 weeks ago by herrwolfe,, news file

comment:11 Changed 3 weeks ago by herrwolfe

  • Keywords review added

The patch extra_requires-3696.diff allows the user to install optional dependencies. Since this method only installs the optional dependencies if requested, it seems as though it would make sense if the list were relatively exhaustive. The optional packages I have listed in the patch were specified by exarkun in comment:9:ticket:6767.

This also closes #6767. Using extra_requires seems to be a more manageable solution than project level dev-requirements.txt files. This is because each set of dependencies can be specified in a single top level file. If needed, each project's optional dependencies could be set up as an entry in the extra_requires dictionary.

Here is an example of how this new functionality can be used: in order to install all of the twisted developer tools and tools that help when working with Conch, one would type

pip install -e . "twisted[conch]"
pip install -e . "twisted[dev]"

I am not sure how this should be tested. I would think that buildbots could install package sets and then the current tests could be run as a check. But, I don't know what additional tests could be written.

Also - if this patch is accepted, I think adding a small section to the narrative docs detailing how this new functionality should be used would be helpful.


Last edited 3 weeks ago by herrwolfe (previous) (diff)

comment:12 Changed 3 weeks ago by glyph

Thank you very much for contributing this patch, herrwolfe. We really need help in this area; I'll do my best to make sure that this patch gets reviewed soon.

comment:13 Changed 3 weeks ago by dstufft

Quick thoughts:

Looking at the INSTALL file, pywin32 is not an optional dependency, it is a mandatory dependency but only on Windows. Thus you should do

import sys;

if sys.platform == "win32":
    requirements += ["pywin32"]

Then inside of a setup.cfg you should do

requires-dist =
    zope.interface >= 3.6.0
    pywin32; sys_platform == 'pywin32'

The setup.cfg is for wheel files and it will override the install_requires. It's a duplication but it makes this work better for people.

You also probably want a "ssl" extra that installs pyOpenSSL too.

Note: I've just talked to glyph and i'm not sure if pywin32 is an actual dependency or if INSTALL is just full of lies. I'm leaving this comment here just incase y'all want to do that since it says how to do it.

comment:14 Changed 3 weeks ago by zooko

According to pywin32 is required if you use (or possibly just import?) endpoints.

comment:15 Changed 3 weeks ago by herrwolfe

It seems like there are a fair amount of tests that can be run *if* many different optional dependencies are installed. Could a possible testing strategy be: install the optional dependency then ensure that those tests that should run, both do run and pass?

An example of this would be: install the optional "docs" dependencies, make sure all tests that require the presence of sphinx and pydoctor actually run.

Also - I have a feeling that the tests included with Twisted currently cover a much larger number of optional dependencies than I have included in my patch.

comment:16 Changed 3 weeks ago by herrwolfe

Per a conversation on IRC, the initial testing strategy should be geared more towards verifying the setuptools is working as expected and *NOT* that packages are being downloaded.

comment:17 Changed 3 weeks ago by glyph

I'd like to clarify my intent here: we should really have test coverage of everything, including packages being downloaded, things being on PyPI under the right names, etc. But this is a code change, and we should have test coverage for it that makes sure that the code does what we think it does, irrespective of issues like PyPI downtime. Generally speaking Twisted code changes should be accompanied by simple unit tests, and we should strive to do other, more comprehensive integration testing efforts elsewhere, out of the critical path of getting changes landed. Any test where failures can occur due to random infrastructure outages rather than a code change being wrong should not be part of the usual review process / buildbot lifecycle.

So: let's just write some simple unit tests for this, and make sure that setuptools is being informed of the right metadata rather than trying to write a whole lot of scaffolding to make sure the whole thing Really Really Works™.

(Thanks again herrwolfe.)

comment:18 Changed 3 weeks ago by herrwolfe

Glyph - thanks for your comment.

It looks like extra_requires functionality adds extra package information to the requires.txt file generated by distutils. I should be able to examine either the output of some of the distutils functions or, failing that, examine the generated requires.txt file.

Thanks again!

Last edited 3 weeks ago by herrwolfe (previous) (diff)

comment:19 Changed 3 weeks ago by dstufft

For the record, egg_info is what pip uses to generate requires.txt, which is a setuptools file not a distutils file. Reading that file is also how pip determines what to install.

comment:20 Changed 2 weeks ago by herrwolfe

A small update - it looks like the setuptools version of distutils.core.Distribution is the one that supports extra_requires. The version specified in distutils.core.Distrubution does not.

comment:21 Changed 2 weeks ago by dstufft

Yes, however distutils will silently ignore it and any tool that matters for installation will force setuptools even if you import distutils.

If this is for the tests, you can do this:

$ python -c "import setuptools, tokenize; __file__='/path/to/'; exec(compile(getattr(tokenize, 'open', open)(__file__).read().replace('\\r\\n', '\\n'), __file__, 'exec'))" egg_info

This should do basically the same thing as if you were importing setuptools in the without forcing you to actually import setuptools in the

comment:22 Changed 2 weeks ago by dstufft

Although now that I read the code, I see that it's already importing setuptools so that snippet doesn't actually help :(

Changed 2 weeks ago by herrwolfe

extra_requires implementation, tests, news file

comment:23 Changed 2 weeks ago by herrwolfe

Patch extras_require-3696-2.diff contains a very simple test that shows that the Distribution object exposed by setuptools supports extra_requires.

One thing to note, I did need to import the Distribution object form setuptools instead of from distutils.core to get this test to pass. Outside of this test, I can only think an integration test might show more. Such a test would build the egg_info for the package and examine the resulting requires.txt file.

Thanks for your feedback and help.

Note: See TracTickets for help on using tickets.