Store python_requires information instead of guessing it #449
Conversation
| # ansible-core 2.14 supports Python 3.9+ | ||
| return '>=3.9' | ||
| raise ValueError( | ||
| f'Python requirements for ansible-core {ansible_core_version} should be part of' |
There was a problem hiding this comment.
I don't understand why you think setting a python_requires is even necessary in the ansible package. pip already inspects python_requires of dependencies like ansible-core as a part of its dependency resolution process and acts accordingly.
There was a problem hiding this comment.
Do old versions of pip (that handle python_requires at all) also do that?
Anyway, I think this information is useful even if it can be indirectly derived, since https://pypi.org/project/ansible/ also shows that information (in Meta), and I'm not sure whether it would do that when the package wouldn't explicitly declare it. Also it makes this value available in https://pypi.org/pypi/ansible/json, which other programs might find useful.
There was a problem hiding this comment.
Do old versions of pip (that handle
python_requiresat all) also do that?
Yes, that's the idea. python_requires is treated similarly to other constraints, like dependency restrictions. Unless there's a bug, that is. But assuming that it's buggy upfront w/o testing seems like a premature optimization to me.
Anyway, I think this information is useful even if it can be indirectly derived, since pypi.org/project/ansible also shows that information (in
Meta), and I'm not sure whether it would do that when the package wouldn't explicitly declare it. Also it makes this value available in pypi.org/pypi/ansible/json, which other programs might find useful.
https://packaging.python.org/en/latest/specifications/core-metadata/#requires-python
The idea of this metadata field is to declare that a given package itself has restrictions of the Python runtime. But what you're doing is trying to guess whether the dependency is compatible. That dependency is what should have this declaration. OTOH, since ansible bundles the code from collections, it could derive its value from them and specify whatever the collections declare (well, the common denominator, I guess), since those collections are the actual content of the distribution being shipped.
There was a problem hiding this comment.
I disagree. Ansible is tightly coupled to ansible-core, so it exposing the same python_requires as ansible-core is not a premature optimization.
OTOH, since
ansiblebundles the code from collections, it could derive its value from them and specify whatever the collections declare (well, the common denominator, I guess), since those collections are the actual content of the distribution being shipped.
That would be totally wrong from my POV. Just because one collection requires Python 3.10 it doesn't mean that other collections cannot work with Python 3.9. Besides collections cannot declare their supported Pyton version, so this discussion is moot anyway.
There was a problem hiding this comment.
I disagree. Ansible is tightly coupled to ansible-core, so it exposing the same
python_requiresas ansible-core is not a premature optimization.
Coupled, yes. But does not bundle it. Which is why it has its own metadata (name, version, deps). And Requires-Python is just one of those dependency declarations.
OTOH, since
ansiblebundles the code from collections, it could derive its value from them and specify whatever the collections declare (well, the common denominator, I guess), since those collections are the actual content of the distribution being shipped.That would be totally wrong from my POV. Just because one collection requires Python 3.10 it doesn't mean that other collections cannot work with Python 3.9. Besides collections cannot declare their supported Pyton version, so this discussion is moot anyway.
Okay, the right solution would be the lowest supported by any collection, then.
There was a problem hiding this comment.
I agree with @felixfontein here. The only reasonable python_requires for ansible is the same one as ansible-core. It wouldn't make sense to include Python 2.7 in the version range even though most modules still support that. Modules aren't run by the ansible controller. The requirement for controller plugins is that they support and interface with the ansible bundle's ansible-core version. Sure, in isolation, many controller plugins will work with older ansible-core/Python versions, but that's not relevant to the ansible bundle. Controller plugins in ansible 6 by definition aren't compatible with Python 3.6, because the controller, ansible-core 2.13, doesn't support that. Setting this to the same value as ansible-core is the most reasonable approach, and it provides a stronger defence against dependency issues, even if pip is supposed to figure this out on its own.
There was a problem hiding this comment.
I never talked about modules that execute on remote targets, though. You're right, only what executes on the controller should influence the metadata.
But, with my PyPA hat on, my argument is that this is a premature attempt to solve a non-existing problem by declaring unnecessary metadata. If you think that this theoretical problem with ansible-core is actually possible, would you mind demonstrating it?
There was a problem hiding this comment.
The controller-side plugins in community.general still support Python 2.7. (The libraries that some of these require might not, though.)
I still don't see why we should remove this value, even if it would be a premature optimization (which I disagree it is).
There was a problem hiding this comment.
I think that the argument about copying the restriction from ansible-core is weak because it's already there in the dependency. By doing so, we'd just be providing inaccurate metadata. But if the argument is that "we think that this bundle of collections should work in certain runtimes", maybe it'd be reasonable for as long as it's decided based on the collections being included and factors that influence the project directly.
There was a problem hiding this comment.
By doing so, we'd just be providing inaccurate metadata.
It is not inaccurate, it is very accurate. Ansible (the Python package) is ansible-core of a very specific version (which has a very specific Python requirement) combined with the content of collections.
felixfontein
force-pushed
the
python-version
branch
from
0c4105e to
d57ff56
Compare
felixfontein
force-pushed
the
python-version
branch
from
d57ff56 to
929620a
Compare
|
(The last CI failure will only go away once antsibull-core does a new release.) |
felixfontein
changed the title
|
antsibull-core 1.3.0 has been released, so CI should now pass. |
|
@briantist @gotmax23 @webknjaz thanks for reviewing and the discussions! |
See #448 (comment).
Requires ansible-community/antsibull-core#10 to be merged first, otherwise CI will fail.