Modernize install #76021

sivel · 2021-10-12T16:45:25Z

SUMMARY

This PR:

modernizes our install, reducing dynamic code in setup.py, addition of setup.cfg
drops dependencies on distutils in setup.py
Switches to entry_points console_scripts instead of symlinked bin files
~~deprecates ansible.module_utils.ansible_release~~
- ~~This is done to largely remove symlinks within lib/, although it needed to be done anyway~~
- Reverted, out of scope for this PR

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

setup.cfg

ADDITIONAL INFORMATION

TODO:

test symlinks

lib/ansible/cli/scripts/ansible_cli_stub.py

mattclay · 2021-10-12T17:23:53Z

setup.cfg

@@ -0,0 +1,54 @@
+# Minimum target setuptools 39.0.1 to cover Ubuntu 18.04


Can you explain this comment (and the one below it) further?

Are they related to our minimum setuptools version in the Makefile?

ansible/Makefile

Line 186 in b121142

$(PYTHON) -c 'import setuptools, sys; sys.exit(int(not (tuple(map(int, setuptools.__version__.split("."))) > (39, 2, 0))))'

That entry in the Makefile is interesting, I didn't even know it was there. I added this comment operating off of the assumption that the community team will package ansible-core 2.13 for Ubuntu 18.04. However, Ubuntu 18.04 only has setuptools 39.0.1. Which means that I needed to target the minimum feature level to what was accessible in 39.0.1.

Given that this check is here, can we raise our minimum setuptools version?

setup.cfg

test/integration/targets/entry_points/runme.sh

lib/ansible/cli/__init__.py

mattclay · 2021-10-12T19:21:32Z

The _PY38_MIN = sys.version_info[:2] >= (3, 8) check could be simplified to _PY38_MIN = sys.version_info >= (3, 8).

test/integration/targets/entry_points/runme.sh

webknjaz · 2021-10-13T14:54:40Z

setup.cfg

+name = ansible-core
+version = attr: lib.ansible.release.__version__
+description = Radically simple IT automation
+long_description = file: README.rst


Wild idea: why not add some changelog pointers too?

Suggested change

long_description = file: README.rst

long_description = file: README.rst, docs/docsite/rst/porting_guides/porting_guide_core_2.13.rst

Here's an example project doing this: https://pypi.org/project/attrs/#release-information.

…ry_point

webknjaz · 2021-10-18T18:47:58Z

lib/ansible/__main__.py

+    dist = distribution('ansible-core')
+    ep_map = {ep.name: ep for ep in dist.entry_points if ep.group == 'console_scripts'}
+
+    parser = argparse.ArgumentParser(prog='python -m ansible', add_help=False)


It's not going to always be python. Sometimes it's python3.10 or python3 or ./path/to/venv/bin/python.
Maybe it could be something along the lines of

Suggested change

parser = argparse.ArgumentParser(prog='python -m ansible', add_help=False)

mod_path = Path(__file__)

if mod_path.parts[-1] == '__main__.py':

mod_path = f'{sys.executable} -m {(mod_path / "..").resolve().parts[-1]}'

parser = argparse.ArgumentParser(prog=str(mod_path), add_help=False)

meh, it's close enough for me. I'd rather not do gymnastics to figure it out.

I'm not going to push for this. Just remembered one case where pip (with a Fedora patch on top) used __main__.py in their output and that was rather confusing.

webknjaz · 2021-10-18T18:49:39Z

lib/ansible/cli/__init__.py

+    display = Display()
+except Exception as e:
+    print('ERROR: %s' % e, file=sys.stderr)
+    sys.exit(5)


Mind putting this magic value into a constant with a descriptive name?
I'd read much better if it was

Suggested change

sys.exit(5)

sys.exit(RC_CLI_INIT_FAILURE)

or something like that.

I think I might defer this to another PR later. I don't necessarily exactly know what rc 5 is supposed to indicate. I just copied it from the previous file. I know there is another PR attempting to better define the exit codes.

pyproject.toml

webknjaz · 2021-10-19T17:55:33Z

test/integration/targets/entry_points/runme.sh

+base_dir="$(dirname "$(dirname "$(dirname "$(dirname "${OUTPUT_DIR}")")")")"
+bin_dir="$(dirname "$(command -v pip)")"
+
+# deps are already installed, using --no-deps to avoid re-installing them


They won't be reinstalled without --no-deps either, unless there are conflicts or you also add -U. The only thing this will skip is the dependency tree resolution.

Yeah, the problem we ran into, is that the venv is created with --system-site-packages and even though they are accessible that way, pip still tries to install them into the venv, as if they weren't satisfied. The problem comes about with crazy dances we have to do in code to install the correct version of cryptography.

webknjaz · 2021-10-19T17:57:21Z

test/integration/targets/entry_points/runme.sh

