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

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

Merged
merged 1 commit into from
Feb 9, 2021

Conversation

apollo13
Copy link
Contributor

@apollo13 apollo13 commented Feb 8, 2021

Resolves: #3254

  • Added tests for changed code.
  • Updated documentation for changed code.

I ticked the documentation checkbox simply because I think there is no part in the documentation that I could adjust for this.

@abn This issue got introduced in 1188b31 which you authored; do you think you could give this PR a review?

The main logic change here is that I changed marker merging from intersect to union. The rationale for this can be followed easier if you look at the following (commented) dependency graph from the ticket:

pyinstaller 4.0 PyInstaller bundles a Python application and all its dependencies into a single package.
├── altgraph *  # PyInstaller always requires altgraph
├── macholib >=1.8 # This has a marker to only install on darwin
│   └── altgraph >=0.15  # This is a dependency also of macholib

Now the situation is as follows: Since altgraph is a non-marked dependency by PyInstaller it should always be installed, there is no way around this. An intersection of * and sys_platform='darwin' yields sys_platform='darwin' which would result altgraph to be only installed on darwin. An union ensure that it is installed always.

@@ -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'"
Copy link
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.

@kasteph kasteph requested a review from abn February 9, 2021 19:39
@kasteph kasteph merged commit 73437a8 into python-poetry:master Feb 9, 2021
Copy link

github-actions bot commented Mar 1, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1.1.3 regression. poetry exports wrong constraints
2 participants