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

format_requirement is lowercasing markers, which violates PEP 508 #660

Closed
jfly opened this issue May 7, 2018 · 6 comments
Closed

format_requirement is lowercasing markers, which violates PEP 508 #660

jfly opened this issue May 7, 2018 · 6 comments
Labels
bug Something is not working markers Related to environment markers needs reproduce Need to reproduce an issue PR wanted Feature is discussed or bug is confirmed, PR needed

Comments

@jfly
Copy link

jfly commented May 7, 2018

I discovered this issue while using pipenv. See pypa/pipenv#2113. I believe this bug was introduced in
#452 as a band-aid fix to #431.

I've never actually used pip-tools myself, so I'm not sure how to reproduce this issue with pip-tools, but it should be pretty clear from the pipenv bug report what's going on here.

I'm working on a fix for this over in pypa/pipenv#2123, but I thought it would be useful to report this upstream ASAP, in case this is already a known issue, or you have a better idea for a fix.

@vphilippon
Copy link
Member

Hi @jfly, thanks for taking the time to report here too!

I'm pretty sure you've put the finger right on it. We'll have to improve that lowercasing logic a bit.

@vphilippon vphilippon added the bug Something is not working label May 10, 2018
@atugushev atugushev added the PR wanted Feature is discussed or bug is confirmed, PR needed label Oct 17, 2019
@atugushev atugushev added the markers Related to environment markers label Nov 22, 2019
@AndydeCleyre AndydeCleyre added the needs reproduce Need to reproduce an issue label Sep 1, 2021
@AndydeCleyre
Copy link
Contributor

I can work on this if someone can provide a reproduction process.

@jfly
Copy link
Author

jfly commented Sep 1, 2021

@AndydeCleyre I still have never actually used pip-tools directly, but you can see a repro in the test I added to pipenv over here: https://github.com/pypa/pipenv/pull/2123/files#diff-ce6487d3c4a96b746d4e7c37ce59de79cd9f46b7aa0d2e9e26443ee135b517e1. Is that enough to go off of?

@AndydeCleyre
Copy link
Contributor

Thanks, but I'm not sure yet. pip-tools doesn't currently operate on Pipfiles, so I need to test with a requirements.in file (same syntax as requirements.txt).

With this requirements.in:

pytz ; platform_python_implementation == 'CPython'

And the following command:

$ pip-compile requirements.in

The output looks good:

pytz==2021.1 ; platform_python_implementation == 'CPython'

If I use the wrong case in the input file (platform_python_implementation == 'cpython'), the marker doesn't match the environment, and the output is blank.

Is that the case we're talking about here? If so, what's the proper thing to do when a user uses the wrong case for the marker value in the input file? Should we try to catch common mistakes?

@AndydeCleyre
Copy link
Contributor

I don't see that this is an issue, at least any longer. If anyone can demonstrate it (provide a reproduction), please do, otherwise I'll close this in a few weeks.

@jfly
Copy link
Author

jfly commented Sep 25, 2021

@AndydeCleyre I just spent a little while reading through pip-tools source code, and I see the same code that pipenv copied here:

line = str(ireq.req).lower()

Note how this code lowercases the entire requirement, which was a problem over in pipenv because it could include markers that shouldn't be lowercased. However, over in pip-tools, it looks like markers get passed around separately and are tacked onto the formatted requirements string after the lowercasing.

I was unable to find a codepath in pip-tools that led to this format_requirement function getting called with an InstallRequirement with a non-null markers attribute. Maybe there is a path, but I just don't know pip-tools well enough to find it if it does!

I think it's ok to close this out.

@jfly jfly closed this as completed Sep 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working markers Related to environment markers needs reproduce Need to reproduce an issue PR wanted Feature is discussed or bug is confirmed, PR needed
Projects
None yet
Development

No branches or pull requests

4 participants