Thanks for the review!
On 18/12/13 21:42, intrigeri wrote:
> Hi,
>
> here's a quick initial review of the 1.5-1 packaging. Note that I've
> not looked at the upstream code, assuming some people better skilled
> than me in this area, in the Tor Project, have done this already.
> If this is not the case, please tell me.
>
> First of all, if this is your first serious packaging task, this is
> a high-quality result, and I'm impressed.
>
> 1. debian/copyright gives copyright 2011-2013 to David Fifield, while
> the upstream source only mentions 2012. The correct place to fix
> upstream copyright information, if that was the intent, is upstream
> sources: we're merely relaying it in debian/copyright.
>
> 2. Please upgrade to the latest available standards-version.
>
> 4. Is the flashproxy-facilitator's dependency on python really needed?
> I have nearly no experience in Python software packaging, but isn't
> the role of ${python:Depends} to do just that?
>
> 6. A the facilitator's postinst depends on /usr/share/doc to be
> available. This is a violation of Policy 12.3: "Packages must not
> require the existence of any files in /usr/share/doc/ in order to
> function." These files must be moved to /usr/share/$PACKAGE and may
> be symlinked from /usr/share/doc/$PACKAGE.
>
> 7. In the facilator's postinst, instead of install + cat, you could
> just use install to copy reg-email.pass where it belongs.
>
> 8. Why install symlinks in the facilitator's postinst, instead of ship
> them in the package (e.g. with dh_link(1))? This would avoid the
> need to remove them in postrm (and having to handle the case when
> the administrator changed these files).
>
Fixed and pushed to git.
> 9. This comment leaves me thinking:
>
> # NOTE: debian/rules build (which runs tests) must be run outside of fakeroot
> # since the tests open sockets, which fakeroot stubs out.
>
> It could be worth supporting fakeroot environments, no?
> Can't you detect fakeroot and (loudly) skip tests if applicable?
>
> 10. What does this mean, exactly?
>
> # TODO(infinity0): handle setup.py better
>
> Any potential issue you foresee?
>
I've elaborated the comments, neither are a problem. For (9), you are supposed to run `debian/rules build` separately outside of fakeroot anyways, and dpkg-buildpackage does this automatically. (As opposed to relying on `make` to automatically run (build) when you call `fakeroot debian/rules binary`, which would run it inside the fakeroot.)
> 3. Please make the package Lintian -clean, if possible in pedantic
> mode, e.g. like this:
>
> lintian --info --display-info --pedantic --color auto *.changes
>
> Lintian has good advices for you, e.g. using DEP-3 for the
> Debian-specific patch to indicate it should not be forwarded
> upstream :)
>
Fixed - The only two left IMO are false positives:
- debian-watch-file-is-missing: currently N/A, upstream does not release the tarballs separately outside of git tags. I work closely enough with upstream that it's not so important anyways.
- using-first-person-in-description: does not exactly apply, "we" used here is descriptive and neutral rather than instructive.
> 11. I find it a bit scary not to install the upstream LICENSE file,
> and to rely on Debian's copyright instead. As soon at the Debian
> packaging misses an upstream update (and they are *already* in
> mismatch, actually), then we're violating the upstream license.
> I recommend installing the upstream LICENSE file.
>
Unfortunately that results in a lintian warning: http://lintian.debian.org/tags/extra-license-file.html
I've fixed LICENSE in a patch anyways, which has also been applied upstream.
> 5. We don't ship software in Debian that has dependencies outside
> Debian to be useful at all. So, in the current state of things, the
> node-flashproxy binary package (and its README.Debian) is a no-no.
> Time to ping Mike Gabriel on #721558 (and offer your help if you
> wish), perhaps.
>
> 12. I have not tested the resulting binary packages with piuparts.
> I suggest you do it if you can afford it: discovering issues
> before the QA team knocks at the door is nicer both for them, and
> for you :)
>
I'll deal with these last two points and follow-up to this email.
Thanks again!
X
--
GPG: 4096R/1318EFAC5FBBDBCE
git://github.com/infinity0/pubkeys.git
Attachment:
signature.asc
Description: OpenPGP digital signature