From e9dc3247dc82201e272b618172ce11106cbd62fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C6=B0=C6=A1ng=20Qu=E1=BB=91c=20Kh=C3=A1nh?= Date: Sat, 13 Aug 2022 18:17:09 +0900 Subject: [PATCH] Issue 4371 incorrect dependencies when install dev packages (#5234) * Add test, ensure dev lock use default packages as constraints. * Use default packages as constraints when locking develop packages. * Add test, ensure installing dev-packages use default packages as constraints. (#4371) (#2987) * Use default packages as constraints when installing provided dev packages. * change vistir.path.normalize_path to pipenv.utils.shell.normalize_path * Add function that get contraints from packages. * Add test for get_constraints_from_deps function * Use get_constraints_from_deps to get constraints * Use @cached_property instead of @property * Use standalone utility to write constraints file * prepare_constraint_file use precomputed constraints. * Add news fragment. --- news/5234.bugfix.rst | 2 + pipenv/core.py | 24 ++++++- pipenv/resolver.py | 6 +- pipenv/utils/dependencies.py | 54 +++++++++++++++ pipenv/utils/resolver.py | 87 +++++++++++++++++-------- tests/integration/test_install_basic.py | 26 ++++++++ tests/integration/test_lock.py | 35 ++++++++++ tests/unit/test_utils.py | 14 ++++ 8 files changed, 217 insertions(+), 31 deletions(-) create mode 100644 news/5234.bugfix.rst diff --git a/news/5234.bugfix.rst b/news/5234.bugfix.rst new file mode 100644 index 00000000..83cbad39 --- /dev/null +++ b/news/5234.bugfix.rst @@ -0,0 +1,2 @@ +Use ``packages`` as contraints when locking ``dev-packages`` in Pipfile. +Use ``packages`` as contraints when installing new ``dev-packages``. diff --git a/pipenv/core.py b/pipenv/core.py index 25910412..b6df9467 100644 --- a/pipenv/core.py +++ b/pipenv/core.py @@ -28,10 +28,12 @@ from pipenv.utils.constants import MYPY_RUNNING from pipenv.utils.dependencies import ( convert_deps_to_pip, get_canonical_names, + get_constraints_from_deps, is_pinned, is_required_version, is_star, pep423_name, + prepare_constraint_file, python_version, ) from pipenv.utils.indexes import get_source_list, parse_indexes, prepare_pip_source_args @@ -41,6 +43,7 @@ from pipenv.utils.shell import ( cmd_list_to_shell, find_python, is_python_command, + normalize_path, project_python, subprocess_run, system_which, @@ -789,6 +792,7 @@ def batch_install( trusted_hosts=trusted_hosts, extra_indexes=extra_indexes, use_pep517=use_pep517, + use_constraint=False, # no need to use constraints, it's written in lockfile ) c.dep = dep @@ -1096,8 +1100,9 @@ def do_lock( for k, v in lockfile[section].copy().items(): if not hasattr(v, "keys"): del lockfile[section][k] - # Resolve dev-package dependencies followed by packages dependencies. - for is_dev in [True, False]: + + # Resolve package to generate constraints before resolving dev-packages + for is_dev in [False, True]: pipfile_section = "dev-packages" if is_dev else "packages" if project.pipfile_exists: packages = project.parsed_pipfile.get(pipfile_section, {}) @@ -1453,12 +1458,14 @@ def pip_install( block=True, index=None, pre=False, + dev=False, selective_upgrade=False, requirements_dir=None, extra_indexes=None, pypi_mirror=None, trusted_hosts=None, use_pep517=True, + use_constraint=False, ): piplogger = logging.getLogger("pipenv.patched.pip._internal.commands.install") if not trusted_hosts: @@ -1539,9 +1546,18 @@ def pip_install( ) pip_command.extend(pip_args) if r: - pip_command.extend(["-r", vistir.path.normalize_path(r)]) + pip_command.extend(["-r", normalize_path(r)]) elif line: pip_command.extend(line) + if dev and use_constraint: + default_constraints = get_constraints_from_deps(project.packages) + constraint_filename = prepare_constraint_file( + default_constraints, + directory=requirements_dir, + sources=None, + pip_args=None, + ) + pip_command.extend(["-c", normalize_path(constraint_filename)]) pip_command.extend(prepare_pip_source_args(sources)) if project.s.is_verbose(): click.echo(f"$ {cmd_list_to_shell(pip_command)}", err=True) @@ -2128,9 +2144,11 @@ def do_install( selective_upgrade=selective_upgrade, no_deps=False, pre=pre, + dev=dev, requirements_dir=requirements_directory, index=index_url, pypi_mirror=pypi_mirror, + use_constraint=True, ) if c.returncode: sp.write_err( diff --git a/pipenv/resolver.py b/pipenv/resolver.py index 9cce20c1..2866bee1 100644 --- a/pipenv/resolver.py +++ b/pipenv/resolver.py @@ -745,12 +745,15 @@ def resolve_packages(pre, clear, verbose, system, write, requirements_dir, packa else None ) - def resolve(packages, pre, project, sources, clear, system, requirements_dir=None): + def resolve( + packages, pre, project, sources, clear, system, dev, requirements_dir=None + ): return resolve_deps( packages, which, project=project, pre=pre, + dev=dev, sources=sources, clear=clear, allow_global=system, @@ -769,6 +772,7 @@ def resolve_packages(pre, clear, verbose, system, write, requirements_dir, packa results, resolver = resolve( packages, pre=pre, + dev=dev, project=project, sources=sources, clear=clear, diff --git a/pipenv/utils/dependencies.py b/pipenv/utils/dependencies.py index b4ca1a70..70b4c315 100644 --- a/pipenv/utils/dependencies.py +++ b/pipenv/utils/dependencies.py @@ -280,6 +280,60 @@ def convert_deps_to_pip( return f.name +def get_constraints_from_deps(deps): + """Get contraints from Pipfile-formatted dependency""" + from pipenv.vendor.requirementslib.models.requirements import Requirement + + def is_constraint(dep): + # https://pip.pypa.io/en/stable/user_guide/#constraints-files + # constraints must have a name, they cannot be editable, and they cannot specify extras. + return dep.name and not dep.editable and not dep.extras + + constraints = [] + for dep_name, dep in deps.items(): + new_dep = Requirement.from_pipfile(dep_name, dep) + if is_constraint(new_dep): + c = new_dep.as_line().strip() + constraints.append(c) + return constraints + + +def prepare_constraint_file( + constraints, + directory=None, + sources=None, + pip_args=None, +): + from pipenv.vendor.vistir.path import ( + create_tracked_tempdir, + create_tracked_tempfile, + ) + + if not directory: + directory = create_tracked_tempdir(suffix="-requirements", prefix="pipenv-") + + constraints_file = create_tracked_tempfile( + mode="w", + prefix="pipenv-", + suffix="-constraints.txt", + dir=directory, + delete=False, + ) + + if sources and pip_args: + skip_args = ("build-isolation", "use-pep517", "cache-dir") + args_to_add = [ + arg for arg in pip_args if not any(bad_arg in arg for bad_arg in skip_args) + ] + requirementstxt_sources = " ".join(args_to_add) if args_to_add else "" + requirementstxt_sources = requirementstxt_sources.replace(" --", "\n--") + constraints_file.write(f"{requirementstxt_sources}\n") + + constraints_file.write("\n".join([c for c in constraints])) + constraints_file.close() + return constraints_file.name + + def is_required_version(version, specified_version): """Check to see if there's a hard requirement for version number provided in the Pipfile. diff --git a/pipenv/utils/resolver.py b/pipenv/utils/resolver.py index 596358be..b8349e95 100644 --- a/pipenv/utils/resolver.py +++ b/pipenv/utils/resolver.py @@ -17,11 +17,15 @@ from pipenv.patched.pip._internal.network.cache import SafeFileCache from pipenv.patched.pip._internal.operations.build.build_tracker import ( get_build_tracker, ) +from pipenv.patched.pip._internal.req.constructors import ( + install_req_from_parsed_requirement, +) from pipenv.patched.pip._internal.req.req_file import parse_requirements from pipenv.patched.pip._internal.utils.hashes import FAVORITE_HASH from pipenv.patched.pip._internal.utils.temp_dir import global_tempdir_manager from pipenv.project import Project from pipenv.vendor import click +from pipenv.vendor.cached_property import cached_property from pipenv.vendor.requirementslib import Pipfile, Requirement from pipenv.vendor.requirementslib.models.requirements import Line from pipenv.vendor.requirementslib.models.utils import DIRECT_URL_RE @@ -32,9 +36,11 @@ from .dependencies import ( HackedPythonVersion, clean_pkg_version, convert_deps_to_pip, + get_constraints_from_deps, get_vcs_deps, is_pinned_requirement, pep423_name, + prepare_constraint_file, translate_markers, ) from .indexes import parse_indexes, prepare_pip_source_args @@ -117,6 +123,7 @@ class Resolver: skipped=None, clear=False, pre=False, + dev=False, ): self.initial_constraints = constraints self.req_dir = req_dir @@ -126,6 +133,7 @@ class Resolver: self.hashes = {} self.clear = clear self.pre = pre + self.dev = dev self.results = None self.markers_lookup = markers_lookup if markers_lookup is not None else {} self.index_lookup = index_lookup if index_lookup is not None else {} @@ -420,6 +428,7 @@ class Resolver: req_dir: str = None, clear: bool = False, pre: bool = False, + dev: bool = False, ) -> "Resolver": if not req_dir: @@ -450,6 +459,7 @@ class Resolver: skipped=skipped, clear=clear, pre=pre, + dev=dev, ) @classmethod @@ -522,29 +532,13 @@ class Resolver: return self._pip_args def prepare_constraint_file(self): - from pipenv.vendor.vistir.path import create_tracked_tempfile - - constraints_file = create_tracked_tempfile( - mode="w", - prefix="pipenv-", - suffix="-constraints.txt", - dir=self.req_dir, - delete=False, + constraint_filename = prepare_constraint_file( + self.initial_constraints, + directory=self.req_dir, + sources=self.sources, + pip_args=self.pip_args, ) - skip_args = ("build-isolation", "use-pep517", "cache-dir") - args_to_add = [ - arg - for arg in self.pip_args - if not any(bad_arg in arg for bad_arg in skip_args) - ] - if self.sources: - requirementstxt_sources = " ".join(args_to_add) if args_to_add else "" - requirementstxt_sources = requirementstxt_sources.replace(" --", "\n--") - constraints_file.write(f"{requirementstxt_sources}\n") - constraints = self.initial_constraints - constraints_file.write("\n".join([c for c in constraints])) - constraints_file.close() - return constraints_file.name + return constraint_filename @property def constraint_file(self): @@ -552,6 +546,17 @@ class Resolver: self._constraint_file = self.prepare_constraint_file() return self._constraint_file + @cached_property + def default_constraint_file(self): + default_constraints = get_constraints_from_deps(self.project.packages) + default_constraint_filename = prepare_constraint_file( + default_constraints, + directory=self.req_dir, + sources=None, + pip_args=None, + ) + return default_constraint_filename + @property def pip_options(self): if self._pip_options is None: @@ -630,12 +635,33 @@ class Resolver: ) return self._parsed_constraints + @cached_property + def parsed_default_constraints(self): + pip_options = self.pip_options + pip_options.extra_index_urls = [] + parsed_default_constraints = parse_requirements( + self.default_constraint_file, + constraint=True, + finder=self.finder, + session=self.session, + options=pip_options, + ) + return parsed_default_constraints + + @cached_property + def default_constraints(self): + default_constraints = [ + install_req_from_parsed_requirement( + c, + isolated=self.pip_options.build_isolation, + user_supplied=False, + ) + for c in self.parsed_default_constraints + ] + return default_constraints + @property def constraints(self): - from pipenv.patched.pip._internal.req.constructors import ( - install_req_from_parsed_requirement, - ) - if self._constraints is None: self._constraints = [ install_req_from_parsed_requirement( @@ -646,6 +672,9 @@ class Resolver: ) for c in self.parsed_constraints ] + # Only use default_constraints when installing dev-packages + if self.dev: + self._constraints += self.default_constraints return self._constraints @contextlib.contextmanager @@ -870,6 +899,7 @@ def actually_resolve_deps( sources, clear, pre, + dev, req_dir=None, ): if not req_dir: @@ -878,7 +908,7 @@ def actually_resolve_deps( with warnings.catch_warnings(record=True) as warning_list: resolver = Resolver.create( - deps, project, index_lookup, markers_lookup, sources, req_dir, clear, pre + deps, project, index_lookup, markers_lookup, sources, req_dir, clear, pre, dev ) resolver.resolve() hashes = resolver.resolve_hashes() @@ -1064,6 +1094,7 @@ def resolve_deps( python=False, clear=False, pre=False, + dev=False, allow_global=False, req_dir=None, ): @@ -1094,6 +1125,7 @@ def resolve_deps( sources, clear, pre, + dev, req_dir=req_dir, ) except RuntimeError: @@ -1122,6 +1154,7 @@ def resolve_deps( sources, clear, pre, + dev, req_dir=req_dir, ) except RuntimeError: diff --git a/tests/integration/test_install_basic.py b/tests/integration/test_install_basic.py index 91b9d336..bc3d919a 100644 --- a/tests/integration/test_install_basic.py +++ b/tests/integration/test_install_basic.py @@ -509,3 +509,29 @@ requests = {version="*", index="pypi"} f.write(contents) c = p.pipenv("install") assert c.returncode == 0 + +@pytest.mark.dev +@pytest.mark.install +def test_install_dev_use_default_constraints(PipenvInstance): + # See https://github.com/pypa/pipenv/issues/4371 + # See https://github.com/pypa/pipenv/issues/2987 + with PipenvInstance(chdir=True) as p: + + c = p.pipenv("install requests==2.14.0") + assert c.returncode == 0 + assert "requests" in p.lockfile["default"] + assert p.lockfile["default"]["requests"]["version"] == "==2.14.0" + + c = p.pipenv("install --dev requests") + assert c.returncode == 0 + assert "requests" in p.lockfile["develop"] + assert p.lockfile["develop"]["requests"]["version"] == "==2.14.0" + + # requests 2.14.0 doesn't require these packages + assert "idna" not in p.lockfile["develop"] + assert "certifi" not in p.lockfile["develop"] + assert "urllib3" not in p.lockfile["develop"] + assert "chardet" not in p.lockfile["develop"] + + c = p.pipenv("run python -c 'import urllib3'") + assert c.returncode != 0 diff --git a/tests/integration/test_lock.py b/tests/integration/test_lock.py index 0208c212..28e7f21f 100644 --- a/tests/integration/test_lock.py +++ b/tests/integration/test_lock.py @@ -791,3 +791,38 @@ requests = {requirement} 'sha256:ec22d826a36ed72a7358ff3fe56cbd4ba69dd7a6718ffd450ff0e9df7a47ce6a'], 'index': 'local', 'version': '==2.19.1'} assert p.lockfile['default']['requests'] == expected_result + + +@pytest.mark.dev +@pytest.mark.lock +@pytest.mark.install +def test_dev_lock_use_default_packages_as_constraint(PipenvInstance): + # See https://github.com/pypa/pipenv/issues/4371 + # See https://github.com/pypa/pipenv/issues/2987 + with PipenvInstance(chdir=True) as p: + with open(p.pipfile_path, 'w') as f: + contents = """ +[packages] +requests = "<=2.14.0" + +[dev-packages] +requests = "*" + """.strip() + f.write(contents) + + c = p.pipenv("lock --dev") + assert c.returncode == 0 + assert "requests" in p.lockfile["default"] + assert p.lockfile["default"]["requests"]["version"] == "==2.14.0" + assert "requests" in p.lockfile["develop"] + assert p.lockfile["develop"]["requests"]["version"] == "==2.14.0" + + # requests 2.14.0 doesn't require these packages + assert "idna" not in p.lockfile["develop"] + assert "certifi" not in p.lockfile["develop"] + assert "urllib3" not in p.lockfile["develop"] + assert "chardet" not in p.lockfile["develop"] + + c = p.pipenv("install --dev") + c = p.pipenv("run python -c 'import urllib3'") + assert c.returncode != 0 diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 3aa22dbb..76835bdb 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -131,6 +131,20 @@ def test_convert_deps_to_pip(deps, expected): def test_convert_deps_to_pip_one_way(deps, expected): assert dependencies.convert_deps_to_pip(deps, r=False) == [expected.lower()] +@pytest.mark.utils +@pytest.mark.parametrize( + "deps, expected", + [ + ({"requests": {}}, ["requests"]), + ({"FooProject": {"path": ".", "editable" : "true"}}, []), + ({"FooProject": {"version": "==1.2"}}, ["fooproject==1.2"]), + ({"requests": {"extras": ["security"]}}, []), + ({"requests": {"extras": []}}, ["requests"]), + ({"extras" : {}}, ["extras"]), + ], +) +def test_get_constraints_from_deps(deps, expected): + assert dependencies.get_constraints_from_deps(deps) == expected @pytest.mark.parametrize("line,result", [ ("-i https://example.com/simple/", ("https://example.com/simple/", None, None, [])),