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

Add new hook 'setuptools.finalize_distribution_options' #1877

Merged
merged 4 commits into from Nov 23, 2019

Conversation

@jaraco
Copy link
Member

jaraco commented Oct 19, 2019

Summary of changes

Allows plugins like 'setuptools_scm' to alter distribution options (such as in pypa/setuptools_scm#364).

Closes #1055

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d. See documentation for details
…ike 'setuptools_scm' to alter distribution options.
@RonnyPfannschmidt

This comment has been minimized.

Copy link
Contributor

RonnyPfannschmidt commented Nov 2, 2019

@jaraco thanks for beginning this, is there anything i can/should do to help bring this along

@jaraco

This comment has been minimized.

Copy link
Member Author

jaraco commented Nov 16, 2019

Now that setuptools dogfoods this feature, it's covered by the existing tests.

@jaraco jaraco requested a review from pganssle Nov 16, 2019
@jaraco

This comment has been minimized.

Copy link
Member Author

jaraco commented Nov 16, 2019

At this point, I'm prepared to accept this feature at least experimentally for setuptools_scm to leverage. I still have one lingering concern:

  • Now that setuptools uses the entry points to finalize options, it's possible that finalization will happen in a different order than before. It's not obvious to me that the order of execution is important (for those operations that were previously hard-coded). I'd like not to specify the order of the operations if it's not important.

To that end, I've added this patch locally:

diff --git a/setuptools/dist.py b/setuptools/dist.py
index 44990431..b2bdc409 100644
--- a/setuptools/dist.py
+++ b/setuptools/dist.py
@@ -7,6 +7,7 @@ import re
 import os
 import warnings
 import numbers
+import random
 import distutils.log
 import distutils.core
 import distutils.cmd
@@ -734,8 +735,9 @@ class Distribution(_Distribution):
 
         def by_order(hook):
             return getattr(hook, 'order', 0)
-        eps = pkg_resources.iter_entry_points(hook_key)
-        for ep in sorted(eps, key=by_order):
+        eps = list(pkg_resources.iter_entry_points(hook_key))
+        random.shuffle(eps)
+        for ep in eps:
             ep.load()(self)
 
     def _finalize_setup_keywords(self):

And then run the tests several times, which gives me strong confidence that there's no order dependency on the built-in finalizers, so I'm comfortable leaving the default finalizers at the default order index of 0.

Paul, since you were looking at the reported ticket, would you review this change?

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Contributor

RonnyPfannschmidt commented Nov 16, 2019

@jaraco awesome, this is great, cant wait to pull in the setuptools_scm follow-up and apply it to pytest/others

@jaraco jaraco changed the title WIP: Add new hook 'setuptools.finalize_distribution_options' Add new hook 'setuptools.finalize_distribution_options' Nov 16, 2019
@jaraco jaraco merged commit 1a5d06b into master Nov 23, 2019
8 checks passed
8 checks passed
Summary 1 potential rule
Details
codecov/patch 100% of diff hit (target 84.55%)
Details
codecov/project 84.55% (+<.01%) compared to a007982
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@jaraco jaraco deleted the feature/distribution-options-hooks branch Nov 23, 2019
@agronholm

This comment has been minimized.

Copy link
Contributor

agronholm commented Nov 23, 2019

Thanks, I started work on a similar patch but didn't make much headway. I'm relieved to see this is getting done.

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