Skip to content

Commit

Permalink
Add warning for potential extras_require misconfiguration
Browse files Browse the repository at this point in the history
Fixes: #3467
  • Loading branch information
frenzymadness authored and abravalheri committed Aug 6, 2022
1 parent d90cf84 commit 506e7e7
Show file tree
Hide file tree
Showing 2 changed files with 93 additions and 0 deletions.
48 changes: 48 additions & 0 deletions setuptools/config/setupcfg.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
Optional, Tuple, TypeVar, Union)

from distutils.errors import DistutilsOptionError, DistutilsFileError
from setuptools.extern.packaging.requirements import Requirement, InvalidRequirement
from setuptools.extern.packaging.version import Version, InvalidVersion
from setuptools.extern.packaging.specifiers import SpecifierSet
from setuptools._deprecation_warning import SetuptoolsDeprecationWarning
Expand Down Expand Up @@ -174,6 +175,43 @@ def parse_configuration(
return meta, options


def warn_accidental_env_marker_misconfig(section_name, section_options, parsed):
"""Because users sometimes misinterpret this configuration:
[options.extras_require]
foo = bar;python_version<"4"
It looks like one requirement with an environment marker
but because there is no newline, it's parsed as two requirements
with a semicolon as separator.
Therefore, if:
* input string does not contain a newline AND
* parsed result contains two requirements AND
* parsing of the two parts from the result ("<first>;<second>")
leads in a valid Requirement with a valid marker
a UserWarning is shown to inform the user about the possible problem.
"""

for name, (file, requirements) in section_options.items():
if "\n" not in requirements and len(parsed[name]) == 2:
original_requirements_str = ";".join(parsed[name])
try:
req = Requirement(original_requirements_str)
except InvalidRequirement:
pass
else:
if req.marker is None:
continue
msg = (
f"One of the parsed requirements in {section_name} section "
f"looks like a valid environment marker: '{parsed[name][1]}'\n"
"Make sure that the config is correct and check "
"https://setuptools.pypa.io/en/latest/userguide/declarative_config.html#opt-2" # noqa: E501
)
warnings.warn(msg, UserWarning)


class ConfigHandler(Generic[Target]):
"""Handles metadata supplied in configuration files."""

Expand Down Expand Up @@ -421,6 +459,13 @@ def parse_section(self, section_options):
try:
self[name] = value

if name == "install_requires":
warn_accidental_env_marker_misconfig(
"install_requires",
{name: (_, value)},
{name: self.target_obj.install_requires},
)

except KeyError:
pass # Keep silent for a new option may appear anytime.

Expand Down Expand Up @@ -702,6 +747,9 @@ def parse_section_extras_require(self, section_options):
section_options,
self._parse_requirements_list,
)

warn_accidental_env_marker_misconfig("extras_require", section_options, parsed)

self['extras_require'] = parsed

def parse_section_data_files(self, section_options):
Expand Down
45 changes: 45 additions & 0 deletions setuptools/tests/config/test_setupcfg.py
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,51 @@ def test_extras_require(self, tmpdir):
}
assert dist.metadata.provides_extras == set(['pdf', 'rest'])

@pytest.mark.parametrize(
"config",
[
"[options.extras_require]\nfoo = bar;python_version<'3'",
"[options.extras_require]\nfoo = bar;os_name=='linux'",
"[options.extras_require]\nfoo = bar;python_version<'3'\n",
"[options.extras_require]\nfoo = bar;os_name=='linux'\n",
"[options]\ninstall_requires = bar;python_version<'3'",
"[options]\ninstall_requires = bar;os_name=='linux'",
"[options]\ninstall_requires = bar;python_version<'3'\n",
"[options]\ninstall_requires = bar;os_name=='linux'\n",
],
)
def test_warn_accidental_env_marker_misconfig(self, config, tmpdir):
fake_env(tmpdir, config)
match = (
r"One of the parsed requirements in (install_requires|extras_require) "
"section looks like a valid environment marker.*"
)
with pytest.warns(UserWarning, match=match):
with get_dist(tmpdir) as _:
pass

@pytest.mark.parametrize(
"config",
[
"[options.extras_require]\nfoo =\n bar;python_version<'3'",
"[options.extras_require]\nfoo = bar;baz\nboo = xxx;yyy",
"[options.extras_require]\nfoo =\n bar;python_version<'3'\n",
"[options.extras_require]\nfoo = bar;baz\nboo = xxx;yyy\n",
"[options.extras_require]\nfoo =\n bar\n python_version<'3'\n",

This comment has been minimized.

Copy link
@mgorny

mgorny Jan 21, 2023

Contributor

It this sample actually valid? Unless I'm missing something, it's erroneous because it leaves the env marker is on lone line — unless the test is merely supposed to confirm that the warning isn't thrown here.

This comment has been minimized.

Copy link
@abravalheri

abravalheri Jan 21, 2023

Contributor

@mgorny, this is valid syntax and there is python_version package in PyPI. In my opinion this is a valid entry.

This comment has been minimized.

Copy link
@abravalheri

abravalheri Jan 21, 2023

Contributor

In this case, the second entry is not a marker, but a different package. Therefore no warning should be shown.

This comment has been minimized.

Copy link
@mgorny

mgorny Jan 21, 2023

Contributor

But doesn't this mean that the version string is < '3' rather than < 3? This doesn't seem to work, at least for pip:

ERROR: Could not find a version that satisfies the requirement python-version<'3' (from unknown) (from versions: 0.0.2)
ERROR: No matching distribution found for python-version<'3'

and packaging-22+ throws on it.

This comment has been minimized.

Copy link
@abravalheri

abravalheri Jan 21, 2023

Contributor

I see! Thank you very much for the clarification. Yes, we probably should fix it.

This comment has been minimized.

Copy link
@mgorny

mgorny Jan 21, 2023

Contributor

Do you want me to make a PR for it or will you just handle it? If the former, should I just remove these two cases or remove the quoting around the number?

This comment has been minimized.

Copy link
@abravalheri

abravalheri Jan 21, 2023

Contributor

If you can provide a PR, that would be super helpful. Thank you very much!

I think the best would be to remove the quote.

"[options]\ninstall_requires =\n bar;python_version<'3'",
"[options]\ninstall_requires = bar;baz\nboo = xxx;yyy",
"[options]\ninstall_requires =\n bar;python_version<'3'\n",
"[options]\ninstall_requires = bar;baz\nboo = xxx;yyy\n",
"[options]\ninstall_requires =\n bar\n python_version<'3'\n",
],
)
def test_nowarn_accidental_env_marker_misconfig(self, config, tmpdir, recwarn):
fake_env(tmpdir, config)
with get_dist(tmpdir) as _:
pass
# The examples are valid, no warnings shown
assert not any(w.category == UserWarning for w in recwarn)

def test_dash_preserved_extras_require(self, tmpdir):
fake_env(tmpdir, '[options.extras_require]\n' 'foo-a = foo\n' 'foo_b = test\n')

Expand Down

0 comments on commit 506e7e7

Please sign in to comment.