[FR] Improve error feedback for improper one-liner requirement with environment markers in setup.cfg
#3467
Comments
frenzymadness
added
bug
Needs Triage
|
From the configparser perspective, the difference is this: versus |
|
The (broken) wheel has this in the metadata: |
|
Hi @frenzymadness , this is a limitation in terms of
The rationale is that if you don't include newlines in the value, the character used to split different requirements is |
|
I've fixed the problem in release 1.4.1 so now it's even easier to reproduce. The old version fails to install no matter which version of Python you use: Python 3.9 The new version works correctly and omits toml for Python 3.11: Python 3.9 |
|
My assumption is that: Is read as: While: Is read as: This behavior seems consistent with the examples from the docs: Albeit, quite confusing with environment markers. |
Oh, I sent my comment before seeing this. I agree that this is a known limitation, however I would argue that it is a trap. We've discussed with @frenzymadness and we belive a warning might be presented when the entire line:
WDYT? |
|
Thanks for the info @abravalheri ! What do you think about an automated check for this? I think it'd be easy to implement it. Something like: If you have extras_require on a single line and after split by |
|
@hroncok yes, that is precisely the documented behaviour.
@frenzymadness , I am afraid this behaviour is not a bug but rather an intentional design decision. If you prefer having more clarity in terms of the configuration language syntax, maybe you would like to try Footnotes
|
That could be a nice addition. Thank you very much @hroncok and @frenzymadness . You one of you guys would like to give it a try in a PR? |
|
We will try it soon. |
abravalheri
changed the title
setup.cfg
|
Before I try to create a test for it, would something like this be considered as an acceptable solution? --- a/setuptools/config/setupcfg.py
+++ b/setuptools/config/setupcfg.py
@@ -10,11 +10,13 @@ import functools
from collections import defaultdict
from functools import partial
from functools import wraps
+from itertools import chain
from typing import (TYPE_CHECKING, Callable, Any, Dict, Generic, Iterable, List,
Optional, Tuple, TypeVar, Union)
from distutils.errors import DistutilsOptionError, DistutilsFileError
from setuptools.extern.packaging.version import Version, InvalidVersion
+from setuptools.extern.packaging.markers import Marker, InvalidMarker
from setuptools.extern.packaging.specifiers import SpecifierSet
from setuptools._deprecation_warning import SetuptoolsDeprecationWarning
@@ -702,6 +704,19 @@ class ConfigOptionsHandler(ConfigHandler["Distribution"]):
section_options,
self._parse_requirements_list,
)
+
+ for requirement in chain(*parsed.values()):
+ try:
+ Marker(requirement)
+ except InvalidMarker:
+ pass
+ else:
+ msg = ( "One of the parsed requirements in extras_require section "
+ f"looks like a valid environment marker: '{requirement}'\n"
+ "Make sure that the config is correct and check "
+ "https://setuptools.pypa.io/en/latest/userguide/declarative_config.html#opt-2")
+ warnings.warn(msg, UserWarning)
+
self['extras_require'] = parsed
def parse_section_data_files(self, section_options):implemented in It basically checks all the parsed requirements – it actually tries to create a marker from them – and reports a warning if it succeedes. |
|
Hi @frenzymadness, thank you very much for having a look on this. I am not entirely sure about this approach... For example, is it a given that valid markers are invalid/reserved package names? (I just found a few packages on PyPI that might invalidate this assumption, e.g. |
|
You are right that shows the warning even it's a correct definition. Any idea how to make it more robust? |
|
I imagine that the best we can do here is to reduce the probability of false positives... How about performing the check only if:
? (This way we can only check the second requirement for a marker) Would this check cover the most likely scenarios where the warning is useful? |
|
Thanks for the help. I've run all tests to see what structure the --- a/setuptools/config/setupcfg.py
+++ b/setuptools/config/setupcfg.py
@@ -15,6 +15,7 @@ from typing import (TYPE_CHECKING, Callable, Any, Dict, Generic, Iterable, List,
from distutils.errors import DistutilsOptionError, DistutilsFileError
from setuptools.extern.packaging.version import Version, InvalidVersion
+from setuptools.extern.packaging.markers import Marker, InvalidMarker
from setuptools.extern.packaging.specifiers import SpecifierSet
from setuptools._deprecation_warning import SetuptoolsDeprecationWarning
@@ -702,6 +703,35 @@ class ConfigOptionsHandler(ConfigHandler["Distribution"]):
section_options,
self._parse_requirements_list,
)
+
+ # 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
+ # * the second requirement is a valid environment 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:
+ second_requirement = parsed[name][1]
+ try:
+ Marker(second_requirement)
+ except InvalidMarker:
+ pass
+ else:
+ msg = ( "One of the parsed requirements in extras_require section "
+ f"looks like a valid environment marker: '{second_requirement}'\n"
+ "Make sure that the config is correct and check "
+ "https://setuptools.pypa.io/en/latest/userguide/declarative_config.html#opt-2")
+ warnings.warn(msg, UserWarning)
+
self['extras_require'] = parsed
def parse_section_data_files(self, section_options):
|
|
Another idea from @hroncok is: If there is no |
If the marker attribute is not None. |
That sounds good and resilient! I have a single note about the implementation details: ideally (if possible) it would be nice to split the added code (or at least the inner part of the for-loop) in a separated auxiliary function (but that is already part of the code review :P). Thank you very much @frenzymadness for working on this. |
|
Ok, we can move the discussion to the PR: #3481 |
setuptools version
63.2.0
Python version
3.9 and 3.11
OS
Fedora Linux
Additional environment information
No response
Description
Project: https://github.com/thoth-station/micropipenv
The line I have a problem with: https://github.com/thoth-station/micropipenv/blob/2127293c4c9fa344fd1535c5189c39ca7ba4594d/setup.cfg#L40
It seems to me that there is a difference between this:
and this:
Micropipenv version 1.4.0 contains the [toml] definition on a single line and when you try to install micropipenv[toml], it does not work (Python 3.9.13):
the same happens when you use Python 3.11.
But when I use the multiline definition:
it works correctly and omits toml in env with Python 3.11 and installs it if Python<3.11.
When I build a sdist from the package, there is a difference in requires.txt in egg-info directory:
single line definition:
two lines:
Expected behavior
I'd expect the two definitions mentioned above to behave the same way.
How to Reproduce
You can try to install "micropipenv[toml]==1.4.0" and then you can change the extras_require definition as described above to see that it fixes the problem.
Output
The text was updated successfully, but these errors were encountered: