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.

Sign up for GitHub

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

Store python_requires information instead of guessing it #449

Merged
merged 3 commits into from
Oct 7, 2022
Merged

Store python_requires information instead of guessing it #449

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69bac17
Store python_requires information instead of guessing it.
felixfontein Oct 3, 2022
aeeae51
Fix variable name.
felixfontein Oct 3, 2022
929620a
Do not load deps file as a YAML file, but parse appropriately.
felixfontein Oct 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelogs/fragments/449-python_requires.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
minor_changes:
- "Now requires antsibull-core >= 1.3.0 (https://github.com/ansible-community/antsibull/pull/449)."
- "The ``python_requires`` information is now extracted from ansible-core and stored in the ``.build`` and ``.deps`` files instead of guessing it from the Ansible version (https://github.com/ansible-community/antsibull/pull/449)."
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ antsibull-lint = "antsibull.cli.antsibull_lint:main"
[tool.poetry.dependencies]
python = "^3.6.1"
antsibull-changelog = ">= 0.14.0"
antsibull-core = ">= 1.2.0, < 2.0.0"
antsibull-core = ">= 1.3.0, < 2.0.0"
antsibull-docs = ">= 1.0.0, < 2.0.0"
asyncio-pool = "*"
jinja2 = "*"
Expand Down
42 changes: 35 additions & 7 deletions src/antsibull/build_ansible_commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,8 @@ def write_setup(ansible_version: PypiVer,
ansible_core_version: PypiVer,
collection_exclude_paths: t.List[str],
collection_deps: str,
package_dir: str) -> None:
package_dir: str,
python_requires: str) -> None:
setup_filename = os.path.join(package_dir, 'setup.py')

setup_tmpl = Template(get_antsibull_data('ansible-setup_py.j2').decode('utf-8'))
Expand All @@ -202,6 +203,7 @@ def write_setup(ansible_version: PypiVer,
ansible_core_version=ansible_core_version,
collection_exclude_paths=collection_exclude_paths,
collection_deps=collection_deps,
python_requires=python_requires,
PypiVer=PypiVer,
)

Expand All @@ -215,12 +217,13 @@ def write_python_build_files(ansible_version: PypiVer,
collection_deps: str,
package_dir: str,
release_notes: t.Optional[ReleaseNotes] = None,
debian: bool = False) -> None:
debian: bool = False,
python_requires: str = '>=3.8') -> None:
copy_boilerplate_files(package_dir)
write_manifest(package_dir, release_notes, debian)
write_setup(
ansible_version, ansible_core_version, collection_exclude_paths, collection_deps,
package_dir)
package_dir, python_requires)


def write_debian_directory(ansible_version: PypiVer,
Expand Down Expand Up @@ -345,13 +348,32 @@ def _is_alpha(version: PypiVer) -> bool:
return version.is_prerelease and pre is not None and pre[0] == 'a'


def _extract_python_requires(ansible_core_version: PypiVer, deps: t.Dict[str, str]) -> str:
felixfontein marked this conversation as resolved.
Show resolved Hide resolved
python_requires = deps.pop('_python', None)
if python_requires is not None:
return python_requires
if ansible_core_version < PypiVer('2.12.0a'):
# Ansible 2.9, ansible-base 2.10, and ansible-core 2.11 support Python 2.7 and Python 3.5+
return '>=2.7,!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*,!=3.4.*'
if ansible_core_version < PypiVer('2.14.0a'):
# ansible-core 2.12 and 2.13 support Python 3.8+
return '>=3.8'
if ansible_core_version < PypiVer('2.15.0a'):
# 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'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do old versions of pip (that handle python_requires at 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree. Ansible is tightly coupled to ansible-core, so it exposing the same python_requires as 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 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.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

@webknjaz webknjaz Oct 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

' dependency information')


def prepare_command() -> int:
app_ctx = app_context.app_ctx.get()

build_filename = os.path.join(app_ctx.extra['data_dir'], app_ctx.extra['build_file'])
build_file = BuildFile(build_filename)
build_ansible_version, ansible_core_version, deps = build_file.parse()
ansible_core_version_obj = PypiVer(ansible_core_version)
python_requires = _extract_python_requires(ansible_core_version_obj, deps)

# If we're building a feature frozen release (betas and rcs) then we need to
# change the upper version limit to not include new features.
Expand Down Expand Up @@ -417,7 +439,8 @@ def prepare_command() -> int:
deps_file.write(
dependency_data.ansible_version,
dependency_data.ansible_core_version,
dependency_data.deps)
dependency_data.deps,
python_requires=python_requires)

# Write Galaxy requirements.yml file
galaxy_filename = os.path.join(app_ctx.extra['dest_data_dir'], app_ctx.extra['galaxy_file'])
Expand Down Expand Up @@ -473,6 +496,8 @@ def rebuild_single_command() -> int:
deps_filename = os.path.join(app_ctx.extra['data_dir'], app_ctx.extra['deps_file'])
deps_file = DepsFile(deps_filename)
dependency_data = deps_file.parse()
python_requires = _extract_python_requires(
PypiVer(dependency_data.ansible_core_version), dependency_data.deps)

# Determine included collection versions
ansible_core_version = PypiVer(dependency_data.ansible_core_version)
Expand Down Expand Up @@ -545,7 +570,7 @@ def rebuild_single_command() -> int:
write_build_script(app_ctx.extra['ansible_version'], ansible_core_version, package_dir)
write_python_build_files(app_ctx.extra['ansible_version'], ansible_core_version,
collection_exclude_paths, '', package_dir, release_notes,
app_ctx.extra['debian'])
app_ctx.extra['debian'], python_requires)
if app_ctx.extra['debian']:
write_debian_directory(app_ctx.extra['ansible_version'], ansible_core_version,
package_dir)
Expand Down Expand Up @@ -651,6 +676,7 @@ def build_multiple_command() -> int:
build_file = BuildFile(build_filename)
build_ansible_version, ansible_core_version, deps = build_file.parse()
ansible_core_version_obj = PypiVer(ansible_core_version)
python_requires = _extract_python_requires(ansible_core_version_obj, deps)

# TODO: implement --feature-frozen support

Expand Down Expand Up @@ -693,7 +719,8 @@ def build_multiple_command() -> int:
collection_deps_str = '\n' + ',\n'.join(collection_deps)
write_build_script(app_ctx.extra['ansible_version'], ansible_core_version_obj, package_dir)
write_python_build_files(app_ctx.extra['ansible_version'], ansible_core_version_obj,
[], collection_deps_str, package_dir)
[], collection_deps_str, package_dir,
python_requires=python_requires)

make_dist(package_dir, app_ctx.extra['sdist_dir'])

Expand All @@ -703,6 +730,7 @@ def build_multiple_command() -> int:
deps_file.write(
str(app_ctx.extra['ansible_version']),
str(ansible_core_version_obj),
{collection: str(version) for collection, version in included_versions.items()})
{collection: str(version) for collection, version in included_versions.items()},
python_requires=python_requires)

return 0
7 changes: 1 addition & 6 deletions src/antsibull/data/ansible-setup_py.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,12 +173,7 @@ setup(
'Source Code': 'https://github.com/ansible/ansible',
},
license='GPLv3+',
{%- if version.major < 5 %}
python_requires='>=2.7,!=3.0.*,!=3.1.*,!=3.2.*,!=3.3.*,!=3.4.*',
{%- else %}
{# Ansible X depends on ansible-core 2.(X+7), which depends on Python 3.(floor((X+11)/2)) - at least for now :) #}
python_requires='>=3.{{ ((version.major + 11) / 2) | round(0, 'floor') }}',
{%- endif %}
python_requires='{{ python_requires }}',
packages=['ansible_collections'],
{% if version.major >= 6 and collection_exclude_paths %}
exclude_package_data={
Expand Down
18 changes: 15 additions & 3 deletions src/antsibull/new_ansible.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import aiohttp
import asyncio_pool # type: ignore[import]
from packaging.version import Version as PypiVer
import semantic_version as semver

from antsibull_core import app_context
Expand All @@ -36,7 +37,7 @@ async def get_version_info(collections, pypi_server_url):
lib_ctx = app_context.lib_ctx.get()
async with asyncio_pool.AioPool(size=lib_ctx.thread_max) as pool:
pypi_client = AnsibleCorePyPiClient(aio_session, pypi_server_url=pypi_server_url)
requestors['_ansible_core'] = await pool.spawn(pypi_client.get_versions())
requestors['_ansible_core'] = await pool.spawn(pypi_client.get_release_info())
galaxy_client = GalaxyClient(aio_session)

for collection in collections:
Expand Down Expand Up @@ -91,13 +92,24 @@ def new_ansible_command():
os.path.join(app_ctx.extra['data_dir'], app_ctx.extra['pieces_file']))
dependencies = asyncio.run(get_version_info(collections, app_ctx.pypi_url))

ansible_core_version = dependencies.pop('_ansible_core')[0]
ansible_core_release_infos = dependencies.pop('_ansible_core')
ansible_core_versions = [
(PypiVer(version), data[0]['requires_python'])
for version, data in ansible_core_release_infos.items()
]
ansible_core_versions.sort(reverse=True, key=lambda tuple: tuple[0])
felixfontein marked this conversation as resolved.
Show resolved Hide resolved

ansible_core_version, python_requires = ansible_core_versions[0]
dependencies = find_latest_compatible(
ansible_core_version, dependencies, allow_prereleases=app_ctx.extra['allow_prereleases'])

build_filename = os.path.join(app_ctx.extra['dest_data_dir'], app_ctx.extra['build_file'])
build_file = BuildFile(build_filename)
build_file.write(app_ctx.extra['ansible_version'], ansible_core_version, dependencies)
build_file.write(
app_ctx.extra['ansible_version'],
ansible_core_version,
dependencies,
python_requires=python_requires)

changelog = ChangelogData.ansible(app_ctx.extra['dest_data_dir'])
changelog.changes.save()
Expand Down