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

Honor declared ordering for Provides-Extra entries in package metadata. #1690

Merged
merged 7 commits into from Oct 7, 2019

Conversation

@jaraco
Copy link
Member

commented Feb 17, 2019

Alternative approach to #1305, this approach simply replaces the set with an ordered set, retaining the order of extras as found in the package's 'extras_require' dictionary. On older Pythons where the dictionary ordering is non-deterministic, that non-determinism will still be present.

Summary of changes

Closes

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d. See documentation for details
@jaraco jaraco force-pushed the feature/deterministic-provides-extras branch from 94c5b66 to 7f7780e Feb 17, 2019
@pganssle

This comment has been minimized.

Copy link
Member

commented Feb 17, 2019

@jaraco Haven't looked into it too much, do we need to bring in OrderedSet? Are there set operations later on that are used on it? I'm assuming the set is being used because the requirements are supposed to be unique. If it doesn't have to be a set, maybe we can get away with the unique subset of the list?

def unique_list(it):
    unique = set()
    out = []
    for item in it:
        if item not in unique:
            unique.add(item)
            out.append(item)
    return out
@pganssle

This comment has been minimized.

Copy link
Member

commented Feb 17, 2019

Ah, it does use add, I guess OrderedSet is fine for this.

@jaraco

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2019

I agree. Adding a dependency for this change is probably overkill. Especially if it only doesn’t address the root issue - that before Python 3.6, extras are going to be defined in a non-deterministic order. So maybe it makes sense to order them and remove duplicates explicitly.

@jaraco jaraco changed the title Honor declared ordering for Provides-Extra entries in package metadata. WIP: Honor declared ordering for Provides-Extra entries in package metadata. Feb 17, 2019
@jaraco

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2019

@stevenengler Given the goal of having reproducible builds, would it be sufficient to solve that only on Python 3.6+ plus Python 2.7 (without PYTHONHASHSEED set)? If the solution could restrict to those environments, an approach similar to this one would be possible without enforcing a order (deferring to the order indicated by the package).

I'm guessing builders will want reproducible builds on all supported Python versions, but it would be simpler and less intrusive not to do so, and sets forth a pattern for giving packagers more control over the ordering of fields in the metadata.

@jaraco jaraco force-pushed the feature/deterministic-provides-extras branch from d953a7d to 7f7780e Feb 20, 2019
@jaraco

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2019

would it be sufficient to solve that only on Python 3.6+ plus Python 2.7

Let's assume yes.

@jaraco jaraco changed the title WIP: Honor declared ordering for Provides-Extra entries in package metadata. Honor declared ordering for Provides-Extra entries in package metadata. Sep 11, 2019
@yan12125

This comment has been minimized.

Copy link

commented Sep 30, 2019

Hi, what is the status of this pull request?

would it be sufficient to solve that only on Python 3.6+ plus Python 2.7

Let's assume yes.

This is definitely sufficient for Arch Linux, as only the latest 3.x and 2.x versions are used for Python packages. Not sure about other major distros like Debian or Fedora, though.

@jaraco jaraco merged commit 7a909b2 into master Oct 7, 2019
0 of 4 checks passed
0 of 4 checks passed
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
@jaraco

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2019

Hi, what is the status of this pull request?

I've had a shortcut on my desktop nagging me to deal with this since I last commented on it. I just hadn't gotten around to it. I have now and have merged the changes.

@jaraco jaraco deleted the feature/deterministic-provides-extras branch Oct 7, 2019
@@ -1,3 +1,4 @@
packaging==16.8
pyparsing==2.2.1
six==1.10.0
ordered-set

This comment has been minimized.

Copy link
@benoit-pierre

benoit-pierre Oct 7, 2019

Member

Shouldn't this be pinned to the particular version vendored-in?

This comment has been minimized.

Copy link
@jaraco

jaraco Oct 7, 2019

Author Member

Perhaps. I realized that the vendored dependencies are implicitly pinned when paver update_vendored is run... so I'm okay with leaving this declaration unpinned. But you're right to be concerned - why the inconsistency? Just because I'd rather remove the pins... but those other dependencies have probably advanced since they were last updated. My desire is to unpin the others and run another update operation.

This comment has been minimized.

Copy link
@jaraco

jaraco Oct 7, 2019

Author Member

I do think it would be valuable if the dependency isn't declared as a pinned one to capture the effectively pinned version during the update operation.

This comment has been minimized.

Copy link
@benoit-pierre

benoit-pierre Oct 7, 2019

Member

On the other hand, pinning the dependencies make it easier to know which version of each package is vendored and to update only some of those dependencies.

This comment has been minimized.

Copy link
@jaraco

jaraco Oct 7, 2019

Author Member

I've done that for now in 734d09c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.