pyproject-transition #922

Merged
witten merged 7 commits from kaliko/borgmatic:pyproject-transition into main 2024-10-26 23:38:35 +00:00
Contributor

Moving to pyproject.toml

Moving to pyproject.toml
kaliko added 3 commits 2024-10-14 16:15:40 +00:00
kaliko force-pushed pyproject-transition from 806d544fca to 9e29ce788f 2024-10-14 16:17:32 +00:00 Compare
kaliko added 2 commits 2024-10-14 16:33:28 +00:00
kaliko added 1 commit 2024-10-15 07:47:07 +00:00
Author
Contributor

I'm not sure how PyCQA is handled within borgmatic project. The doc suggest tox is the main entrypoint.

Then I think pycqa config could be gathered within tox.ini conf file when pyproject.toml is not supported.

Here are the last 2 tools I did not merge in pyproject.toml:

flake8

No support for pyproject.toml, but there are currently two locations with flake8 directives within the project:

  • setup.cfg
  • .flake8

Why not merge the conf in .flake8 or tox.ini ?

pycodestyle

No support for pyproject.toml. Keep config in setup.cfg or move it to tox.ini ?

I'm not sure how PyCQA is handled within borgmatic project. The [doc](https://torsion.org/borgmatic/docs/how-to/develop-on-borgmatic/#automated-tests) suggest tox is the main entrypoint. Then I think pycqa config could be gathered within `tox.ini` conf file when `pyproject.toml` is not supported. Here are the last 2 tools I did not merge in `pyproject.toml`: ### flake8 No support for pyproject.toml, but there are currently two locations with flake8 directives within the project: - `setup.cfg` - `.flake8` Why not merge the conf in `.flake8` or `tox.ini` ? ### pycodestyle No support for pyproject.toml. Keep config in `setup.cfg` or move it to `tox.ini` ?
kaliko force-pushed pyproject-transition from c7cefd5922 to b029d1cb67 2024-10-15 10:52:34 +00:00 Compare
witten reviewed 2024-10-22 16:47:59 +00:00
witten left a comment
Owner

Thank you so much for taking the time to do all this! I've played around with this a little and left my thoughts/questions in the comments.

Thank you so much for taking the time to do all this! I've played around with this a little and left my thoughts/questions in the comments.
@ -4,0 +58,4 @@
skip = ".tox"
[tool.codespell]
skip = ".git,.tox,build"
Owner

This pyproject.toml looks good to me. But it seems to result in an unimportable borgmatic module, I'm guessing because setup.py is gone and something is still relying on it to consider borgmatic a module.

For instance, when I run tests, a particular test that tries to run the borgmatic executable fails:

$ tox
...
E               subprocess.CalledProcessError: Command '('borgmatic', '--version')' returned non-zero exit status 1.

/usr/lib/python3.12/subprocess.py:571: CalledProcessError
-------------------------------------------------------------- Captured stderr call --------------------------------------------------------------
Traceback (most recent call last):
  File "/home/witten/Downloads/borgmatic/.tox/py312/bin/borgmatic", line 5, in <module>
    from borgmatic.commands.borgmatic import main
ModuleNotFoundError: No module named 'borgmatic.commands'
...

And I can reproduce this by manually running borgmatic in the virtualenv:

$ . .tox/py312/bin/activate
$ borgmatic --version
Traceback (most recent call last):
  File "/home/witten/Downloads/borgmatic/.tox/py312/bin/borgmatic", line 5, in <module>
    from borgmatic.commands.borgmatic import main
ModuleNotFoundError: No module named 'borgmatic.commands'

So I'm not sure what's missing here.

