From 8f00d60d623fdc1b8614c5e454edebf8cec504b8 Mon Sep 17 00:00:00 2001 From: rajdeep Date: Sun, 28 Oct 2018 15:38:18 -0400 Subject: [PATCH 1/5] Disallow files for license inputs The ability to handle files was originally added and documented based on a misunderstanding of what the `license` field should include. The field should be the name of the license, not the full text. It is likely that anyone actually using this was outputing malformed PKG-INFO files, because most license files contain newlines. See GH issue #1551 --- setup.cfg | 1 - setuptools/config.py | 22 ++++++++++++++++++++-- setuptools/tests/test_egg_info.py | 25 ++++++++++++++++++++++++- 3 files changed, 44 insertions(+), 4 deletions(-) diff --git a/setup.cfg b/setup.cfg index 57ab623435..dd404469fe 100644 --- a/setup.cfg +++ b/setup.cfg @@ -26,4 +26,3 @@ universal = 1 license_file = LICENSE [bumpversion:file:setup.py] - diff --git a/setuptools/config.py b/setuptools/config.py index d1ac6734dc..3a6da20f34 100644 --- a/setuptools/config.py +++ b/setuptools/config.py @@ -246,6 +246,24 @@ def _parse_bool(cls, value): value = value.lower() return value in ('1', 'true', 'yes') + @classmethod + def _exclude_files_parser(cls, key): + """Returns a parser function to make sure field inputs + are not files. + + Parses a value after getting the key so error messages are + more informative. + + :param key: + :rtype: callable + """ + def parser(value): + exclude_directive = 'file:' + if value.startswith(exclude_directive): + raise ValueError('Only strings are accepted for the {0} field, files are not accepted'.format(key)) + return value + return parser + @classmethod def _parse_file(cls, value): """Represents value as a string, allowing including text @@ -255,7 +273,6 @@ def _parse_file(cls, value): directory with setup.py. Examples: - file: LICENSE file: README.rst, CHANGELOG.md, src/file.txt :param str value: @@ -449,6 +466,7 @@ def parsers(self): parse_list = self._parse_list parse_file = self._parse_file parse_dict = self._parse_dict + exclude_files_parser = self._exclude_files_parser return { 'platforms': parse_list, @@ -460,7 +478,7 @@ def parsers(self): DeprecationWarning), 'obsoletes': parse_list, 'classifiers': self._get_parser_compound(parse_file, parse_list), - 'license': parse_file, + 'license': exclude_files_parser('license'), 'description': parse_file, 'long_description': parse_file, 'version': self._parse_version, diff --git a/setuptools/tests/test_egg_info.py b/setuptools/tests/test_egg_info.py index f97b3f1d81..e1105044d3 100644 --- a/setuptools/tests/test_egg_info.py +++ b/setuptools/tests/test_egg_info.py @@ -148,6 +148,26 @@ def test_expected_files_produced(self, tmpdir_cwd, env): ] assert sorted(actual) == expected + def test_license_is_a_string(self, tmpdir_cwd, env): + setup_config = DALS(""" + [metadata] + name=foo + version=0.0.1 + license=file:MIT + """) + + setup_script = DALS(""" + from setuptools import setup + + setup() + """) + + build_files({'setup.py': setup_script, + 'setup.cfg': setup_config}) + + with pytest.raises(ValueError): + self._run_egg_info_command(tmpdir_cwd, env) + def test_rebuilt(self, tmpdir_cwd, env): """Ensure timestamps are updated when the command is re-run.""" self._create_project() @@ -598,7 +618,10 @@ def _run_egg_info_command(self, tmpdir_cwd, env, cmd=None, output=None): env=environ, ) if code: - raise AssertionError(data) + if 'ValueError' in data: + raise ValueError(data) + else: + raise AssertionError(data) if output: assert output in data From 2b038993e835d5a0d328910bc855b53a7c21bc6e Mon Sep 17 00:00:00 2001 From: rajdeep Date: Sun, 28 Oct 2018 15:53:47 -0400 Subject: [PATCH 2/5] Add changelog file --- changelog.d/1551.breaking.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/1551.breaking.rst diff --git a/changelog.d/1551.breaking.rst b/changelog.d/1551.breaking.rst new file mode 100644 index 0000000000..c0e477ce81 --- /dev/null +++ b/changelog.d/1551.breaking.rst @@ -0,0 +1 @@ +File inputs for the `license` field in `setup.cfg` files now explicitly raise an error. From 8b8e2c8b3b83151dad1719a8cb676e919766c1c9 Mon Sep 17 00:00:00 2001 From: Paul Ganssle Date: Sat, 29 Dec 2018 10:53:13 -0500 Subject: [PATCH 3/5] Change how license field ValueError is tested Both the old and new approaches are deeply unsatisfying to me, but without reworking how these test commands are run, I think this is about as close as we can get to enforcing that this specific call raises ValueError. --- setuptools/tests/test_egg_info.py | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/setuptools/tests/test_egg_info.py b/setuptools/tests/test_egg_info.py index e1105044d3..979ff18e4e 100644 --- a/setuptools/tests/test_egg_info.py +++ b/setuptools/tests/test_egg_info.py @@ -165,9 +165,20 @@ def test_license_is_a_string(self, tmpdir_cwd, env): build_files({'setup.py': setup_script, 'setup.cfg': setup_config}) - with pytest.raises(ValueError): + # This command should fail with a ValueError, but because it's + # currently configured to use a subprocess, the actual traceback + # object is lost and we need to parse it from stderr + with pytest.raises(AssertionError) as exc: self._run_egg_info_command(tmpdir_cwd, env) + # Hopefully this is not too fragile: the only argument to the + # assertion error should be a traceback, ending with: + # ValueError: .... + # + # assert not 1 + tb = exc.value.args[0].split('\n') + assert tb[-3].lstrip().startswith('ValueError') + def test_rebuilt(self, tmpdir_cwd, env): """Ensure timestamps are updated when the command is re-run.""" self._create_project() @@ -617,11 +628,8 @@ def _run_egg_info_command(self, tmpdir_cwd, env, cmd=None, output=None): data_stream=1, env=environ, ) - if code: - if 'ValueError' in data: - raise ValueError(data) - else: - raise AssertionError(data) + assert not code, data + if output: assert output in data From 4a8dedf068f165f2e44cf3f3fab3e43a102aafe9 Mon Sep 17 00:00:00 2001 From: Paul Ganssle Date: Sat, 29 Dec 2018 11:12:41 -0500 Subject: [PATCH 4/5] Remove file: from documentation for license field --- docs/setuptools.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/setuptools.txt b/docs/setuptools.txt index bca211bdc3..f68edf0ccd 100644 --- a/docs/setuptools.txt +++ b/docs/setuptools.txt @@ -2370,7 +2370,7 @@ author_email author-email str maintainer str maintainer_email maintainer-email str classifiers classifier file:, list-comma -license file:, str +license str description summary file:, str long_description long-description file:, str long_description_content_type str 38.6.0 From 3db95bcc3dcc72dbb47d7dfc987ae646b249addd Mon Sep 17 00:00:00 2001 From: Paul Ganssle Date: Sat, 29 Dec 2018 11:13:43 -0500 Subject: [PATCH 5/5] Add explicit test for license in setup.cfg --- setuptools/tests/test_config.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/setuptools/tests/test_config.py b/setuptools/tests/test_config.py index 736c184dca..53b8a956c4 100644 --- a/setuptools/tests/test_config.py +++ b/setuptools/tests/test_config.py @@ -1,10 +1,12 @@ import contextlib import pytest + from distutils.errors import DistutilsOptionError, DistutilsFileError from mock import patch from setuptools.dist import Distribution, _Distribution from setuptools.config import ConfigHandler, read_configuration from . import py2_only, py3_only +from .textwrap import DALS class ErrConfigHandler(ConfigHandler): """Erroneous handler. Fails to implement required methods.""" @@ -146,6 +148,24 @@ def test_basic(self, tmpdir): assert metadata.download_url == 'http://test.test.com/test/' assert metadata.maintainer_email == 'test@test.com' + def test_license_cfg(self, tmpdir): + fake_env( + tmpdir, + DALS(""" + [metadata] + name=foo + version=0.0.1 + license=Apache 2.0 + """) + ) + + with get_dist(tmpdir) as dist: + metadata = dist.metadata + + assert metadata.name == "foo" + assert metadata.version == "0.0.1" + assert metadata.license == "Apache 2.0" + def test_file_mixed(self, tmpdir): fake_env(