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.

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.

> 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?).

> [...] 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.

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.

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.

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.

> 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.

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.

> 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.

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.

Does current code work with None or empty tuple?

> Does current code work with None or empty tuple?

Yes, it works with both None and ().

The patch changes this.

I think the change is acceptable.

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

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

I think this is for field 'content' only.

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

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

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.

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?

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'

Éric, could you please take a look at issue19610_v4.diff? I'd like to commit the patch this weekend. Thanks!

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?

Latest patch seems good.  Berker, would you have the time to adapt for 3.7 and submit as a PR?

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

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

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

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.

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.

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, 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 = 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

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

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.

Filed a bug https://github.com/simplejson/simplejson/issues/198 for simplejson.

> 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.

> 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.

> 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.

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).

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.

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.

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',
      )

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.

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.

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.

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

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

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.