Skip to content

[packaging] Added conditional dependencies for wheel#74

Merged
nemesifier merged 1 commit intoopenwisp:masterfrom
ritwickdsouza:cond-depend-wheel
May 25, 2017
Merged

[packaging] Added conditional dependencies for wheel#74
nemesifier merged 1 commit intoopenwisp:masterfrom
ritwickdsouza:cond-depend-wheel

Conversation

@ritwickdsouza
Copy link
Contributor

References #72

@ritwickdsouza
Copy link
Contributor Author

@nemesisdesign I ran the tests locally on a clean virtualenv and it passed.
On travis I'm getting this ImportError: No module named 'jinja2'

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Not sure what causes the problem on travis, I'm testing.

continue
# add line to requirements
requirements.append(line.replace('\n', ''))
# add py2-ipaddress if python2
Copy link
Member

Choose a reason for hiding this comment

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

not sure this block of code can be removed, it may still be required for non-wheel packages.

Copy link
Member

Choose a reason for hiding this comment

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

I'm testing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh.

Copy link
Member

Choose a reason for hiding this comment

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

no, I'm wrong. Ignore this! Good work.

Copy link
Member

Choose a reason for hiding this comment

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

I was wrong again, that bit of code is necessary for non-wheel packages (old fashion tar.gz).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How did you test it ?

Copy link
Member

Choose a reason for hiding this comment

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

you can test it by generating the packages with:

python setup.py sdist bdist_wheel

Then create 2 new virtualenvs, one with python2 and one with python3, and in each virtualenv you do:

pip install -U pip wheel setuptools
pip install -r requirements-test.txt

Then, for each virtualenv you do:

cd dist/
pip install ./<name-of-tar.gz-package>
cd ..
./runtests.py

I did the same also for the wheel package (uninstalling the dependencies and reinstalling the package).

Without this test we would risk to release a broken package and would piss many people off!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know. 👍

@nemesifier
Copy link
Member

I can reproduce the travis issue wit the following procedure:

git clone https://github.com/ritwickdsouza/netjsonconfig.git tmp-netjsonconfig
cd tmp-netjsonconfig
git checkout cond-depend-wheel
mkvirtualenv tmp --python=/usr/bin/python3
pip install -U pip wheel setuptools
python setup.py develop
pip install -r requirements-test.txt
nosetests

@nemesifier
Copy link
Member

nemesifier commented May 24, 2017

@ritwickdsouza keep me up to date (I suppose you are busy with your exams)

@coveralls
Copy link

coveralls commented May 24, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 0badc8b on ritwickdsouza:cond-depend-wheel into 6f15fa5 on openwisp:master.

@ritwickdsouza
Copy link
Contributor Author

ritwickdsouza commented May 24, 2017

@nemesisdesign Yeah, was busy with exams. Anyway I had introduced the bug by mistake. Works fine now.

@coveralls
Copy link

coveralls commented May 25, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 15c435b on ritwickdsouza:cond-depend-wheel into e7513af on openwisp:master.

@nemesifier
Copy link
Member

Also, please rebase from master with:

git checkout master
git pull https://github.com/openwisp/netjsonconfig.git master
git checkout cond-depend-wheel
git rebase master

@coveralls
Copy link

coveralls commented May 25, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 10027ad on ritwickdsouza:cond-depend-wheel into e7513af on openwisp:master.

@coveralls
Copy link

coveralls commented May 25, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling ce4d39b on ritwickdsouza:cond-depend-wheel into e7513af on openwisp:master.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Great, thanks! 👍

@nemesifier nemesifier merged commit 63673b4 into openwisp:master May 25, 2017
@ritwickdsouza ritwickdsouza deleted the cond-depend-wheel branch May 25, 2017 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants