support setup_requires in setup.cfg

[benoit-pierre]

Fix #1054.

The patch is not straightforward because: parsing setup.cfg is not part of Distribution.__init__ (it's done later explicitly by distutils.setup), but installing setup_requires' requirements is (and this must be kept early, so all options passed to setup are correctly interpreted)... Additionally, instantiating a Distribution object with no arguments should not raise an error if there's an invalid setup.cfg in the current directory.

[RonnyPfannschmidt]

in light of pypa/setuptools-scm#181 - could this add a entry-point for the section keys as well?

[benoit-pierre]

Amended to support the following use-case:

• setup.py:

from setuptools import setup
setup(name='project')

• setup.cfg:

[metadata]
version = attr: setuptools_scm.get_version
[options]
setup_requires =
    setuptools_scm

[idlesign]

@benoit-pierre, thank you. I wonder why name='project' is in setup.py? It seems that bare setup() will do.

We end up with two parse steps essentially, but having in mind all the good it brings to us, I think it's not a big deal. So if this branch passes PyPI top test, we need @jaraco to review the implementation.

[RonnyPfannschmidt]

@benoit-pierre that's exactly the way i didn't want to go about it, setuptools_scm should be configured, version shouldn't be set to just the function, that's just a mess

[idlesign]

@RonnyPfannschmidt
That's not about setuptools_scm, that's a common case.
If a function call approach doesn't fit your need, then, please, explain why exactly, as I've asked you in pypa/setuptools-scm#181

[RonnyPfannschmidt]

@idlesign the main problem there is that the config parsing does not support extension via entrypoints - as long as one can't add the parameters one adds to setup calls via a entrypoints to the config parsing as well, its all moot

[RonnyPfannschmidt]

and turning off error handling instead of addressing the underlying issue is just a problem

[idlesign]

Hm, this pull request in clearly not about entry points it's about setup_requires.

[RonnyPfannschmidt]

@idlesign you clearly upvoted the amendment that happened to this pr to support the use case i never wanted to see supported, which is what i complained about

a bigger problem for setup_requires is, that the keywords they add to setup.py aren't automatically supported in setup.cfg, which to me is the real problem/bummer

in order to support that one needs the ability to add own parsers for sections/keys in the setuptools plugins

setup.py keyword arguments are added via entrypoints, a similar mechanism would be needed for setup.cfg options added by setup_requires

...

[idlesign]

you clearly upvoted the amendment that happened to this pr to support the use case i never wanted to see supported

I can repeat: that's not about setuptools_scm, that's a common case.

If you want to talk about entry_points let's move to pypa/setuptools-scm#181

[benoit-pierre]

@RonnyPfannschmidt: @idlesign is right, this PR is not about setuptools_scm, it's about supporting setup_requires in setup.cfg, that's it. The reason for the amendment (that happen to help with setuptools_scm) is to fix a supported use-case. I agree that ignoring some errors during the first parsing(s) is far from pretty but we the way setuptools work I don't see a way around that. Adding another set of entrypoints for the configuration is not going to help with the fact that the right entrypoint may not yet be available when parsing is done before setup requirements have been installed...

[webknjaz]

Nitpick: IMHO it would look nicer if done via decorator rather than closure.

[benoit-pierre]

Is the added complexity worth it? I'll will set setup.__doc__ to distutils.core.setup.__doc__, however.

[webknjaz]

Readability matters :)

[benoit-pierre]

But that's my point: it's more complex, hence less readable.

[jaraco]

I worry that this one seemingly compatible change will have some unexpected consequences. There are probably many projects that still use distutils.core.setup directly, especially those that have been configured to run without setuptools entirely, but where pip, by shim-installing setuptools, causes setuptools behavior to be included even when distutils.core.setup is the function invoked by the setup script. For those packages, this new behavior will be bypassed.

[jaraco]

Now that I review a little further, I see that this flag that's being set will only affect packages using setup.cfg, so those packages can necessarily be expected to be invoking setup from setuptools... and they simply won't get this new behavior if they're somehow invoking distutils.core.setup().

[webknjaz]

I'm not sure what distutils.core.setup() return type is, but I'd prepend this line with return , just to be on the safe side.

[benoit-pierre]

The resulting distribution object should be returned. And this behavior is actually tested, except the tests are not picked up by pytest (because they are in setuptools/tests/__init__.py)!

