The great toxification (part 4) #4120

stephenfin · 2017-10-05T09:20:32Z

Subject: Clean up requirements

Clean up requirements (test-reqs.txt, setup.py) to ensure virtualenv and non-virtualenv builds are identical

Feature or Bugfix

Feature

Purpose

To paraphrase the Zen of Python (import this):

tox is one honking great idea -- let's use more of it!

Start modernizing the test infrastructure that's using make and other hand-crafted tools by replacing them with tox-based derivatives.

This is part four.

Detail

This is a multi-step process, with the end goal of completely removing the Makefile and running everything through tox. The first fours steps of these can be done in parallel, but combined they block the final two.

Make tox a little more friendly by enabling extras like coverage, test profiling etc. by default, and closing gaps with the current Makefile targets such as PyLint and building docs in particular formats
Replace/remove custom tooling found in utils in favor of modern alternatives like flake8
Centralize as much configuration as possible inside setup.cfg and tox.ini, to avoid polluting the top level directory.
Clean up requirements (test-reqs.txt, setup.py) to ensure virtualenv and non-virtualenv builds are identical (this change)

The remaining two steps are blocked by the above:

Switch CIs to use tox instead of the Makefile, and update CONTRIBUTING.rst to refer to tox. Deprecate the Makefile
(Bonus) Integrate codecov.io to make use of the coverage stats we now have

Relates

N/A

tk0miya

Basically LGTM, but it seems all tests are failed. could you check it please?

tk0miya · 2017-10-05T14:34:37Z

setup.py

@@ -63,25 +63,27 @@
    ':python_version<"3.5"': [
        'typing'
    ],
+    ':python_version<"3.4"': [
+        'enum34'


Hmm? Why did you add this to dependency?

It's used in the Sphinx source, though currently surrounded by a try-except ImportError. I figured we should just make it a requirement so we can remove that try-except. Did I misinterpret something?

Yes, it's not dependency. It is used to inspect user's program on autodoc module when the program uses enum. So it is not requirements of Sphinx itself.

OK, I'll add it to tests instead so

Wait - we distribute autodoc as part of Sphinx. Therefore shouldn't we provide all the dependencies for that module with Sphinx?

autodoc uses enum only if it is installed. It means autodoc imports it only if the target program uses it. So enum34 is a requirement of target program.
In other words, it is not "requirements", but also "optional".

OK, I'll move this to test so

tk0miya · 2017-10-05T14:41:18Z

setup.py

    'websupport': [
        'sqlalchemy>=0.9',
        'whoosh>=2.0',
    ],
    'test': [
        'pytest',
-        'mock',  # it would be better for 'test:python_version in 2.7'
-        'simplejson',  # better: 'test:platform_python_implementation=="PyPy"'


+1; now pypy provides optimized json package :-)

stephenfin · 2017-10-05T19:24:07Z

Basically LGTM, but it seems all tests are failed. could you check it please?

This is done now

tk0miya · 2017-10-06T01:50:47Z

.travis.yml

-  - pip install -r test-reqs.txt
-  - if [[ $TRAVIS_PYTHON_VERSION == '3.6' ]]; then python3.6 -m pip install mypy typed-ast; fi
+  - pip install .[test,websupport]
+  - if [[ $TRAVIS_PYTHON_VERSION == '3.6' ]]; then python3.6 -m pip install mypy typed-ast flake8; fi


Is it okay to execute flake8 only with python3.6? Once I saw a blog article saying flake8 should be executed with all interpreters. Please let me know if you know.

This is a good point, but I don't think it's something to worry about too much for now. Part 5 will introduce tox-travis so we'll have a chance to completely rework what jobs we run and maybe use flake8 on Py2.7 too

I've modified this now so it'll build against all versions of Python

tk0miya · 2017-10-09T05:18:12Z

@shimizukawa I need your help for this. Could you review this please?

shimizukawa

LGTM with nits

shimizukawa · 2017-10-10T12:55:13Z

setup.py

-    requires.append('colorama>=0.3.5')
-
-if sys.version_info < (3, 5):
-    requires.append('typing')


I think this removing lines means 'drop support pip-1.5.6'.
Maybe such environment is very rare nowadays, however it would be better if it is indicated on CHANGES.

@shimizukawa I've refrained from touching CHANGES because it causes merge conflicts and you guys usually do it. Should I do this or can I simply call it out in the commit message as I've done and leave the CHANGES update to maintainers?

On a side note, have you seen reno? Is this something you might be open to using in Sphinx in a future release?

Okay, I'll update CHANGES on merging.

This version is silly old and anyone that's _still_ using this is not going to be building Sphinx from source. Simply remove it. Signed-off-by: Stephen Finucane <stephen@that.guru>

Simply installing packages will ensure that most of the dependencies in 'setup.py' are installed, meaning 'test-reqs.txt' need only contain those necessary for testing. The only notable change is that the 'simplejson' module is dropped from the requirements list. This included as a dependency for the PyPy target, but it appears that this is not necessary today (though it may have been when the target was added in 2011). This retains 'setup.py test' which, as noted in the tox docs [1], is sometimes expected for downstream distribution testing. We may wish to find a way to synchronize requirements between 'test-reqs.txt' and this section in the future, but that's work for another day. [1] https://tox.readthedocs.io/en/latest/example/basic.html#integration-with-setup-py-test-command Signed-off-by: Stephen Finucane <stephen@that.guru>

We were never really using this file for its specified purpose [1]. Now that we have everything cleanly organized in 'setup.py', there really isn't any reason to keep it around. Remove it. [1] https://caremad.io/posts/2013/07/setup-vs-requirement/ Signed-off-by: Stephen Finucane <stephen@that.guru>

stephenfin · 2017-10-10T13:13:58Z

I've resolved the merge conflict. In the process, I realized that there's actually no reason whatsoever to keep test-req.txt around so I've removed it. We weren't really using it for it's true purpose so getting rid of it seemed like the thing to do.

stephenfin · 2017-10-16T12:55:26Z

@tk0miya @shimizukawa Is there anything more I need to do for this?

tk0miya · 2017-10-17T06:00:09Z

It seems all review are passed now. So I'll merge this tonight (in JST)

tk0miya · 2017-10-17T16:48:58Z

Merged! Thank you for your contribution!

stephenfin · 2017-10-18T08:42:17Z

Lovely. Thanks for the reviews, @tk0miya 😄

This was referenced Oct 5, 2017

The great tox-ification #4105

Closed

Some testing related enhancements #4113

Merged

stephenfin force-pushed the the-great-toxification-4 branch 2 times, most recently from aa0217b to c845424 Compare October 5, 2017 10:09

tk0miya requested changes Oct 5, 2017

View reviewed changes

tk0miya requested a review from shimizukawa October 5, 2017 14:43

stephenfin force-pushed the the-great-toxification-4 branch 6 times, most recently from 5a68613 to 4eb1407 Compare October 5, 2017 19:09

tk0miya reviewed Oct 9, 2017

View reviewed changes

tk0miya approved these changes Oct 9, 2017

View reviewed changes

shimizukawa approved these changes Oct 10, 2017

View reviewed changes

stephenfin added 2 commits October 10, 2017 14:03

setup.py: Stop caring about pip 1.5.6

dbd58d4

This version is silly old and anyone that's _still_ using this is not going to be building Sphinx from source. Simply remove it. Signed-off-by: Stephen Finucane <stephen@that.guru>

stephenfin force-pushed the the-great-toxification-4 branch from 4eb1407 to eb8efce Compare October 10, 2017 13:07

stephenfin force-pushed the the-great-toxification-4 branch from 9d8d6c9 to 67753b7 Compare October 10, 2017 13:13

tk0miya merged commit cfa2eff into sphinx-doc:master Oct 17, 2017

stephenfin deleted the the-great-toxification-4 branch October 18, 2017 08:42

github-actions bot locked as resolved and limited conversation to collaborators Sep 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The great toxification (part 4) #4120

The great toxification (part 4) #4120

stephenfin commented Oct 5, 2017

tk0miya left a comment

tk0miya Oct 5, 2017

stephenfin Oct 5, 2017

tk0miya Oct 5, 2017

stephenfin Oct 5, 2017

stephenfin Oct 5, 2017

tk0miya Oct 5, 2017

stephenfin Oct 5, 2017

tk0miya Oct 5, 2017

stephenfin commented Oct 5, 2017

tk0miya Oct 6, 2017

stephenfin Oct 9, 2017

stephenfin Oct 10, 2017

tk0miya commented Oct 9, 2017

shimizukawa left a comment

shimizukawa Oct 10, 2017

stephenfin Oct 10, 2017 •

edited

tk0miya Oct 17, 2017

stephenfin commented Oct 10, 2017

stephenfin commented Oct 16, 2017

tk0miya commented Oct 17, 2017

tk0miya commented Oct 17, 2017

stephenfin commented Oct 18, 2017

The great toxification (part 4) #4120

The great toxification (part 4) #4120

Conversation

stephenfin commented Oct 5, 2017

Feature or Bugfix

Purpose

Detail

Relates

tk0miya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenfin commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tk0miya commented Oct 9, 2017

shimizukawa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenfin Oct 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenfin commented Oct 10, 2017

stephenfin commented Oct 16, 2017

tk0miya commented Oct 17, 2017

tk0miya commented Oct 17, 2017

stephenfin commented Oct 18, 2017

stephenfin Oct 10, 2017 •

edited