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.

Sign up for GitHub

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

Jump to bottom

Issue 4371 incorrect dependencies when install dev packages #5234

Merged
merged 16 commits into from
Aug 13, 2022
Merged

Issue 4371 incorrect dependencies when install dev packages #5234

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
6d1363c
Add test, ensure dev lock use default packages as constraints.
dqkqd Aug 8, 2022
192ad9e
Use default packages as constraints when locking develop packages.
dqkqd Aug 8, 2022
79471df
Add test, ensure installing dev-packages use default packages as cons…
dqkqd Aug 8, 2022
30a6b1a
Use default packages as constraints when installing provided dev pack…
dqkqd Aug 8, 2022
f09d653
Remove flaky
dqkqd Aug 9, 2022
7ab7b3d
change vistir.path.normalize_path to pipenv.utils.shell.normalize_path
dqkqd Aug 9, 2022
23569fb
Add function that get contraints from packages.
dqkqd Aug 9, 2022
83813f9
Add test for get_constraints_from_deps function
dqkqd Aug 9, 2022
1eb8d6d
Use get_constraints_from_deps to get constraints
dqkqd Aug 9, 2022
11969be
Use @cached_property instead of @property
dqkqd Aug 12, 2022
0c098c8
Use @lru_cache and @property instead of @cached_property for python 3.7
dqkqd Aug 12, 2022
c5cb0f0
Use standalone utility to write constraints file
dqkqd Aug 12, 2022
d8ed209
Use pipenv.vendor.cached_property.
dqkqd Aug 13, 2022
beb8d3d
prepare_constraint_file use precomputed constraints.
dqkqd Aug 13, 2022
6b3f23b
Change default value in prepare_constraint_file.
dqkqd Aug 13, 2022
b04442f
Add news fragment.
dqkqd Aug 13, 2022
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
31 changes: 3 additions & 28 deletions pipenv/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@
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_default_constraint_file,
python_version,
)
from pipenv.utils.indexes import get_source_list, parse_indexes, prepare_pip_source_args
Expand Down Expand Up @@ -1446,31 +1446,6 @@ def write_requirement_to_file(
return r


def write_constraint_to_file(
project: Project,
requirements_dir: Optional[str] = None,
) -> str:
if not requirements_dir:
requirements_dir = vistir.path.create_tracked_tempdir(
prefix="pipenv", suffix="requirements"
)

f = tempfile.NamedTemporaryFile(
prefix="pipenv-", suffix="-constraint.txt", dir=requirements_dir, delete=False
)

constraints = get_constraints_from_deps(project.packages)
for line in constraints:
if project.s.is_verbose():
click.echo(
f"Writing default constraints to temporary file: {line!r}", err=True
)
f.write(vistir.misc.to_bytes(line))
r = f.name
f.close()
return r


def pip_install(
project,
requirement=None,
Expand Down Expand Up @@ -1573,9 +1548,9 @@ def pip_install(
elif line:
pip_command.extend(line)
if dev and use_constraint:
constraint_filename = write_constraint_to_file(
constraint_filename = prepare_default_constraint_file(
project,
requirements_dir=requirements_dir,
dir=requirements_dir,
)
pip_command.extend(["-c", normalize_path(constraint_filename)])
pip_command.extend(prepare_pip_source_args(sources))
Expand Down
29 changes: 27 additions & 2 deletions pipenv/utils/dependencies.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,9 +284,9 @@ def get_constraints_from_deps(deps):
"""Get contraints from Pipfile-formatted dependency"""
from pipenv.vendor.requirementslib.models.requirements import Requirement

# https://pip.pypa.io/en/stable/user_guide/#constraints-files
# constraints must have a name, they cannot be editable, and they cannot specify extras.
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 = []
Expand All @@ -298,6 +298,31 @@ def is_constraint(dep):
return constraints


def prepare_default_constraint_file(
project,
dir=None,
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid shadowing built in python function name dir -- I recommend naming it directory.

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 fixed it.

):
from pipenv.vendor.vistir.path import (
create_tracked_tempdir,
create_tracked_tempfile,
)

if not dir:
dir = create_tracked_tempdir(suffix="-requirements", prefix="pipenv-")

default_constraints_file = create_tracked_tempfile(
mode="w",
prefix="pipenv-",
suffix="-default-constraints.txt",
dir=dir,
delete=False,
)
default_constraints = get_constraints_from_deps(project.packages)
default_constraints_file.write("\n".join([c for c in default_constraints]))
default_constraints_file.close()
return default_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
71 changes: 29 additions & 42 deletions pipenv/utils/resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@
HackedPythonVersion,
clean_pkg_version,
convert_deps_to_pip,
get_constraints_from_deps,
get_vcs_deps,
is_pinned_requirement,
pep423_name,
prepare_default_constraint_file,
translate_markers,
)
from .indexes import parse_indexes, prepare_pip_source_args
Expand Down Expand Up @@ -140,12 +140,15 @@ def __init__(
self.requires_python_markers = {}
self._pip_args = None
self._constraints = None
self._default_constraints = None
self._parsed_constraints = None
self._parsed_default_constraints = None
self._resolver = None
self._finder = None
self._ignore_compatibility_finder = None
self._session = None
self._constraint_file = None
self._default_constraint_file = None
self._pip_options = None
self._pip_command = None
self._retry_attempts = 0
Expand Down Expand Up @@ -560,26 +563,13 @@ def constraint_file(self):
self._constraint_file = self.prepare_constraint_file()
return self._constraint_file

def prepare_default_constraint_file(self):

from pipenv.vendor.vistir.path import create_tracked_tempfile

default_constraints_file = create_tracked_tempfile(
mode="w",
prefix="pipenv-",
suffix="-default-constraints.txt",
dir=self.req_dir,
delete=False,
)
default_constraints = get_constraints_from_deps(self.project.packages)
default_constraints_file.write("\n".join([c for c in default_constraints]))
default_constraints_file.close()
return default_constraints_file.name

@property
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this a @cached_property instead so that we don't have to implement that ourselves with if self._default_constraint_file is None: logic?

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 used @cached_property but falling some tests on MacOS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those tests also failed without @cached_property as well, so maybe @cached_property is not the issue. Because @cached_property does not work with python 3.7, I used @property and @lru_cache instead.

Copy link
Member

Choose a reason for hiding this comment

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

I think the problem is you need to import @cached_property from the vendor'd dependencies, go with from pipenv.vendor.cached_property import cached_property until #5169 is completed.

Copy link
Member

Choose a reason for hiding this comment

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

The only tests that are expected could fail, and likely on mac OS right now is test_convert_deps_to_pip and test_convert_deps_to_pip_one_way. Still investigating how these failures started in the main branch, so if you see that specifically its safe to ignore. The other tests should all pass.

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 ran with the main branch, but there are more failed tests on macOS other than test_convert_deps_to_pip and test_convert_deps_to_pip_one_way.

Copy link
Member

Choose a reason for hiding this comment

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

@dqkqd It is possible that the new version of setuptools that was released yesterday has affected things. I will re-run the main branch actions, but you can see the last time it ran upon merge ony those *_depts_to_pip tests had failed: https://github.com/pypa/pipenv/runs/7758310446?check_suite_focus=true

Copy link
Member

@matteius matteius Aug 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@dqkqd I fixed the unexpected test failures upstream in main branch, in this PR: #5241

I am still not sure why those *_depts_to_pip tests have sporadic failures in the CI and Mac OS, but thats another can of worms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@matteius I updated as you said. Not sure why it passed *_depts_to_pip cases. I didn't even modify convert_deps_to_pip.

Copy link
Member

Choose a reason for hiding this comment

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

@dqkqd It is a "flakey" test case, maybe I'll mark it as such and it won't fail the build as often. I haven't gotten to the bottom of why sometimes it behaves differently, it has failed on Mac OS most frequently, but I have seen it once in a while fail on other systems.

@lru_cache()
def default_constraint_file(self):
return self.prepare_default_constraint_file()
if self._default_constraint_file is None:
self._default_constraint_file = prepare_default_constraint_file(
self.project, dir=self.req_dir
)
return self._default_constraint_file

@property
def pip_options(self):
Expand Down Expand Up @@ -660,38 +650,34 @@ def parsed_constraints(self):
return self._parsed_constraints

@property
Copy link
Member

Choose a reason for hiding this comment

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

Recommend also making this a @cached_property

@lru_cache()
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
if self._parsed_default_constraints is None:
self._parsed_default_constraints = parse_requirements(
self.default_constraint_file,
constraint=True,
finder=self.finder,
session=self.session,
options=pip_options,
)
return self._parsed_default_constraints

@property
Copy link
Member

Choose a reason for hiding this comment

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

And this one also I think would be better as a cached_property.

@lru_cache()
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
if self._default_constraints is None:
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 self._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(
Expand All @@ -702,6 +688,7 @@ 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
Expand Down