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.

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

setup.py test not installing elements from extra_requires with markers #1093

Closed
alex opened this Issue Jul 20, 2017 · 10 comments

Comments

Projects
None yet
3 participants
@alex
Copy link
Member

alex commented Jul 20, 2017

Steps to reproduce (requires using Python2):

$ git clone https://github.com/alex/cryptography
$ cd cryptography
$ git checkout other-style-environment-markers
$ virtualenv .venv
$ source .venv/bin/activate
$ python setup.py test
[...]
Traceback (most recent call last):
  File "setup.py", line 307, in <module>
    **keywords_with_side_effects(sys.argv)
  File "/Users/alex_gaynor/.pyenv/versions/2.7.13/lib/python2.7/distutils/core.py", line 151, in setup
    dist.run_commands()
  File "/Users/alex_gaynor/.pyenv/versions/2.7.13/lib/python2.7/distutils/dist.py", line 953, in run_commands
    self.run_command(cmd)
  File "/Users/alex_gaynor/.pyenv/versions/2.7.13/lib/python2.7/distutils/dist.py", line 972, in run_command
    cmd_obj.run()
  File "/Users/alex_gaynor/.virtualenvs/tempenv-465426949608b/lib/python2.7/site-packages/setuptools/command/test.py", line 210, in run
    with self.project_on_sys_path():
  File "/Users/alex_gaynor/.pyenv/versions/2.7.13/lib/python2.7/contextlib.py", line 17, in __enter__
    return self.gen.next()
  File "/Users/alex_gaynor/.virtualenvs/tempenv-465426949608b/lib/python2.7/site-packages/setuptools/command/test.py", line 153, in project_on_sys_path
    require('%s==%s' % (ei_cmd.egg_name, ei_cmd.egg_version))
  File "/Users/alex_gaynor/.virtualenvs/tempenv-465426949608b/lib/python2.7/site-packages/pkg_resources/__init__.py", line 971, in require
    needed = self.resolve(parse_requirements(requirements))
  File "/Users/alex_gaynor/.virtualenvs/tempenv-465426949608b/lib/python2.7/site-packages/pkg_resources/__init__.py", line 857, in resolve
    raise DistributionNotFound(req, requirers)
pkg_resources.DistributionNotFound: The 'ipaddress' distribution was not found and is required by cryptography

You can see the patch that triggers this here: pyca/cryptography#3796

@alex

This comment has been minimized.

Copy link
Member Author

alex commented Jul 20, 2017

So, the problem appears to be here: https://github.com/pypa/setuptools/blob/master/setuptools/command/test.py#L188-L196

extras_require is totally ignored -- I don't know the setuptools code quite well enough to know where to find the code that processes extras_require though.

@benoit-pierre

This comment has been minimized.

Copy link
Member

benoit-pierre commented Jul 20, 2017

If you're using setuptools < 36.2.0, then you can put those requirements in install_requires:

 setup.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git i/setup.py w/setup.py
index fec78409..c929cace 100644
--- i/setup.py
+++ w/setup.py
@@ -279,12 +279,12 @@ setup(
         "idna >= 2.1",
         "asn1crypto >= 0.21.0",
         "six >= 1.4.1",
+        "enum34; python_version < '3'",
+        "ipaddress; python_version < '3'",
+        "cffi >= 1.7; python_implementation != 'PyPy'",
     ],
     tests_require=test_requirements,
     extras_require={
-        ":python_version < '3'": ["enum34", "ipaddress"],
-        ":python_implementation != 'PyPy'": ["cffi >= 1.7"],
-
         "test": test_requirements,
         "docstest": [
             "doc8",

Except generated wheels won't be correct, because markers are stripped from the metadata. setuptools >= 36.2.0 aims to fix that by automatically moving requirements in install_requires that are using markers to extras_require, which of course is problematic with this use case...

@alex

This comment has been minimized.

Copy link
Member Author

alex commented Jul 20, 2017

@benoit-pierre Yeah... I would love nothing more than to do it that way, unfortunately setuptools + pip gained support for the extras_require version before they gained support for "inline markers", so to avoid breaking quite so much of the world, I'm trying to use the extras_require version.

@benoit-pierre

This comment has been minimized.

Copy link
Member

benoit-pierre commented Jul 20, 2017

 setuptools/command/test.py | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git i/setuptools/command/test.py w/setuptools/command/test.py
index c14b1242..f13cb732 100644
--- i/setuptools/command/test.py
+++ w/setuptools/command/test.py
@@ -11,7 +11,7 @@ from setuptools.extern import six
 from setuptools.extern.six.moves import map, filter
 
 from pkg_resources import (resource_listdir, resource_exists, normalize_path,
-                           working_set, _namespace_packages,
+                           working_set, _namespace_packages, evaluate_marker,
                            add_activation_listener, require, EntryPoint)
 from setuptools import Command
 from setuptools.py31compat import unittest_main
@@ -204,7 +204,11 @@ class test(Command):
         """
         ir_d = dist.fetch_build_eggs(dist.install_requires or [])
         tr_d = dist.fetch_build_eggs(dist.tests_require or [])
-        return itertools.chain(ir_d, tr_d)
+        er_d = []
+        for k, v in dist.extras_require.items():
+            if k.startswith(':'):
+                er_d.extend(dist.fetch_build_eggs(v))
+        return itertools.chain(ir_d, tr_d, er_d)
 
     def run(self):
         installed_dists = self.install_dists(self.distribution)
@alex

This comment has been minimized.

Copy link
Member Author

alex commented Jul 20, 2017

Is that correct? It looks like it will install all items in extras_require, regardless of if their marker matches.

@benoit-pierre

This comment has been minimized.

Copy link
Member

benoit-pierre commented Jul 21, 2017

Oups! Forgot to actually call evaluate_marker:

if k.startswith(':') and evaluate_marker(k[1:]):

Now to add a test to the testsuite...

@alex

This comment has been minimized.

Copy link
Member Author

alex commented Jul 21, 2017

Not an expert in the setuptools code, but looks plausible to me!

@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Jul 26, 2017

@benoit-pierre Is this issue still open? It seems like a regression I'd like to correct asap.

@alex

This comment has been minimized.

Copy link
Member Author

alex commented Jul 26, 2017

@jaraco I don't think this is a regression, although I'd be appreciative if it was fixed quickly :-)

@benoit-pierre

This comment has been minimized.

Copy link
Member

benoit-pierre commented Jul 26, 2017

@jaraco: it's not a regression, see #1106.

@jaraco jaraco closed this in #1106 Jul 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.