Repository: pypa/setuptools
Pull request #1769 (2019-05-22 21:51:38)

Perform a more thorough check of package_data structure
## Summary of changes

`package_data` and `exclude_package_data` expect to receive a dictionary whose values are lists rather than strings. Previously, passing a str value did not cause any warning or error, which is dangerous because users accidentally input a glob/path that then gets iterated into individual characters.

Refs https://github.com/pypa/setuptools/issues/1459

### Pull Request Checklist

- [x] Changes have tests
- [x] News fragment added in changelog.d. See [documentation](http://setuptools.readthedocs.io/en/latest/developer-guide.html#making-a-pull-request) for details


dhimmel: Any advice on what test I should write and where I should put it would be appreciated.
dhimmel: As of https://github.com/pypa/setuptools/pull/1769/commits/a86588f27dc70d1cd3205e670edb6541a40cc05d, the two messages are:

```
DistutilsSetupError: 'package_data' must be a dictionary mapping package names to lists of wildcard patterns
```
```
WARNING: 'package_data' must be a dictionary mapping package names to lists of wildcard patterns, but the value for 'manubot' is a str not list
```
pganssle: @dhimmel Thank you for for looking into this. I think you'll want to add tests into [`test_dist`](https://github.com/pypa/setuptools/blob/master/setuptools/tests/test_dist.py). If you switch over to use a warning instead of `distutils.log.warn`, you can use the `pytest.raises` and `pytest.warns` context managers to test both of these scenarios.
dhimmel: Thanks @pganssle for the suggestions.

As far as the `check_package_data` implementation goes, I considered a few options:

1. warn if dictionary value is string
2. warn if dictionary value is not list
3. error if dictionary value is string
4. error if dictionary value is not list

I've implemented 1 because 3 & 4 could cause `setup.py` files that currently work to begin failing. Does anyone have a preference between 1 or 2. Note that 1 and 2 is also an option, where the warning message would note what type of value was received and that it wasn't a list.
dhimmel: As of https://github.com/pypa/setuptools/pull/1769/commits/b8904e2280adb9e970cd26e6b1a9c584eba6bb8c, [Python 2 tests fail](https://travis-ci.org/pypa/setuptools/jobs/536407495#L519) when a unicode str is passed as a dictionary key. This became apparent because `test_dist.py` invokes `from __future__ import unicode_literals`. I am guessing we do not want `check_package_data` data to fail if a dictionary like the following is provided:

```python
    package_data = {
        u'hello': ['*.msg'],
    }
```

Therefore, I will update check_package_data to be more lenient. But please let me know if Python 2 should only accept `str` and not `unicode` keys? 
pganssle: @dhimmel Python 2 should definitely accept unicode strings for keys.
benoit-pierre: Would re-using `assert_string_list` make more sense? Have you checked the implementation for `check_extras`?
dhimmel: > Would re-using `assert_string_list` make more sense?

Thanks for the the suggestion. In https://github.com/pypa/setuptools/pull/1769/commits/93af6a52cc00bc5b92a0e197705ab92958160441, I update the docstring for `assert_string_list` because it seems to raise an error when `value=None`. One issue I have with `assert_string_list` is that the implementation doesn't actually test for a list of strings. For example, it would pass with a tuple or generator of strings. Is that intended or not... IDK?

The code for `check_package_data` could be simplified by outsourcing to a function like `assert_string_list`, However my worry is about backwards compatibility. Currently, setup.py files that use str (rather than list of str) values for `package_data` will not fail installation and the package may function properly, despite the unintended interpretation of the the `package_data` configuration. Because of this, I assumed we'd want to take the following approach:

+ keep all existing checks that raise errors, i.e. don't increase the ability to accept bad input
+ issue warnings for any input that doesn't conform to the documentation, but that was previously accepted

If we are to stick by those guidelines, we can add `assert_string_list` to check that the dict values are lists of strings, but we'd have to except the `DistutilsSetupError` from `assert_string_list` and convert it to a warning. So yes, we could add an extra check, but this wouldn't shorten/simplify `check_package_data`. Or @benoit-pierre do you think backwards incompatibility is okay, where specs that used to pass now error?
benoit-pierre: > > Would re-using `assert_string_list` make more sense?
> 
> Thanks for the the suggestion. In [93af6a5](https://github.com/pypa/setuptools/commit/93af6a52cc00bc5b92a0e197705ab92958160441), I update the docstring for `assert_string_list` because it seems to raise an error when `value=None`. One issue I have with `assert_string_list` is that the implementation doesn't actually test for a list of strings. For example, it would pass with a tuple or generator of strings. Is that intended or not... IDK?

That's OK, yes.

> The code for `check_package_data` could be simplified by outsourcing to a function like `assert_string_list`, However my worry is about backwards compatibility. Currently, setup.py files that use str (rather than list of str) values for `package_data` will not fail installation and the package may function properly, despite the unintended interpretation of the the `package_data` configuration. Because of this, I assumed we'd want to take the following approach:
> 
>     * keep all existing checks that raise errors, i.e. don't increase the ability to accept bad input
> 
>     * issue warnings for any input that doesn't conform to the documentation, but that was previously accepted
> 
> 
> If we are to stick by those guidelines, we can add `assert_string_list` to check that the dict values are lists of strings, but we'd have to except the `DistutilsSetupError` from `assert_string_list` and convert it to a warning. So yes, we could add an extra check, but this wouldn't shorten/simplify `check_package_data`. Or @benoit-pierre do you think backwards incompatibility is okay, where specs that used to pass now error?

IMHO, printing a warning instead of erroring out does not make sense if a keyword value is not valid. Passing a string instead of a list of string for `package_data` as always been invalid, resulting in missing package data, so I don't see it as a backward incompatible change to enforce it.

dhimmel: > IMHO, printing a warning instead of erroring out does not make sense if a keyword value is not valid

Okay in https://github.com/pypa/setuptools/pull/1769/commits/0435d7a3590c56fe435d2cbc3e4cac055dd3e6a9 I switch to raising an error for any invalid values.

> Passing a string instead of a list of string for package_data as always been invalid, resulting in missing package data, so I don't see it as a backward incompatible change to enforce it.

Note that the old behavior may not always result in missing package data, because on Linux the path `/` could include everything? Furthermore, missing package data usually makes it so some specific functionality breaks. However, this PR will now make it so the entire installation breaks. Not saying I'm against raising an error, just that I want the risks to be clear.

> That's OK, yes.

I'm not sure that all iterables should be considered lists for the purposes of `assert_string_list`. For example, if the value is a generator that gets consumed during these initial checks, then that is a problem?
dhimmel: @pganssle @benoit-pierre ready for re-review. Also advice appreciated on whether this PR should address the potential issue with consumable iterables described [above](https://github.com/pypa/setuptools/pull/1769#issuecomment-495655136).
dhimmel: @pganssle @benoit-pierre ready for re-review.
benoit-pierre: Sorry for the delay.

I think it would be more useful if the exception message in case of invalid key showed the key value.

Was it you intent to check the exception message in the tests, or to provide a custom message when the check fails (see https://docs.pytest.org/en/latest/deprecations.html#raises-message-deprecated)? IMHO the later is better, and the tests can be parameterized (with added check for the "not a dictionary" case):
```diff
 setuptools/dist.py            | 12 ++++-----
 setuptools/tests/test_dist.py | 58 ++++++++++++++++++++++++-------------------
 2 files changed, 38 insertions(+), 32 deletions(-)

diff --git i/setuptools/dist.py w/setuptools/dist.py
index 4e9190b3..8e3e1f92 100644
--- i/setuptools/dist.py
+++ w/setuptools/dist.py
@@ -306,15 +306,15 @@ def check_test_suite(dist, attr, value):
 
 def check_package_data(dist, attr, value):
     """Verify that value is a dictionary of package names to glob lists"""
-    msg = (
-        "{!r} must be a dictionary mapping package names to lists of "
-        "string wildcard patterns".format(attr)
-    )
     if not isinstance(value, dict):
-        raise DistutilsSetupError(msg)
+        raise DistutilsSetupError(
+            "{!r} must be a dictionary mapping package names to lists of "
+            "string wildcard patterns".format(attr))
     for k, v in value.items():
         if not isinstance(k, six.string_types):
-            raise DistutilsSetupError(msg)
+            raise DistutilsSetupError(
+                "{!r} must be strings (got {!r})".format(
+                    'keys of {!r} dict'.format(attr), k))
         assert_string_list(dist, 'values of {!r} dict'.format(attr), v)
 
 
diff --git i/setuptools/tests/test_dist.py w/setuptools/tests/test_dist.py
index 091b3687..38cc1039 100644
--- i/setuptools/tests/test_dist.py
+++ w/setuptools/tests/test_dist.py
@@ -3,6 +3,7 @@
 from __future__ import unicode_literals
 
 import io
+import re
 from distutils.errors import DistutilsSetupError
 from setuptools.dist import (
     _get_unpatched,
@@ -270,35 +271,40 @@ def test_maintainer_author(name, attrs, tmpdir):
             assert line in pkg_lines_set
 
 
-@pytest.mark.filterwarnings('error')
-def test_check_package_data_okay():
-    package_data = {
+CHECK_PACKAGE_DATA_TESTS = (
+    # Valid.
+    ({
         '': ['*.txt', '*.rst'],
         'hello': ['*.msg'],
-    }
-    assert check_package_data(None, 'package_data', package_data) is None
-
-
-def test_check_package_data_invalid_key():
-    package_data = {
+    }, None),
+    # Not a dictionary.
+    ((
+        ('', ['*.txt', '*.rst']),
+        ('hello', ['*.msg']),
+    ), (
+        "'package_data' must be a dictionary mapping package"
+        " names to lists of string wildcard patterns"
+    )),
+    # Invalid key type.
+    ({
         400: ['*.txt', '*.rst'],
-    }
-    expected_message = (
-        "'package_data' must be a dictionary mapping package names to lists "
-        "of string wildcard patterns"
-    )
-    with pytest.raises(DistutilsSetupError, message=expected_message):
-        check_package_data(None, 'package_data', package_data)
-
-
-def test_check_package_data_invalid_value():
-    """error if str values: https://github.com/pypa/setuptools/issues/1459"""
-    package_data = {
+    }, (
+        "\"keys of 'package_data' dict\" "
+        "must be strings (got 400)"
+    )),
+    # Invalid value type.
+    ({
         'hello': '*.msg',
-    }
-    expected_message = (
+    }, (
         "\"values of 'package_data' dict\" "
         "must be a list of strings (got '*.msg')"
-    )
-    with pytest.raises(DistutilsSetupError, message=expected_message):
-        check_package_data(None, 'package_data', package_data)
+    )),
+)
+
+@pytest.mark.parametrize('package_data, expected_message', CHECK_PACKAGE_DATA_TESTS)
+def test_check_package_data(package_data, expected_message):
+    if expected_message is None:
+        assert check_package_data(None, 'package_data', package_data) is None
+    else:
+        with pytest.raises(DistutilsSetupError, match=re.escape(expected_message)):
+            check_package_data(None, 'package_data', package_data)
```
benoit-pierre: As for the problem with generators, there's only one option: raise an error if a generator is passed, as we can't change the API of the `check_xxx` functions (`distutils` territory) to validate and return the validated value (returning the result of consuming it if a generator is passed).
dhimmel: Thanks @benoit-pierre for the suggested patch. I applied it in https://github.com/pypa/setuptools/pull/1769/commits/d6fb01217455fac79c1ab58fe318d04d78f2391c. 

You were correct that I misunderstood the "message" parameter of `pytest.raises` as described in the deprecation notice:

> It is a common mistake to think this parameter will match the exception message, while in fact it only serves to provide a custom message in case the pytest.raises check fails.

I will now go through everything again and think about whether it makes sense to address the generator issue in this PR.

Patch:
diff --git a/changelog.d/1769.change.rst b/changelog.d/1769.change.rst
new file mode 100644
index 000000000..d48a23b63
--- /dev/null
+++ b/changelog.d/1769.change.rst
@@ -0,0 +1,1 @@
+Improve ``package_data`` check: ensure the dictionary values are lists/tuples of strings.
diff --git a/setuptools/dist.py b/setuptools/dist.py
index ea6411b17..f0f030b55 100644
--- a/setuptools/dist.py
+++ b/setuptools/dist.py
@@ -213,8 +213,12 @@ def check_importable(dist, attr, value):
 
 
 def assert_string_list(dist, attr, value):
-    """Verify that value is a string list or None"""
+    """Verify that value is a string list"""
     try:
+        # verify that value is a list or tuple to exclude unordered
+        # or single-use iterables
+        assert isinstance(value, (list, tuple))
+        # verify that elements of value are strings
         assert ''.join(value) != value
     except (TypeError, ValueError, AttributeError, AssertionError):
         raise DistutilsSetupError(
@@ -307,20 +311,17 @@ def check_test_suite(dist, attr, value):
 
 def check_package_data(dist, attr, value):
     """Verify that value is a dictionary of package names to glob lists"""
-    if isinstance(value, dict):
-        for k, v in value.items():
-            if not isinstance(k, str):
-                break
-            try:
-                iter(v)
-            except TypeError:
-                break
-        else:
-            return
-    raise DistutilsSetupError(
-        attr + " must be a dictionary mapping package names to lists of "
-        "wildcard patterns"
-    )
+    if not isinstance(value, dict):
+        raise DistutilsSetupError(
+            "{!r} must be a dictionary mapping package names to lists of "
+            "string wildcard patterns".format(attr))
+    for k, v in value.items():
+        if not isinstance(k, six.string_types):
+            raise DistutilsSetupError(
+                "keys of {!r} dict must be strings (got {!r})"
+                .format(attr, k)
+            )
+        assert_string_list(dist, 'values of {!r} dict'.format(attr), v)
 
 
 def check_packages(dist, attr, value):


Test patch:
diff --git a/setuptools/tests/test_dist.py b/setuptools/tests/test_dist.py
index 390c3dfcf..c771a19a1 100644
--- a/setuptools/tests/test_dist.py
+++ b/setuptools/tests/test_dist.py
@@ -3,7 +3,13 @@
 from __future__ import unicode_literals
 
 import io
-from setuptools.dist import DistDeprecationWarning, _get_unpatched
+import re
+from distutils.errors import DistutilsSetupError
+from setuptools.dist import (
+    _get_unpatched,
+    check_package_data,
+    DistDeprecationWarning,
+)
 from setuptools import Distribution
 from setuptools.extern.six.moves.urllib.request import pathname2url
 from setuptools.extern.six.moves.urllib_parse import urljoin
@@ -263,3 +269,48 @@ def test_maintainer_author(name, attrs, tmpdir):
         else:
             line = '%s: %s' % (fkey, val)
             assert line in pkg_lines_set
+
+
+CHECK_PACKAGE_DATA_TESTS = (
+    # Valid.
+    ({
+        '': ['*.txt', '*.rst'],
+        'hello': ['*.msg'],
+    }, None),
+    # Not a dictionary.
+    ((
+        ('', ['*.txt', '*.rst']),
+        ('hello', ['*.msg']),
+    ), (
+        "'package_data' must be a dictionary mapping package"
+        " names to lists of string wildcard patterns"
+    )),
+    # Invalid key type.
+    ({
+        400: ['*.txt', '*.rst'],
+    }, (
+        "keys of 'package_data' dict must be strings (got 400)"
+    )),
+    # Invalid value type.
+    ({
+        'hello': str('*.msg'),
+    }, (
+        "\"values of 'package_data' dict\" must be a list of strings (got '*.msg')"
+    )),
+    # Invalid value type (generators are single use)
+    ({
+        'hello': (x for x in "generator"),
+    }, (
+        "\"values of 'package_data' dict\" must be a list of strings "
+        "(got <generator object"
+    )),
+)
+
+
+@pytest.mark.parametrize('package_data, expected_message', CHECK_PACKAGE_DATA_TESTS)
+def test_check_package_data(package_data, expected_message):
+    if expected_message is None:
+        assert check_package_data(None, 'package_data', package_data) is None
+    else:
+        with pytest.raises(DistutilsSetupError, match=re.escape(expected_message)):
+            check_package_data(None, str('package_data'), package_data)