Issue 19610: Give clear error messages for invalid types used for setup.py params (e.g. tuple for classifiers) (original) (raw)
Created on 2013-11-15 11:24 by berker.peksag, last changed 2022-04-11 14:57 by admin. This issue is now closed.
Messages (47)
Author: Berker Peksag (berker.peksag) * 
Date: 2013-11-15 11:24
Python 3.4:
$ ../cpython/./python setup.py sdist upload -r test --show-response ... ... Traceback (most recent call last): File "setup.py", line 24, in '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 '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.
Author: STINNER Victor (vstinner) *
Date: 2013-11-15 11:51
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.
Author: STINNER Victor (vstinner) *
Date: 2013-11-15 11:52
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?).
Author: Berker Peksag (berker.peksag) *
Date: 2013-11-17 08:44
[...] 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.
Author: Antoine Pitrou (pitrou) *
Date: 2013-12-22 17:18
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.
Author: Éric Araujo (eric.araujo) *
Date: 2013-12-22 19:57
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.
Author: Éric Araujo (eric.araujo) *
Date: 2014-03-25 09:28
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.
Author: STINNER Victor (vstinner) *
Date: 2014-03-25 11:25
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.
Author: Éric Araujo (eric.araujo) *
Date: 2014-03-26 21:32
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.
Author: Berker Peksag (berker.peksag) *
Date: 2014-05-16 15:56
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.
Author: Berker Peksag (berker.peksag) *
Date: 2014-11-01 09:54
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.
Author: Serhiy Storchaka (serhiy.storchaka) *
Date: 2015-03-06 22:54
Does current code work with None or empty tuple?
Author: Berker Peksag (berker.peksag) *
Date: 2015-03-07 07:27
Does current code work with None or empty tuple?
Yes, it works with both None and ().
Author: Serhiy Storchaka (serhiy.storchaka) *
Date: 2015-03-07 13:43
The patch changes this.
Author: Éric Araujo (eric.araujo) *
Date: 2015-03-09 03:30
I think the change is acceptable.
Author: Serhiy Storchaka (serhiy.storchaka) *
Date: 2015-03-09 06:52
What about other list parameters? platform, keywords, provides, requires, obsoletes?
Author: Serhiy Storchaka (serhiy.storchaka) *
Date: 2015-03-09 06:53
I don't know in which cases a value should be a (key, value) tuple.
I think this is for field 'content' only.
Author: Éric Araujo (eric.araujo) *
Date: 2015-03-09 13:42
I think classifiers and keywords are the only commonly used fields. This issue could be limited to classifiers, or also include other list fields.
Author: Serhiy Storchaka (serhiy.storchaka) *
Date: 2015-03-10 18:07
I think it should include other list fields if we don't want to open separate issues for every list field.
Author: Berker Peksag (berker.peksag) *
Date: 2015-03-23 12:40
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.
Author: Éric Araujo (eric.araujo) *
Date: 2015-04-02 14:16
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?
Author: Berker Peksag (berker.peksag) *
Date: 2015-04-08 06:15
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 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'
Author: Berker Peksag (berker.peksag) *
Date: 2015-05-12 14:28
Éric, could you please take a look at issue19610_v4.diff? I'd like to commit the patch this weekend. Thanks!
Author: Henk-Jaap Wagenaar (cryvate) *
Date: 2017-11-22 19:36
This is still present, and also silently affects Python 2.7 as evidenced here: https://github.com/pypa/setuptools/issues/1163
I am happy to adapt Berker Peksags patch to a PR, if he is?
Author: Éric Araujo (eric.araujo) *
Date: 2017-11-22 19:42
Latest patch seems good. Berker, would you have the time to adapt for 3.7 and submit as a PR?
Author: Berker Peksag (berker.peksag) *
Date: 2017-11-22 19:51
Thanks for the ping and for the review! I will open a PR this week.
Author: Berker Peksag (berker.peksag) *
Date: 2017-11-23 13:28
Éric and Henk-Jaap: I've now opened PR 4519.
Author: Berker Peksag (berker.peksag) *
Date: 2017-11-23 18:34
New changeset dcaed6b2d954786eb5369ec2e8dfdeefe3cdc6ae by Berker Peksag in branch 'master': bpo-19610: setup() now raises TypeError for invalid types (GH-4519) https://github.com/python/cpython/commit/dcaed6b2d954786eb5369ec2e8dfdeefe3cdc6ae
Author: Neil Schemenauer (nascheme) *
Date: 2017-12-02 19:34
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.
Author: Neil Schemenauer (nascheme) *
Date: 2017-12-02 20:34
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.
Author: Berker Peksag (berker.peksag) *
Date: 2017-12-02 20:55
Classifiers were always documented as lists () and passing a non-list type was raised a cryptic exception message as already reported in my first message, https://github.com/pypa/setuptools/issues/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 = 1then 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.2Author: Berker Peksag (berker.peksag) *
Date: 2017-12-02 20:57
(Sorry, I messed up fields in the issue.)
Author: Serhiy Storchaka (serhiy.storchaka) *
Date: 2017-12-02 21:37
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.
Author: Serhiy Storchaka (serhiy.storchaka) *
Date: 2017-12-02 21:46
Filed a bug https://github.com/simplejson/simplejson/issues/198 for simplejson.
Author: Neil Schemenauer (nascheme) *
Date: 2017-12-02 21:47
Classifiers were always documented as lists () 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.
Author: Berker Peksag (berker.peksag) *
Date: 2017-12-02 22:07
Filed a bug https://github.com/simplejson/simplejson/issues/198 for simplejson.
I've opened https://github.com/simplejson/simplejson/pull/197 to make CLASSIFIERS a list.
I've also opened https://github.com/ianare/exif-py/pull/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.
Author: Neil Schemenauer (nascheme) *
Date: 2017-12-02 22:48
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.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2017-12-03 02:20
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).
Author: Neil Schemenauer (nascheme) *
Date: 2017-12-03 04:57
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.
Author: Serhiy Storchaka (serhiy.storchaka) *
Date: 2017-12-03 06:31
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.
Author: Antoine Pitrou (pitrou) *
Date: 2017-12-03 09:27
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', )
Author: Alyssa Coghlan (ncoghlan) *
Date: 2017-12-03 13:40
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.
Author: Éric Araujo (eric.araujo) *
Date: 2017-12-03 19:28
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.
Author: Neil Schemenauer (nascheme) *
Date: 2017-12-04 18:14
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.
Author: Neil Schemenauer (nascheme) *
Date: 2017-12-05 02:58
New changeset 8837dd092fe5ad5184889104e8036811ed839f98 by Neil Schemenauer in branch 'master': bpo-19610: Warn if distutils is provided something other than a list to some fields (#4685) https://github.com/python/cpython/commit/8837dd092fe5ad5184889104e8036811ed839f98
Author: Berker Peksag (berker.peksag) *
Date: 2017-12-05 05:44
Thank you for fixing this, Neil. Can we close this issue now?
Author: Marco Sulla (Marco Sulla) *
Date: 2020-03-04 19:52
This is IMHO broken.
_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.
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.
History
Date
User
Action
Args
2022-04-11 14:57:53
admin
set
github: 63809
2020-03-04 19:52:23
Marco Sulla
set
nosy: + Marco Sulla
messages: +
2017-12-05 17:27:01
nascheme
set
status: open -> closed
resolution: fixed
stage: patch review -> resolved
2017-12-05 05:44:21
berker.peksag
set
priority: release blocker -> normal
messages: +
2017-12-05 02:58:15
nascheme
set
messages: +
2017-12-04 18:14:16
nascheme
set
messages: +
2017-12-04 17:07:08
vstinner
set
nosy: - vstinner
2017-12-03 19:28:53
eric.araujo
set
messages: +
2017-12-03 13:40:49
ncoghlan
set
messages: +
2017-12-03 09:27:21
pitrou
set
messages: +
2017-12-03 06:31:13
serhiy.storchaka
set
messages: +
2017-12-03 04:57:58
nascheme
set
keywords: + needs review, - patch
messages: +
2017-12-03 04:55:06
nascheme
set
stage: resolved -> patch review
pull_requests: + <pull%5Frequest4598>
2017-12-03 02:20:55
ncoghlan
set
messages: +
2017-12-02 22:48:15
nascheme
set
messages: +
2017-12-02 22:07:46
berker.peksag
set
messages: +
2017-12-02 21:47:09
nascheme
set
files: + pip-errors.txt
messages: +
2017-12-02 21:46:31
serhiy.storchaka
set
messages: +
2017-12-02 21:37:19
serhiy.storchaka
set
messages: +
2017-12-02 21:09:12
ned.deily
set
nosy: + ncoghlan
2017-12-02 20:57:00
berker.peksag
set
priority: normal -> release blocker
nosy: + ned.deily
messages: +
2017-12-02 20:55:55
berker.peksag
set
priority: release blocker -> normal
nosy: - ned.deily
messages: +
2017-12-02 20:34:02
nascheme
set
priority: normal -> release blocker
nosy: + ned.deily
messages: +
2017-12-02 19:34:17
nascheme
set
status: closed -> open
nosy: + nascheme
messages: +
resolution: fixed -> (no value)
2017-11-23 18:34:50
berker.peksag
set
status: open -> closed
resolution: fixed
stage: patch review -> resolved
2017-11-23 18:34:22
berker.peksag
set
messages: +
2017-11-23 13:28:46
berker.peksag
set
messages: +
2017-11-23 13:12:22
berker.peksag
set
pull_requests: + <pull%5Frequest4456>
2017-11-22 19:51:16
berker.peksag
set
messages: +
versions: - Python 3.6
2017-11-22 19:42:54
eric.araujo
set
messages: +
2017-11-22 19:36:15
cryvate
set
nosy: + cryvate
messages: +
versions: + Python 3.6, Python 3.7, - Python 3.5
2015-05-12 14:28:08
berker.peksag
set
messages: +
2015-04-08 06:15:31
berker.peksag
set
files: + issue19610_v4.diff
messages: +
2015-04-02 14:16:49
eric.araujo
set
messages: +
title: setup.py does not allow a tuple for classifiers -> Give clear error messages for invalid types used for setup.py params (e.g. tuple for classifiers)
2015-03-23 12:40:51
berker.peksag
set
files: + issue19610_v3.diff
messages: +
2015-03-10 18:07:48
serhiy.storchaka
set
messages: +
2015-03-09 13:42:57
eric.araujo
set
messages: +
2015-03-09 06:53:54
serhiy.storchaka
set
messages: +
2015-03-09 06:52:16
serhiy.storchaka
set
messages: +
2015-03-09 03:30:55
eric.araujo
set
messages: +
2015-03-07 13:43:02
serhiy.storchaka
set
messages: +
2015-03-07 07:27:34
berker.peksag
set
messages: +
2015-03-06 22:54:25
serhiy.storchaka
set
nosy: + serhiy.storchaka
messages: +
2014-11-01 09:54:55
berker.peksag
set
files: + issue19610_v2.diff
messages: +
2014-09-26 12:50:11
berker.peksag
set
assignee: berker.peksag
2014-05-16 15:56:25
berker.peksag
set
files: + issue19610_catch_distribution.diff
stage: needs patch -> patch review
messages: +
versions: - Python 2.7, Python 3.4
2014-03-26 21:32:34
eric.araujo
set
messages: +
2014-03-25 11:25:21
vstinner
set
messages: +
2014-03-25 09:28:24
eric.araujo
set
title: setup.py should allow a tuple for classifiers -> setup.py does not allow a tuple for classifiers
messages: +
versions: + Python 2.7, Python 3.5
2013-12-22 19:57:38
eric.araujo
set
messages: +
2013-12-22 17:51:08
pitrou
set
title: TypeError in distutils.command.upload -> setup.py should allow a tuple for classifiers
stage: patch review -> needs patch
type: behavior -> enhancement
versions: - Python 3.3
2013-12-22 17🔞09
pitrou
set
nosy: + pitrou
messages: +
2013-11-17 08:46:07
berker.peksag
set
files: + issue19610.diff
2013-11-17 08:44:55
berker.peksag
set
messages: +
2013-11-15 11:52:39
vstinner
set
messages: +
2013-11-15 11:51:22
vstinner
set
nosy: + eric.araujo, tarek, vstinner
messages: +
2013-11-15 11:25:13
berker.peksag
set
files: + fix-upload-cmd.diff
keywords: + patch
2013-11-15 11:24:52
berker.peksag
create