Skip to content

Fixed propagation of markers to dependencies. Fixes #3254 #3660

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

Merged
kasteph merged 1 commit into from
Feb 9, 2021
Merged

Fixed propagation of markers to dependencies. Fixes #3254 #3660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions poetry/packages/locker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,9 +269,9 @@ def __walk_dependency_level(
if key not in nested_dependencies:
nested_dependencies[key] = requirement
else:
nested_dependencies[key].marker = nested_dependencies[
key
].marker.intersect(requirement.marker)
nested_dependencies[key].marker = nested_dependencies[key].marker.union(
requirement.marker
)

return cls.__walk_dependency_level(
dependencies=next_level_dependencies,
Expand Down
196 changes: 195 additions & 1 deletion tests/utils/test_exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,200 @@ def test_exporter_can_export_requirements_txt_with_standard_packages_and_markers
assert expected == content


def test_exporter_can_export_requirements_txt_poetry(tmp_dir, poetry):
"""Regression test for #3254"""

poetry.locker.mock_lock_data(
{
"package": [
{
"name": "poetry",
"version": "1.1.4",
"category": "main",
"optional": False,
"python-versions": "*",
"dependencies": {"keyring": "*"},
},
{
"name": "junit-xml",
"version": "1.9",
"category": "main",
"optional": False,
"python-versions": "*",
"dependencies": {"six": "*"},
},
{
"name": "keyring",
"version": "21.8.0",
"category": "main",
"optional": False,
"python-versions": "*",
"dependencies": {
"SecretStorage": {
"version": "*",
"markers": "sys_platform == 'linux'",
}
},
},
{
"name": "secretstorage",
"version": "3.3.0",
"category": "main",
"optional": False,
"python-versions": "*",
"dependencies": {"cryptography": "*"},
},
{
"name": "cryptography",
"version": "3.2",
"category": "main",
"optional": False,
"python-versions": "*",
"dependencies": {"six": "*"},
},
{
"name": "six",
"version": "1.15.0",
"category": "main",
"optional": False,
"python-versions": "*",
},
],
"metadata": {
"python-versions": "*",
"content-hash": "123456789",
"hashes": {
"poetry": [],
"keyring": [],
"secretstorage": [],
"cryptography": [],
"six": [],
"junit-xml": [],
},
},
}
)
set_package_requires(
poetry, skip={"keyring", "secretstorage", "cryptography", "six"}
)

exporter = Exporter(poetry)

exporter.export("requirements.txt", Path(tmp_dir), "requirements.txt")

with (Path(tmp_dir) / "requirements.txt").open(encoding="utf-8") as f:
content = f.read()

# The dependency graph:
# junit-xml 1.9 Creates JUnit XML test result documents that can be read by tools such as Jenkins
# └── six *
# poetry 1.1.4 Python dependency management and packaging made easy.
# ├── keyring >=21.2.0,<22.0.0
# │ ├── importlib-metadata >=1
# │ │ └── zipp >=0.5
# │ ├── jeepney >=0.4.2
# │ ├── pywin32-ctypes <0.1.0 || >0.1.0,<0.1.1 || >0.1.1
# │ └── secretstorage >=3.2 -- On linux only
# │ ├── cryptography >=2.0
# │ │ └── six >=1.4.1
# │ └── jeepney >=0.6 (circular dependency aborted here)
expected = {
"poetry": dependency_from_pep_508("poetry==1.1.4"),
"junit-xml": dependency_from_pep_508("junit-xml==1.9"),
"keyring": dependency_from_pep_508("keyring==21.8.0"),
"secretstorage": dependency_from_pep_508(
"secretstorage==3.3.0; sys_platform=='linux'"
),
"cryptography": dependency_from_pep_508(
"cryptography==3.2; sys_platform=='linux'"
),
"six": dependency_from_pep_508("six==1.15.0"),
}

for line in content.strip().split("\n"):
dependency = dependency_from_pep_508(line)
assert dependency.name in expected
expected_dependency = expected.pop(dependency.name)
assert dependency == expected_dependency
print(dependency.marker)
print(expected_dependency.marker)
assert dependency.marker == expected_dependency.marker


def test_exporter_can_export_requirements_txt_pyinstaller(tmp_dir, poetry):
"""Regression test for #3254"""

poetry.locker.mock_lock_data(
{
"package": [
{
"name": "pyinstaller",
"version": "4.0",
"category": "main",
"optional": False,
"python-versions": "*",
"dependencies": {
"altgraph": "*",
"macholib": {
"version": "*",
"markers": "sys_platform == 'darwin'",
},
},
},
{
"name": "altgraph",
"version": "0.17",
"category": "main",
"optional": False,
"python-versions": "*",
},
{
"name": "macholib",
"version": "1.8",
"category": "main",
"optional": False,
"python-versions": "*",
"dependencies": {"altgraph": ">=0.15"},
},
],
"metadata": {
"python-versions": "*",
"content-hash": "123456789",
"hashes": {"pyinstaller": [], "altgraph": [], "macholib": []},
},
}
)
set_package_requires(poetry, skip={"altgraph", "macholib"})

exporter = Exporter(poetry)

exporter.export("requirements.txt", Path(tmp_dir), "requirements.txt")

with (Path(tmp_dir) / "requirements.txt").open(encoding="utf-8") as f:
content = f.read()

# Rationale for the results:
# * PyInstaller has an explicit dependency on altgraph, so it must always be installed.
# * PyInstaller requires macholib on Darwin, which in turn requires altgraph.
# The dependency graph:
# pyinstaller 4.0 PyInstaller bundles a Python application and all its dependencies into a single package.
# ├── altgraph *
# ├── macholib >=1.8 -- only on Darwin
# │ └── altgraph >=0.15
expected = {
"pyinstaller": dependency_from_pep_508("pyinstaller==4.0"),
"altgraph": dependency_from_pep_508("altgraph==0.17"),
"macholib": dependency_from_pep_508("macholib==1.8; sys_platform == 'darwin'"),
}

for line in content.strip().split("\n"):
dependency = dependency_from_pep_508(line)
assert dependency.name in expected
expected_dependency = expected.pop(dependency.name)
assert dependency == expected_dependency
assert dependency.marker == expected_dependency.marker


def test_exporter_can_export_requirements_txt_with_nested_packages_and_markers(
tmp_dir, poetry
):
Expand Down Expand Up @@ -241,7 +435,7 @@ def test_exporter_can_export_requirements_txt_with_nested_packages_and_markers(
"c==7.8.9; sys_platform == 'win32' and python_version < '3.7'"
),
"d": dependency_from_pep_508(
"d==0.0.1; python_version < '3.7' and platform_system == 'Windows' and sys_platform == 'win32'"
"d==0.0.1; platform_system == 'Windows' and python_version < '3.7' or sys_platform == 'win32' and python_version < '3.7'"

@apollo13 apollo13 Feb 8, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@abn This change changes tested behavior but I'd argue that the previous behavior was in fact wrong:

  • a has python_version < 3.7
  • a depends on b & c which have the markers platform_system == 'Windows' and sys_platform == 'win32' respectively
  • b and c both depends on d and as such their markers should trickle down, but d should get installed whenever one of the parent markers is true, not only when both are true.

On the other hand this python_version<3.7 is required for all packages, so the constraint could be rewritten as:

(platform_system == 'Windows' or sys_platform == 'win32' ) and python_version < '3.7'"

but that would probably be more work (and the current form is technically correct). For large dependency trees it might be worth to get the python version out of the nested constraints as far as possible though. But I guess this might require plenty of specialcasing.

),
}

Expand Down