+bin_dir="$(dirname "$(command -v pip)")"
+
+# deps are already installed, using --no-deps to avoid re-installing them
+pip install "${base_dir}" --disable-pip-version-check --no-deps


Consider using pip wheel or python -m build to create a dist and then pip install that, instead of installing from dir which is not something the end-users will trigger in the wild.

Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>

bin/ansible

lib/ansible/cli/__init__.py

lib/ansible/cli/adhoc.py

nitzmahone · 2021-10-18T23:58:46Z

setup.py

-# * https://github.com/pypa/setuptools/commit/bd110264
-from distutils.command.build_scripts import build_scripts as BuildScripts
-from distutils.command.sdist import sdist as SDist
+install_requires = (here / 'requirements.txt').read_text(encoding='utf-8').splitlines()


FYI, IIRC this is one of the things that would break in a zipfile where you'd have to move to pkgutil.get_data or some other managed package data access...

In the modern world, it'd be https://importlib-resources.rtfd.io/en/latest/using.html#file-system-or-zip-file

setup.py

mattclay reviewed Oct 12, 2021

View reviewed changes

lib/ansible/cli/__init__.py Outdated Show resolved Hide resolved

mattclay reviewed Oct 12, 2021

View reviewed changes

lib/ansible/cli/__init__.py Show resolved Hide resolved

mattclay reviewed Oct 12, 2021

View reviewed changes

test/integration/targets/entry_points/runme.sh Outdated Show resolved Hide resolved

webknjaz reviewed Oct 13, 2021

View reviewed changes

sivel added 7 commits October 18, 2021 12:30

Annotation improvements for ansible-test

9695cea

Address disallowed-name in ansible-console

8cd18dd

Omit several sanity tests from sdist that are only needed in CI

795fde9

Address shebang skips for examples/ in code

a413503

Exclusions for sanity tests not included in sdist

b87c094

changelog typo

37a70af

Test python -m ansible, remove --help to not conflict with actual ent…

febdd6c

…ry_point

sivel force-pushed the modernize-install branch from 4c3ce6c to febdd6c Compare October 18, 2021 17:31

Add pyproject.toml address issue with version attr

b293d78

webknjaz reviewed Oct 18, 2021

View reviewed changes

mattclay and others added 3 commits October 18, 2021 15:19

Use short names for entry points.

1a050c6

fix ansible-test editable install

5cf450b

Move scripts to setup.cfg, adjust entry_points test

594a3c3

mkrizek removed the needs_triage Needs a first human triage before being processed. label Oct 19, 2021

sivel and others added 3 commits October 19, 2021 10:51

Shorten line length with a helper function

746e700

Support ansible-test with -m usage.

a177bb4

Add some comments about import order

d3bc801

webknjaz reviewed Oct 19, 2021

View reviewed changes

pyproject.toml Outdated Show resolved Hide resolved

webknjaz reviewed Oct 19, 2021

View reviewed changes

constrain setuptools in pyproject.toml

01c0c4b

Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>

nitzmahone reviewed Oct 19, 2021

View reviewed changes

sivel added 2 commits October 19, 2021 13:09

Add zip_safe=False just to be careful for now

ed489b0

Move cli_executor to a classmethod

c09b6c0

nitzmahone approved these changes Oct 19, 2021

View reviewed changes

mattclay approved these changes Oct 19, 2021

View reviewed changes

sivel merged commit 66a8331 into ansible:devel Oct 19, 2021

ansible locked and limited conversation to collaborators Nov 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize install #76021

Modernize install #76021

sivel commented Oct 12, 2021 •

edited

Loading

mattclay Oct 12, 2021

sivel Oct 12, 2021

mattclay Oct 12, 2021

mattclay commented Oct 12, 2021

webknjaz Oct 13, 2021

webknjaz Oct 18, 2021

sivel Oct 18, 2021

webknjaz Oct 18, 2021

webknjaz Oct 18, 2021

sivel Oct 18, 2021

webknjaz Oct 19, 2021

sivel Oct 19, 2021

webknjaz Oct 19, 2021

webknjaz Oct 19, 2021

nitzmahone Oct 18, 2021

webknjaz Oct 19, 2021

		@@ -0,0 +1,54 @@
		# Minimum target setuptools 39.0.1 to cover Ubuntu 18.04

	long_description = file: README.rst
	long_description = file: README.rst, docs/docsite/rst/porting_guides/porting_guide_core_2.13.rst

-    parser = argparse.ArgumentParser(prog='python -m ansible', add_help=False)
+    mod_path = Path(__file__)
+    if mod_path.parts[-1] == '__main__.py':
+        mod_path = f'{sys.executable} -m {(mod_path / "..").resolve().parts[-1]}'
+    parser = argparse.ArgumentParser(prog=str(mod_path), add_help=False)

Modernize install #76021

Modernize install #76021

Conversation

sivel commented Oct 12, 2021 • edited Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattclay commented Oct 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sivel commented Oct 12, 2021 •

edited

Loading