InvalidRequirement produced for git URL in extra

[hauntsaninja]

Originally filed at pypa/packaging#494

mkdir reqparse
cd reqparse

cat > setup.py <<HERE
from setuptools import setup

setup(
    name="reqparse",
    extras_require={
        "test": [
            "gym-retro @ git+https://github.com/openai/retro.git@v0.8.0",
        ]
    },
)
HERE

python3 -m venv env
source env/bin/activate
python3 -m pip install packaging -e .

cat reqparse.egg-info/requires.txt

# this now gives me
# packaging.requirements.InvalidRequirement: Parse error at "'extra =='": Expected string_end
python3 -c '
import importlib.metadata
from packaging.requirements import Requirement
Requirement(importlib.metadata.distribution("reqparse").requires[0])
'

This results in the equivalent of:

from packaging.requirements import Requirement
Requirement('gym-retro@ git+https://github.com/openai/retro.git@v0.8.0; extra == "test"')

uranusjr suggested adding a space before the semicolon here: https://github.com/python/cpython/blob/908fd691f96403a3c30d85c17dd74ed1f26a60fd/Lib/importlib/metadata/__init__.py#L678

Happy to make a PR for that — if so, should I also make a PR to CPython, or is there a separate process for porting?

[FFY00]

Happy to make a PR for that — if so, should I also make a PR to CPython, or is there a separate process for porting?

Please do. The fix will be committed here and then synced to CPython later.

[jaraco]

Would someone explain what the problem is and why adding the space corrects that condition?

[hauntsaninja]

The issue is that importlib.metadata.requires produces requirements that cause packaging.requirements.Requirement to raise an error: packaging.requirements.InvalidRequirement: Parse error at "'extra =='": Expected string_end

Grammar stuff is tricky, but I thiiiiink packaging's behaviour is compliant to the grammar outlined in PEP 508. What I think is happening is that the semicolon gets parsed as part of the urlspec (probably because semicolon is included in pchar in the grammar), so we're not able to match quoted_marker? in (name:n wsp* extras?:e wsp* urlspec:v (wsp+ | end) quoted_marker?. By adding a whitespace, we ensure this isn't the case, since that causes the parser to recognise the end of urlspec.

[pradyunsg]

In other more different reductive words, you need a space before the semicolon, because this is one of the paper cuts in PEP 508's URLs. 🤷🏻‍♂️

[uranusjr]

What I think is happening is that the semicolon gets parsed as part of the urlspec (probably because semicolon is included in pchar in the grammar)

You're correct. Semicolon is a valid character in a URL, so the PEP 508 parser's behaviour is correct to include it in the urlspec run. You could argue that the rules should be smarter to accomplish this punctuation-before-space case (most Markdown parsers implement this so you can stick a comma right after a URL and get the desired rendering result), but adding spaces around the URL is the safest format for tools to emit.

[jaraco]

Thanks for the deeper explanation. That's exactly what I was looking for. I also looked at the grammar and the complete grammar. Interestingly, both supply different definitions for url_req. The complete grammar provides a better definition because it doesn't require whitespace at the end of a url_req whereas the grammar does.

Either way, the spec does require the space between the urlspec and the ;, so importlib_metadata is indeed incorrectly formatting the req.

[jaraco]

I do observe, though, that name_req does not require the space. The space is only required if a url_req is used.

[jaraco]

Thanks @hauntsaninja and others for reporting, triaging, and drafting a patch. Fix is rolling out in 4.8.3.

Does this fix need a backport to 2.x (Python 2.7 support)? That is, are there use-cases on older Pythons affected by this bug that would benefit from the fix?

[jaraco]

I realize this change needs a port to CPython, but so also does 4.6.4 and later. I'm unsure if the changes from 4.7 and 4.8 can be safely backported in CPython, so I'll probably need to introduce those changes and the bugfix changes separately.

[jaraco]

Ported to CPython in bpo-46105.