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

Issue 4371 incorrect dependencies when install dev packages #5234

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions news/5234.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Use ``packages`` as contraints when locking ``dev-packages`` in Pipfile.
Use ``packages`` as contraints when installing new ``dev-packages``.
24 changes: 21 additions & 3 deletions pipenv/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,12 @@
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
Expand All @@ -41,6 +43,7 @@
cmd_list_to_shell,
find_python,
is_python_command,
normalize_path,
project_python,
subprocess_run,
system_which,
Expand Down Expand Up @@ -788,6 +791,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

Expand Down Expand Up @@ -1095,8 +1099,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, {})
Expand Down Expand Up @@ -1452,12 +1457,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:
Expand Down Expand Up @@ -1538,9 +1545,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)
Expand Down Expand Up @@ -2127,9 +2143,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(
Expand Down
6 changes: 5 additions & 1 deletion pipenv/resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -769,6 +772,7 @@ def resolve(packages, pre, project, sources, clear, system, requirements_dir=Non
results, resolver = resolve(
packages,
pre=pre,
dev=dev,
project=project,
sources=sources,
clear=clear,
Expand Down
54 changes: 54 additions & 0 deletions pipenv/utils/dependencies.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
87 changes: 60 additions & 27 deletions pipenv/utils/resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,15 @@
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
Expand All @@ -32,9 +36,11 @@
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
Expand Down Expand Up @@ -117,6 +123,7 @@ def __init__(
skipped=None,
clear=False,
pre=False,
dev=False,
):
self.initial_constraints = constraints
self.req_dir = req_dir
Expand All @@ -126,6 +133,7 @@ def __init__(
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 {}
Expand Down Expand Up @@ -420,6 +428,7 @@ def create(
req_dir: str = None,
clear: bool = False,
pre: bool = False,
dev: bool = False,
) -> "Resolver":

if not req_dir:
Expand Down Expand Up @@ -450,6 +459,7 @@ def create(
skipped=skipped,
clear=clear,
pre=pre,
dev=dev,
)

@classmethod
Expand Down Expand Up @@ -522,36 +532,31 @@ def pip_args(self):
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):
if self._constraint_file is None:
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:
Expand Down Expand Up @@ -630,12 +635,33 @@ def parsed_constraints(self):
)
return self._parsed_constraints

@property
def constraints(self):
from pipenv.patched.pip._internal.req.constructors import (
install_req_from_parsed_requirement,
@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):
if self._constraints is None:
self._constraints = [
install_req_from_parsed_requirement(
Expand All @@ -646,6 +672,9 @@ def constraints(self):
)
for c in self.parsed_constraints
]
# Only use default_constraints when installing dev-packages
if self.dev:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain more why we only include the default_constraints when dev is specified? I don't think I quite understand out of the gate. I got to looking at this more because this is the only place that uses self.dev, and I am noticing how many parameters pip_install takes already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think constraint is used only when installing dev-packages. If it does with new normal packages, the installing package could not overwrite existing packages, because it use them as contraint, and therefore might not be installed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok thanks, that makes sense.

self._constraints += self.default_constraints
return self._constraints

@contextlib.contextmanager
Expand Down Expand Up @@ -870,6 +899,7 @@ def actually_resolve_deps(
sources,
clear,
pre,
dev,
req_dir=None,
):
if not req_dir:
Expand All @@ -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()
Expand Down Expand Up @@ -1064,6 +1094,7 @@ def resolve_deps(
python=False,
clear=False,
pre=False,
dev=False,
allow_global=False,
req_dir=None,
):
Expand Down Expand Up @@ -1094,6 +1125,7 @@ def resolve_deps(
sources,
clear,
pre,
dev,
req_dir=req_dir,
)
except RuntimeError:
Expand Down Expand Up @@ -1122,6 +1154,7 @@ def resolve_deps(
sources,
clear,
pre,
dev,
req_dir=req_dir,
)
except RuntimeError:
Expand Down
Loading