Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#6767 enhancement closed duplicate (duplicate)

Add a dev-requirements.txt file

Reported by: Richard Wall Owned by: Chris Wolfe
Priority: normal Milestone:
Component: core Keywords:
Cc: Hynek Schlawack, hawkowl@… Branch:
Author:

Description

A dev-requirements.txt file would contain a list of pip installable packages that are required for Twisted development eg

pydoctor twistedchecker braid (maybe)

We'd also write a few paragraphs about how the file is intended to be used. eg

Attachments (6)

add-dev-requirements-file-7030-1.patch (7.7 KB) - added by Chris Wolfe 4 years ago.
Patch containing the contents of dev-requirements.txt against trunk.
add-dev-requirements-file-6767-2.patch (204 bytes) - added by Chris Wolfe 4 years ago.
Corrected the patch to contain the correct file.
add-dev-requirements-file-6767-3.patch (226 bytes) - added by Chris Wolfe 4 years ago.
Add pyopenssl to the patch
add-dev-requirements-6767-4.patch (412 bytes) - added by Chris Wolfe 4 years ago.
comments, versions
add-dev-requirements-6767.5.patch (2.2 KB) - added by Chris Wolfe 4 years ago.
News file, dev-requirements.txt revisions
add-dev-requirements-6767.6.patch (944 bytes) - added by Chris Wolfe 4 years ago.
actual News file, dev-requirements.txt revisions

Download all attachments as: .zip

Change History (31)

comment:1 Changed 4 years ago by Hynek Schlawack

Cc: Hynek Schlawack added

comment:2 Changed 4 years ago by hawkowl

Cc: hawkowl@… added

