Skip to content

Maintain requires order in METADATA.#2839

Merged
jaraco merged 7 commits into
pypa:mainfrom
msuozzo:patch-1
Jan 7, 2022
Merged

Maintain requires order in METADATA.#2839
jaraco merged 7 commits into
pypa:mainfrom
msuozzo:patch-1

Conversation

@msuozzo

@msuozzo msuozzo commented Oct 30, 2021

Copy link
Copy Markdown
Contributor

Summary of changes

Remove a source of instability in the ordering of requires entries. The order is generally specified by the package author and maintained through the build process. From a brief attempt to trace through all the related interfaces, it seems this is the only code that changes the order of the requirements and so would be the only source of the observed inconsistency of Requires-Dist entries.

For example, workflows that build -> install -> build would have the order of requires changed by these sorted() calls.

If this sorting is desirable, the other setuptools and wheel interfaces that interact with the requirements should also sort them. Otherwise, it's valuable to retain the order across all parts of the system.

Pull Request Checklist

It seems that workflows that build -> install -> build would have the order of requires changed by these sorted() calls. From a brief attempt to trace through all the related interfaces, it seems this is the only code that changes the order of the requirements and so would be the only source of the observed inconsistency of Requires-Dist entries.

If this sorting is desirable, the other setuptools and wheel interfaces that interact with the requirements should also sort them. Otherwise, it's valuable to retain the order across all parts of the system.
@msuozzo

msuozzo commented Oct 30, 2021

Copy link
Copy Markdown
Contributor Author

@di this seems to impact wheel reproducibility.

@jaraco

jaraco commented Nov 1, 2021

Copy link
Copy Markdown
Member

I like this approach. I prefer not retain order rather than enforce order.

I'm not sure who added those sorted lines, but if the tests pass, then the sorting behavior is not tested, so it's fine to change it. Would you consider adding a test to capture the expectation this change introduces (so that someone else doesn't come along and suggest to sort the values again)?

@di di left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This makes sense to me, I see no reason why these would need to be sorted.

@jaraco jaraco closed this Nov 3, 2021
@jaraco jaraco reopened this Nov 3, 2021
@jaraco

jaraco commented Nov 4, 2021

Copy link
Copy Markdown
Member

The tests seem to be indicating that the sorting is needed. Would you be willing to investigate and determine what expectation is missed?

@msuozzo

msuozzo commented Nov 5, 2021

Copy link
Copy Markdown
Contributor Author

Yep I haven't had the time to check out the failures but I'm planning on circling back to this in a bit.

@msuozzo

msuozzo commented Nov 28, 2021

Copy link
Copy Markdown
Contributor Author

The sorting looks like it was only required because of a separate order-stability issue with pkg_resources. frozenset isn't stable while dict is, at least for recent Python versions, so the type+logic change should ensure the requires order remains constant.

@msuozzo

msuozzo commented Dec 1, 2021

Copy link
Copy Markdown
Contributor Author

Is this good to merge now? Anything more I should test?

@jaraco

jaraco commented Dec 5, 2021

Copy link
Copy Markdown
Member

Would you consider adding a test to capture the expectation this change introduces (so that someone else doesn't come along and suggest to sort the values again)?

@msuozzo

msuozzo commented Dec 16, 2021

Copy link
Copy Markdown
Contributor Author

Ah didn't see that comment before. Added a test to more reliably fail if the order changes.

@msuozzo

msuozzo commented Jan 4, 2022

Copy link
Copy Markdown
Contributor Author

Friendly ping

@abravalheri

Copy link
Copy Markdown
Contributor

Sorry @msuozzo, we might need to get the CI back into shape before running your tests (just to make sure everything is OK).

@msuozzo

msuozzo commented Jan 4, 2022

Copy link
Copy Markdown
Contributor Author

No worries!

Comment thread pkg_resources/__init__.py Outdated
@jaraco jaraco merged commit 4072575 into pypa:main Jan 7, 2022
abravalheri added a commit to abravalheri/setuptools that referenced this pull request Jan 7, 2022
PR pypa#2839 accidentally misplaced the news fragment file under root.
This commit fix that.
@abravalheri abravalheri mentioned this pull request Jan 7, 2022
2 tasks
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.

4 participants