bpo-19610: Warn if distutils is provided something other than a list to some fields #4685

nascheme · 2017-12-03T04:55:05Z

The commit of dcaed6b causes some 3rd party packages to fail to install, due to them passing a tuple for meta data fields. Rather than raise TypeError, generate a RuntimeWarning and convert the value using list().

https://bugs.python.org/issue19610

serhiy-storchaka

Needed to update Doc/distutils/apiref.rst too.

serhiy-storchaka · 2017-12-03T06:08:09Z

Lib/distutils/tests/test_dist.py

@@ -354,8 +354,11 @@ def test_classifier_invalid_type(self):
        attrs = {'name': 'Boa', 'version': '3.0',
                 'classifiers': ('Programming Language :: Python :: 3',)}
        msg = "'classifiers' should be a 'list', not 'tuple'"
-        with self.assertRaises(TypeError, msg=msg):
-            Distribution(attrs)
+        with self.assertWarns(RuntimeWarning) as warns:


Would is better to use assertWarnsRegex() here and in other tests. Otherwise msg isn't used.

serhiy-storchaka · 2017-12-03T06:10:55Z

Lib/distutils/tests/test_dist.py

-            Distribution(attrs)
+        with self.assertWarns(RuntimeWarning) as warns:
+            d = Distribution(attrs)
+            self.assertIsInstance(d.metadata.classifiers, list)


Would be better to move these checks outside of the with block. Otherwise it isn't clear what operation raises a RuntimeWarning.

merwok · 2017-12-03T19:31:02Z

Lib/distutils/dist.py

-    import warnings
-except ImportError:
-    warnings = None
+import warnings


It’s possible that the import was how it was because distutils is used to build CPython’s own extension modules. In doubt, let’s not change this.

Note that distutils also has its own PEP-282-style logging system, which may or may not easier to use here. The output is controlled by verbose/quiet options passed to setup.py rather than the interpreter warning configuration system, so testing with pip would be needed to make sure that package authors would see the messages by default.

The try/except dates back to Python 1.5 + 2.0 days. It was added because those versions didn't have 'warnings' and distutils was targeting those versions as well. I think removing it is safe but I will revert as it is a separate change from this issue.

Using the logging system sounds like a good idea. I'll look into that.

I have made the requested changes; please review again.

I tweaked the wording of the warning message, not sure it is the best.

merwok

Thanks for the patch. Tests look good; left comment about conditional use of warnings.

bedevere-bot · 2017-12-03T19:32:56Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

serhiy-storchaka · 2017-12-03T20:07:30Z

While we a here could you please add value = list(value) in set_requires() and set_obsoletes(). They already work with arbitrary iterables, but this can have unexpected effect in upload.py and register.py.

nascheme · 2017-12-04T01:23:52Z

I have made the requested changes; please review again.

bedevere-bot · 2017-12-04T01:23:54Z

Thanks for making the requested changes!

@serhiy-storchaka, @merwok: please review the changes made to this pull request.

merwok · 2017-12-04T15:39:28Z

Lib/distutils/dist.py

-                        warnings.warn(msg)
-                    else:
-                        sys.stderr.write(msg + "\n")
+                    warnings.warn(msg)


I thought you wanted to move this in a separate PR?

merwok · 2017-12-04T15:40:56Z

Lib/distutils/dist.py

+    elif not isinstance(value, list):
+        # passing a tuple or an iterator perhaps, warn and convert
+        typename = type(value).__name__
+        msg = f"Warning: '{fieldname}' should be a list, got type '{typename}'"


Is the prefix Warning redundant? Otherwise message seems good.

Another style nit: Can't we use {fieldname!r} instead of '{fieldname}'?

merwok · 2017-12-04T15:41:23Z

Lib/distutils/dist.py

@@ -26,6 +26,18 @@
 # to look for a Python module named after the command.
 command_re = re.compile(r'^[a-zA-Z]([a-zA-Z0-9_]*)$')

+def _ensure_list(value, fieldname):


Minor: would you mind keeping two blank lines before and after this function?

berkerpeksag

This looks pretty good to me, thank you! I just left some comments on code style.

berkerpeksag · 2017-12-05T01:07:49Z

Lib/distutils/tests/test_dist.py

@@ -11,7 +11,8 @@
 from distutils.dist import Distribution, fix_help_options, DistributionMetadata
 from distutils.cmd import Command

-from test.support import TESTFN, captured_stdout, run_unittest
+from test.support import TESTFN, captured_stdout, captured_stderr, \


Style nit: Replacing \ with

from test.support import ( TESTFN, captured_stdout, captured_stderr, run_unittest, )

may be better.

berkerpeksag · 2017-12-05T01:16:12Z

Lib/distutils/dist.py

@@ -1231,7 +1223,7 @@ def set_requires(self, value):
        import distutils.versionpredicate
        for v in value:
            distutils.versionpredicate.VersionPredicate(v)
-        self.requires = value
+        self.requires = list(value)


Should we add tests for the requires and obsolotes fields similar to what you already added for the classifiers field?:

attrs = {..., 'classifiers': ('Programming Language :: Python :: 3',)} self.assertIsInstance(d.metadata.classifiers, list) self.assertEqual(d.metadata.classifiers, list(attrs['classifiers']))

If I don't miss something, the line list(value) is not covered by the current tests:

https://github.com/python/cpython/blob/bce095d9b0595a528a95bae3f764ee195a0c0d63/Lib/distutils/tests/test_dist.py#L301-L315
https://github.com/python/cpython/blob/bce095d9b0595a528a95bae3f764ee195a0c0d63/Lib/distutils/tests/test_dist.py#L323-L337

berkerpeksag · 2017-12-05T01:18:09Z

Lib/distutils/dist.py

+    elif not isinstance(value, list):
+        # passing a tuple or an iterator perhaps, warn and convert
+        typename = type(value).__name__
+        msg = f"Warning: '{fieldname}' should be a list, got type '{typename}'"


Another style nit: Can't we use {fieldname!r} instead of '{fieldname}'?

berkerpeksag · 2017-12-05T01:20:08Z

Lib/distutils/tests/test_dist.py

-        msg = "'platforms' should be a 'list', not 'tuple'"
-        with self.assertRaises(TypeError, msg=msg):
-            Distribution(attrs)
+        msg = "should be a list"


Style nit: I think we can now inline this in line 395:

self.assertIn(msg, error.getvalue())

Introduce a helper function _ensure_list() that reduces code duplication.

the-knights-who-say-ni added the CLA signed label Dec 3, 2017

bedevere-bot added the awaiting merge label Dec 3, 2017

serhiy-storchaka reviewed Dec 3, 2017

View reviewed changes

serhiy-storchaka added the type-bug An unexpected behavior, bug, or error label Dec 3, 2017

serhiy-storchaka requested review from berkerpeksag and ncoghlan December 3, 2017 18:33

serhiy-storchaka approved these changes Dec 3, 2017

View reviewed changes

merwok reviewed Dec 3, 2017

View reviewed changes

merwok requested changes Dec 3, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Dec 3, 2017

serhiy-storchaka approved these changes Dec 3, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Dec 3, 2017

bedevere-bot added awaiting change review and removed awaiting merge labels Dec 4, 2017

merwok reviewed Dec 4, 2017

View reviewed changes

berkerpeksag reviewed Dec 5, 2017

View reviewed changes

nascheme added 6 commits December 4, 2017 18:18

Import warnings unconditionally. The try/except was for Python 2.0.

2459267

Rather than raise TypeError, warn and call list() on the value.

a440128

Fix tests, revise NEWS and whatsnew text.

4a7bfe3

Improve tests using assertWarnsRegex().

8024d47

Revise documentation, a string is okay as well.

8a4c09e

Revert change to try/except for warnings import.

eeecfdd

nascheme added 5 commits December 4, 2017 18:18

Use distutils.log rather than warnings module to warn.

76a1cec

Introduce a helper function _ensure_list() that reduces code duplication.

Ensure 'requires' and 'obsoletes' are real lists.

d27b625

style nits

1f79d9f

Test that requires and obsoletes are turned to lists.

e31669a

style nits

67503b5

nascheme force-pushed the distutil_warn_list branch from bce095d to 67503b5 Compare December 5, 2017 02:35

nascheme merged commit 8837dd0 into python:master Dec 5, 2017

bedevere-bot removed the awaiting change review label Dec 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-19610: Warn if distutils is provided something other than a list to some fields #4685

bpo-19610: Warn if distutils is provided something other than a list to some fields #4685

nascheme commented Dec 3, 2017 •

edited by bedevere-bot

Loading

serhiy-storchaka left a comment

serhiy-storchaka Dec 3, 2017

serhiy-storchaka Dec 3, 2017

merwok Dec 3, 2017

merwok Dec 3, 2017

nascheme Dec 3, 2017

nascheme Dec 3, 2017

merwok left a comment

bedevere-bot commented Dec 3, 2017

serhiy-storchaka commented Dec 3, 2017

nascheme commented Dec 4, 2017

bedevere-bot commented Dec 4, 2017

merwok Dec 4, 2017

merwok Dec 4, 2017

berkerpeksag Dec 5, 2017

merwok Dec 4, 2017

berkerpeksag left a comment

berkerpeksag Dec 5, 2017

berkerpeksag Dec 5, 2017

berkerpeksag Dec 5, 2017

berkerpeksag Dec 5, 2017

bpo-19610: Warn if distutils is provided something other than a list to some fields #4685

bpo-19610: Warn if distutils is provided something other than a list to some fields #4685

Conversation

nascheme commented Dec 3, 2017 • edited by bedevere-bot Loading

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merwok left a comment

Choose a reason for hiding this comment

bedevere-bot commented Dec 3, 2017

serhiy-storchaka commented Dec 3, 2017

nascheme commented Dec 4, 2017

bedevere-bot commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berkerpeksag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nascheme commented Dec 3, 2017 •

edited by bedevere-bot

Loading