[benoit-pierre]

Yes, using setup(dependency_links=[]) should override a corresponding dependency_links setting in the configuration. If not passed as argument to setup() or set in the configuration, a default value will still be set.

[webknjaz]

I think that if both self.setup_requires and dist.setup_requires are not None some warning is required, so that it will alert end-user who accidentally put the option into two places that only one is applied (and two lists are not merged).

[benoit-pierre]

That would mean always parsing the configuration. What about other options? When passed as arguments to setup(), they always override a corresponding setting in setup.cfg, no?

[webknjaz]

what about at least elif dist.setup_requires is not None: here?

[benoit-pierre]

Rebased on #1157.

[benoit-pierre]

Apparently, a setup.cfg's option will override its corresponding setup() argument if the value passed evaluates to false:

• setup.py:

from setuptools import setup
dist = setup(zip_safe=False, install_requires=[])
print('install_requires', dist.install_requires)
print('zip_safe', dist.zip_safe)

• setup.cfg:

[options]
zip_safe = True
install_requires = foobar

• output of python setup.py --name:

UNKNOWN
install_requires ['foobar']
zip_safe True

Which I find pretty horrible...

[idlesign]

Which I find pretty horrible...

Yeap, this should be addressed separately.

[benoit-pierre]

I would expect for setup arguments to always take precedence over setup.cfg.

[benoit-pierre]

Anyway, amended to make the behavior consistent.

