Give clear error messages for invalid types used for setup.py params (e.g. tuple for classifiers)

[berkerpeksag]

BPO	19610
Nosy	@nascheme, @ncoghlan, @pitrou, @tarekziade, @ned-deily, @merwok, @berkerpeksag, @serhiy-storchaka, @cryvate, @Marco-Sulla
PRs	bpo-19610: setup() now raises TypeError for invalid types #4519 bpo-19610: Warn if distutils is provided something other than a list to some fields #4685
Files	setup.py fix-upload-cmd.diff issue19610.diff issue19610_catch_distribution.diff issue19610_v2.diff issue19610_v3.diff issue19610_v4.diff pip-errors.txt

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/berkerpeksag'
closed_at = <Date 2017-12-05.17:27:01.310>
created_at = <Date 2013-11-15.11:24:52.594>
labels = ['3.7', 'type-feature', 'library']
title = 'Give clear error messages for invalid types used for setup.py params (e.g. tuple for classifiers)'
updated_at = <Date 2020-03-04.19:52:23.051>
user = 'https://github.com/berkerpeksag'

bugs.python.org fields:

activity = <Date 2020-03-04.19:52:23.051>
actor = 'Marco Sulla'
assignee = 'berker.peksag'
closed = True
closed_date = <Date 2017-12-05.17:27:01.310>
closer = 'nascheme'
components = ['Distutils']
creation = <Date 2013-11-15.11:24:52.594>
creator = 'berker.peksag'
dependencies = []
files = ['32641', '32642', '32665', '35266', '37097', '38655', '38862', '47312']
hgrepos = []
issue_num = 19610
keywords = ['needs review']
message_count = 47.0
messages = ['202938', '202941', '202943', '203130', '206821', '206828', '214799', '214810', '214915', '218673', '230443', '237395', '237425', '237447', '237590', '237596', '237597', '237648', '237789', '239014', '239915', '240250', '242974', '306750', '306753', '306755', '306818', '306853', '307450', '307455', '307457', '307458', '307463', '307464', '307465', '307468', '307475', '307483', '307486', '307488', '307495', '307514', '307521', '307588', '307622', '307645', '363385']
nosy_count = 10.0
nosy_names = ['nascheme', 'ncoghlan', 'pitrou', 'tarek', 'ned.deily', 'eric.araujo', 'berker.peksag', 'serhiy.storchaka', 'cryvate', 'Marco Sulla']
pr_nums = ['4519', '4685']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue19610'
versions = ['Python 3.7']

[berkerpeksag]

Python 3.4:

$ ../cpython/./python setup.py sdist upload -r test --show-response
...
...
Traceback (most recent call last):
  File "setup.py", line 24, in <module>
    'License :: OSI Approved :: Mozilla Public License 2.0 (MPL 2.0)',
  File "/home/berker/projects/cpython/Lib/distutils/core.py", line 148, in setup
    dist.run_commands()
  File "/home/berker/projects/cpython/Lib/distutils/dist.py", line 930, in run_commands
    self.run_command(cmd)
  File "/home/berker/projects/cpython/Lib/distutils/dist.py", line 949, in run_command
    cmd_obj.run()
  File "/home/berker/projects/cpython/Lib/distutils/command/upload.py", line 65, in run
    self.upload_file(command, pyversion, filename)
  File "/home/berker/projects/cpython/Lib/distutils/command/upload.py", line 165, in upload_file
    body.write(value)
TypeError: 'str' does not support the buffer interface

Python 3.3:

$ python3.3 setup.py sdist upload -r test --show-response
...
...
Traceback (most recent call last):
  File "setup.py", line 24, in <module>
    'License :: OSI Approved :: Mozilla Public License 2.0 (MPL 2.0)',
  File "/usr/local/lib/python3.3/distutils/core.py", line 148, in setup
    dist.run_commands()
  File "/usr/local/lib/python3.3/distutils/dist.py", line 917, in run_commands
    self.run_command(cmd)
  File "/usr/local/lib/python3.3/distutils/dist.py", line 936, in run_command
    cmd_obj.run()
  File "/usr/local/lib/python3.3/distutils/command/upload.py", line 66, in run
    self.upload_file(command, pyversion, filename)
  File "/usr/local/lib/python3.3/distutils/command/upload.py", line 155, in upload_file
    body.write(value)
TypeError: 'str' does not support the buffer interface

I also attached my setup.py.

[vstinner]

I never undersood why, but classifiers must be a list, not a tuple. This is a bug in my opinion.

upload.upload_file() doesn't check if the tuple contains exactly 2 items. If the value is a tuple, it doesn't encode the value. This is another bug. I don't know in which cases a value should be a (key, value) tuple.

[vstinner]

upload.upload_file() ... doesn't encode the value.

fix-upload-cmd.diff should fix this specific bug, but the first bug (accept tuple for classifiers) should be fixed before or you will get an unexpected behaviour (only send 1 classifier?).

[berkerpeksag]

[...] but the first bug (accept tuple for classifiers) should be fixed
before or you will get an unexpected behaviour (only send 1
classifier?).

Here is a new patch that accepts tuple for classifiers.

Thanks for the review, Victor.

[pitrou]

I don't think accepting a tuple for classifiers is a bugfix. Furthermore, the latest patch is much too intrusive and may break legitimate uses.

[merwok]

Classifiers have always been documented as a list; I don’t think a tuple makes more sense here (even if it does no harm), but more importantly I think it’s a bad idea to have a setup.py that would work in 3.5 and not in 3.2-3.4. I suggest rejecting this.

[merwok]

I’m open to a patch that would make it clear in the docs that classifiers must be a list.

A patch to detect bad type for classifiers in the check command would also be acceptable for 3.5, or to catch it earlier, a check in the Distribution class.

[vstinner]

A patch to detect bad type for classifiers in the check command would also be acceptable for 3.5, or to catch it earlier, a check in the Distribution class.

Why only in Python 3.5? Does it make sense to pass something different
than a list in older Python versions?

I would prefer to see a fix the bug fixed in Python 2.7, 3.4 and 3.5.

[merwok]

You seem to misunderstand me Victor: There is no bug here, classifiers should be a list and are documented as such. It is possible to make this clearer in the docs for all versions. In addition, we could make this easier for users who don’t see that part of the docs by warning them (in the check command, or from the Distribution class), but as a new feature this would go in 3.5 only.

[berkerpeksag]

A patch to detect bad type for classifiers in the check command would
also be acceptable for 3.5, or to catch it earlier, a check in the
Distribution class.

Thanks for the idea, Éric. New patch attached.

[berkerpeksag]

Updated patch. I've tweaked tests and documentation a bit. Alternatively, I can leave Doc/distutils/setupscript.rst untouched and add a whatsnew entry to Doc/whatsnew/3.5.rst.

[serhiy-storchaka]

Does current code work with None or empty tuple?

[berkerpeksag]

Does current code work with None or empty tuple?

Yes, it works with both None and ().

[serhiy-storchaka]

The patch changes this.

[merwok]

I think the change is acceptable.

[serhiy-storchaka]

What about other list parameters? platform, keywords, provides, requires, obsoletes?

[serhiy-storchaka]

I don't know in which cases a value should be a (key, value) tuple.

I think this is for field 'content' only.

[merwok]

I think classifiers and keywords are the only commonly used fields. This issue could be limited to classifiers, or also include other list fields.

[serhiy-storchaka]

I think it should include other list fields if we don't want to open separate issues for every list field.

[berkerpeksag]

Here is a new patch. I didn't touched provides, requires and obsoletes fields since they are not used much in the setuptools era.

Distribution.finalize_options() already converts string types to lists for platforms and keywords fields. I didn't changed that behavior.

[merwok]

Thanks, reviewed.

When running a setup.py that uses a tuple for classifiers, is the error message in the terminal user-friendly, or do we get a full traceback?

[berkerpeksag]

Thanks for the review, Éric. New patch attached.

When running a setup.py that uses a tuple for classifiers, is the error message in the terminal user-friendly, or do we get a full traceback?

A full traceback:

Traceback (most recent call last):
  File "setup.py", line 37, in <module>
    platforms=('Windows', 'Any'),
  File "/home/berker/projects/cpython/default/Lib/distutils/core.py", line 108, in setup
    _setup_distribution = dist = klass(attrs)
  File "/home/berker/projects/cpython/default/Lib/distutils/dist.py", line 253, in __init__
    getattr(self.metadata, "set_" + key)(val)
  File "/home/berker/projects/cpython/default/Lib/distutils/dist.py", line 1212, in set_platforms
    raise TypeError(msg % type(value).__name__)
TypeError: 'platforms' should be a 'list', not 'tuple'

[berkerpeksag]

Thanks for the ping and for the review! I will open a PR this week.

[berkerpeksag]

Éric and Henk-Jaap: I've now opened PR 4519.

[berkerpeksag]

New changeset dcaed6b by Berker Peksag in branch 'master':
bpo-19610: setup() now raises TypeError for invalid types (GH-4519)
dcaed6b

[nascheme]

I don't see a good reason to add this check. I would guess there could be lots of 3rd party packages that are no uninstallable on Python 3.7. E.g.

python3 -m pip install exifread
...
TypeError: 'classifiers' should be a 'list', not 'tuple'

If people are determined to add extra type checking, make it a warning for 3.7 so setup.py files can be fixed.

[nascheme]

I tried building the top packages from python3wos.appspot.com. Only simplejson-3.13.2.tar.gz fails to build due to this change. However, given that it is the top downloaded module, I think think making a change to Python that makes it uninstallable by "pip" is a bad idea. There needs to be a transition process. I'm setting priority to "release blocker". People can downgrade if they disagree with me.

I tried changing the TypeError raises with Deprecation warnings. That doesn't have any effect because DeprecationWarning is filtered by default. Enabling it still has no effect because apparently pip filters it out.

So, I think some other way to warn people would need to be implemented. E.g. have distutils print to stderr. Change pip to warn as well.

[berkerpeksag]

Classifiers were always documented as lists (msg214915) and passing a non-list type was raised a cryptic exception message as already reported in my first message, pypa/setuptools#1163 and https://reinbach.com/blog/setuptools-classifiers-upload-python3-5/

Any usage of classifiers=(...,) is already broken in Python 3 in some way (see the setup.py I attached or https://reinbach.com/blog/setuptools-classifiers-upload-python3-5/ for a quick reproducer) since they can't upload a new release.

Also, this is only an issue when sdist is the only way to install the project. exifread only provides a wheel for Python 2. I cloned it and add

[wheel]
universal = 1

then I created a universal wheel and tried to install it in Python 3.7.0a2+:

Processing /home/berker/projects/test/exif-py/dist/ExifRead-2.1.2-py2.py3-none-any.whl
Installing collected packages: ExifRead
Successfully installed ExifRead-2.1.2

[berkerpeksag]

(Sorry, I messed up fields in the issue.)

[serhiy-storchaka]

In simplejson classifiers is a result of filter(). This is a list in Python 2 and an iterator in Python 3. It can be uploaded using Python 2.

[serhiy-storchaka]

Filed a bug simplejson/simplejson#198 for simplejson.

[nascheme]

Classifiers were always documented as lists (msg214915) and passing a non-list type was raised a cryptic exception message as already reported in my first message

That doesn't matter. You can't break a bunch of packages in a 3.x release with no warning just because people are doing something against what the documentation says. That's not how we develop Python. How is a user of a 3rd party package going to fix this? They have to ask the 3rd party package author to fix their setup.py code. Until then, they cannot use Python 3.7. This patch needs to be reverted and reworked, IMHO.

I trying installing the top packages listed on https://pythonwheels.com . A number of them fail because of this change, see attached text file.

[berkerpeksag]

Filed a bug simplejson/simplejson#198 for
simplejson.

I've opened simplejson/simplejson#197 to make CLASSIFIERS a list.

I've also opened ianare/exif-py#80 to create a universal wheel for exifread.

That's not how we develop Python.

Thank you, but I don't need a lecture from you. Feel free to propose your solution in the form of pull request instead of acting like a project manager and telling people what to do.

[nascheme]

Thank you, but I don't need a lecture from you. Feel free to propose our solution in the form of pull request instead of acting like a project manager and telling people what to do.

I'm sorry you are offended. My pull request would consist of the patch being reverted. It is not ready to go in. If a change breaks a significant amount of previously working Python code, it needs very careful consideration and should be introduced in a way to minimize breakage and allow people time to fix their code.

Repeatably pointing to the documentation and saying that existing code is broken because it doesn't respect documented requirements is not okay.

[ncoghlan]

I'd prefer to see this change go in the other direction: instead of enforcing eager type checks, we should be unconditionally wrapping the given values in a "list(arg)" call, such that more typical iterable duck-typing behaviour applies.

That will transparently fix any code that isn't already passing a list, will ensure that internal code can safely assume that the instance *attributes* are always lists (no matter what the caller passes in), and means that even when the caller is passing in a list, the instance will have its own copy (rather than retaining a reference to the caller's copy).

[nascheme]

I like Nick's idea of calling list() to fix the argument. I've created a PR that implements it. I also generate a RuntimeWarning since if we document them as needing to be lists, we should at least warn for invalid types. The RuntimeWarning will be seen if you call setup.py directly which should have the effect of eventually pushing package authors to fix their code.

[serhiy-storchaka]

Perhaps it is worth to backport warnings to 2.7 in Py3k mode. This could help to detect some issues earlier.

In 3.6 fields could be converted to lists silently, without warnings.

[pitrou]

Le 03/12/2017 à 05:57, Neil Schemenauer a écrit :

I like Nick's idea of calling list() to fix the argument.

I don't think that's a good idea. Suppose someone is passing a string
by mistake:

setup(...,
      classifiers='Programming Language :: Python :: 3 :: Only',
      )

[ncoghlan]

Prohibiting strings and bytes on the grounds of "Yes they're iterable, but are more likely intended as atomic here, so treat them as ambiguous and refuse to guess" would be fine. (Although I'll also note the classifier case will already fail on upload, since they won't be valid classifiers)

The only part I'm not OK with is upgrading a historically unenforced type restriction that only sometimes causes problems into an eagerly enforced one that breaks currently working code.

[merwok]

I am sorry this snowballed. The intent in my messages here and in my PR review was to exchange a late, unfriendly traceback with a clear, early message, but only for package authors. I forgot that a Python 3.7 could execute an invalid setup.py from an existing tarball, as Neil pointed with the pip install exifread example. Even if these packages get PRs to build wheels, it’s still bad to break existing sdists. +1 to reverting the patch, +1 to reverting the breakage and also fixing the original problem with a warning and explicit conversion.

[nascheme]

Don't be sorry. We are all passionate about making Python better. distutils will be better once we gets this sorted out. Berker deserves credit for seeing an issue and developing on a fix for it. The collaboration between all the core developers is making the final fix better than what any one of us could make.

The process is working quite well IMHO. I had a small patch a few days ago that I was considering just committing without reviews. I went through the review process though and the patch was better as a result of the review feedback I got.

The documentation says that the argument needs to be a list of strings or a string. So, unless we change the documentation, a string must be accepted without warnings or errors. The current PR works pretty well. Figuring out how to best warn is the trickiest bit. Sometimes it seems like DepreciationWarning doesn't work as we would like, since it often gets suppressed. It seems pip is especially "good" at hiding it.

[nascheme]

New changeset 8837dd0 by Neil Schemenauer in branch 'master':
bpo-19610: Warn if distutils is provided something other than a list to some fields (bpo-4685)
8837dd0

[berkerpeksag]

Thank you for fixing this, Neil. Can we close this issue now?

[Marco-Sulla]

This is IMHO broken.

1. _ensure_list() allows strings, because, documentation says, they are split in finalize_options(). But finalize_options() does only split keywords and platforms. It does _not_ split classifiers.

2. there's no need that keywords, platforms and classifiers must be a list. keywords and platforms can be any iterable, and classifiers can be any non text-like iterable.

Indeed, keywords are written to file using ','.join(), and platforms and classifiers are written using DistributionMetadata._write_list(). They both accepts any iterable, so I do not understand why such a strict requirement.