Re: RFS: dhcpd-pools
Hi Kilian,
On 06/25/2011 11:29 AM, Kilian Krause wrote:
Here some further comments to what you may also have received on IRC:
Thanks for your thorough review.
- debian/compat is still at 7 - any reason to not make it 8? (stable and bpo
for oldstable have debhelper 8)
No reason, fixed. Also bumped the build-depend on debhelper to >=8.0.0
- using dh-autoreconf feels like to wrong solution to a problem to me. Is
that really required to autoreconf? What for?
This is because I'm using git-buildpackage, and pulling upstream through
git, not release tarballs, as documented in the git-buildpackage
documentation[1].
I have the following branches in my git repository:
- master - used for Debian packaging
- upstream - upstream release I am working on (based on upstream tag)
- upstream-head - upstream's current git HEAD
I'm building with:
git-buildpackage --git-export-dir=../build --git-export=HEAD -k5238B839
- debian/changelog with
Format: http://anonscm.debian.org/viewvc/dep/web/deps/dep5.mdwn?revision=174&view=markup
is most probably too specific about which dep5 format you intended to
fulfill. Not exactly wrong just feels a bit "funny". ;-)
Before the review session on IRC yesterday, I had:
Format: http://dep.debian.net/deps/dep5
It was pointed out that it was best practice to link to the revision:
I of my 139 installed packages that have a DEP 5 copyright file:
- 28 links to http://dep.debian.net/deps/dep5/
- 111 links to http://svn.debian.org/wsvn/dep/web/deps/dep5.mdwn + rev
- all dead links currently due to the websvn => viewvc change
DEP 5 uses this link as an example:
http://svn.debian.org/wsvn/dep/web/deps/dep5.mdwn?op=file&rev=REVISION
liblucene2-java actually uses it directly. :( (Filing bug in a moment)
I changed it to the anonscm.d.o link, because of this.
After that I have through some more about it, now that DEP 5 is frozen I
guess that, it might be better to just change it back to the dep.d.n
link again.
- The tarball doesn't match upsteam's - yet there's no notion of why this
difference exists:
-rw-rw-r-- 1 kk kk 51500 25. Jun 12:56 dhcpd-pools_2.15.orig.tar.gz
-rw-rw-r-- 1 kk kk 388230 25. Jun 13:04 tarballs/dhcpd-pools_2.15.orig.tar.gz
Either you seriously do a repack and alter the version number to ~dfsg (if
it's a DFSG-driven repack) or a ~debian if it's rather a cosmetic repack.
From a first glance it seems that you're working around exactly that
autoreconf issue (reintroducing what you stripped out) and eventually some
Git that you could educate upstream to leave out of the RELEASE tarball.
Please get this sorted out in a clean way so that upstream's tarball is
used unchanged if you intend to maintain your current version number as
is.
It is a purely cosmetic repack, based on upstreams release git tag.
I wouldn't mind changing the version number to include ~debian, but in
that case ~git would make more sense, as there are no debian specific
changes in the repack, but ~git might also sound more scary than
~gittag. If ~debian is recommended/mandatory in package version where
orig.tar.gz is generated by git-buildpackage, I would expect it to be
mentioned in the git-buildpackage documentaion. If it is I would be
happy to make patch against git-buildpackage adding that.
- dpkg-gencontrol: warning: package dhcpd-pools: unused substitution variable ${perl:Depends}
should also be fixed - i.e. make sure it finds all *.pl files and does
look at them.
Added it, the only perl file is contrib/snmptest.pl, and it doesn't have
any non standard perl dependencies. So ${perl:Depends} only gets
replaced with perl itself.
- I: dhcpd-pools: hyphen-used-as-minus-sign usr/share/man/man1/dhcpd-pools.1.gz:13
may also be worth fixing while we're at it
I have ignored this, due to upstream having a commit[2] removing that
line, scheduled for next release. Should I backport it?
- licensecheck still reports:
./src/getopt.c: GPL (with incorrect FSF address)
./src/getopt.h: GPL (with incorrect FSF address)
./src/getopt1.c: GPL (with incorrect FSF address)
which you may want to tell upstream about.
Will submit patch upstream. Have also added them to debian/copyright as
GPL-2.0+.
I assume that contrib/snmptest.pl is GPL-3.0+.
And just for the record ltmain.sh is GPLv2 (and isn't mentioned as
exception to the all-is-GPL3 in debian/copyright)
ltmain.sh is created by autoreconf, and is distributed through that, it
is neither a part of the orig.tar.gz, debian.tar.gz nor the resulting
deb. It is through distributed in upstream release tarball.
- Spelling in debian/control should be fixed for at least:
accomodiate => accommodate
- In debian/control you first talk about "ISC dhcp" and later about "ISC dhcpd"
meaning the same thing though - the DHCP server. Please use one wording
only.
- CSV is an abbreviation that should be capitalized in the description
- "Users of the command" most probably means "Users of this tool" I guess.
Fixed, I took that description directly from upstream, will push the
corrections upstream.
[1]
http://honk.sigxcpu.org/projects/git-buildpackage/manual-html/gbp.import.html#AEN195
[2] http://git.asbjorn.biz/?p=debian/dhcpd-pools.git;a=commitdiff;h=2cb7369e
--
Best regards
Asbjørn Sloth Tønnesen
Reply to: