When formatting a requirement, only lowercase its name. #2123
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/cc @jtratner on the cache question |
|
Thanks for the PR, I think I typed a comment about this on mobile but I have no idea if I sent t or if that was on the issue. Probably related -- #2125 |
|
@jfly ok, so first of all thanks a ton for digging into this. To answer your questions
@pytest.mark.markers
@flaky
def test_platform_python_implementation_marker(PipenvInstance, pypi):
"""Markers should be converted during locking to help users who input this incorrectly
"""
with PipenvInstance(pypi=pypi) as p:
import platform
with open(p.pipfile_path, 'w') as f:
contents = """
[packages]
pytz = {{version = "*", platform_python_implementation = "== '{0}'"}}
""".format(platform.python_implementation()).strip()
f.write(contents)
c = p.pipenv('install')
assert c.return_code == 0
assert p.lockfile['default']['funcsigs']['markers'] == "os_name == '{0}'".format(platform.python_implementation())
python_implementations = ('CPython', 'IronPython', 'Jython', 'PyPy',)
for dep in results:
# Add version information to lockfile.
lockfile['develop'].update(
{dep['name']: {'version': '=={0}'.format(dep['version'])}}
)
# Add Hashes to lockfile
lockfile['develop'][dep['name']]['hashes'] = dep['hashes']
# Add index metadata to lockfile.
if 'index' in dep:
lockfile['develop'][dep['name']]['index'] = dep['index']
# Add PEP 508 specifier metadata to lockfile if dep isnt top level
# or top level dep doesn't itself have markers
if 'markers' in dep:
if dep['name'] in dev_packages and not any(key in dev_packages[dep['name']] for key in allowed_marker_keys):
continue
implementation = dep['markers'].get('platform_python_implementation', '')
if implementation:
marker = first(impl for impl in python_implementations if lower(impl) == lower(implementation))
else:
marker = dep['markers']
lockfile['develop'][dep['name']]['markers'] = marker |
Ok, I've reported this to them in jazzband/pip-tools#660.
If we want to be PEP 508 compliant, then yes, I think we do. If I'm understanding this correctly, then for the same reasons I mentioned in #2113 (comment), I believe that since
Thanks for the pointer. I messed around with that a bit, and came up with the following test: @pytest.mark.markers
@flaky
@pytest.mark.markers
@flaky
def test_platform_python_implementation_marker(PipenvInstance, pypi):
"""Markers should be converted during locking to help users who input this incorrectly
"""
with PipenvInstance(pypi=pypi) as p:
with open(p.pipfile_path, 'w') as f:
contents = """
[packages]
pytz = {{version = "*", platform_python_implementation = "== '{0}'"}}
""".format(platform.python_implementation()).strip()
f.write(contents)
c = p.pipenv('install')
assert c.return_code == 0
#import pdb; pdb.set_trace()#<<<
assert p.lockfile['default']['pytz']['markers'] == "platform_python_implementation == '{0}'".format(platform.python_implementation())However, this test actually passes even without any changes to the Pipenv source code, so it's definitely not testing the bug I'm trying to fix =( I stepped through the Pipenv code during this test, and the reason it's not triggering the bug is because we end up on this line of resolver.py: pipenv/pipenv/patched/piptools/resolver.py Line 279 in a16b1d0 and pipenv/pipenv/patched/piptools/resolver.py Line 299 in a16b1d0 I looked a bit into how to trigger that code, and the only solution I can think of would be to add a new package to the
Hmm... do you mean "it leaves things lowercased"? That looks like it could work, but it feels pretty silly to me to first lowercase a string, and then do a lookup later on to figure out it is actually supposed to be capitalized. It also doesn't do the right thing for other marker types such as sys_platform, right? |
|
Whoops, I don't know how I accidentally closed this, sorry for the noise! |
|
Ping. Any thoughts on how to proceed with this? Thanks! |
|
Yeah sorry for the delay, feel free to add the requisite wheels and make sure we are handling this case properly but I'll probably merge this soon either way if tests still pass |
This fixes pypa#2113. This bug was introduced in <jazzband/pip-tools#452> as a band-aid fix to <jazzband/pip-tools#431>. Pipenv then copied that code in <pypa@2553ebc#diff-b56b95ccea8595a0f6f24ea753842976>, and inherited this latent bug. Maybe the right fix is for pypa/packaging to lowercase the name? There's a comment here <https://github.com/pypa/packaging/blob/16.8/packaging/requirements.py#L86> about normalizing the requirement's name, which might be what this is referring to. To test this, I invented a new, very simple python package called `depends-on-marked-package`. The setup.py for this package is just: ```python import setuptools setuptools.setup( name="depends-on-marked-package", version="0.0.1", packages=setuptools.find_packages(), install_requires=['pytz; platform_python_implementation=="CPython"'], ) ``` This is a simplified version of gevent's setup.py's install_requires upon greenlet.
|
Ok, I've updated my PR with a test! Rather than adding all of gevent and greenlet, I invented a new, super simple package called import setuptools
setuptools.setup(
name="depends-on-marked-package",
version="0.0.1",
packages=setuptools.find_packages(),
install_requires=['pytz; platform_python_implementation=="CPython"'],
)Without my change to how we format markers, I see the test failWith my change, the test does pass!The way I've set up this test, I imagine it will only pass in a CPython environment. I'm not sure if that will be a problem or not. |
|
Thanks for all your hard work on this! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #2113.
This bug was introduced in
jazzband/pip-tools#452 as a band-aid fix to
jazzband/pip-tools#431. Pipenv then copied
that code in 2553ebc#diff-b56b95ccea8595a0f6f24ea753842976, and inherited this latent bug.
Maybe the right fix is for pypa/packaging to lowercase the name? There's
a comment here
https://github.com/pypa/packaging/blob/16.8/packaging/requirements.py#L86
about normalizing the requirement's name, which might be what this is
referring to.
Testing
Demo of how I tested this out:
Before
Note the lowercase "cpython" in the output.
After
Note the correctly capitalized "CPython" in the output:
TODO