Use specific version of python for requirements #348

artragis · 2017-10-23T07:32:08Z

This aims to fix #344

For now, I'm not used to jython and pypy so I don't know how to handle them.

Moreover I'm not sure using the sys.version instead of sys.version_info is the right thing to do.

coveralls · 2017-10-23T07:41:46Z

Coverage increased (+0.08%) to 87.531% when pulling 5cdbe0e on artragis:use_specific_version into ffac016 on nose-devs:master.

coveralls · 2017-10-23T07:50:48Z

Coverage increased (+0.08%) to 87.531% when pulling 7ec4414 on artragis:use_specific_version into ffac016 on nose-devs:master.

coveralls · 2017-10-23T10:14:41Z

Coverage remained the same at 87.449% when pulling 9ffc7a4 on artragis:use_specific_version into ffac016 on nose-devs:master.

katrinabrock · 2017-10-23T13:02:57Z

Cool, thanks. Let's update the..travis.yml too to use this instead of its own thing. That will also tell us if it's working for pypy.

artragis · 2017-10-23T13:50:22Z

I tried something that can help. But as travis does not use tox, I can't see anything change.

coveralls · 2017-10-23T13:54:35Z

Coverage increased (+0.08%) to 87.531% when pulling e6be023 on artragis:use_specific_version into ffac016 on nose-devs:master.

katrinabrock · 2017-10-23T14:13:07Z

What you did is basically what I had in mind.

katrinabrock · 2017-10-23T14:14:34Z

.travis.yml

@@ -11,7 +11,7 @@ python:
 - '3.4'
 - '3.5'
 install:
- travis_retry pip install -r requirements-${TRAVIS_PYTHON_VERSION}.txt --allow-external
+- travis_retry pip install -e .
  argparse


I think argparse is the argument for --allow-external option so should delete both or neither.

thanks a lot.

katrinabrock · 2017-10-23T14:38:04Z

Looking at the travis logs for this pr vs others, it seems like the installation is already happening, that these statements are just for requirements, so maybe try deleting that pip install line all together?

After this, tox integration is definitely the next step. If you want to play around with travis config, feel free to open few dummy PRs to kick off builds.

artragis · 2017-10-23T14:41:10Z

For now, I just deal with travis. Your first comment just made me realize tox was not yet in there so my changes were not tested.

The next run should pass.

katrinabrock · 2017-10-23T14:50:44Z

Looks like it's passing :-) Any reason you're installing in dev mode? -e

artragis · 2017-10-23T14:52:15Z

It's just an habit. If you want me to remove that, I can.

coveralls · 2017-10-23T14:52:27Z

Coverage increased (+0.08%) to 87.531% when pulling 0197c7a on artragis:use_specific_version into ffac016 on nose-devs:master.

katrinabrock · 2017-10-23T14:56:08Z

Yeah, now that it's passing, I'd like to make the code maybe a tiny bit more readable. Don't have time right this moment, but I'll take a look and give more specific comments w/in 24-48hrs.

katrinabrock

Thanks for working on this!

katrinabrock · 2017-10-24T02:02:30Z

.travis.yml

@@ -11,8 +11,7 @@ python:
 - '3.4'
 - '3.5'
 install:
- travis_retry pip install -r requirements-${TRAVIS_PYTHON_VERSION}.txt --allow-external
-  argparse
+- travis_retry pip install -e .


Yeah, should probably not test in dev mode.

katrinabrock · 2017-10-24T02:12:12Z

setup.py

+        REQS.update(list(filter(lambda s: s and not s.startswith('#'), requirements_file.readlines())))
+    with open(os.path.join(directory, 'requirements.txt')) as requirements_file:
+        REQS.update(list(filter(lambda s: s and not s.startswith('#'), requirements_file.readlines())))
+    print(REQS)


Is this a debugging message?

Yes, removed, sorry.

katrinabrock · 2017-10-24T02:14:44Z

setup.py


+def _get_specific_requirements():


This function should probably take version as input and return a list of requirements instead using and setting variables outside of its scope.

Also would be nice to have a docstring, even a oneliner.

katrinabrock · 2017-10-24T02:20:51Z

setup.py

+            version_specific_requirements_path = 'requirements-3.5.txt'
+        else:
+            version_specific_requirements_path = 'requirements-2.7.txt'
+    with open(os.path.join(directory, version_specific_requirements_path)) as requirements_file:


Maybe use pip.req?
http://jelly.codes/articles/python-pip-module/

Otherwise, this should at least be a function to avoid duplicate code.

Yes, I will deduplicate. Thanks for pip.req, I didn't know it :)

in fact pip.req seems not to be a good idea : https://stackoverflow.com/questions/14399534/reference-requirements-txt-for-the-install-requires-kwarg-in-setuptools-setup-py :

You really don't want to do this. Speaking as a pip maintainer pip does not support being called as an API like this at all. In fact pip 1.6 (next version at this time) moves this function. – Donald Stufft Mar 26 '14 at 0:59

You are correct.
but :-( would be nice if they supported this

katrinabrock · 2017-10-24T02:32:47Z

setup.py


+def _get_specific_requirements():
+    directory = dirname(__file__)
+    version_specific_requirements_path = 'requirements-{}.{}.txt'.format(py_version.major, py_version.minor)


Is it possible to make these nested if statements into just one if...elif...elif...else ?
Or maybe something like.

if is_pypy: pypy_name = "pypy" else: pypy_name = "" version_specific_requirements_path = 'requirements-{}{}.{}.txt'.format(pypy_name, py_version.major, py_version.minor)

and then rename the requirements files accordingly.

Overall goal would be to make logic easier to decipher at first glance.

because of pypy3 I think you meant :

pypy_name = '' if is_pypy: pypy_name = "pypy" version_specific_requirements_path = 'requirements-{}{}.{}.txt'.format(pypy_name, py_version.major, py_version.minor)

With that I have to create multiple requirements file for pypy3.3/pypy3.2/pypy3.5, are you ok with that? (pypy doc : http://doc.pypy.org/en/latest/index-of-release-notes.html#cpython-3-5-compatible-versions)

The way you ended up doing it looks great and passes travis.

katrinabrock · 2017-10-24T02:34:33Z

setup.py

    params['test_suite'] = 'unittest.collector'
+    params['extras_require']['doc'] = ['Sphinx>=1.0.5']


so, this answer suggests using this value to solve this whole problem. Not sure if it will work with pypy but maybe worth a shot?

I did not knew it. I think I will make a full rework.

coveralls · 2017-10-24T07:29:46Z

Coverage increased (+0.08%) to 87.531% when pulling 4847d2a on artragis:use_specific_version into ffac016 on nose-devs:master.

coveralls · 2017-10-24T07:35:38Z

Coverage decreased (-0.08%) to 87.367% when pulling 4847d2a on artragis:use_specific_version into ffac016 on nose-devs:master.

coveralls · 2017-10-24T07:57:29Z

Coverage increased (+0.08%) to 87.531% when pulling ac83fab on artragis:use_specific_version into ffac016 on nose-devs:master.

katrinabrock

Looking much better.

Don't necessarily need to make changes, but would like get your response to these last couple comments.

katrinabrock · 2017-10-24T12:54:35Z

setup.py

+    return requirements
+
+
+def per_version_requirements(extra_requires_dict):


Am I missing something? Is this getting called anywhere? If not, how is travis passing?

Also, you don't need to pass in the old version, you can just have something like

params['extra_require'].update(per_version_requirements())

you don't miss anything.

Weird that it was passing though.

In fact, tox is not used so the build is passing but tox is out of service.
I commit once the full tox run is over.

yes, but your travis should still be running this code when you do pip install . Then the tests should fail if proper requirements are not installed.

(Wouldn't fail with the old .travis.yaml, this should have failed with the new one)

katrinabrock · 2017-10-24T12:54:46Z

tox.ini

@@ -3,7 +3,7 @@ envlist=py27,py33,py34,pypy,docs,self27,cov27

 # Default settings for py27, py33, py34 and pypy
 [testenv]
-deps=-r{toxinidir}/requirements.txt
+deps=-e {toxinidir}


-e still here...have you tested it with tox? I haven't been able to make tox generally happy so trusting the committer until we get tox integrated with travis.

For me tox run in a quite verbose way and is just failing in the doc env.
I commit something that makes it working better but tox is not totally ready I think.

Is it closer to passing than on master, the same, or worse for you? If this makes it better, I'll merge. Not going to release until I get tox passing on travis though.

we've git exactky the same error in fact.
I also think that one problem we have for documentation is that we use a very old version of sphinx + we do not add the "sphinx_rtd_theme" module.

I think I found what I missed.

coveralls · 2017-10-24T13:54:21Z

Coverage remained the same at 87.449% when pulling 575f0cc on artragis:use_specific_version into ffac016 on nose-devs:master.

coveralls · 2017-10-24T14:41:06Z

Coverage remained the same at 87.449% when pulling 8ba1d47 on artragis:use_specific_version into ffac016 on nose-devs:master.

katrinabrock · 2017-10-24T15:02:33Z

Current state looks good to me.
Btw - are you aware that you're committing under a different name?

artragis · 2017-10-24T15:04:44Z

Hello, for the name : yes : we use nose2 at work so I use my "working" alias instead of the pseudo I used to (artragis) If you want me to amend, I can.

katrinabrock · 2017-10-24T15:05:50Z

I don't care one way or the other.

artragis · 2017-10-24T15:09:40Z

Ok I come back with a fix to tox as soon as possible :)

artragis force-pushed the use_specific_version branch from 5cdbe0e to 7ec4414 Compare October 23, 2017 07:41

artragis force-pushed the use_specific_version branch from 7ec4414 to 9ffc7a4 Compare October 23, 2017 10:06

artragis force-pushed the use_specific_version branch 2 times, most recently from 11382a9 to 084c6cf Compare October 23, 2017 14:11

katrinabrock reviewed Oct 23, 2017

View reviewed changes

artragis force-pushed the use_specific_version branch 2 times, most recently from da76adc to 62d282e Compare October 23, 2017 14:28

artragis force-pushed the use_specific_version branch from 62d282e to 78d6601 Compare October 23, 2017 14:39

artragis force-pushed the use_specific_version branch from 78d6601 to 0197c7a Compare October 23, 2017 14:44

katrinabrock requested changes Oct 24, 2017

View reviewed changes

artragis force-pushed the use_specific_version branch 2 times, most recently from 75e6e77 to 4847d2a Compare October 24, 2017 07:23

Use specific version of python for requirements

33f8b32

artragis force-pushed the use_specific_version branch from 4847d2a to ac83fab Compare October 24, 2017 07:48

katrinabrock reviewed Oct 24, 2017

View reviewed changes

artragis force-pushed the use_specific_version branch from ac83fab to 575f0cc Compare October 24, 2017 13:42

update tox.ini to remove hand written per-env-statements

8ba1d47

artragis force-pushed the use_specific_version branch from 575f0cc to 8ba1d47 Compare October 24, 2017 14:31

katrinabrock approved these changes Oct 24, 2017

View reviewed changes

katrinabrock merged commit f9851a2 into nose-devs:master Oct 24, 2017

		params['test_suite'] = 'unittest.collector'
		params['extras_require']['doc'] = ['Sphinx>=1.0.5']

		return requirements


		def per_version_requirements(extra_requires_dict):

Use specific version of python for requirements #348

Use specific version of python for requirements #348

Conversation

artragis commented Oct 23, 2017

coveralls commented Oct 23, 2017 • edited Loading

coveralls commented Oct 23, 2017 • edited Loading

coveralls commented Oct 23, 2017 • edited Loading

katrinabrock commented Oct 23, 2017

artragis commented Oct 23, 2017

coveralls commented Oct 23, 2017 • edited Loading

katrinabrock commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katrinabrock commented Oct 23, 2017

artragis commented Oct 23, 2017

katrinabrock commented Oct 23, 2017

artragis commented Oct 23, 2017

coveralls commented Oct 23, 2017 • edited Loading

katrinabrock commented Oct 23, 2017

katrinabrock 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

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

coveralls commented Oct 24, 2017 • edited Loading

coveralls commented Oct 24, 2017 • edited Loading

coveralls commented Oct 24, 2017 • edited Loading

katrinabrock 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 24, 2017 • edited Loading

coveralls commented Oct 24, 2017 • edited Loading

katrinabrock commented Oct 24, 2017

artragis commented Oct 24, 2017

katrinabrock commented Oct 24, 2017

artragis commented Oct 24, 2017

coveralls commented Oct 23, 2017 •

edited

Loading

coveralls commented Oct 23, 2017 •

edited

Loading

coveralls commented Oct 23, 2017 •

edited

Loading

coveralls commented Oct 23, 2017 •

edited

Loading

coveralls commented Oct 23, 2017 •

edited

Loading

coveralls commented Oct 24, 2017 •

edited

Loading

coveralls commented Oct 24, 2017 •

edited

Loading

coveralls commented Oct 24, 2017 •

edited

Loading

coveralls commented Oct 24, 2017 •

edited

Loading

coveralls commented Oct 24, 2017 •

edited

Loading