Fix allow unsafe false pinning bug v2

CHANGELOG.md

@@ -3,6 +3,12 @@
 Features:
 - `--generate-hashes` now generates hashes for all wheels, not only wheels for the currently running platform ([#414](https://github.com/jazzband/pip-tools/issues/414))
 
+# 1.9.1
+
+Bug Fixes:
+- Fixed bug where unsafe packages would get pinned in generated requirements files
+when `--allow-unsafe` was not set. ([#517](https://github.com/jazzband/pip-tools/pull/517)).
+
 # 1.9.0
 
 Features:
@@ -30,7 +36,7 @@ Bug Fixes:
 # 1.8.1
 
 - Recalculate secondary dependencies between rounds (#378)
-- Calculated dependencies could be left with wrong candidates when 
+- Calculated dependencies could be left with wrong candidates when
   toplevel requirements happen to be also pinned in sub-dependencies (#450)
 - Fix duplicate entries that could happen in generated requirements.txt (#427)
 - Gracefully report invalid pip version (#457)

piptools/resolver.py

@@ -2,10 +2,10 @@
 from __future__ import (absolute_import, division, print_function,
                         unicode_literals)
 
-import os
 import copy
 from functools import partial
 from itertools import chain, count
+import os
 
 from first import first
 from pip.req import InstallRequirement
@@ -64,6 +64,7 @@ def __init__(self, constraints, repository, cache=None, prereleases=False, clear
         self.prereleases = prereleases
         self.clear_caches = clear_caches
         self.allow_unsafe = allow_unsafe
+        self.unsafe_constraints = set()
 
     @property
     def constraints(self):
@@ -178,6 +179,15 @@ def _resolve_one_round(self):
         """
         # Sort this list for readability of terminal output
         constraints = sorted(self.constraints, key=_dep_key)
+        unsafe_constraints = []
+        original_constraints = copy.copy(constraints)
+        if not self.allow_unsafe:
+            for constraint in original_constraints:
+                if constraint.name in UNSAFE_PACKAGES:
+                    constraints.remove(constraint)
+                    constraint.req.specifier = None
+                    unsafe_constraints.append(constraint)
+
         log.debug('Current constraints:')
         for constraint in constraints:
             log.debug('  {}'.format(constraint))
@@ -190,21 +200,23 @@ def _resolve_one_round(self):
         log.debug('')
         log.debug('Finding secondary dependencies:')
 
-        ungrouped = []
+        safe_constraints = []
         for best_match in best_matches:
             for dep in self._iter_dependencies(best_match):
                 if self.allow_unsafe or dep.name not in UNSAFE_PACKAGES:
-                    ungrouped.append(dep)
+                    safe_constraints.append(dep)
         # Grouping constraints to make clean diff between rounds
-        theirs = set(self._group_constraints(ungrouped))
+        theirs = set(self._group_constraints(safe_constraints))
 
         # NOTE: We need to compare RequirementSummary objects, since
         # InstallRequirement does not define equality
         diff = {RequirementSummary(t.req) for t in theirs} - {RequirementSummary(t.req) for t in self.their_constraints}
         removed = ({RequirementSummary(t.req) for t in self.their_constraints} -
                    {RequirementSummary(t.req) for t in theirs})
+        unsafe = ({RequirementSummary(t.req) for t in unsafe_constraints} -
+                  {RequirementSummary(t.req) for t in self.unsafe_constraints})
 
-        has_changed = len(diff) > 0 or len(removed) > 0
+        has_changed = len(diff) > 0 or len(removed) > 0 or len(unsafe) > 0
         if has_changed:
             log.debug('')
             log.debug('New dependencies found in this round:')
@@ -213,9 +225,14 @@ def _resolve_one_round(self):
             log.debug('Removed dependencies in this round:')
             for removed_dependency in sorted(removed, key=lambda req: key_from_req(req.req)):
                 log.debug('  removing {}'.format(removed_dependency))
+            log.debug('Unsafe dependencies in this round:')
+            for unsafe_dependency in sorted(unsafe, key=lambda req: key_from_req(req.req)):
+                log.debug('  remembering unsafe {}'.format(unsafe_dependency))
 
         # Store the last round's results in the their_constraints
         self.their_constraints = theirs
+        # Store the last round's unsafe constraints
+        self.unsafe_constraints = unsafe_constraints
         return has_changed, best_matches
 
     def get_best_match(self, ireq):

piptools/scripts/compile.py

@@ -231,11 +231,13 @@ def cli(verbose, dry_run, pre, rebuild, find_links, index_url, extra_index_url,
                           trusted_hosts=pip_options.trusted_hosts,
                           format_control=repository.finder.format_control)
     writer.write(results=results,
+                 unsafe_requirements=resolver.unsafe_constraints,
                  reverse_dependencies=reverse_dependencies,
                  primary_packages={key_from_req(ireq.req) for ireq in constraints if not ireq.constraint},
                  markers={key_from_req(ireq.req): ireq.markers
                           for ireq in constraints if ireq.markers},
-                 hashes=hashes)
+                 hashes=hashes,
+                 allow_unsafe=allow_unsafe)
 
     if dry_run:
         log.warning('Dry-run, so nothing updated.')

piptools/writer.py

@@ -81,44 +81,49 @@ def write_flags(self):
         if emitted:
             yield ''
 
-    def _iter_lines(self, results, reverse_dependencies, primary_packages, markers, hashes):
+    def _iter_lines(self, results, unsafe_requirements, reverse_dependencies,
+                    primary_packages, markers, hashes, allow_unsafe=False):
         for line in self.write_header():
             yield line
         for line in self.write_flags():
             yield line
 
-        unsafe_packages = {r for r in results if r.name in UNSAFE_PACKAGES}
+        unsafe_requirements = {r for r in results if r.name in UNSAFE_PACKAGES} if not unsafe_requirements else unsafe_requirements  # noqa
         packages = {r for r in results if r.name not in UNSAFE_PACKAGES}
 
         packages = sorted(packages, key=self._sort_key)
-        unsafe_packages = sorted(unsafe_packages, key=self._sort_key)
 
         for ireq in packages:
             line = self._format_requirement(
                 ireq, reverse_dependencies, primary_packages,
                 markers.get(ireq.req.name), hashes=hashes)
             yield line
 
-        if unsafe_packages:
+        if unsafe_requirements:
+            unsafe_requirements = sorted(unsafe_requirements, key=self._sort_key)
             yield ''
             yield comment('# The following packages are considered to be unsafe in a requirements file:')
 
-            for ireq in unsafe_packages:
-
-                yield self._format_requirement(ireq,
+            for ireq in unsafe_requirements:
+                req = self._format_requirement(ireq,
                                                reverse_dependencies,
                                                primary_packages,
                                                marker=markers.get(ireq.req.name),
                                                hashes=hashes)
+                if not allow_unsafe:
+                    yield comment('# {}'.format(req))
+                else:
+                    yield req
 
-    def write(self, results, reverse_dependencies, primary_packages, markers, hashes):
+    def write(self, results, unsafe_requirements, reverse_dependencies,
+              primary_packages, markers, hashes, allow_unsafe=False):
         with ExitStack() as stack:
             f = None
             if not self.dry_run:
                 f = stack.enter_context(AtomicSaver(self.dst_file))
 
-            for line in self._iter_lines(results, reverse_dependencies,
-                                         primary_packages, markers, hashes):
+            for line in self._iter_lines(results, unsafe_requirements, reverse_dependencies,
+                                         primary_packages, markers, hashes, allow_unsafe=allow_unsafe):
                 log.info(line)
                 if f:
                     f.write(unstyle(line).encode('utf-8'))

tests/fixtures/fake-index.json

@@ -7,6 +7,9 @@
     "2.0.2": {"": ["vine>=1.1.1"]},
     "2.1.4": {"": ["vine>=1.1.3"]}
   },
+  "appdirs": {
+    "1.4.9": {"": []}
+  },
   "arrow": {
     "0.5.0": {"": ["python-dateutil"]},
     "0.5.4": {"": ["python-dateutil"]}
@@ -46,6 +49,11 @@
         "celery==3.1.18"
     ]}
   },
+  "fake-piptools-test-with-unsafe-deps": {
+    "0.1": {"": [
+        "setuptools==34.0.0"
+    ]}
+  },
   "flask": {
     "0.10.1": {"": [
       "Jinja2>=2.4",
@@ -109,6 +117,9 @@
   "markupsafe": {
     "0.23": {"": []}
   },
+  "packaging": {
+    "16.8": {"": []}
+  },
   "psycopg2": {
     "2.5.4": {"": []},
     "2.6": {"": []}

tests/fixtures/small_fake_package/setup.py

@@ -5,5 +5,5 @@
     version=0.1,
     install_requires=[
         "six==1.10.0",
-        ],
+    ],
 )

tests/test_resolver.py

@@ -105,6 +105,13 @@
          ['flask==0.10.1', 'itsdangerous==0.24', 'markupsafe==0.23',
           'jinja2==2.7.3', 'werkzeug==0.10.4']
          ),
+
+        # Unsafe dependencies should be filtered
+        (['setuptools==35.0.0', 'anyjson==0.3.3'], ['anyjson==0.3.3']),
+
+        (['fake-piptools-test-with-unsafe-deps==0.1'],
+         ['fake-piptools-test-with-unsafe-deps==0.1']
+         ),
     ])
 )
 def test_resolver(resolver, from_line, input, expected, prereleases):
@@ -113,3 +120,25 @@ def test_resolver(resolver, from_line, input, expected, prereleases):
     output = resolver(input, prereleases=prereleases).resolve()
     output = {str(line) for line in output}
     assert output == {str(line) for line in expected}
+
+
+@pytest.mark.parametrize(
+    ('input', 'expected', 'prereleases'),
+
+    ((tup + (False,))[:3] for tup in [
+        (['setuptools==34.0.0'], ['appdirs==1.4.9', 'setuptools==34.0.0', 'packaging==16.8']),
+
+        (['fake-piptools-test-with-unsafe-deps==0.1'],
+         ['appdirs==1.4.9',
+          'setuptools==34.0.0',
+          'packaging==16.8',
+          'fake-piptools-test-with-unsafe-deps==0.1'
+          ]),
+    ])
+)
+def test_resolver__allows_unsafe_deps(resolver, from_line, input, expected, prereleases):
+    input = [line if isinstance(line, tuple) else (line, False) for line in input]
+    input = [from_line(req[0], constraint=req[1]) for req in input]
+    output = resolver(input, prereleases=prereleases, allow_unsafe=True).resolve()
+    output = {str(line) for line in output}
+    assert output == {str(line) for line in expected}

tests/test_writer.py

@@ -69,3 +69,22 @@ def test_format_requirement_environment_marker(from_line, writer):
         marker=ireq.markers)
     assert (result ==
             'test ; python_version == "2.7" and platform_python_implementation == "CPython"')
+
+
+def test_iter_lines__unsafe_dependencies(from_line, writer):
+    ireq = [from_line('test==1.2')]
+    unsafe_req = [from_line('setuptools')]
+    reverse_dependencies = {'test': ['xyz']}
+
+    lines = writer._iter_lines(ireq,
+                               unsafe_req,
+                               reverse_dependencies,
+                               ['test'],
+                               {},
+                               None)
+    str_lines = []
+    for line in lines:
+        str_lines.append(line)
+    assert comment('# The following packages are considered to be unsafe in a requirements file:') in str_lines
+    assert comment('# setuptools') in str_lines
+    assert 'test==1.2' in str_lines