Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Comparing marker string with marker string #632

Open
konstin opened this issue Dec 8, 2022 · 7 comments
Open

Comparing marker string with marker string #632

konstin opened this issue Dec 8, 2022 · 7 comments

Comments

@konstin
Copy link

konstin commented Dec 8, 2022

Currently comparing two marker string is legal in PEP 508, but fails in packaging 22.0:

from packaging.requirements import Requirement
Requirement('numpy; "a" <= "b"').marker.evaluate()
Traceback (most recent call last):
  File "/home/konsti/.local/share/JetBrains/Toolbox/apps/PyCharm-P/ch-0/223.7571.203/plugins/python/helpers/pydev/pydevconsole.py", line 364, in runcode
    coro = func()
  File "<input>", line 1, in <module>
  File "/home/konsti/pep508/.venv/lib/python3.8/site-packages/packaging/markers.py", line 241, in evaluate
    return _evaluate_markers(self._markers, current_environment)
  File "/home/konsti/pep508/.venv/lib/python3.8/site-packages/packaging/markers.py", line 148, in _evaluate_markers
    rhs_value = environment[environment_key]
KeyError: 'b'

The two options are to make this legal in packaging or to ban that from the Dependency specifiers aka PEP 508, imho it the better option would be to just ban it

@pradyunsg
Copy link
Member

Did this work in an older version of packaging?

@konstin
Copy link
Author

konstin commented Dec 8, 2022

no, it just throws a different error

@pradyunsg
Copy link
Member

Could you post that error as well?

@pradyunsg pradyunsg added the bug label Dec 8, 2022
@konstin
Copy link
Author

konstin commented Dec 8, 2022

Sure, i checked and in all versions down to 20.0 numpy; "a" <= "b" raises packaging.markers.UndefinedEnvironmentName: 'b' does not exist in evaluation environment.

@frostming
Copy link

frostming commented Dec 13, 2022

If I interpret PEP 508 correctly, these markers should be allowed:

"a" == "b"
"a" <= "b"

While these are forbidden:

"a" ~= "b"

Quote:

Comparisons in marker expressions are typed by the comparison operator. The <marker_op> operators that are not in <version_cmp> perform the same as they do for strings in Python. The <version_cmp> operators use the PEP 440 version comparison rules when those are defined (that is when both sides have a valid version specifier). If there is no defined PEP 440 behaviour and the operator exists in Python, then the operator falls back to the Python behaviour.

@pradyunsg
Copy link
Member

Bah, this isn’t a logical thing to do and I’d much rather we amend the dependency specification format to disallow such string comparisons and reject these markers.

@konstin
Copy link
Author

konstin commented Dec 13, 2022

I agree with @pradyunsg to change the spec. Imho the best thing would be to change the PEP 508 grammar to

marker_expr   = wsp* env_var wsp* marker_op wsp* python_str
              | wsp* python_str wsp* marker_op wsp* env_var
              | wsp* '(' marker wsp* ')'

This would also disallow other strange comparisons like extra == os_name and make parsing/evaluation easier.

(imho it should also be specified which env_var are PEP 440 and which are compared stringly, but that's a separate issue)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants