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:
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 (13)

extra_requires-3696.diff (2.0 KB) - added by herrwolfe 5 months ago.
dist.py, setup.py, news file
extras_require-3696-2.diff (3.4 KB) - added by herrwolfe 5 months ago.
extra_requires implementation, tests, news file
extras_require-3696-3.diff (4.2 KB) - added by herrwolfe 3 months ago.
extras_require-3696-4.diff (5.5 KB) - added by herrwolfe 3 months ago.
extras_require-3696-5.diff (5.4 KB) - added by herrwolfe 3 months ago.
extras_require-3696-6.diff (5.0 KB) - added by herrwolfe 7 weeks ago.
remove pam, pyGTK, fix flakes
extras_require-3696-7.diff (4.8 KB) - added by herrwolfe 5 weeks ago.
extras_require-3696-8.diff (4.8 KB) - added by herrwolfe 4 weeks ago.
fix _extra_options, add full stop
irc_conversation_docs.txt (14.6 KB) - added by herrwolfe 2 weeks ago.
irc discussion about docs
extras_require-3696-9.diff (13.1 KB) - added by herrwolfe 2 weeks ago.
patch with docs, pypiwin32, more tests
extras_require-3696-10.diff (14.2 KB) - added by herrwolfe 2 weeks ago.
doc fixes, rename plat to platform
extras_require-3696-11.diff (13.0 KB) - added by herrwolfe 2 weeks ago.
comments from adiroiban, thijs, tom.prince
extras_require-3696-12.diff (11.7 KB) - added by herrwolfe 8 days ago.
reduction

Download all attachments as: .zip

Change History (78)

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 5 years ago by davidsarah

Also, twisted.conch depends on pycrypto and pyasn1.

comment:7 Changed 5 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 8 months ago by glyph

#7092 is a duplicate of this.

comment:10 Changed 8 months ago by glyph

#6767 is related to this.

Changed 5 months ago by herrwolfe

dist.py, setup.py, news file

comment:11 Changed 5 months 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 ".[conch]"
pip install -e  ".[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 4 weeks ago by herrwolfe (previous) (diff)

comment:12 Changed 5 months 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 5 months 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 5 months 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 5 months 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 5 months 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 5 months 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 5 months 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 5 months ago by herrwolfe (previous) (diff)

comment:19 Changed 5 months 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 5 months 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 5 months 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 5 months 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 5 months ago by herrwolfe

extra_requires implementation, tests, news file

comment:23 Changed 5 months 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 3 months ago by glyph

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

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

comment:25 follow-up: Changed 3 months 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 3 months 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 3 months 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 3 months ago by herrwolfe

comment:28 in reply to: ↑ 25 Changed 3 months 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 3 months ago by herrwolfe

  • Keywords review added
  • Owner changed from herrwolfe to glyph

comment:30 Changed 3 months 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 3 months 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 3 months 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 3 months ago by herrwolfe

comment:33 Changed 3 months 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 3 months ago by herrwolfe (previous) (diff)

comment:34 Changed 3 months 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 3 months ago by herrwolfe

comment:35 Changed 3 months 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!

comment:36 Changed 7 weeks ago by tom.prince

The pam dependency is incorrect. The dependency is (I think) on https://tracker.debian.org/pkg/python-pam which I do not believe is on pypi, not the pam package available on pypi.

comment:37 follow-up: Changed 7 weeks ago by tom.prince

Also, PyGTK can't (unfortunately) be installed via pip, except on windows. So I don't think specifying an extra for it is currently reasonable.

comment:38 in reply to: ↑ 37 Changed 7 weeks ago by herrwolfe

Thanks for your comments tom.prince

The pam dependency is incorrect. The dependency is (I think) on ​https://tracker.debian.org/pkg/python-pam which I do not believe is on pypi, not the pam package available on pypi.

Would you recommend that I set the code up to pull from the alternate location? Pip can do so.

comment:39 Changed 7 weeks ago by tom.prince

I would be inclined to just drop the pam dependency.

  • The way we implement pam is insecure (see #3728).
  • The package has been unmaintained since 1999, except for some debian-only patches (some of which appear security related.
  • The PAM code is effectively untested, since none of the buildslaves have the dependency installed.

Changed 7 weeks ago by herrwolfe

remove pam, pyGTK, fix flakes

comment:40 Changed 7 weeks ago by herrwolfe

  • Owner glyph deleted

Thanks for your feedback, tom.prince. I've created a new patch extras_require-3696-6.

This patch removes support for optional installation of pam and pyGTK. I've also cleaned up some flake errors.

Thanks!

comment:41 Changed 5 weeks ago by adiroiban

  • Keywords review removed
  • Owner set to herrwolfe

Thanks for the changes... I am looking at https://twistedmatrix.com/trac/attachment/ticket/3696/extras_require-3696-6.diff

  1. pycrypto is still listed under SSL
  1. do we want docs and dev as separate targets? I assume that all developers will need to build the documentation at some point
  1. same for subunit? Why not integrated it into the dev list?

At the beginning of this ticket there were notes about documenting this.
I would like to discuss this as a follow up and make sure we don't forget about advertising this great feature :)

One place could be https://twistedmatrix.com/trac/wiki/TwistedDevelopment#ToolsForDevelopment

I tried to find a wiki page or doc page talking about how to install Twisted, but I could not find it... if there is one, we should put this info there.

Thanks again and many, many thanks for working on this!

comment:42 Changed 5 weeks ago by thijs

#6767 was closed as a duplicate. There's a lot of info about the various dependencies of Twisted in that ticket.

Changed 5 weeks ago by herrwolfe

comment:43 Changed 5 weeks ago by herrwolfe

  • Keywords review added
  • Owner herrwolfe deleted

Thanks for your review, adiroiban.

The patch https://twistedmatrix.com/trac/attachment/ticket/3696/extras_require-3696-7.diff contains all of the changes you requested.

  1. pycrypto is still listed under SSL

Done - I've moved PyCrypto to conch.

  1. do we want docs and dev as separate targets? I assume that all developers will need to build the documentation at some point

Done - I've moved all of the docs dependencies into dev.

  1. same for subunit? Why not integrated it into the dev list?

I'm not sure why I had this separate either, it may have just been a leftover from when I was copying dependencies from #6767. I've moved subunit into the dev list as well.

At the beginning of this ticket there were notes about documenting this.
I would like to discuss this as a follow up and make sure we don't forget about advertising this great feature :)
One place could be https://twistedmatrix.com/trac/wiki/TwistedDevelopment#ToolsForDevelopment
I tried to find a wiki page or doc page talking about how to install Twisted, but I could not find it... if there is one, we should put this info there.

I agree completely that we should document how this feature should be used. I'm a bit torn on whether or not an entry on the wiki or new narrative documentation might be the most useful. I tend to like the narrative docs a bit more as they can be accessible offline and can be checked as part of the build process.

What is the correct process for documenting an as of yet unaccepted feature? Would it be best if we were to open a ticket for new docs once this ticket is completed?

Thanks again for the review!

comment:44 Changed 5 weeks ago by adiroiban

Changes looks good. Thanks!

For the news entry, maybe it would be nice to have a full stop at the end.

Narative documentation is nice since it can be part of the same patch, when you apply the patch you also update the docs... and if the patch is reverted, you will also revert the docs... without forgetting about changing wiki pages.

For this install steps, I prefer to have them documented as part of the root INSTALL file.
That place already talks about setup.py


Runtime and development dependencies was a quick and dirty way to document the dependencies. With this changes, I think that we should remove some of the items from that list, which are now duplicated, and just inform people to check the INSTALL file and setup.py source code

We should still leave that list in the wiki, for all the dependencies not included in this patch. On the wiki page we should also summarize other discussions/actions from this ticket (and related ones) which did not make it to the final patch.
In this we can keep it as a reference for future improvements.

Thanks!

Changed 4 weeks ago by herrwolfe

fix _extra_options, add full stop

comment:45 Changed 4 weeks ago by herrwolfe

I just noticed that two commas were missing from the _extra_options dictionary. I've also added a period to the NEWS file.

The changes are fixed by https://twistedmatrix.com/trac/attachment/ticket/3696/extras_require-3696-8.diff.

comment:46 Changed 4 weeks ago by exarkun

I just noticed that two commas were missing from the _extra_options dictionary. I've also added a period to the NEWS file.

Did an automated test catch this? Will an automated test catch a similar problem in the future? If not, perhaps one should be added. A minimum level of correctness could be checked by trying to parse this list (or the elements of this list - I'm not sure what the parsing API looks like) using the library that's actually going to consume the information (something in distutils? or setuptools? or pip?)

comment:47 Changed 4 weeks ago by herrwolfe

  • Keywords review removed
  • Owner set to herrwolfe

No, an automated test did not catch this. I'll add a few tests for the extra_options.

Thanks!

comment:48 follow-up: Changed 3 weeks ago by herrwolfe

pywin32 no longer looks to be installable from the default PyPi location using pip. The recommended way to install it seems to be to download an installer.

The error I receive when trying to install using pip and PyPi is "No distributions at all found for pywin32". It seems like one can use an external and untrusted link to install using pip.

But, in my opinion, I don't think that using an external untrusted url for the package is a good choice.

As such, in my next patch, I'm going to remove the sections responsible for installing pywin32 as an extra.

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

comment:49 in reply to: ↑ 48 Changed 2 weeks ago by glyph

Replying to herrwolfe:

pywin32 no longer looks to be installable from the default PyPi location using pip. The recommended way to install it seems to be to download an installer.

The error I receive when trying to install using pip and PyPi is "No distributions at all found for pywin32". It seems like one can use an external and untrusted link to install using pip.

But, in my opinion, I don't think that using an external untrusted url for the package is a good choice.

As such, in my next patch, I'm going to remove the sections responsible for installing pywin32 as an extra.

This has been the case for quite a while.

https://sourceforge.net/p/pywin32/bugs/680/

Since the maintainer refuses to create a workable distribution, I think I'm going to go ahead and create a fork.

comment:50 Changed 2 weeks ago by glyph

OK, I did it:

https://pypi.python.org/pypi/pypiwin32

You can put pypiwin32 in the extra.

comment:51 Changed 2 weeks ago by herrwolfe

Awesome - I'll use this in the new patch. Thanks!

Changed 2 weeks ago by herrwolfe

irc discussion about docs

Changed 2 weeks ago by herrwolfe

patch with docs, pypiwin32, more tests

comment:52 Changed 2 weeks ago by herrwolfe

The patch extras_require-3696-9.diff contains

  • narrative docs explaining how to use pip to install optional dependencies
  • the replacement of pywin32 with pypiwin32, enabling pywin32 to be installed with pip. (addressing ticket:3696#comment:50)
  • tests that test each optional extra_requires dictionary is valid and contains the correct options. (addressing ticket:3696#comment:46)

I've also uploaded a record of an IRC conversation in which the docs and installation methods were discussed.

From the discussion it looks like we need to document installing twisted using pip and installing system packages (rpms, debs).
It also looks like it would be helpful for us to document the system dependencies needed for each of twisted's components to be able to function.
This would include things like commoncrypto or openSSL, etc.
For this purpose exarkun pointed me to a dockerfile that has been written (https://github.com/robhaswell/twisted-docker/blob/master/Dockerfile).

My feeling is that while the documentation described above needs to be written, it isn't in the scope of this ticket to do so.

I propose that we open the following new tickets.

  1. Document the system requirements needed for twisted to function
  2. Document installing twisted using pip (I've done this and can upload a patch)
  3. Document installing twisted using packages for various systems (I'm working on this now, have taken the wiki as a starting point)

If the scope of this ticket is only installing optional dependencies, then the new documentation should only need to cover that functionality (which, in my opinion, it currently does).

What do you think?

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

Changed 2 weeks ago by herrwolfe

doc fixes, rename plat to platform

comment:53 Changed 2 weeks ago by herrwolfe

  • Keywords review added
  • Owner herrwolfe deleted

The most recent patch is extras_require-3696-10.diff. The patch contains the following revisions:

  • Removed unnecessary spaces that were present in the new documentation.
  • Renamed all of the extras_require keys that used the word plat to platform. This affects the options all_non_platform , windows_platform , and osx_platform.

comment:54 Changed 2 weeks ago by tom.prince

I don't think dev belongs in the various all. Those dependencies are only useful for developers, and none of the publicly API depends on any of them (except, I guess, subunit). I think many (most?) users will want to install everything but the dependencies, and distributions like fedora, that don't have optional dependencies, will probably want to have all of the non-dev dependencies as dependencies.

comment:55 Changed 2 weeks ago by adiroiban

Many thanks for the work on this.

I agree with Tom Prince, dev is only needed for developers and buildbot.

Regarding the documentation, I feel that it is a bit over-engineered. instalation/index is pretty much empty ... why not create a single installation page?

For docs/installation/howto/optional.rst, while the document is ok, I am not sure if we neeed 'What are optional dependencies? ', Why use optional dependencies?, How do I install an optional dependency? ... or Conclusions

They look very generic and rather than duplicate this information for each project, maybe we can contribute it to the Python Packaging Guide and link new developers to a dedicated page from the general guide https://packaging.python.org/en/latest/installing.html

For How do I install an optional dependency? maybe we can skip the pip install "twisted[option_name_to_install]" and just go to the exact example, I assume that developers will be smart enough to understand what is going on.

For Which options are supported? I am a bit worried that the dependencies are duplicated in setup.py and then in documentation and when updating a dependency we might forget to update the documentation.
Maybe we can just send a link to the setup.py source


What are optional dependencies? has wrong title marker... too long.

Do we use tab indentation for sphinx documentation?

Too bad we don't have a tool to automatically detect these things on local computer.

Thanks!

comment:56 follow-up: Changed 2 weeks ago by thijs

  • Keywords review removed
  • Owner set to herrwolfe

Thanks.

  • the content of a .misc newsfile is always ignored. I wouldn't mind reading about this change in the changelog, so I would suggest moving it to a .feature or .doc file. So, if you don't want to describe a change, use .misc.
  • The sentence on line 67 in twisted/python/test/test_dist.py should exclude 'Test that'
  • 2 blank lines between test methods
  • the indentation in the examples on docs/installation/howto/optional.rst don't need so much leading whitespace, 2 or 4 spaces is enough

comment:57 in reply to: ↑ 56 Changed 2 weeks ago by glyph

  • Keywords review added
  • Owner herrwolfe deleted

Replying to thijs:

Thanks.

Hi thijs,

I don't think this review is sufficient, so I'm putting this back into the queue, although herrwolfe is welcome to respond to all of your entirely valid feedback :).

In order for a review to be sufficient, you should always include a statement as to the status of the change. For example: "fix the above and it should be ready to land" or "fix these things and it should be ready to land ... and here are some other things you can address if you agree that they're important, but they're optional...", or in the case of changes with problematic designs or very large missing things like no test coverage or no documentation, "fix the following and submit for re-review".

Given that you had so little feedback, it seems likely that the change would be ready to merge if this feedback was addressed. But, it also looks like nobody has put the most recent iteration of the patch onto the buildbot, so it seems like your comments about coding-standard issues are likely from manual inspection rather than using our automated infrastructure, which means it's likely that you may have missed something that twistedchecker or pyflakes might have caught. It's always a good policy to put changes onto the buildbot.

Nevertheless - thanks for taking the time to do reviews!

comment:58 follow-up: Changed 2 weeks ago by thijs

Hi glyph. Since the author of the patch added the review keyword we saw three separate developers respond with feedback. If i would split that up into tasks i see around ten bullet points to respond to. I removed the review keyword because in my opinion theres enough to go from here, fix it or respond, and put it back up in review. All im trying to do is speed up the review cycle, there are tickets ip for review for months in general and frankly, its frustrating.

Changed 2 weeks ago by herrwolfe

comments from adiroiban, thijs, tom.prince

comment:59 Changed 2 weeks ago by herrwolfe

Thank you adiroiban, thijs, glyph, and tom.prince for your comments. I apologize for the delay in my response.

I've uploaded a new patch - extras_require-3696-11.diff.

The patch will be a bit confusing, as it integrates *some* changes suggested by adiroiban, but not all. All of tom.prince's and thijs's comments have been addressed by the patch.

My reasons for not making certain suggested changes are below.

Replying to adiroiban:

I agree with Tom Prince, dev is only needed for developers and buildbot.

Ok - I have removed dev from the all_non_platform , osx_platform and windows_platform options.

Regarding the documentation, I feel that it is a bit over-engineered. instalation/index is pretty much empty ... why not create a single installation page?

I am planning on writing documentation in separate tickets describing how to install Twisted using pip or system packages and docs describing the hard system dependencies various Twisted features require. As such, I thought it would simplify things if I started with a new Installing Twisted section. My other goal was to make it very clear that the documentation in optional.rst is *only* concerned with the installation of optional dependencies. Having a single document titled Installing optional dependencies at the top-level seemed awkward to me.

For docs/installation/howto/optional.rst, while the document is ok, I am not sure if we neeed 'What are optional dependencies? ', Why use optional dependencies?, How do I install an optional dependency? ... or Conclusions
They look very generic and rather than duplicate this information for each project, maybe we can contribute it to the Python Packaging Guide and link new developers to a dedicated page from the general guide https://packaging.python.org/en/latest/installing.html

My opinion is that these sections add value for users who are either unfamiliar with installing extras or who don't know why the extras have been chosen.

  • What are optional dependencies is meant to define the term optional dependencies. I thought this term might be unfamilar to new users.
  • Why use optional dependencies gives the reader Twisted's reason for providing the options. In my experience, most projects haven't used optional dependencies to unlock functionality of a system. The tendency (again in my experience only) has seemed to be use optional dependencies for options like dev only. Because of this, I thought it would help to clarify that options here serve multiple purposes.
  • How to install an optional dependency is a section that could be removed if the steps were detailed elsewhere. Since docs describing the installation of optional dependencies is not currently present in the pypa packaging installation tutorial, I feel like the section should be included in the new document. I will do a bit more research about the pypa tutorial and if there is not a ticket already open, open a ticket detailing how to use extras_require syntax.
  • Conclusions was written to satisfy my interpretation of the howto docs standard. I agree that it doesn't provide much *real* information, but I thought it needed to be present.

For How do I install an optional dependency? maybe we can skip the pip install "twisted[option_name_to_install]" and just go to the exact example, I assume that developers will be smart enough to understand what is going on.

Done.

For Which options are supported? I am a bit worried that the dependencies are duplicated in setup.py and then in documentation and when updating a dependency we might forget to update the documentation.
Maybe we can just send a link to the setup.py source

I agree that the maintenance burden here could be troublesome. But, I feel like the benefit of being able to provide links to the packages on PyPi outweighs the burden. Ideally, I'd like to have some sort of a documentation test that can check that the packages in the docs are the same as those specified in the extras_require section of setup.py.

What are optional dependencies? has wrong title marker... too long.

I don't understand what you mean here. When I looked through the doc standard, I didn't see a rule detailing the length of section title. But, I may have missed it. Could you tell me how it needs to change in order to conform to the standard or provide a link to the relevant documentation?

Do we use tab indentation for sphinx documentation?

The current patch uses spaces.

Replying to thijs:

the content of a .misc newsfile is always ignored. I wouldn't mind reading about this change in the changelog, so I would suggest moving it to a .feature or .doc file. So, if you don't want to describe a change, use .misc.

Done.

The sentence on line 67 in twisted/python/test/test_dist.py should exclude 'Test that'

Done.

2 blank lines between test methods

Done.

the indentation in the examples on docs/installation/howto/optional.rst don't need so much leading whitespace, 2 or 4 spaces is enough

Done.

Again - thank you all for your comments!

comment:60 Changed 2 weeks ago by tom.prince

My other goal was to make it very clear that the documentation in optional.rst is *only* concerned with the installation of optional dependencies. Having a single document titled Installing optional dependencies at the top-level seemed awkward to me.

Another option would be to do something like is done here. That is, just indicate the rest of the document is a placeholder. *Note:* I am *not* suggesting that you change the documentation to be like this; it isn't clear to me what the structure of the documentation should be.

Also, there is at least one optional dependency (pygtk), that isn't mentioned here (since it can't be installed via pip). It probably should be, but this can be a separate ticket.

comment:61 in reply to: ↑ 58 Changed 13 days ago by glyph

Replying to thijs:

Hi glyph. Since the author of the patch added the review keyword we saw three separate developers respond with feedback. If i would split that up into tasks i see around ten bullet points to respond to. I removed the review keyword because in my opinion theres enough to go from here, fix it or respond, and put it back up in review. All im trying to do is speed up the review cycle, there are tickets ip for review for months in general and frankly, its frustrating.

Hi Thijs,

I very much appreciate you trying to speed up the review cycle for contributors. It is definitely super frustrating, and I am trying to do the same thing :). The problem with a review without a clear status and next action is that it can serve to lengthen the process even more, because they may have to wait an additional review cycle before finding out about the next issue that they need to address.

If you felt the other review comments plus your own were sufficient, then all you need to say is "please address the other comments left by X and Y, I did not find anything else"; that is also a clear way of indicating the next action to the contributor.

In any case, sticking it on the buildbot first is always a good idea.

-g

comment:62 Changed 11 days ago by herrwolfe

I've opened an issue and provided a PR with the pypa/python-packaging-user-guide about adding the extras_require examples to the tutorial.

The PR can be found on github.

comment:63 Changed 11 days ago by herrwolfe

The PR to add extras_require to the packaging tutorial has been merged in.

Per adiroiban's suggestion I'm going to edit the patch to rely on setuptools' explanation of optional dependencies and the packaging tutorial's explanation of how to install them.

Changed 8 days ago by herrwolfe

reduction

comment:64 Changed 8 days ago by herrwolfe

I've uploaded patch extras_require-3696-12.diff that contains the following changes:

  • removed most of the how-to documentation to instead defer to the setuptools' documentation and the python packaging tutorial.
  • Effectively this reduces the patch to a single section detailing the intended audience of the document and the options available.

exarkun had the idea that we could create a sphinx extension to create the extras_require options when the docs are built. This would remove most, if not all, of the maintenance burden involved in maintaining the extras_require option.

The only issue I can see is that the extension would need to get a link for each of the extras_require packages. This isn't monstrously difficult, but it would need more work than just grabbing the twisted.python.dist.EXTRAS_REQUIRE dictionary and turning into text.

Thanks!

comment:65 Changed 8 days ago by adiroiban

I think that we have a good start.

Once this is merge I can try to create a sphinx extension... the extension can keep its own mapping of project name -> project url and maybe fail if a dependency has no URL defined.

Thanks!

Note: See TracTickets for help on using tickets.