ansible / ansible-builder

An Ansible execution environment builder
Other
287 stars 93 forks source link

Drop requirements-parser in favor of pep508 #645

Closed sivel closed 5 months ago

sivel commented 7 months ago

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:

sivel commented 6 months ago

In a way, I still prefer this PR over https://github.com/ansible/ansible-builder/pull/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 https://github.com/ansible/ansible-builder/issues/472

webknjaz commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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.

Shrews commented 5 months ago

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 commented 5 months ago

Closing in favor of https://github.com/ansible/ansible-builder/pull/663