Merge pull request #2153 from xavfernandez/xfernandez/extras_fix

Fix extras_require and environment markers in sdist

pip/req/req_install.py

@@ -89,7 +89,8 @@ def __init__(self, req, comes_from, source_dir=None, editable=False,
     @classmethod
     def from_editable(cls, editable_req, comes_from=None, default_vcs=None,
                       isolated=False):
-        name, url, extras_override = parse_editable(editable_req, default_vcs)
+        name, url, extras_override, editable_options = parse_editable(
+            editable_req, default_vcs)
         if url.startswith('file:'):
             source_dir = url_to_path(url)
         else:
@@ -98,7 +99,7 @@ def from_editable(cls, editable_req, comes_from=None, default_vcs=None,
         res = cls(name, comes_from, source_dir=source_dir,
                   editable=True,
                   url=url,
-                  editable_options=extras_override,
+                  editable_options=editable_options,
                   isolated=isolated)
 
         if extras_override is not None:
@@ -461,18 +462,6 @@ def egg_info_path(self, filename):
             self._egg_info_path = os.path.join(base, filenames[0])
         return os.path.join(self._egg_info_path, filename)
 
-    def egg_info_lines(self, filename):
-        data = self.egg_info_data(filename)
-        if not data:
-            return []
-        result = []
-        for line in data.splitlines():
-            line = line.strip()
-            if not line or line.startswith('#'):
-                continue
-            result.append(line)
-        return result
-
     def pkg_info(self):
         p = FeedParser()
         data = self.egg_info_data('PKG-INFO')
@@ -484,29 +473,8 @@ def pkg_info(self):
         p.feed(data or '')
         return p.close()
 
-    @property
-    def dependency_links(self):
-        return self.egg_info_lines('dependency_links.txt')
-
     _requirements_section_re = re.compile(r'\[(.*?)\]')
 
-    def requirements(self, extras=()):
-        if self.satisfied_by:
-            for r in self.satisfied_by.requires(extras):
-                yield str(r)
-            return
-        in_extra = None
-        for line in self.egg_info_lines('requires.txt'):
-            match = self._requirements_section_re.match(line.lower())
-            if match:
-                in_extra = match.group(1)
-                continue
-            if in_extra and in_extra not in extras:
-                logger.debug('skipping extra %s', in_extra)
-                # Skip requirement for an extra we aren't requiring
-                continue
-            yield line
-
     @property
     def installed_version(self):
         if self.is_wheel:
@@ -988,6 +956,17 @@ def move_wheel_files(self, wheeldir, root=None):
             isolated=self.isolated,
         )
 
+    def get_dist(self):
+        """Return a pkg_resources.Distribution built from self.egg_info_path"""
+        egg_info = self.egg_info_path('')
+        base_dir = os.path.dirname(egg_info)
+        metadata = pkg_resources.PathMetadata(base_dir, egg_info)
+        dist_name = os.path.splitext(os.path.basename(egg_info))[0]
+        return pkg_resources.Distribution(
+            os.path.dirname(egg_info),
+            project_name=dist_name,
+            metadata=metadata)
+
 
 def _strip_postfix(req):
     """
@@ -1034,8 +1013,15 @@ def _build_editable_options(req):
 
 
 def parse_editable(editable_req, default_vcs=None):
-    """Parses svn+http://blahblah@rev#egg=Foobar into a requirement
-    (Foobar) and a URL"""
+    """Parses an editable requirement into:
+        - a requirement name
+        - an URL
+        - extras
+        - editable options
+    Accepted requirements:
+        svn+http://blahblah@rev#egg=Foobar[baz]&subdirectory=version_subdir
+        .[some_extra]
+    """
 
     url = editable_req
     extras = None
@@ -1065,9 +1051,10 @@ def parse_editable(editable_req, default_vcs=None):
                 pkg_resources.Requirement.parse(
                     '__placeholder__' + extras
                 ).extras,
+                {},
             )
         else:
-            return None, url_no_extras, None
+            return None, url_no_extras, None, {}
 
     for version_control in vcs:
         if url.lower().startswith('%s:' % version_control):
@@ -1109,4 +1096,4 @@ def parse_editable(editable_req, default_vcs=None):
         req = options['egg']
 
     package = _strip_postfix(req)
-    return package, url, options
+    return package, url, None, options

pip/req/req_set.py

@@ -410,67 +410,45 @@ def prepare_files(self, finder):
                 # ###################### #
                 # # parse dependencies # #
                 # ###################### #
+                if (req_to_install.extras):
+                    logger.debug(
+                        "Installing extra requirements: %r",
+                        ','.join(req_to_install.extras),
+                    )
 
                 if is_wheel:
                     dist = list(
                         pkg_resources.find_distributions(location)
                     )[0]
-                    if not req_to_install.req:
-                        req_to_install.req = dist.as_requirement()
-                        self.add_requirement(req_to_install)
-                    if not self.ignore_dependencies:
-                        for subreq in dist.requires(
-                                req_to_install.extras):
-                            if self.has_requirement(
-                                    subreq.project_name):
-                                continue
-                            subreq = InstallRequirement(
-                                str(subreq),
-                                req_to_install,
-                                isolated=self.isolated,
-                            )
-                            reqs.append(subreq)
-                            self.add_requirement(subreq)
-
-                # sdists
-                else:
+                else:  # sdists
+                    if req_to_install.satisfied_by:
+                        dist = req_to_install.satisfied_by
+                    else:
+                        dist = req_to_install.get_dist()
                     # FIXME: shouldn't be globally added:
-                    finder.add_dependency_links(
-                        req_to_install.dependency_links
-                    )
-                    if (req_to_install.extras):
-                        logger.info(
-                            "Installing extra requirements: %r",
-                            ','.join(req_to_install.extras),
+                    if dist.has_metadata('dependency_links.txt'):
+                        finder.add_dependency_links(
+                            dist.get_metadata_lines('dependency_links.txt')
                         )
-                    if not self.ignore_dependencies:
-                        for req in req_to_install.requirements(
-                                req_to_install.extras):
-                            try:
-                                name = pkg_resources.Requirement.parse(
-                                    req
-                                ).project_name
-                            except ValueError as exc:
-                                # FIXME: proper warning
-                                logger.error(
-                                    'Invalid requirement: %r (%s) in '
-                                    'requirement %s',
-                                    req, exc, req_to_install,
-                                )
-                                continue
-                            if self.has_requirement(name):
-                                # FIXME: check for conflict
-                                continue
-                            subreq = InstallRequirement(
-                                req,
-                                req_to_install,
-                                isolated=self.isolated,
-                            )
-                            reqs.append(subreq)
-                            self.add_requirement(subreq)
-                    if not self.has_requirement(req_to_install.name):
-                        # 'unnamed' requirements will get added here
-                        self.add_requirement(req_to_install)
+
+                if not self.ignore_dependencies:
+                    for subreq in dist.requires(
+                            req_to_install.extras):
+                        if self.has_requirement(
+                                subreq.project_name):
+                            # FIXME: check for conflict
+                            continue
+                        subreq = InstallRequirement(
+                            str(subreq),
+                            req_to_install,
+                            isolated=self.isolated,
+                        )
+                        reqs.append(subreq)
+                        self.add_requirement(subreq)
+
+                if not self.has_requirement(req_to_install.name):
+                    # 'unnamed' requirements will get added here
+                    self.add_requirement(req_to_install)
 
                 # cleanup tmp src
                 if (self.is_download or

tests/data/packages/LocalEnvironMarker/.gitignore

@@ -0,0 +1 @@
+/LocalEnvironMarker.egg-info

tests/data/packages/LocalEnvironMarker/setup.py

@@ -0,0 +1,29 @@
+import os
+from setuptools import setup, find_packages
+
+
+def path_to_url(path):
+    """
+    Convert a path to URI. The path will be made absolute and
+    will not have quoted path parts.
+    """
+    path = os.path.normpath(os.path.abspath(path))
+    drive, path = os.path.splitdrive(path)
+    filepath = path.split(os.path.sep)
+    url = '/'.join(filepath)
+    if drive:
+        return 'file:///' + drive + url
+    return 'file://' +url
+
+
+HERE = os.path.dirname(__file__)
+DEP_PATH = os.path.join(HERE, '..', '..', 'indexes', 'simple', 'simple')
+DEP_URL = path_to_url(DEP_PATH)
+
+setup(
+    name='LocalEnvironMarker',
+    version='0.0.1',
+    packages=find_packages(),
+    extras_require={":python_version == '2.7' or python_version == '3.4'": ['simple'] },
+    dependency_links=[DEP_URL]
+)

tests/unit/test_req.py

@@ -55,6 +55,23 @@ def test_no_reuse_existing_build_dir(self, data):
             finder,
         )
 
+    def test_environment_marker_extras(self, data):
+        """
+        Test that the environment marker extras are used with
+        non-wheel installs.
+        """
+        reqset = self.basic_reqset()
+        req = InstallRequirement.from_editable(
+            data.packages.join("LocalEnvironMarker"))
+        reqset.add_requirement(req)
+        finder = PackageFinder([data.find_links], [], session=PipSession())
+        reqset.prepare_files(finder)
+        # This is hacky but does test both case in py2 and py3
+        if sys.version_info[:2] in ((2, 7), (3, 4)):
+            assert reqset.has_requirement('simple')
+        else:
+            assert not reqset.has_requirement('simple')
+
 
 @pytest.mark.parametrize(('file_contents', 'expected'), [
     (b'\xf6\x80', b'\xc3\xb6\xe2\x82\xac'),  # cp1252
@@ -204,17 +221,18 @@ def test_parse_editable_local(
     exists_mock.return_value = isdir_mock.return_value = True
     # mocks needed to support path operations on windows tests
     normcase_mock.return_value = getcwd_mock.return_value = "/some/path"
-    assert parse_editable('.', 'git') == (None, 'file:///some/path', None)
+    assert parse_editable('.', 'git') == (None, 'file:///some/path', None, {})
     normcase_mock.return_value = "/some/path/foo"
     assert parse_editable('foo', 'git') == (
-        None, 'file:///some/path/foo', None,
+        None, 'file:///some/path/foo', None, {},
     )
 
 
 def test_parse_editable_default_vcs():
     assert parse_editable('https://foo#egg=foo', 'git') == (
         'foo',
         'git+https://foo#egg=foo',
+        None,
         {'egg': 'foo'},
     )
 
@@ -223,6 +241,7 @@ def test_parse_editable_explicit_vcs():
     assert parse_editable('svn+https://foo#egg=foo', 'git') == (
         'foo',
         'svn+https://foo#egg=foo',
+        None,
         {'egg': 'foo'},
     )
 
@@ -231,6 +250,7 @@ def test_parse_editable_vcs_extras():
     assert parse_editable('svn+https://foo#egg=foo[extras]', 'git') == (
         'foo[extras]',
         'svn+https://foo#egg=foo[extras]',
+        None,
         {'egg': 'foo[extras]'},
     )
 
@@ -244,11 +264,11 @@ def test_parse_editable_local_extras(
     exists_mock.return_value = isdir_mock.return_value = True
     normcase_mock.return_value = getcwd_mock.return_value = "/some/path"
     assert parse_editable('.[extras]', 'git') == (
-        None, 'file://' + "/some/path", ('extras',),
+        None, 'file://' + "/some/path", ('extras',), {},
     )
     normcase_mock.return_value = "/some/path/foo"
     assert parse_editable('foo[bar,baz]', 'git') == (
-        None, 'file:///some/path/foo', ('bar', 'baz'),
+        None, 'file:///some/path/foo', ('bar', 'baz'), {},
     )