This `pyproject.toml` looks good to me. But it seems to result in an unimportable `borgmatic` module, I'm guessing because `setup.py` is gone and something is still relying on it to consider `borgmatic` a module. For instance, when I run tests, a particular test that tries to run the `borgmatic` executable fails: ```bash $ tox ... E subprocess.CalledProcessError: Command '('borgmatic', '--version')' returned non-zero exit status 1. /usr/lib/python3.12/subprocess.py:571: CalledProcessError -------------------------------------------------------------- Captured stderr call -------------------------------------------------------------- Traceback (most recent call last): File "/home/witten/Downloads/borgmatic/.tox/py312/bin/borgmatic", line 5, in <module> from borgmatic.commands.borgmatic import main ModuleNotFoundError: No module named 'borgmatic.commands' ... ``` And I can reproduce this by manually running borgmatic in the virtualenv: ```bash $ . .tox/py312/bin/activate $ borgmatic --version Traceback (most recent call last): File "/home/witten/Downloads/borgmatic/.tox/py312/bin/borgmatic", line 5, in <module> from borgmatic.commands.borgmatic import main ModuleNotFoundError: No module named 'borgmatic.commands' ``` So I'm not sure what's missing here.
Author
Contributor

Fixed with 601e393ec7 (pyproject/build failed to ship submodules).

Fixed with 601e393ec7 (pyproject/build failed to ship submodules).
witten marked this conversation as resolved
scripts/release Outdated
@ -34,4 +34,2 @@
# Build borgmatic and publish to pypi.
rm -fr dist
python3 setup.py bdist_wheel
python3 setup.py sdist
Owner

Something is missing from the result tarball. Here's what I'm seeing on my machine:

$ tar tfz dist/borgmatic-1.9.0.dev0.tar.gz
borgmatic-1.9.0.dev0/
borgmatic-1.9.0.dev0/AUTHORS
borgmatic-1.9.0.dev0/LICENSE
borgmatic-1.9.0.dev0/MANIFEST.in
borgmatic-1.9.0.dev0/PKG-INFO
borgmatic-1.9.0.dev0/README.md
borgmatic-1.9.0.dev0/borgmatic/
borgmatic-1.9.0.dev0/borgmatic/__init__.py
borgmatic-1.9.0.dev0/borgmatic/config/
borgmatic-1.9.0.dev0/borgmatic/config/schema.yaml
borgmatic-1.9.0.dev0/borgmatic/execute.py
borgmatic-1.9.0.dev0/borgmatic/logger.py
borgmatic-1.9.0.dev0/borgmatic/signals.py
borgmatic-1.9.0.dev0/borgmatic/verbosity.py
borgmatic-1.9.0.dev0/borgmatic.egg-info/
borgmatic-1.9.0.dev0/borgmatic.egg-info/PKG-INFO
borgmatic-1.9.0.dev0/borgmatic.egg-info/SOURCES.txt
borgmatic-1.9.0.dev0/borgmatic.egg-info/dependency_links.txt
borgmatic-1.9.0.dev0/borgmatic.egg-info/entry_points.txt
borgmatic-1.9.0.dev0/borgmatic.egg-info/requires.txt
borgmatic-1.9.0.dev0/borgmatic.egg-info/top_level.txt
borgmatic-1.9.0.dev0/pyproject.toml
borgmatic-1.9.0.dev0/sample/
borgmatic-1.9.0.dev0/sample/systemd/
borgmatic-1.9.0.dev0/sample/systemd/borgmatic-user.service
borgmatic-1.9.0.dev0/sample/systemd/borgmatic-user.timer
borgmatic-1.9.0.dev0/sample/systemd/borgmatic.service
borgmatic-1.9.0.dev0/sample/systemd/borgmatic.timer
borgmatic-1.9.0.dev0/setup.cfg

Notice how most of the source subdirectories are missing: borgmatic/actions/, borgmatic/borg/, etc.

Something is missing from the result tarball. Here's what I'm seeing on my machine: ``` $ tar tfz dist/borgmatic-1.9.0.dev0.tar.gz borgmatic-1.9.0.dev0/ borgmatic-1.9.0.dev0/AUTHORS borgmatic-1.9.0.dev0/LICENSE borgmatic-1.9.0.dev0/MANIFEST.in borgmatic-1.9.0.dev0/PKG-INFO borgmatic-1.9.0.dev0/README.md borgmatic-1.9.0.dev0/borgmatic/ borgmatic-1.9.0.dev0/borgmatic/__init__.py borgmatic-1.9.0.dev0/borgmatic/config/ borgmatic-1.9.0.dev0/borgmatic/config/schema.yaml borgmatic-1.9.0.dev0/borgmatic/execute.py borgmatic-1.9.0.dev0/borgmatic/logger.py borgmatic-1.9.0.dev0/borgmatic/signals.py borgmatic-1.9.0.dev0/borgmatic/verbosity.py borgmatic-1.9.0.dev0/borgmatic.egg-info/ borgmatic-1.9.0.dev0/borgmatic.egg-info/PKG-INFO borgmatic-1.9.0.dev0/borgmatic.egg-info/SOURCES.txt borgmatic-1.9.0.dev0/borgmatic.egg-info/dependency_links.txt borgmatic-1.9.0.dev0/borgmatic.egg-info/entry_points.txt borgmatic-1.9.0.dev0/borgmatic.egg-info/requires.txt borgmatic-1.9.0.dev0/borgmatic.egg-info/top_level.txt borgmatic-1.9.0.dev0/pyproject.toml borgmatic-1.9.0.dev0/sample/ borgmatic-1.9.0.dev0/sample/systemd/ borgmatic-1.9.0.dev0/sample/systemd/borgmatic-user.service borgmatic-1.9.0.dev0/sample/systemd/borgmatic-user.timer borgmatic-1.9.0.dev0/sample/systemd/borgmatic.service borgmatic-1.9.0.dev0/sample/systemd/borgmatic.timer borgmatic-1.9.0.dev0/setup.cfg ``` Notice how most of the source subdirectories are missing: `borgmatic/actions/`, `borgmatic/borg/`, etc.
Author
Contributor

Package discovery was broken indeed and somehow my test failed to raise the issue (some leftovers interfered I guess). Anyway I fixed this in 601e393ec7.

I'm hoping to have more time next week to work on your comments,
Thanks for the review.

Package discovery was broken indeed and somehow my test failed to raise the issue (some leftovers interfered I guess). Anyway I fixed this in 601e393ec7. I'm hoping to have more time next week to work on your comments, Thanks for the review.
Owner

Cool, yeah, the tarball contents look much better now!

Cool, yeah, the tarball contents look much better now!
witten marked this conversation as resolved
@ -35,3 +35,2 @@
rm -fr dist
python3 setup.py bdist_wheel
python3 setup.py sdist
python3 -m build
Owner

When I try this inside a Python 3 virtualenv, the command doesn't appear to work:

$ python3 -m build
/home/witten/Downloads/borgmatic/.tox/py312/bin/python3: No module named build

Looks like it might need to be added to test_requirements.txt? And/or to [build-system] in pyproject.toml?

When I try this inside a Python 3 virtualenv, the command doesn't appear to work: ``` $ python3 -m build /home/witten/Downloads/borgmatic/.tox/py312/bin/python3: No module named build ``` Looks like [it might need to be added]( https://build.pypa.io/en/stable/installation.html) to `test_requirements.txt`? And/or to `[build-system]` in `pyproject.toml`?
Author
Contributor

I believe there are two different issues here. One regarding tox and another one concerning the release script.

My understanding of recent python packaging is that build module is only a build frontend supporting pyproject.toml to call the build-system (cf. doc here).

I believe we don't' need to tell tox to use it, and apparently tox can build the package without it, probably using plain pip (an alternative build frontend ).

Then there is no need to add build module to test_requirements.txt IMHO.

But I fixed scripts/release to use it instead setup.py.
Then I guess we should add build dependency somewhere in the release process.
Could be test_requirements.txt if you run the script within a tox venv, but I believe it's better to release from a pristine environment to avoid side effects of pre-populated venv with tons of deps for test/coverage/lint/etc….

But this is something I let you decide, I don't know how you release borgmatic.

I believe there are two different issues here. One regarding **tox** and another one concerning the **release script**. My understanding of recent python packaging is that ``build`` module is only a build frontend supporting ``pyproject.toml`` to call the ``build-system`` (cf. [doc here](https://packaging.python.org/en/latest/tutorials/packaging-projects/#choosing-a-build-backend)). I believe we don't' need to tell tox to use it, and apparently tox can build the package without it, probably using plain ``pip`` (an alternative build frontend ). Then there is no need to add ``build`` module to ``test_requirements.txt`` IMHO. But I fixed ``scripts/release`` to use it instead ``setup.py``. Then I guess we should add ``build`` dependency somewhere in the release process. Could be ``test_requirements.txt`` if you run the script within a tox venv, but I believe it's better to release from a pristine environment to avoid side effects of pre-populated venv with tons of deps for test/coverage/lint/etc…. But this is something I let you decide, I don't know how you release borgmatic.
Owner

Thanks for the explanation. I typically release borgmatic manually by running scripts/release on my machine (not from a virtualenv), although I'm open to changing the process if it would help. But if the release script is the only place that requires a build dependency, I'm fine just manually installing it on my machine for now. And then if/when the release script moves to, say, CI, that automated process can install build accordingly.

Thanks for the explanation. I typically release borgmatic manually by running `scripts/release` on my machine (not from a virtualenv), although I'm open to changing the process if it would help. But if the release script is the only place that requires a `build` dependency, I'm fine just manually installing it on my machine for now. And then if/when the release script moves to, say, CI, that automated process can install `build` accordingly.
witten marked this conversation as resolved
setup.py Outdated
@ -37,4 +0,0 @@
'setuptools',
),
extras_require={"Apprise": ["apprise"]},
include_package_data=True,
Owner

Looks like the pyproject.toml equivalent of this line is missing? I think the main reason this exists is so that the configuration schema file gets installed. (It's at borgmatic/config/schema.yaml in the source.) But also the systemd and other sample files (sample/) probably get included too.

Looks like the `pyproject.toml` equivalent of this line is missing? I think the main reason this exists is so that the configuration schema file gets installed. (It's at `borgmatic/config/schema.yaml` in the source.) But also the systemd and other sample files (`sample/`) probably get included too.
Author
Contributor

These files (systemd units and schema) are declared in MANIFEST.in and by default, include-package-data is true in pyproject.toml.

I can see these files in the resulting source dist tar ball.

These files (systemd units and schema) are declared in ``MANIFEST.in`` and by default, [include-package-data is true in pyproject.toml](https://setuptools.pypa.io/en/latest/userguide/datafiles.html#configuration-options). I can see these files in the resulting source dist tar ball.
Owner

Awesome, thanks!

Awesome, thanks!
witten marked this conversation as resolved
setup.py Outdated
@ -27,4 +0,0 @@
'validate-borgmatic-config = borgmatic.commands.validate_config:main',
]
},
obsoletes=['atticmatic'],
Owner

This didn't get carried forward to the pyproject.toml, but that's probably okay given how much it's ancient history.

This didn't get carried forward to the `pyproject.toml`, but that's probably okay given how much it's ancient history.
Author
Contributor

Right, it can still be ported to pyproject.toml with an extra conf :

[tool.setuptools]
obsoletes = ['atticmatic']
Right, it can still be ported to `pyproject.toml` with an extra conf : ```ini [tool.setuptools] obsoletes = ['atticmatic'] ```
Owner

I really don't feel strongly either way. Just thought I'd mention it.

I really don't feel strongly either way. Just thought I'd mention it.
witten marked this conversation as resolved
setup.py Outdated
@ -19,4 +0,0 @@
'Topic :: Security :: Cryptography',
'Topic :: System :: Archiving :: Backup',
],
packages=find_packages(exclude=['tests*']),
Owner

I don't know if something like this is necessary in pyproject.toml so tests don't get installed into the final package? Or is that done automatically? I didn't see tests end up in the resulting tarball, so maybe it's fine as is.

I don't know if something like this is necessary in `pyproject.toml` so tests don't get installed into the final package? Or is that done automatically? I didn't see tests end up in the resulting tarball, so maybe it's fine as is.
Author
Contributor

It's done automatically indeed.

And the resulting tar ball does not contain any test files.

But to make sure to package all modules in borgmatic package I used tool.setuptools.packages.find then I believe anything outside ./borgmatic folder in not covered anyway.

Please not that tool.setuptools.packages.find also set namespaces = false, from the doc “This will prevent any folder without an init.py file from being scanned.“ src.

It's [done automatically indeed](https://setuptools.pypa.io/en/latest/userguide/package_discovery.html#flat-layout). And the resulting tar ball does not contain any test files. But to make sure to package all modules in borgmatic package I used `tool.setuptools.packages.find` then I believe anything outside `./borgmatic` folder in not covered anyway. Please not that `tool.setuptools.packages.find` also set `namespaces = false`, from the doc “This will prevent any folder without an __init__.py file from being scanned.“ [src](https://setuptools.pypa.io/en/latest/userguide/package_discovery.html#finding-simple-packages).
Owner

That makes sense. The only thing potentially missing that I could see of being use is the sample/ directory, as I'm vaguely recalling that some Linux distribution packages of borgmatic programmatically consume those files.

That makes sense. The only thing potentially missing that I could see of being use is the `sample/` directory, as I'm vaguely recalling that some Linux distribution packages of borgmatic [programmatically consume those files](https://gitlab.archlinux.org/archlinux/packaging/packages/borgmatic/-/blob/main/PKGBUILD?ref_type=heads#L46).
Author
Contributor

We discussed it already :)

These files are in the source distribution. The MANIFEST.in is declaring it and setuptools includes them by default, no need to explicitly declare

We used systemd unit file in Debian as well, but we got the source from the tagged version in github, then we get the whole source tree. I'm not familiar with PKGBUILD but I think they use the git source as well, not the pypi source distribution. Anyway thanks for shipping these, it's helpful for packaging.

We discussed it already :) [These files are in the source distribution. The MANIFEST.in is declaring it and setuptools includes them by default, no need to explicitly declare ](https://projects.torsion.org/borgmatic-collective/borgmatic/pulls/922/files#issuecomment-8753) We used systemd unit file in Debian as well, but we got the source from the tagged version in github, then we get the whole source tree. I'm not familiar with PKGBUILD but I think they use the git source as well, not the pypi source distribution. Anyway thanks for shipping these, it's helpful for packaging.
Owner

Okay, just making sure since you quoted: "This will prevent any folder without an __init.py__ file from being scanned." But if the MANIFEST.in still takes care of sample/, great.

Getting the tagged version from GitHub should be fine, although note that projects.torsion.org is the canonical source for the source and GitHub is "only" a mirror.

Okay, just making sure since you quoted: "This will prevent any folder without an \_\_init.py\_\_ file from being scanned." But if the MANIFEST.in still takes care of `sample/`, great. Getting the tagged version from GitHub should be fine, although note that projects.torsion.org is the canonical source for the source and GitHub is "only" a mirror.
witten marked this conversation as resolved
Owner

I'm not sure how PyCQA is handled within borgmatic project. The doc suggest tox is the main entrypoint.

That's correct.

Then I think pycqa config could be gathered within tox.ini conf file when pyproject.toml is not supported.

I'm not familar with PyCQA. What is its config? It looks like Tox does support pyproject.toml (via legacy INI format or even native TOML).. or its config can be kept in the legacy tox.ini.

No support for pyproject.toml, but there are currently two locations with flake8 directives within the project:

setup.cfg
.flake8

Why not merge the conf in .flake8 or tox.ini ?

Either of those approaches sound fine to me.

pycodestyle

No support for pyproject.toml. Keep config in setup.cfg or move it to tox.ini ?

Also either approach is fine with me. Note however the comment above about potentially moving Tox config to pyproject.toml, which is maybe a backdoor way to get both flake8 and pycodestyle config into pyproject.toml!

> I'm not sure how PyCQA is handled within borgmatic project. The doc suggest tox is the main entrypoint. That's correct. > Then I think pycqa config could be gathered within tox.ini conf file when pyproject.toml is not supported. I'm not familar with PyCQA. What is its config? It looks like [Tox does support `pyproject.toml`](https://tox.wiki/en/latest/config.html#pyproject-toml-ini) (via legacy INI format or even native TOML).. or its config can be kept in the legacy `tox.ini`. > No support for pyproject.toml, but there are currently two locations with flake8 directives within the project: > > setup.cfg > .flake8 > > Why not merge the conf in .flake8 or tox.ini ? Either of those approaches sound fine to me. > pycodestyle > > No support for pyproject.toml. Keep config in setup.cfg or move it to tox.ini ? Also either approach is fine with me. Note however the comment above about potentially moving Tox config to `pyproject.toml`, which is maybe a backdoor way to get both flake8 and pycodestyle config into `pyproject.toml`!
kaliko added 1 commit 2024-10-22 19:32:39 +00:00
kaliko force-pushed pyproject-transition from d5afdeebda to 601e393ec7 2024-10-22 20:05:04 +00:00 Compare
Author
Contributor

Then I think pycqa config could be gathered within tox.ini conf file when pyproject.toml is not supported.

I'm not familar with PyCQA. What is its config? It looks like Tox does support pyproject.toml (via legacy INI format or even native TOML).. or its config can be kept in the legacy tox.ini.

Sorry I was just referring to PyCQA as the collection of tools used in borgmatic for QA (tox, black, etc…).

Anyway, I did see the legacy_tox_ini integration of ini tox config in pyproject.toml but it looks ugly (I know it's highly subjective :D).
Personally I prefer to merge everything in tox.ini, but the decision is yours as the main developer.

Are you taking over from there? Or do you want me to make some more changes?

Cheers,
k

> > Then I think pycqa config could be gathered within tox.ini conf file when pyproject.toml is not supported. > > I'm not familar with PyCQA. What is its config? It looks like [Tox does support `pyproject.toml`](https://tox.wiki/en/latest/config.html#pyproject-toml-ini) (via legacy INI format or even native TOML).. or its config can be kept in the legacy `tox.ini`. Sorry I was just referring to PyCQA as the collection of tools used in borgmatic for QA (tox, black, etc…). Anyway, I did see the `legacy_tox_ini` integration of ini tox config in `pyproject.toml` but it looks ugly (I know it's highly subjective :D). Personally I prefer to merge everything in `tox.ini`, but the decision is yours as the main developer. Are you taking over from there? Or do you want me to make some more changes? Cheers, k
Owner

You're the one who instigated this work item, so while I certainly have opinions, if you wanna leave stuff in tox.ini... that's totally fine with me! I will mention though that there's also a "native" TOML format for stuffing tox.ini contents into pyproject.toml. Maybe you were referring to that as ugly as well though. 😄

Anyway, to answer your question: If you're feeling up to continuing this, it would be great if you could!

You're the one who instigated this work item, so while I certainly have opinions, if you wanna leave stuff in `tox.ini`... that's totally fine with me! I will mention though that there's also a ["native" TOML format](https://tox.wiki/en/latest/config.html#pyproject-toml-native) for stuffing `tox.ini` contents into `pyproject.toml`. Maybe you were referring to that as ugly as well though. 😄 Anyway, to answer your question: If you're feeling up to continuing this, it would be great if you could!
kaliko added 1 commit 2024-10-26 18:11:57 +00:00
kaliko changed title from WIP: pyproject-transition to pyproject-transition 2024-10-26 18:26:51 +00:00
Author
Contributor

I rebased the branch on latest changes from main to control and it still looks fine (I did not pushed rebased code though).

These change are good to merge now.

I rebased the branch on latest changes from main to control and it still looks fine (I did not pushed rebased code though). These change are good to merge now.
Owner

Looks great! Thank you so much for taking on this work.. It really needed doing.

Looks great! Thank you so much for taking on this work.. It really needed doing.
witten merged commit c7ca9bf844 into main 2024-10-26 23:38:35 +00:00
Owner

Released in borgmatic 1.9.0!

Released in borgmatic 1.9.0!
Owner

FYI: #932.

FYI: #932.
Sign in to join this conversation.
No Reviewers
No Milestone
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#922
No description provided.