build: migrate to pyproject.toml

[tonyandrewmeyer]

Modernisation of distribution and build tooling: this PR attempts to bring ops up-to-date with current packaging best practice. The particular focus is on moving away from setup.py and using pyproject.toml as the source of truth for configuration, with a PEP 517/PEP 518 compliant build system.

Metadata

The project metadata has moved from setup.py to pyproject.toml. This is generally a simple move, except:

• The author name and email has been updated from Charmcraft (likely a bad copy-and-paste) to Charm Tech.
• pyproject.toml allows for more project URLs, so "Homepage", "Repository", "Issues", "Documentation", and "Changelog" have been included.

A few test_infra tests were removed as a result, as they were previously validating custom code in setup.py and now that functionality is provided by the build backend (for example, including the contents of the README).

Source distribution changes

I've added a MANIFEST.in file to more explicitly define which files are included. There are more files than previously, however:

• CHANGES.md
• CODE_OF_CONDUCT.md (this is more for interacting, so could be excluded, but it's linked from the README so seems wrong to not be included)
• HACKING.md
• test/bin, test/charms, test/smoke
• test/pebble_cli.py
• tox.ini

Dependency management

The list of dependencies has moved from setup.py and requirements.txt to pyproject.toml. We no longer keep two lists of the dependencies in sync, so a test_infra test can be removed. The dev requirements have been split out into groups for each tox environment, and are located in tox.ini.

The documentation dependencies have moved from docs/requirements.in to pyproject.toml in an extra-dependencies section. The docs/requirements.txt file can be generated using pip-compile, which removes the need for the (undocumented) docs/update_requirements.sh script. tox -e docs will also run the pip-compile step, so normally contributors should not need to install and run pip-compile themselves, just do the normal steps of running tox -e docs to locally inspect the docs, and commit the updated lock file if there are changes.

If anyone is relying on requirements.txt or requirements-dev.txt to exist (e.g. as we do with the CI that tests against key charms) that will break, but it seems unlikely that anyone is downstream doing that.

CI changes

• We now verify that building and (more importantly) installing works on a matrix of macOS and Ubuntu in Pythons 3.8-3.12. This was previously only Ubuntu and only 3.11 (until recently, whatever Python version the GitHub Action defaulted to). It's unlikely that installing will break for macOS only or for specific Python versions, but possible.
• We now use build as the build frontend (setuptools remains the build backend) for building distributions to publish to PyPI (ideally we have access to test.pypi.org in order to verify that this works correctly before merging). Also moves back to using the default GitHub Action Python version, since we are no longer impacted by the distutils removal.
• Similarly, uses build as the build frontend for validating that building works correctly.

Doc changes

• Expanded HACKING.md to include more detailed information about the tools that we use for development. The "Dependencies" section was also a sub-section of the "Documentation" section, which I think was an error, so promote it to top-level.

Version

In ops 2.8, ops.__version__ is:

• <tag>-<#commits>-g<hex>[-dirty] (or just <tag> if there are no local commits) if there is a .git folder and git describe --tags --dirty runs (note that this means that importing ops from a Git clone will always spawn a git subprocess)
• 1.0.dev0+unknown if running from a non-built source (e.g. a GitHub tarball).
• <tag> if running from a built source (e.g. from PyPI), or <tag>-<#commits>-g<hex>[-dirty] if running from a 'dirty' built source (e.g. a local python setup.py sdist) (note that this is from a static file generated in the build process and does not spawn any subprocess)

This PR replaces that with a much simpler system:

• ops/version.py has a static, manually managed, version string (in this module for backwards compatibility).
• Prior to publishing a release, the release manager gets a PR merged that sets that string to the appropriate value.
• Immediately after publishing a release, the release manager gets a PR merged that sets that string to the expected next release, with .dev0 appended.

Further changes

We expect to also change from using pyflakes (and extensions), isort, and (potentially autopep8) to using ruff. Either as part of that change, or as a further follow-up, I intend to propose an (optional) pre-commit configuration that would optionally automatic some of the tooling (both the formatting and linting, and also the pip-tools management introduced in this PR).

Fixes #893, #1039

[tonyandrewmeyer]

A bit of an essay, sorry @benhoyt 😞. If we can decide where to go with the version I can cut that section way down and the rest is more straightforward. Happy to (voice) chat on this if that's simpler, of course.

[tonyandrewmeyer]

Hmm, failures might be because of the pinned versions not being available for the Python versions (everything passes locally). Will look into that and ensure things are passing before un-drafting.

[benhoyt]

Thanks for the details. Yeah, let's discuss voice when you're back. I lean towards committing version.py (or just having __version__ in ops/__init__.py) and either automating that or leaving it a manual step.

[benhoyt]

Per discussion, we're going to go with the straight version string in ops/version.py. Then the release will be a small 3-step manual process:

1. Update CHANGES.md and bump the version string and commit.
2. Create the GitHub release (which also tags it).
3. After the release is done, change the version string to 2.<next>.0-dev, e.g., 2.11.0-dev.

[benhoyt]

I wonder -- should/could we make these tox actions, rather than having the developer manually install pip-compile and enter these commands (also below)?

[tonyandrewmeyer]

It's only required when you actually want to bump a dependency, but yes a tool is likely better.

We could have it like this instead, avoiding any lock file at all:

[testenv:docs]
description = Build the Sphinx docs
deps = .[docs]
commands =
    sphinx-build -W --keep-going docs/ docs/_build/html

The biggest issue is that if the dependencies change, pip doesn't pick up on that, and you generally have to blow away the .tox/env folder, which is not much better than running a command.

We could have the dev dependencies in tox.ini, particularly since they're being spilt out, and especially if we don't group many of them (or if there is a way to say "use this same dependency list for multiple envs", other than having them in another file). One advantage of having them in standard places is that tools (like security scanners) easily find them, though, and I'm not sure if they would there.

Or I can just do as you suggest and we keep the generated files (probably not in version control, per a different thread) and I can add a tox -e update-deps (name to be bikesheded later).

[tonyandrewmeyer]

Just having the .[unit] style deps value in tox.ini is no good - it avoids needing any compilation of what's in pyproject.toml but the failure to pick up on changes is too inconvenient.

In terms of speed (I did these 3 times and picked the median - not super accurate, but should be enough to spot order-of-magnitude differences), for a full tox:

	Clean	Existing env
current main	1m47s	1m15s
compile & sync	1m38s	1m27s
sync only	1m23s	1m12s
deps-in-tox.ini	1m19s	1m12s

However, most of the time is (thankfully!) in the tests. For a quick env, like lint:

	Clean	Existing env
current main	13s	3s
compile & sync	11s	8s
sync only	6s	3s
deps-in-tox.ini	5s	3s

I did some more investigation, and dependabot will find dependencies in pyproject.toml without having a lock file committed, but won't find them in tox.ini. Other tools seem mixed, but as far as I know we aren't using any at the moment, so I guess we could address that when/if that changes.

I don't want to be significantly slower than current main. So that eliminates having both the pip-compile and pip-sync in tox.ini.

Having the dependencies in optional-dependencies sections in pyproject.toml is recommended in some places and in others it seems like that's used only for optional features rather than dev dependencies. Being able to do a local install with pip install .[static,unit] is nice, but it seems like the vast majority of the time people will be using tox, and doing pip install ops[unit] doesn't seem like it's of use. Having requirements files generated seems inconvenient if we're not using them for anything else.

So, on balance, I think putting them directly in tox.ini is the cleanest for now, and we can reconsider this if the scanning issue ever comes up.

Except for the docs: readthedocs doesn't have tox, so we do need to have a generated requirements.txt file for there.

[benhoyt]

It seems weird to me that we have to both include some dirs and exclude others (above). Why is that? Would it be more explicit to exclude everything and then start including explicit files/dirs?

[tonyandrewmeyer]

The recommended approach is to include things broadly and then exclude them. The excludes remove from the collected list so need to be after the includes (I had this wrong originally).

This is mostly a leftover from the earlier version of the PR where I was going to use setuptools-scm for the version. I'm not sure we want to bother with this now. The difference between the default of not having a MANIFEST.in and having the current one is just this:

Only in implicit/ops-2.10.0.dev0: docs
Only in implicit/ops-2.10.0.dev0: .github
Only in explicit/ops-2.10.0.dev0/test/charms/test_main/lib: ops

The last one is interesting - it's not a file, it's a link. The default behaviour is that it's just skipped, but if it's included then it actually follows the link and there's a second copy of ops in that location in the tar.gz. Neither of these seem correct, but the latter seems a bit better in that I assume everything would still work for the test in that case, whereas it would not with it missing.

I feel that if you get the sdist then you're not looking for the .github folder, or the content of the docs folder. But it's also not much of an issue to have them there and the release bundle that Github makes presumably includes them.

I lean towards keeping the MANIFEST.in as it is now, but am open to arguments that we just get rid of it.

[benhoyt]

Do we need to have this file, or commit it to version control? I get it for requirements.txt, but do we need the -dev one?

[tonyandrewmeyer]

No, and actually even the requirements.txt isn't strictly needed. I did more reading on this, and there seems to be mixed opinions on putting the lock file in source control when (a) it's for a library and (b) it's Python.

If we consider what value it's providing: it's saying what dependencies were used when CI ran the tests, particularly when a build was done. However, we have that information in the runner output as well. If downstream users want to know what dependencies are being used then it's not our lock file that matters, it's theirs. So it's really only for people working on ops, to avoid inconsistent behaviour due to their environments being different. It seems like we would probably actually want to hit that problem so that we know that it should be handled (e.g. avoid some specific version) and can figure it out by just comparing freeze output.

So, on balance, I think I'm back to not having these in source control at all (which is what I had in some early version of this PR).

[benhoyt]

I wonder if we can improve things here while we're at it. I introduced this change to put all the tox tasks in a single requirements-dev.txt, so it's my fault, but it's a bit annoying as it means if you change the pyright version (say) tox -e unit also has to rebuild the environment. I wonder if there's either a way to share the tox environment between all dev tasks, or to separate the deps (so tox -e static only depends only pyright)?

Also happy to just do this later in a separate PR if we care to -- just food for thought.

[tonyandrewmeyer]

I'm happy to include this change - I agree that it's probably better. It does mean there's a bit of duplication, ie. I've got this now:

[project.optional-dependencies]
docs = [
    "sphinx==6.2.1",
    "sphinx-design",
    "furo",
    "sphinx-tabs",
    "lxd-sphinx-extensions",
    "sphinx-copybutton",
]
lint = [
    "autopep8~=1.6",
    "flake8~=6.1",
    "flake8-docstrings~=1.7",
    "flake8-builtins~=2.1",
    "isort~=5.11",
    "pep8-naming~=0.13",
    "pyproject-flake8~=6.1",
]
static = [
    "pyright==1.1.345",
    "pytest~=7.2",
    "typing_extensions~=4.2",
]
unit = [
    "coverage[toml]~=7.0",
    "pytest~=7.2",
    "typing_extensions~=4.2",
]
smoke = [
    "coverage[toml]~=7.0",
    "pytest~=7.2",
    "pytest-operator~=0.23",
]

(I have tested all except smoke).

I could group pytest, typing_extensions, and coverage into "test" but I don't think there's a way to say "this group requires this other group as well" - obviously we can handle that in tox.ini for our use and anyone just running from tox, but maybe it becomes unclear to other people?

Or I could decrease the number of groups (I already have fmt and lint use the same one), so static, unit, and smoke all use the same one? static needs pytest because we type check the tests, I think.

[benhoyt]

Let's keep them separate and simple for now (even if a little bit of duplication). We can always tweak it later.

Out of interest, why does static require pytest and typing_extensions?

[tonyandrewmeyer]

Out of interest, why does static require pytest and typing_extensions?

typing_extensions because static is checking types 😄 - it's the library that backports Python typing to older Python versions (we only use Required and NotRequired in TypedDicts currently, I think). I believe unit needs it as well because pytest (or unittest) sets typing.TYPE_CHECKING to True.

static needs pytest not for the tool but because it's imported (in the tests, which we are also type-checking). These build on top of the main dependencies, so the core ones are already there.

[benhoyt]

For some reason this tox -e docs fails for me, with this output (even after a rm -rf .tox):

$ tox -e docs
docs: commands_pre[0]> pip-compile --extra=docs -o docs/requirements.txt pyproject.toml
Backend subprocess exited when trying to invoke get_requires_for_build_wheel
Failed to parse /home/ben/w/operator/pyproject.toml
docs: exit 2 (3.59 seconds) /home/ben/w/operator> pip-compile --extra=docs -o docs/requirements.txt pyproject.toml pid=459533
  docs: FAIL code 2 (3.60=setup[0.02]+cmd[3.59] seconds)
  evaluation failed :( (3.63 seconds)

However, the first time I tried it it worked! (maybe due to existing stuff in .tox/docs?).

Also, I noticed that when it did work, after running it I had local mods to docs/requirements.txt with it saying "This file was autogenerated ... with Python 3.11". Seems awkward to have the Python version in there.

[tonyandrewmeyer]

Hmm, if I blow away .tox and docs/_build then it works both first try and then second. If I blow them away, do a tox -e docs with main and then (without removing anything, other than what git does) switch to the branch and do tox -e docs again, it also works.

This might be because you're using (I think) Python 3.8 and I used 3.11. The readthedocs config is set to use Python 3.11, but unless we actually require that it's probably better to pin at whatever 3.8 supports.

I've regenerated it with Python 3.8. Does it work for you now?

Seems awkward to have the Python version in there.

This is part of the pip-compile output - I don't see a config option to not have it there. We don't have to use pip-tools for this - I think it is currently the best tool that works well with both pyproject.toml and tox, but there are other options:

1. We could have it do the same thing as the readthedocs CI, pip install .[docs]. The only downside is that if you change a docs requirement in pyproject.toml and have an existing .tox/docs it won't pick up the change (this isn't an issue for the CI runner where it's always a fresh environment).
2. We could go back to having an update_requirements.sh script for the docs (but document that this time 😄).
3. We could use something that's not pip-tools to generate the requirements.txt file (for our purposes it can be as simple as python -c "import pytoml;print('\n'.join(pytoml.load(open('pyproject.toml'))['project']['optional-dependencies']['docs']))")
4. We could duplicate the dependency list in tox.ini and pyproject.toml and add some tooling to make sure they don't get out of sync.
5. Something else :)

Any preference?

[benhoyt]

Weird, I couldn't figure out the exact reason for the failure out on my machine. I ended up cloning the project to a new, and then it worked, so I figured it was something cached in my other dir. I ended up doing a git clean -d -x -f and now it's working again...

Now that it's working, I think it's probably fine as is, thanks.

[benhoyt]

Thanks Tony, looking good to me. This is "just" CI and building tooling stuff, so merging without further review. If it needs further tweaks, we can do that in follow-up PRs.