Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Tox for easily testing with multiple Python interpreters #372

Closed
wants to merge 3 commits into from
Closed

Use Tox for easily testing with multiple Python interpreters #372

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c58630e
maintenance: bump min version for pycdio
undu Feb 2, 2019
1f85298
Incorporate dependencies in setup.py
undu Feb 2, 2019
84f1077
new: use tox for testing on travis
undu Feb 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,6 @@ whipper.egg-info/

# From coverage report
.coverage

# tox cache
.tox/
35 changes: 18 additions & 17 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,31 @@
dist: xenial
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Up until now we've generally tried to support as old versions of things as possible, and I didn't see a change in that mentioned anywhere, so switching to a newer distro version should probably be explained/argued for in more detail.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated to Xenial because the version of gobject-introspection packed in Trusty was incompatible with the version of PyGObject that pip pulls with the current restrictions.

I could make it go back to manually install the os-packaged versions, but then we lose the poer of having a single source for dependency definition. I could look into how to define versions of libraries to install depending on a library version in the OS.

Since that is not trivial I preferred to update from trusty to xenial, as trusty is going out of support on April. I also didn't occur to me this was also testing the package in a specifically old version on purpose, I though it was only used to run the unit tests. Making the tests to pass more important than the environment this happened on.

sudo: required

language: bash

env:
- FLAKE8=false
- FLAKE8=true
language: python
python:
- "2.7"

install:
# Dependencies
- sudo apt-get -qq update
- sudo pip install --upgrade -qq pip
- sudo apt-get -qq install cdparanoia cdrdao flac gir1.2-glib-2.0 libcdio-dev libiso9660-dev libsndfile1-dev python-gi python-musicbrainzngs python-mutagen python-setuptools sox swig libcdio-utils
- sudo pip install pycdio==0.21 requests setuptools_scm
- sudo apt-get -qq install cdparanoia cdrdao flac libgirepository1.0-dev gir1.2-glib-2.0 libiso9660-dev libsndfile1-dev python-cddb sox swig libcdio-utils

# Testing dependencies
- sudo apt-get -qq install python-twisted-core
- sudo pip install flake8
# Travis' Ubuntu is very old, libcdio needs to be compiled
- wget https://ftp.gnu.org/gnu/libcdio/libcdio-2.0.0.tar.gz
- tar -xvzf libcdio-2.0.0.tar.gz
- pushd libcdio-2.0.0
- ./configure
- sudo make install
- popd

# Build bundled C utils
- cd src
- pushd src
- sudo make install
- cd ..
- popd
#
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Suggested change
#

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why it was added either

# Testing dependencies
- sudo pip install tox-travis

# Installing
- sudo python setup.py install
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably still want to test whether installing works at all. We don't have a lot of functionality tests as it is (do we have any?), only unit tests, and this seems like a fairly basic one to have…

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This command is run by tox as part of the process it uses to run commands.

It generate a source distribution of the package, a virtual environment and then installs the package onto the virtual environment.

Excerpt from a tox command:

$ tox -e py27-lint
GLOB sdist-make: /home/pau/code/tui/whipper/setup.py
py27-lint create: /home/pau/code/tui/whipper/.tox/py27-lint
py27-lint inst: /home/pau/code/tui/whipper/.tox/.tmp/package/1/whipper-0.7.4.dev50+gcb3515c.zip
py27-lint installed: DEPRECATION: Python 2.7 will reach the end of its life on January 1st, 2020. Please upgrade your Python as Python 2.7 won't be maintained after that date. A future version of pip will drop support for Python 2.7.,certifi==2018.11.29,chardet==3.0.4,configparser==3.7.1,entrypoints==0.3,enum34==1.1.6,flake8==3.7.5,functools32==3.2.3.post2,idna==2.8,mccabe==0.6.1,musicbrainzngs==0.6,mutagen==1.42.0,pycairo==1.18.0,pycdio==2.0.0,pycodestyle==2.5.0,pyflakes==2.1.0,PyGObject==3.30.4,requests==2.21.0,typing==3.6.6,urllib3==1.24.1,whipper==0.7.4.dev50+gcb3515c


script:
- if [ ! "$FLAKE8" = true ]; then python -m unittest discover; fi
- if [ "$FLAKE8" = true ]; then flake8 --benchmark --statistics; fi
script: tox
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach fails to take advantage of Travis' parallel building and that the whole of the test will either fail or pass, rather than (currently) code style check and unit tests pass or fail independently of each other.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I don't think that linting takes so much time that the shaving of time is substantial, it saves resources too as the installation of dependencies only happens once.

In any case, once testing for several python interpreters is enabled in this file these will happen on parallel.

I can look on how to run linting and unittesting in parallel

12 changes: 5 additions & 7 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
FROM debian:buster

RUN apt-get update \
&& apt-get install -y cdrdao python-gobject-2 python-musicbrainzngs python-mutagen python-setuptools \
python-requests libsndfile1-dev flac sox \
libiso9660-dev python-pip swig make pkgconf \
&& apt-get install -y cdrdao libcairo2-dev python-cddb libsndfile1-dev flac sox \
libiso9660-dev python-pip swig make pkgconf git \
eject locales \
autoconf libtool curl \
&& pip install pycdio==2.0.0
autoconf libtool curl

# libcdio-paranoia / libcdio-utils are wrongfully packaged in Debian, thus built manually
# see https://github.com/whipper-team/whipper/pull/237#issuecomment-367985625
Expand All @@ -24,7 +22,7 @@ RUN curl -o - 'https://ftp.gnu.org/gnu/libcdio/libcdio-paranoia-10.2+0.94+2.tar.
&& autoreconf -fi \
&& ./configure --disable-dependency-tracking --disable-example-progs --disable-static \
&& make install \
&& cd .. \
&& cd .. \
&& rm -rf libcdio-paranoia-10.2+0.94+2

RUN ldconfig
Expand All @@ -46,7 +44,7 @@ RUN echo "LC_ALL=en_US.UTF-8" >> /etc/environment \
RUN mkdir /whipper
COPY . /whipper/
RUN cd /whipper/src && make && make install \
&& cd /whipper && python2 setup.py install \
&& cd /whipper && python2 -m pip install --no-cache-dir . \
&& rm -rf /whipper \
&& whipper -v

Expand Down
15 changes: 6 additions & 9 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,9 @@ Some dependencies aren't available in the PyPI. They can be probably installed u
- [flac](https://xiph.org/flac/)
- [sox](http://sox.sourceforge.net/)

PyPI installable dependencies are listed in the [requirements.txt](https://github.com/whipper-team/whipper/blob/master/requirements.txt) file and can be installed issuing the following command:
PyPI installable dependencies are listed in the [setup.py](https://github.com/whipper-team/whipper/blob/master/setup.py) file and they are installed automatically when installing whipper with pip:

`pip install -r requirements.txt`
`python2 -m pip install -e .`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the -m invocation instead of calling pip directly?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sometime pip is not on the user's path, or it might be python3's pip.

This means that novice users cipypasting the instruction will have a lesser chance of encountering situations where things subtly break, at least in my opinion. (I can change if you want me to)


### Fetching the source code

Expand All @@ -171,9 +171,7 @@ cd ..

### Finalizing the build

Install whipper: `python2 setup.py install`

Note that, depending on the chosen installation path, this command may require elevated rights.
Install whipper: `python2 -m pip install --user .`

## Usage

Expand Down Expand Up @@ -257,13 +255,12 @@ disc_template = new/%%A/%%y - %%d/%%A - %%d
# ...
```

## Running uninstalled
## Developing

To make it easier for developers, you can run whipper straight from the
source checkout:
To make it easier for developers, whipper can be installed in live mode:

```bash
python2 -m whipper -h
python2 -m pip install --user -e .
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're changing the meaning and purpose of this section. Is this intentional? Also, you're not installing whipper here (as the description preceding the command claims), only its dependencies.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's intentional, with the changes centered on building whipper as a package it makes sense to try to use it as much as possible to depend more on the dependencies being installed by pip.

The command does install the package on live mode. This means that any change in the code is instantly picked up as part of the package. This is done through symlinks, instead of the usual copying of files that happens when installing files.

This is something that would not be used in the long run, as tox would replace this workflow: it can be change to run a specific test with a specific python interpreter. I would need to check how tests are organized to make this happen, though.

```

## Logger plugins
Expand Down
6 changes: 0 additions & 6 deletions requirements.txt
View file
Open in desktop

This file was deleted.

15 changes: 15 additions & 0 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
from setuptools import setup, find_packages

INSTALL_DEPS = [
'musicbrainzngs',
'mutagen',
'pycdio>0.20',
'PyGObject',
'requests']
TEST_DEPS = ['Twisted']
LINT_DEPS = ['flake8']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of putting these in "constants" like this rather than just adding them directly to the _requires? It puts the data further away from where it's used, and AFAICT it's not needed for reuse anywhere else either.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a tradeoff of locality against structural clarity. More complex rules of versions can be added later on, and it seem they might be needed in some cases. n the end it's a matter of preference.


setup(
name="whipper",
use_scm_version=True,
Expand All @@ -18,4 +27,10 @@
data_files=[
('share/metainfo', ['com.github.whipper_team.Whipper.metainfo.xml']),
],
python_requires='>=2.7, <3',
install_requires=INSTALL_DEPS,
extras_require={
'test': TEST_DEPS,
'lint': LINT_DEPS
},
)
16 changes: 16 additions & 0 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
[tox]
envlist =
py{27,34,35,36,37,py,py3}-unit,
py{27,34,35,36,37,py,py3}-lint,
skip_missing_interpreters = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not supporting Python 3 (yet) or PyPy (yet?), so there's no reason to run or care for tests for those. Wouldn't it be better to just not list them rather than ignore them if non-existent?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On travis this are not run because only the environment is set up for python 2.7.

For local dev the tests never get tun on pypy nor py3 because the package is dfefined as a python 2.7 only package.

I'll remove the line, I don't think it hurts knowing this will be useful for the porting effort.


[testenv]
passenv = CI TRAVIS TRAVIS_*
extras =
py{27,34,35,36,37,py,py3}-unit: test
py{27,34,35,36,37,py,py3}-lint: lint
commands =
py{27,34,35,36,37,py,py3}-unit: python -m unittest discover
py{27,34,35,36,37,py,py3}-lint: python -m flake8 --benchmark --statistics
ignore_outcome =
py{27,34,35,36,37,py,py3}-lint: True