Bug#849754: RFS: guerillabackup/0.0.0-1
Hi Andreas,
It took me quite a while to address all your remarks...
Andreas Henriksson wrote:
> Hello halfdog,
>
> Thanks for your interest in debian packaging....
>
> On Fri, Dec 30, 2016 at 03:16:55PM +0000, halfdog wrote:
> > Package: sponsorship-requests
> > Severity: normal
> >
> > Dear mentors,
> >
> > I am looking for a sponsor for my package "guerillabackup"
> [...]
> > dget -x https://mentors.debian.net/debian/pool/main/g/guerillabackup/guer
> illabackup_0.0.0-1.dsc
> [...]
> >
> > As also stated in comment to https://mentors.debian.net/package/guerillabac
> kup
> > to avoid reviewers wasting time searching for dirty little package
> > secrets, here are some pointers to things, I had problems with,
> > when creating the package. Reviewers might disagree with my proposed
> > solution, any feedback is very welcome!
> >
> > * Upstream Debian file templates: to support building of native
> > packages using only the upstream source, Debian package files
> > and build instructions are included already in upstream. To
> > avoid duplication of them when not (yet) needed, they are copied
> > within "rules" in target "override_dh_auto_configure"
>
> Not a fan here. :P
> From a Debian(-only) perspective this complicates things for no
> real gain. If you package things in Debian it's probably very
> unlikely people will get their packages from elsewhere, specially
> if they need to both build it themselves and follow a procedure
> for doing so that's completely alien...... (but what do I know
> about the actual problem you're trying to solve.)
I only hoped to perform some dedup, but ...
> I'm hoping DEP14 can instead be a replacement solution
> for handling this (and also other reasons).
... if I understand http://dep.debian.net/deps/dep14/ correctly,
building for different vendors in future should follow another
scheme anyway, where deduplication is not an option. So I removed
that stuff and duplicated all relevant upstream debian/* files
to the non-native Debian quilt files also.
> > * (Re)starting units on upgrade: As stated in documentation, two
> > services can be used also from commandline (on demand) or as
> > non-root user, depending on end user use cases. Thus it is intended,
> > that the two systemd units are not enabled by default. Also
> > a user may start them manually without enabling them. With upgrade
> > following problem may arise: with standard debhelper means it
> > was not possible to
> > 1) stop all running units and
> > 2) after update start only those, that were running beforehand.
> > Solution:
> > 1) do not use debhelper for stopping/starting of units,
> > 2) find out in "prerm" which units are currently running, stop them and
> > 3) in "postinst" start only those, that were running in step 1)
>
> Pretty please do not try to reinvent systemd integration on your own.
> This is just way to easy to get wrong. If the current helpers does
> not work for you it's either likely because you're using them wrong
> and/or because they should be improved. Anything else is likely just
> causing extra work and pain.
>
> Please swing by either the irc channel or contact the mailing list
> for the Debian systemd maintainers. They're very skilled and usually
> happy to help (time permitting). They are likely also the people
> you need to get to review your package anyway if you invent your
> own maintainer script scheme.
I tried to get response from the mailing list, see
http://lists.alioth.debian.org/pipermail/pkg-systemd-maintainers/2017-March/014551.html
Also got to the IRC channel "#debian-systemd" with same result.
As there are no responses proposing better solutions and the conditional
service restarting code works as expected, I would propose keeping
the current solution until bugreports are received. If insufficient,
I would try to contact them again.
> > * Use of .pyc files: As I do not fully understand the consequences
> > of using .pyc files, especially in conditions where backup might
> > be more important, e.g. when disks start already failing and
> > py/pyc files might fall out of sync, I decided not to use them
> > until I understand the possible risks. As codebase is very small
> > and program is long-running, overhead from JIT-compiling should
> > be not an issue.
>
> Not an expert on python packaging myself, but I think Debian python
> packaging helpers should generate postinst code to automatically
> generate the .pyc files on install. I guess the reason you can't
> ship them is because then you need to build them with the lowest
> supported capability set of the architecture (which itself is
> likely hard to do). In short, the debian way is to just rm *.pyc *.pyo
> and trust the helpers to do the right thing.
Same argument as with security considerations below: availability,
stability in those bordercases might not be a relevant issue for
the first version of the package. So .pyc objects are now generated
the same way as all other Debian packages do, see
"/usr/lib/guerillabackup/lib/guerillabackup/__pycache__" after
package installation.
> Below is a full packaging review with some pointers, hope it helps:
>
> Vcs-* -> should point to debian packaging repository, not upstream.
> (Note: does not have to be a separate repository. See DEP14 branch
> naming scheme for example. Still, Vcs-* fields should point to Debian
> specific parts. For information pointing to upstream see
> https://wiki.debian.org/UpstreamMetadata )
I changed them to point to a future private GIT to store the Debian
specific stuff. This should also ease migration of this part of
content to Alioth if requestd/needed.
Vcs-Git: https://github.com/halfdog/guerillabackup-debian.git
Vcs-Browser: https://github.com/halfdog/guerillabackup-debian
The repository will be filled as soon as configuration reaches
stability within review process.
I also added "upstream/metadata":
Name: guerillabackup
Homepage: https://github.com/halfdog/guerillabackup
Contact: Report via https://github.com/halfdog/guerillabackup/issues
Security-Contact: halfdog <me at halfdog.net>
Repository: https://github.com/halfdog/guerillabackup.git
Repository-Browse: https://github.com/halfdog/guerillabackup
Changelog: https://github.com/halfdog/guerillabackup/raw/master/doc/Changelog.txt
Bug-Database: https://github.com/halfdog/guerillabackup/issues
Bug-Submit: https://github.com/halfdog/guerillabackup/issues
> Section -> please consider looking up the correct section for backup
> utilities at https://packages.debian.org/unstable/
> (Please note the section name is not the one on the list, click the link
> to see the actual name.)
I changed from Section/Priority from "misc/extra" to "utils/optional",
the value used in similar package "backuppc".
> python2 only is unfortunate. Please note that porting everything in
> Debian to python3 was already a goal for this release. If you're not
> using python3 in a new package at this point that leaves me wondering
> about your chances for ever being part of the next release.
> (Python is not by strongest side though, specially not debian packaging
> of it, so please find more advanced advice on this from someone else.)
This remark was the hardest one to address. As the Python2 code
is quite stable, I still can use it on embedded devices - but
there is no need for packaging, those devices are not likely to
allow package installation anyway. So currently, continued Python2
development is not a requirement any more. If it should be one
again, guerillabackup needs a fork after upstream V0.0.0.
To continue with this RFS, I added a patch to transform the V0.0.0
Python2 upstream code to Debian Python3 code. The new code went
through a 2 month burn-in testing and seems to be stable now.
This patch will be dropped after upstream V0.0.1 will also be
ported to Python3 by applying the very same patch.
Would that satisfy your remark?
> dh compat 9 -> consider using latest dh compat 10 instead. This
> automatically includes dh-systemd, etc. It will also use 'restart
> /after/ upgrade' by default.
Upgraded to 10.
> * DEP14 rather than copying packaging during configure.
Duplicated, DEP14 branch solution will be applied as soon as there
are branches.
> * restart instead of stop+start. Also contact debian systemd maintainers
> for advice.
>
> Multiple services: Need to manually list each for dh_systemd_* to pick
> them up.
This is also related to "(Re)starting units on upgrade" from above.
The separate stop/start was intended due to security reasons.
In my opinion, there should have been border cases, where an attacker
could gain advantages when triggering loading of incompatible
next-version Python code from a currently running service while
packages were already unpacked to disk but restart not yet triggered.
This situation might be even more grave, when the update fails
for some issue, leaving a service in inconsistent state for quite
a while. Disregarding this minor concearns, I switched to your
proposed solution, applying "deb-systemd-invoke restart" instead.
> Maintainer scripts: Avoid them.
> Please consider shipping the files with correct permissions rather than
> setting them in postinst. (That would also close the unpack->configure
> window of them having the incorrect permissions.)
I did not know, that pre-packaging modification is the preferred
way to do that. Does this amendment to "debian/rules" address your
remark appropriately?
override_dh_fixperms:
dh_fixperms
chmod -R 00700 -- /var/lib/guerillabackup
chmod 00700 -- /etc/guerillabackup/keys
> Please do not invent your own systemd integration glue rather than using
> the existing helpers. It's just way to easy to get it wrong and you're
> adding extra work on people reviewing your package.
This is also related to "(Re)starting units on upgrade" from above.
I know that and respect it, but I do not know how to do any better.
> Your postrm file seems to contain nothing useful. Just remove it and
> it'll be completely generated when needed. Following earlier advice
> the postrm, etc. files should also become pointless -> removed.
I did not notice, that even when the file is missing, packaging tool
will generate one including only the default code - which is exactly
what I wanted to have. Hence "postrm" is not needed any more, removed.
I hope, this addresses your review comments in this round.
Best regards,
hd
Reply to: