pyproject-transition #922
Loading…
x
Reference in New Issue
Block a user
No description provided.
Delete Branch "kaliko/borgmatic:pyproject-transition"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Moving to pyproject.toml
806d544fca
to9e29ce788f
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 whenpyproject.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
ortox.ini
?pycodestyle
No support for pyproject.toml. Keep config in
setup.cfg
or move it totox.ini
?c7cefd5922
tob029d1cb67
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"
This
pyproject.toml
looks good to me. But it seems to result in an unimportableborgmatic
module, I'm guessing becausesetup.py
is gone and something is still relying on it to considerborgmatic
a module.For instance, when I run tests, a particular test that tries to run the
borgmatic
executable fails:And I can reproduce this by manually running borgmatic in the virtualenv:
So I'm not sure what's missing here.
Fixed with
601e393ec7
(pyproject/build failed to ship submodules).@ -34,4 +34,2 @@
# Build borgmatic and publish to pypi.
rm -fr dist
python3 setup.py bdist_wheel
python3 setup.py sdist
Something is missing from the result tarball. Here's what I'm seeing on my machine:
Notice how most of the source subdirectories are missing:
borgmatic/actions/
,borgmatic/borg/
, etc.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.
Cool, yeah, the tarball contents look much better now!
@ -35,3 +35,2 @@
rm -fr dist
python3 setup.py bdist_wheel
python3 setup.py sdist
python3 -m build
When I try this inside a Python 3 virtualenv, the command doesn't appear to work:
Looks like it might need to be added to
test_requirements.txt
? And/or to[build-system]
inpyproject.toml
?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 supportingpyproject.toml
to call thebuild-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 totest_requirements.txt
IMHO.But I fixed
scripts/release
to use it insteadsetup.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.
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 abuild
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 installbuild
accordingly.@ -37,4 +0,0 @@
'setuptools',
),
extras_require={"Apprise": ["apprise"]},
include_package_data=True,
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 atborgmatic/config/schema.yaml
in the source.) But also the systemd and other sample files (sample/
) probably get included too.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.
Awesome, thanks!
@ -27,4 +0,0 @@
'validate-borgmatic-config = borgmatic.commands.validate_config:main',
]
},
obsoletes=['atticmatic'],
This didn't get carried forward to the
pyproject.toml
, but that's probably okay given how much it's ancient history.Right, it can still be ported to
pyproject.toml
with an extra conf :I really don't feel strongly either way. Just thought I'd mention it.
@ -19,4 +0,0 @@
'Topic :: Security :: Cryptography',
'Topic :: System :: Archiving :: Backup',
],
packages=find_packages(exclude=['tests*']),
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.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 setnamespaces = false
, from the doc “This will prevent any folder without an init.py file from being scanned.“ src.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.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.
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.
That's correct.
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 legacytox.ini
.Either of those approaches sound fine to me.
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 intopyproject.toml
!d5afdeebda
to601e393ec7
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 inpyproject.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
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 stuffingtox.ini
contents intopyproject.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!
WIP: pyproject-transitionto pyproject-transitionI 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.
Looks great! Thank you so much for taking on this work.. It really needed doing.
Released in borgmatic 1.9.0!
FYI: #932.