Hi again,
this is my review of the targetcli-fb packaging.
The following things (many related to what I discussed in my
configshell-fb review, so I'm going to be terse here), I've
also fixed in a fork I pushed to github.
debian/pyversions: obsolete, esp. with Python3, remove it
debian/source/format: missing
debian/control:
- Uploaders: missing (see my configshell-fb mail for details)
- Build-Depends-Indep unnecessary (see my configshell-fb mail)
- Missing build-dependencies (unit tests fail import during
build time otherwise):
python3-configshell-fb
python3-rtlslib-fb
python3-dbus
debian/rules:
- can be vastly simplified via -Spybuild
debian/copyright: DEP-5 format would be nice, I did some git
archeology and created one
README.md, targetcli.8, setup.py:
- if you want to package upstream, based on tarballs (as is
proper in Debian), you need to make sure that the contents
of everything _except_ the debian/ directory is identical
to the contents of the tarball. In this case, there were
two more commits in the main repository since the last
releease before you added your Debian packaging changes.
There are two ways around this: either use a git snapshot
as the base version (with a custom orig tarball), or don't
have these changes except as debian/patches (that will be
dropped after the next upstream release).
I've opted for the second here, just to show you how the
3.0 (quilt) format works (and hence have reverted the
changes in the tree). Therefore: DO NOT MERGE MY CHANGES.
They are just an illustration of how debian packaging can
look like.
The proper way to do this would be to branch off the
branch containing the Debian packaging from the upstream
release (and merging continuously).
This demonstrates why maintaining Debian packaging in an
upstream repository is not necessarily a good idea,
because it really complicates these kinds things.
What I liked about the package is that it's now Python3 based, I
really want to get rid of Python2-only stuff ASAP. :-)
Anyway, you'll find my updates here:
https://github.com/chris-se/targetcli-fb/commits/debian-packaging
As I said, because of the upstream commits since release and
debian/patches situation: DO NOT MERGE this.
That all said, there's one major issue that isn't resolved here:
There's no startup logic! I see neither init script nor systemd
service file. There was something upstream (that was minimally
patched in Debian) in the old version, but that's gone now, as
far as I can tell. In that case, the correct course of action for
Debian packaging would be to add this in the debian/ directory and
install it from there. I don't have time to fix that today, and I
think this is something that can wait until after the packaging
is imported into Alioth. But just to keep in mind that this needs
to be done before the package can enter Debian.
Anyway, if I hear positive feedback from both you (Christophe) and
Ritesh about getting this onto Alioth ASAP, I'll just rebase your
commits on the actual upstream version (and not previous git master)
when adding your commits to the packaging repository.
Regards,
Christian