TwistedChecker is in a usable state and installable from pypi now (as just pip install twistedchecker). There's currently no usage docs for it (yet), and the --diff feature doesn't seem to work, I think, so I might need to go and look at that (since dumping raw twistedchecker output is kinda terrible if it's a big file that happens to be old).

comment:3 Changed 4 years ago by Thijs Triemstra

#7030 was marked as a duplicate (and also includes a patch).

Changed 4 years ago by Chris Wolfe

Patch containing the contents of dev-requirements.txt against trunk.

Changed 4 years ago by Chris Wolfe

Corrected the patch to contain the correct file.

comment:4 Changed 4 years ago by Chris Wolfe

Keywords: review added

The file add-dev-requirements-file-6767-1.patch does not contain the correct patch.

The file add-dev-requirements-file-6767-2.patch is the correct patch and contains the dependencies discussed on irc.freenode.net/#twisted-dev at 13:23 CST on 12 March 2014.

Changed 4 years ago by Chris Wolfe

Add pyopenssl to the patch

comment:5 Changed 4 years ago by Adi Roiban

I did now saw this ticket

Here is my take on dev-requirements.txt while working on Travi-CI integration: https://github.com/chevah/twisted/blob/travis-ci/dev-requirements.txt

Since pyopenssl pyasn and pycrypt dependencies are not development dependencies, but runtime dependencies, I have created a separate file for them:

https://github.com/chevah/twisted/blob/travis-ci/requirements.txt


What do you think about splitting runtime and development dependencies?

I prefer to have runtime dependencies included in setup.py install_required field, so maybe we should fix setup.py to read dependencies from a dedicated (separate file).

just listing package names without explicit version does not always work as the environment my have a very old version which is not supported by Twisted.

Maybe I am paranoid, but I prefer to have have a reference version ...

maybe have something like pyopenssl>=0.11.0 ? What do you think?


pyflakes , pydoctor and twisted-dev-tools are also required.

Since review receive comments due to failure in twistechecker, I would argue that twistedchecker should also be required.


But instead of dev-requirements.txt file, why not only depend on twisted-dev-tools and have pyflakes, pydoctor, twistechecker .. etc as a dependency of twisted-dev-tools.

In this way, twisted development setup should be as simple as pip install twisted-dev-tool==SOME.LATEST.VERSION

Just asking. Maybe is a bad idea.


Why is pyobjc a development dependency for Twisted ?

I guess it is only required on OSX ... or Linux / WIndows users should also install it? I was able to run tests and build documentation without that package.

Maybe we should add a comment or some sort of documentation for dependencies.

comment:6 Changed 4 years ago by Adi Roiban

Keywords: review removed
Owner: set to Chris Wolfe

Hi herrwolfe,

This is my first review, so please let me know if I made something wrong.

Please see above comments and let me know what do you think.

Thanks!

comment:7 Changed 4 years ago by Chris Wolfe

Keywords: review added

Hi adiroban,

My apologies for taking so long to get back, I've been out of town and unable to receive email.

I am mainly interested in adding a file that lists all of the dependencies needed to develop twisted.

The goal behind the addition of the dev-requirements.txt file is to simplify the development of twisted using virtual environments.

I do agree that pinning versions of these dependencies would be a good idea. At this time, I do not know which versions of these dependencies should be required.

The patch I've submitted was based on the dependencies that Glyph had suggested in an IRC discussion; I however can't speak to which requirements are *truly* required.

Thanks for your review, Chris

comment:8 Changed 4 years ago by Adi Roiban

Don't worry for the delay. The whole Twisted review process is damn slow anyway.

Looking forward for a review from a core-developer.

Thanks!

comment:9 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed

Thanks.

pycrypto

This is an optional dependency only need for Conch.

pyasn1

This is an optional dependency only need for Conch.

pyobjc

This is an optional dependency only need for cfreactor.

sphinx

This is an important dependency.

pyopenssl

This is an optional dependency only need for TLS.

zope.interface is an important dependency.

There are some additional optional dependencies:

  • gmpy makes Conch a lot faster
  • pywin32 is required for process support on Windows
  • pyserial is required for serial ports
  • pygtk is required for the gtk class of reactors (or pygi or pygobject or something)
  • wxpython is required for wxsupport / wxreactor
  • gadfly, sqlite, pypgsql, psycopg, mysqldb, kinterbasdb (at least one) are required for twisted.enterprise.adbapi
  • SOAPpy is required for Twisted Web's SOAP support
  • pypam is required for twisted.cred PAM integration
  • subunit is required for trial's subunit output plugin

And there are some additional development tools:

  • twistedchecker is required for automated coding standard compliance checks
  • pydoctor is required to generate the api documentation
  • cython is required to update iocpreactor and some Failure unit tests

If you just want to add all of these to dev-requirements.txt then I suppose that's consistent with the motivation for this ticket. I think there'll be a problem as soon as you have pyobjc (only available on OS X) and pywin32 (only available on Windows). Perhaps there should be dev-requirements.txt with cross-platform requirements, osx-dev-requirements.txt with OS X requirements, and win-dev-requirements.txt with Windows requirements. Then at least anyone can install all of the development requirements with just two commands. I don't really know much about the requirements.txt tool chain or what expectations people have for these files, perhaps there's a better solution than this one - consider this just a suggestion to explore in case you don't have another idea.

You may also want to consider whether each subproject should have its own requirements declared (in the subproject topfiles) directory. I don't know if it is helpful for someone who wants to work on Twisted Web to have to install pygtk. On the other hand maybe people won't care about this (and if they do we can always split the files up later).

I suggest that known version requirements be included as well. The version for some dependencies is already documented in various places, for example the top-level INSTALL file says that pyOpenSSL 0.10 is preferred (but #5014 is about to change that to say that 0.10 is required). If there are dependencies where the version isn't clear then we should probably take a look at what's installed on Buildbot and claim that's it (following the rule of thumb that "if it's not tested then it's broken").

My last suggestion is that you add a comment to the top of the file (I *think* # is legal comment syntax for this file) explaining how the entries were selected. If the preference is for even optional dependencies to be included, explain that. If the final decision is to list only the hard, mandatory dependencies without which you really can't do anything productive with Twisted then please explain that.

Thanks again.

comment:10 in reply to:  9 Changed 4 years ago by Chris Wolfe

Keywords: review added

Replying to exarkun: Exarkun - Thank you for the review.

My goal with this patch (add-dev-requirements-6767-4.patch) is to allow a new user to easily install project dependencies allowing them to test the application's base functionality and check newly written code for compliance with Twisted's conventions.

To this end, it seems to me that a user would want to pull in all of the dependencies that are either

  • used by a basic buildbot build
  • useful in checking twisted code

There are some additional optional dependencies:

  • gmpy makes Conch a lot faster
  • pywin32 is required for process support on Windows
  • pyserial is required for serial ports
  • pygtk is required for the gtk class of reactors (or pygi or pygobject or something)
  • wxpython is required for wxsupport / wxreactor
  • gadfly, sqlite, pypgsql, psycopg, mysqldb, kinterbasdb (at least one) are required for twisted.enterprise.adbapi
  • SOAPpy is required for Twisted Web's SOAP support
  • pypam is required for twisted.cred PAM integration
  • subunit is required for trial's subunit output plugin

I don't really think it is necessary to pull in pyObjC, pyWin32, or many of the other optional dependencies. If a new user is working on a feature relating to this optional code, then they will need to track down the dependency and install it on their own.

You may also want to consider whether each subproject should have its own requirements declared (in the subproject topfiles) directory. I don't know if it is helpful for someone who wants to work on Twisted Web to have to install pygtk. On the other hand maybe people won't care about this (and if they do we can always split the files up later).

Right now, I think it would be simplest to have a single file in the top level of the application. Also, since a user would likely want all dependencies inside a single virtual environment, they would not be creating new virtual environments for each of the modules.

I suggest that known version requirements be included as well. The version for some dependencies is already documented in various places, for example the top-level INSTALL file says that pyOpenSSL 0.10 is preferred (but #5014 is about to change that to say that 0.10 is required). If there are dependencies where the version isn't clear then we should probably take a look at what's installed on Buildbot and claim that's it (following the rule of thumb that "if it's not tested then it's broken").

I agree with this and have integrated the version numbers I found in either INSTALL or on buildbot into my patch.

My last suggestion is that you add a comment to the top of the file (I *think* # is legal comment syntax for this file) explaining how the entries were selected. If the preference is for even optional dependencies to be included, explain that. If the final decision is to list only the hard, mandatory dependencies without which you really can't do anything productive with Twisted then please explain that.

I agree and have integrated this change into my patch.

Thank you, Chris

Changed 4 years ago by Chris Wolfe

comments, versions

comment:11 Changed 4 years ago by Chris Wolfe

Owner: Chris Wolfe deleted

comment:12 Changed 4 years ago by Adi Roiban

Keywords: review removed
Owner: set to Chris Wolfe

Patch needs a news file.

I found exarkun comment very informative and I have updated the development wiki page to include that info:

https://twistedmatrix.com/trac/wiki/TwistedDevelopment#Runtimeanddevelopmentdepenencies

Here is my version based on exarkun comment that sphinx is important and that each dependency should have a comment describing why is it needed.

pyflakes is required, together with twistedchecker

# Dependencies which are available on all platforms
#
# Using these dependencies, you should be able to run Twisted and check code
# for compliance with Twisted's coding conventions.
#
zope.interface==4.0.2

# Helper for commiters to interact with Twisted development infrastructure.
twisted-dev-tools==0.0.2

# User for checking Twisted coding standard.
twistedchecker==0.2.0
pyflakes==0.8.1

# Used for generating API and narrative documentation.
pydoctor==0.5
sphinx==1.2.2

Thanks

Changed 4 years ago by Chris Wolfe

News file, dev-requirements.txt revisions

comment:13 Changed 4 years ago by Chris Wolfe

Keywords: review added
Owner: Chris Wolfe deleted

I have added a news file describing pip and virtualenv usage to the Sphinx documentation similar to the file found in the pyca/cryptography project. I've also updated the dev-requirements.txt file to include adiroiban's changes.

The patch add-dev-requirements-6767-5.patch contains these changes.

comment:14 Changed 4 years ago by Adi Roiban

I can not see the news file in the diff.

Not sure what to say about getting-started documentation.. the content is OK but not sure if this should go into narrative docs or wiki.

Thanks!

comment:15 Changed 4 years ago by Chris Wolfe

The file docs/core/development/getting_started.rst is how I have interpreted the news file from the cryptography project. They had other sections, e.g. those detailing testing, that are covered elsewhere in Twisted's documentation.

Thanks you!

comment:16 Changed 4 years ago by Glyph

Anyone interested in this should also be interested in #3696.

If we had setuptools extras, then we could have twisted[all] (dependencies computed per platform, on the fly), twisted[ssl], twisted[ssh] et cetera, to specify each category of optional dependency. It would be nice to have this specified in one place so a simple pip install -e .[something] would get you everything and that those optional dependencies would carry over to PyPI.

comment:17 Changed 4 years ago by Chris Wolfe

Keywords: review removed
Owner: set to Chris Wolfe

Changed 4 years ago by Chris Wolfe

actual News file, dev-requirements.txt revisions

comment:18 Changed 4 years ago by Chris Wolfe

Keywords: review added
Owner: Chris Wolfe deleted

I misinterpreted what the news file was meant to represent. I have added the news file to the patch and have also removed the sphinx docs per Glyph's request on IRC.

comment:19 Changed 4 years ago by Chris Wolfe

Keywords: review removed
Owner: set to Chris Wolfe

Need to add news file to correct place.

comment:20 in reply to:  9 Changed 4 years ago by Trevor Bramwell

Replying to exarkun:

You may also want to consider whether each subproject should have its own requirements declared (in the subproject topfiles) directory. I don't know if it is helpful for someone who wants to work on Twisted Web to have to install pygtk. On the other hand maybe people won't care about this (and if they do we can always split the files up later).

I messed around with trying to make this work for a bit. pip has a flag -r that allows you to install requirement files, along with chain them by listing something like -r other/dir/requirements.txt in the top file.

I went through and tried to pin down the exact libraries each subproject needed and created a requirements.txt file under topfiles as suggested. This way a simple pip install -r requirements.txt at the root of Twisted would install the requirements needed by each subproject. This does not work because pip currently does not allow duplicate package listings. For example twisted.mail, and twisted.names both required pyopenssl, but if both of their requirements are included in the top requirements, pip will fail.

The structure looks something like:

# requirments.txt
zope.interface
-r twisted/mail/topfiles/requirements.txt
-r twisted/names/topfiles/requirements.txt

This approach could still work, but only if subproject requirements were installed individually.

comment:21 Changed 3 years ago by Chris Wolfe

My apologies for leaving this ticket in limbo - I've been working on the related ticket #3696 that glyph referenced in comment:16 instead of this.

comment:22 Changed 3 years ago by Adi Roiban

Resolution: duplicate
Status: newclosed

I think that we should close this as it is a duplicate of #3696

comment:23 Changed 3 years ago by dstufft

Generally you want to define your dependencies inside of the setup.py instead of a requirements.txt. Essentially setup.py should be used to metadata about the project (dependencies included) and requirements files should be used to setup a particular environment.

So you might have a dev-requirements.txt that has a -e . in it or something similar. It might also install things like a static analysis tool or what have you, it should not include mandatory or optional dependencies of Twisted (or any subproject) though. That should be in the setup.py and if you want this file to install it use the -e . construct.

I wrote more in depth about setup.py vs requirements.txt here: https://caremad.io/2013/07/setup-vs-requirement/

comment:24 Changed 3 years ago by Glyph

If #3696 is implemented as specified, then dev-requirements.txt would just contain -e .[all][dev] or something (how do you specify multiple extras?)

comment:25 Changed 3 years ago by dstufft

comma separated: [all,dev]

Note: See TracTickets for help on using tickets.