Alternatively we could stop parsing the configuration during Distribution.__init__ completely (including in fetch_build_egg, which IMHO would be cleaner (you're expected to call parse_config_files explicitly if you want to use the configuration), and handle setup_requires before creating the Distribution object:

 setuptools/__init__.py | 20 +++++++++++++++++++-
 setuptools/dist.py     | 30 +++++++++++++-----------------
 2 files changed, 32 insertions(+), 18 deletions(-)

diff --git c/setuptools/__init__.py w/setuptools/__init__.py
index 04f76740..420ad7e5 100644
--- c/setuptools/__init__.py
+++ w/setuptools/__init__.py
@@ -109,7 +109,25 @@ class PEP420PackageFinder(PackageFinder):
 
 find_packages = PackageFinder.find
 
-setup = distutils.core.setup
+
+def _install_setup_requires(attrs):
+    keep = '''
+    dependency_links setup_requires
+    '''.split()
+    dist = Distribution(dict(
+        (k, v) for k, v in attrs.items() if k in keep
+    ))
+    # Honor setup.cfg's options.
+    dist.parse_config_files(ignore_option_errors=True)
+    if dist.setup_requires:
+        dist.fetch_build_eggs(dist.setup_requires)
+
+def setup(**attrs):
+    # Make sure we have any requirements needed to interpret 'attrs'.
+    _install_setup_requires(attrs)
+    return distutils.core.setup(**attrs)
+setup.__doc__ = distutils.core.setup.__doc__
+
 
 _Command = monkey.get_unpatched(distutils.core.Command)
 
diff --git c/setuptools/dist.py w/setuptools/dist.py
index a2ca8795..e46541ad 100644
--- c/setuptools/dist.py
+++ w/setuptools/dist.py
@@ -316,23 +316,19 @@ class Distribution(Distribution_parse_config_files, _Distribution):
         have_package_data = hasattr(self, "package_data")
         if not have_package_data:
             self.package_data = {}
-        _attrs_dict = attrs or {}
-        if 'features' in _attrs_dict or 'require_features' in _attrs_dict:
+        attrs = attrs or {}
+        if 'features' in attrs or 'require_features' in attrs:
             Feature.warn_deprecated()
         self.require_features = []
         self.features = {}
         self.dist_files = []
-        self.src_root = attrs and attrs.pop("src_root", None)
+        self.src_root = attrs.pop("src_root", None)
         self.patch_missing_pkg_info(attrs)
-        self.long_description_content_type = _attrs_dict.get(
+        self.long_description_content_type = attrs.get(
             'long_description_content_type'
         )
-        # Make sure we have any eggs needed to interpret 'attrs'
-        if attrs is not None:
-            self.dependency_links = attrs.pop('dependency_links', [])
-            assert_string_list(self, 'dependency_links', self.dependency_links)
-        if attrs and 'setup_requires' in attrs:
-            self.fetch_build_eggs(attrs['setup_requires'])
+        self.dependency_links = attrs.pop('dependency_links', [])
+        self.setup_requires = attrs.pop('setup_requires', [])
         for ep in pkg_resources.iter_entry_points('distutils.setup_keywords'):
             vars(self).setdefault(ep.name, None)
         _Distribution.__init__(self, attrs)
@@ -427,14 +423,15 @@ class Distribution(Distribution_parse_config_files, _Distribution):
         req.marker = None
         return req
 
-    def parse_config_files(self, filenames=None):
+    def parse_config_files(self, filenames=None, ignore_option_errors=False):
         """Parses configuration files from various levels
         and loads configuration.
 
         """
         _Distribution.parse_config_files(self, filenames=filenames)
 
-        parse_configuration(self, self.command_options)
+        parse_configuration(self, self.command_options,
+                            ignore_option_errors=ignore_option_errors)
         self._finalize_requires()
 
     def parse_command_line(self):
@@ -497,19 +494,18 @@ class Distribution(Distribution_parse_config_files, _Distribution):
         """Fetch an egg needed for building"""
         from setuptools.command.easy_install import easy_install
         dist = self.__class__({'script_args': ['easy_install']})
-        dist.parse_config_files()
         opts = dist.get_option_dict('easy_install')
         keep = (
             'find_links', 'site_dirs', 'index_url', 'optimize',
             'site_dirs', 'allow_hosts'
         )
-        for key in list(opts):
-            if key not in keep:
-                del opts[key]  # don't use any other settings
+        opts.update((k, v)
+                    for k, v in self.get_option_dict('easy_install').items()
+                    if k in keep)
         if self.dependency_links:
             links = self.dependency_links[:]
             if 'find_links' in opts:
-                links = opts['find_links'][1].split() + links
+                links = opts['find_links'][1] + links
             opts['find_links'] = ('setup', links)
         install_dir = self.get_egg_cache_dir()
         cmd = easy_install(

[idlesign]

I like the second way better from the point of view of clarity.
Except that keep = ''' literal splitting stuff.

[jaraco]

These two lines can be replaced by attrs.setdefault('parse_config', True).

[jaraco]

Now that I review a little further, I see that this flag that's being set will only affect packages using setup.cfg, so those packages can necessarily be expected to be invoking setup from setuptools... and they simply won't get this new behavior if they're somehow invoking distutils.core.setup().

[jaraco]

Since this parameter is private and not meant for any user to invoke, it should probably be called _parse_config or similar.

[jaraco]

This use of the ignore_option_errors flag concerns me. It bothered me in the config.py module, but now that complexity is tied to several method signatures in the Distribution, which is monkey patched into distutils. I don't yet have a better recommendation, however.

[jaraco]

Alternatively we could stop parsing the configuration during Distribution.init completely

I like this idea a lot. This one aspect of setuptools, the forced injection of complex behavior (network access, downloading, building, installing in a temporary location) has created much consternation. Making this behavior explicit and decoupled from distutils behavior feels like the right move and would likely simplify many of these complicated scenarios.

The aforementioned risk would still exist - packages invoking distutils.core.setup would bypass the behavior. Still, I think the pain would be small but the benefit would be substantial.

[benoit-pierre]

I rebased on master and updated the patch to handle setup_requires in setup.

The aforementioned risk would still exist - packages invoking distutils.core.setup would bypass the behavior. Still, I think the pain would be small but the benefit would be substantial.

With the following setup.py:

from distutils.core import setup
setup(name='pkga', version='1.0', setup_requires='barbazquux')

• when installing directly with python setup.py install, setup_requires is not handled: /usr/lib/python3.6/distutils/dist.py:261: UserWarning: Unknown distribution option: 'setup_requires' (even if setuptools is installed).
• but the behavior is different with pip install .: distutils.errors.DistutilsError: Could not find suitable distribution for Requirement.parse('barbazquux').
• with this patch, the behavior when installing with pip is the same as when directly installing from source, except for the fact that no warning is shown.

Considering the difference in behavior with master, and the fact that setup_requires not always being correctly handled with or without this PR, I don't think this is a valid supported use-case.

FWIW, I also run pip's full testsuite with this PR version of setuptools with no issues.