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

Drop requirements-parser in favor of pep508 #645

Closed
wants to merge 7 commits into from
Closed

Conversation

sivel
Copy link
Member

@sivel sivel commented Jan 30, 2024

This PR is not backwards compatible, as it changes the expectations of what a "requirement" are, from the pip definition, to the definition dictated by pep508. Although it should be mentioned that we never gauranteed full support of pip requirements.txt, so with that in mind, this is technically not a breaking PR, but should be noted that it could break some uses.

See:

@github-actions github-actions bot added the needs_triage New item that needs to be triaged label Jan 30, 2024
@sivel
Copy link
Member Author

sivel commented Feb 6, 2024

In a way, I still prefer this PR over #647

Even through the alternative means we have to care less, I feel like it opens us up to more issues in the long run.

And I think the dep on packaging introduced here, even though we're still keeping an extra dep, also helps us with more advanced functionality for the "escape hatch" we want as part of #472

@webknjaz
Copy link
Member

webknjaz commented Feb 6, 2024

from the pip definition, to the definition dictated by pep508.

They are supposed to be the same: https://pip.pypa.io/en/stable/reference/requirement-specifiers/. Do you know of any differences?

@sivel
Copy link
Member Author

sivel commented Feb 6, 2024

They are supposed to be the same: https://pip.pypa.io/en/stable/reference/requirement-specifiers/. Do you know of any differences?

Here are a few things that work in a pip requirements.txt that aren't pep508:

-r another-requirements.txt
--extra-index-url http://pypi.example.org/simple
https://github.com/ansible/ansible/archive/refs/heads/devel.zip#egg=ansible
git+ssh://github.com/ansible/ansible.git@devel#egg=ansible

@webknjaz
Copy link
Member

webknjaz commented Feb 6, 2024

@sivel PEP 508 talks about individual package specifiers, not requiremets.txt. Requirements has always been a pip-specific format. requirements version specs are still supposed to be PEP 508 compliant, it just allows pip CLI args in addition to that, which doesn't contradict the spec that only focuses on the specifiers: https://pip.pypa.io/en/stable/reference/requirements-file-format/#structure.

@webknjaz
Copy link
Member

A fresh idea: could also implement a draft version of PEP 735 and be done with it. It's a way forward in the wider ecosystem, anyway.

@sivel sivel mentioned this pull request Mar 14, 2024
4 tasks
@Shrews
Copy link
Contributor

Shrews commented Mar 25, 2024

FWIW, this PR still allows use of -r or --requirement within the requirements file (we handle that option by recursively including file contents before handing off to pip). One of the tests for that is modified by this change, but I verified it still works (test might need to revert back).

So the advantage of this PR (that I can see) is just the 1-to-1 replacement of requirements-parser with packaging. We still have the issue of folks wanting support of more pip-specific options, which we've pushed back on so far.

@sivel
Copy link
Member Author

sivel commented Mar 25, 2024

Closing in favor of #663

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs_triage New item that needs to be triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants