Opened 6 years ago

Last modified 8 days ago

#3696 enhancement new

Add extras_require to top-level setup.py to list Twisted optional dependancies

Reported by: thijs Owned by: glyph
Priority: normal Milestone:
Component: core Keywords: review
Cc: zooko Branch: branches/extras-3696
(diff, github, buildbot, log)
Author: glyph Launchpad Bug:

Description

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

Attachments (5)

extra_requires-3696.diff (2.0 KB) - added by herrwolfe 8 weeks ago.
dist.py, setup.py, news file
extras_require-3696-2.diff (3.4 KB) - added by herrwolfe 7 weeks ago.
extra_requires implementation, tests, news file
extras_require-3696-3.diff (4.2 KB) - added by herrwolfe 9 days ago.
extras_require-3696-4.diff (5.5 KB) - added by herrwolfe 9 days ago.
extras_require-3696-5.diff (5.4 KB) - added by herrwolfe 8 days ago.

Download all attachments as: .zip

Change History (40)

comment:1 Changed 6 years ago by glyph

  • Owner changed from glyph to radix

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

comment:2 Changed 6 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 http://allmydata.org/trac/tahoe/ticket/774 (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 5 months ago by glyph

#7092 is a duplicate of this.

comment:10 Changed 5 months ago by glyph

#6767 is related to this.

Changed 8 weeks ago by herrwolfe

dist.py, setup.py, news file

comment:11 Changed 8 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.

Thanks!

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

comment:12 Changed 8 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 8 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

[metadata]
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 8 weeks ago by zooko

According to https://tahoe-lafs.org/trac/tahoe-lafs/ticket/2028 pywin32 is required if you use (or possibly just import?) endpoints.

comment:15 Changed 7 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 7 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 7 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 7 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 7 weeks ago by herrwolfe (previous) (diff)

comment:19 Changed 7 weeks ago by dstufft

For the record, setup.py 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 7 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 7 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/setup.py'; 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 setup.py without forcing you to actually import setuptools in the setup.py.

comment:22 Changed 7 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 7 weeks ago by herrwolfe

extra_requires implementation, tests, news file

comment:23 Changed 7 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.

comment:24 Changed 2 weeks ago by glyph

  • Author set to glyph
  • Branch set to branches/extras-3696

(In [43020]) Branching to extras-3696.

comment:25 follow-up: Changed 11 days ago by glyph

  • Keywords review removed
  • Owner set to herrwolfe
  1. I think the test is probably sufficient for now, but we should really continue the conversation about how to better test our deployment going forward elsewhere. (I don't want it to block this ticket, but I also would like to have better coverage of our existing installation stuff.)
  2. The requirements themselves aren't really right.
    1. PyCrypto is required for SSH, not SSL.
    2. (SSL should be called, or at least be aliased to, TLS).
    3. We should really have an [all] extra which gets you all the optional dependencies for your current platform. (This could easily be a separate ticket though.)
    4. "Cocoa" should be more conventionally called "mac" or "osx". (In fact "cocoa" is a misnomer here; we support CoreFoundation, which is a lower level than Cocoa, which generally refers to the whole UI SDK layer.)
    5. We require a certain version of pyOpenSSL, don't we?

Sorry for the slow pace of review on this. Trying to improve it :).

comment:26 Changed 11 days ago by herrwolfe

Glyph - thanks for the review! I'll get the changes integrated soon and put it back up for review.

comment:27 Changed 11 days ago by herrwolfe

Glyph - you are correct about there being a version requirement for pyOpenSSL. After #5014 was closed, the minimum acceptable version was set to 0.11.

Changed 9 days ago by herrwolfe

comment:28 in reply to: ↑ 25 Changed 9 days ago by herrwolfe

Replying to glyph:

  1. I think the test is probably sufficient for now, but we should really continue the conversation about how to better test our deployment going forward elsewhere. (I don't want it to block this ticket, but I also would like to have better coverage of our existing installation stuff.)
  2. The requirements themselves aren't really right.
  1. PyCrypto is required for SSH, not SSL.

This has been changed - PyCrypto is now listed under the conch options.

  1. (SSL should be called, or at least be aliased to, TLS).

This has been changed - TLS has replaced SSL

  1. We should really have an [all] extra which gets you all the optional dependencies for your current platform. (This could easily be a separate ticket though.)

I think all that would need to be done to support this, would be writing a function that looks up the platform, and then builds the 'all' option at runtime. But, I think it might help to use a second ticket to define what packages should fall under the "all" build for each platform.

  1. "Cocoa" should be more conventionally called "mac" or "osx". (In fact "cocoa" is a misnomer here; we support CoreFoundation, which is a lower level than Cocoa, which generally refers to the whole UI SDK layer.)

This has been done - I've changed Cocoa to "osx".

  1. We require a certain version of pyOpenSSL, don't we?

Yes - the new minimum is 0.11.

I've also added one more test. The test is meant to test that extra_requires has a specific set of keys. I think this is a useful because we will likely need to write some documentation detailing how to install the various optional dependencies using the extra_requires syntax. Without the test, the extra_requires dictionary might have items accidentally removed, without anyone noticing until a build fails.

I've also removed wxPython from the available options. I have not been able to get it to install cleanly using pip.

The new changes are attached in the patch extras_require-3696-3.diff

Thanks!

comment:29 Changed 9 days ago by herrwolfe

  • Keywords review added
  • Owner changed from herrwolfe to glyph

comment:30 Changed 9 days ago by exarkun

We should really have an [all] extra which gets you all the optional dependencies for your current platform. (This could easily be a separate ticket though.)

I think all that would need to be done to support this, would be writing a function that looks up the platform, and then builds the 'all' option at runtime. But, I think it might help to use a second ticket to define what packages should fall under the "all" build for each platform.

It's probably not this simple. The metadata kinda needs to be static for the package. We might get away with playing some tricks here (like saying it's only static per-platform?) - or we might not. Various parts of the packaging toolchain will get upset if setup metadata changes in certain ways, though. At the very least doing something like this probably requires even more testing against different parts of the packaging toolchain on multiple platforms.

An alternative might be to have [linux], [osx], and [windows] instead of [all].

comment:31 Changed 9 days ago by dstufft

I just woke up and didn't look at the patch yet, but you can get away with doing stuff so that it's static per platform (technically static per thing defined in http://legacy.python.org/dev/peps/pep-0426/#environment-markers), but if you release wheels it'll require duplicating your dependency information into a setup.cfg file too.

comment:32 Changed 9 days ago by glyph

Let's forget about [all] for now then; I like exarkun's idea of an extra for each OS that just combines everything for that OS.

Might we want CoreFoundation support but not OpenSSL? I suspect this is not a useful arrangement. But if it is, [linux-plat], [osx-plat], [windows-plat] could indicate the minimal dependencies instead.

Changed 9 days ago by herrwolfe

comment:33 Changed 9 days ago by herrwolfe

I've reworked the patch a bit to allow for the installation of all of the platform independent options in addition to the platform specific ones.

So now, in addition to the options that allow a user to install specific sets of optional dependencies, e.g.[tls], a user could install [all_non_plat] for all platform independent packages, or [windows-plat] for all platform independent packages and pywin-32; that same goes for osx, and linux. I've also assumed that GTK is the linux platform specific option.

The new patch is extras_require-3696-4.diff

Last edited 9 days ago by herrwolfe (previous) (diff)

comment:34 Changed 9 days ago by exarkun

I don't actually see all_non_plat in the patch? Or windows-plat (it seems to be windows_plat). I'm confused by how the last comment and the last patch are related.

Changed 8 days ago by herrwolfe

comment:35 Changed 8 days ago by herrwolfe

Replying to exarkun:

I don't actually see all_non_plat in the patch? Or windows-plat (it seems to be windows_plat). I'm confused by how the last comment and the last patch are related.

Sorry about that - patch extra_requires-3696-4.patch was unintentionally sloppy. I changed the options that glyph had suggested that used dashes to underscores. The reason being, I don't think one can use dashes as valid variable/dictionary key names in python.

I meant to have [non_plat] represent all [all_non_plat] in the previous patch; this has been changed. I've also cleaned up the grammatical errors that I found in my code's comments.

The patch that contains these changes is extras_require-3696-5.diff.

Thanks!

Note: See TracTickets for help on using tickets.