ansible-builder icon indicating copy to clipboard operation
ansible-builder copied to clipboard

Drop requirements-parser in favor of pep508

Open sivel opened this issue 1 year ago • 5 comments

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:

  • https://github.com/ansible/ansible-builder/issues/644

sivel avatar Jan 30 '24 00:01 sivel

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

sivel avatar Feb 06 '24 16:02 sivel

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?

webknjaz avatar Feb 06 '24 17:02 webknjaz

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

sivel avatar Feb 06 '24 17:02 sivel

@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 avatar Feb 06 '24 19:02 webknjaz

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.

webknjaz avatar Feb 21 '24 02:02 webknjaz

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.

Shrews avatar Mar 25 '24 16:03 Shrews

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

sivel avatar Mar 25 '24 18:03 sivel