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

Top level Pipfile sys_platform markers should be transitive #5892

Merged
merged 16 commits into from
Sep 1, 2023

Conversation

matteius
Copy link
Member

@matteius matteius commented Aug 30, 2023

The issue

I want transitive platform locking to work better.

The fix

By following the resolution chain (where the package came from) we can get back to the Pipfile entry and apply the same sys_platform markers the user intended.

Additionally add a top level platform_machine specifier that works the same way.

Note: Adding directly to the markers will behave different with the pip resolver than adding them as distinct specifier keys in the Pipfile.

The checklist

  • Associated issue
  • A news fragment in the news/ directory to describe this fix with the extension .bugfix.rst, .feature.rst, .behavior.rst, .doc.rst. .vendor.rst. or .trivial.rst (this will appear in the release changelog). Use semantic line breaks and name the file after the issue number or the PR #.

@matteius matteius marked this pull request as ready for review August 30, 2023 22:41
Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

LGTM. A few minor ideas...

pipenv/utils/resolver.py Show resolved Hide resolved
pipenv/utils/resolver.py Outdated Show resolved Hide resolved
Comment on lines +476 to +477
markers = self._fold_markers(dependency_tree, comes_from)
if markers:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
markers = self._fold_markers(dependency_tree, comes_from)
if markers:
if markers := self._fold_markers(dependency_tree, comes_from):

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we can use the walrus operator, but it won't work in 3.7 so we essentially can hold this back until we drop 3.7, but I have mixed feelings about it with the open reports about hash collection against some private indexes. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I am fine to remain Walrus-less. My two freelance clients are both Python 3.11-only so I forgot about 3.7 in this case.

@matteius matteius merged commit 7520f69 into main Sep 1, 2023
13 of 19 checks passed
@matteius matteius deleted the transitive-sys-markers branch September 1, 2023 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants