On 07/27/2016 03:28 AM, Sean Whitton wrote:
> I can't sponsor the package, but I hope that the following review is
> useful to you.
Thanks for your very in-depth review, it is very helpful!
> 1. You should install the examples to /usr/share/doc/yabar/examples, not
> /usr/share/yabar/examples. You can use dh_installexamples to do this.
ACK
> 2. Since you are packaging based on git tags not github tarballs (I'm
> looking at force-create in your gbp.conf), you might have the watch file
> look for git tags rather than tarballs. Uscan watch file format version
> 4 can do this. Not a big deal -- just a suggestion.
I'll have a look at it (and it surely sounds nice), but uscan documentation is scarce.
> 3. You should remove the export-dir from gbp.conf: this will override
> settings in personal ~/.gbp.conf, arbitrarily selecting a different
> build area on other developer's machines!
ACK
> 4. Are you sure about Section: misc? How about x11?
You're right, x11 fits better.
> 5. You have a lot of libs listed as dependencies. These should be
> included in ${shlibs:Depends}. Is dh_shlibdeps not generating the
> correct list?
It is generating the correct list :) Fixed.
> 6. You have a build-dependency on git-core, presumably because of the
> call to git-describe(1) in upstream's Makefile. However, you also set
> the VERSION environment variable in debian/rules -- though I think your
> VERSION in d/rules includes the Debian revision, which might not be what
> you want. Remember that your source package needs to be buildable
> outside of a git repository. You should add some comments to the rules
> file explaining why you are setting VERSION and please explain in reply
> to this e-mail why you need the git-core build-dep :)
git-core is now longer required, I just forgot to remove it after switching to dpkg-parsechangelog. Thanks for the hint.
Also, a comment in debian/rules is definitely required :-)
> 7. Could you explain the Suggests: fonts-font-awesome?
While yabar does not require font-awesome in any way, both example configurations show the usage of font-awesome.
Also, most have people I have seen use yabar with font-awesome.
Therefore I think the package fonts-font-awesome is an addition which people should at least consider.
> 8. You could add some paragraph breaks to the extended description.
ACK
> 9. "plain c" in the description should be "plain C"
ACK
> 10. Why priority 'extra' not 'optional'? Do you expect a file conflict?
I did initially put it into 'optional', but was unsure about the following sentence in Debian's Policy, Section 2.5 [0]
> This is all the software that you might reasonably want to install if you didn't know what it was and don't have specialized requirements"
Not sure if yabar is a software "you might reasonably want to install if you didn't know what it was".
On the other hand it also states:
> This is a much larger system and includes the X Window System, a full TeX distribution, and many applications.
Not quite sure on this one.
>
> 11. Why the tight dpkg-dev dependency?
Not required :-)
> 12. As I said before, please merge your debian/changelog entries to a
> single 0.4.0-1 entry, with only the "Initial release (Closes: #xxxxxx)"
> line.
Will do.
> 13. Why a 'low' upload urgency? Counterintuitively, this means that you
> think the package is more likely than usual to be buggy and so it should
> take longer to migrate to testing; it doesn't actually mean "less
> important". Unless you think the upload is buggy, you should use
> priority=medium.
Thanks for the explanation. Unfortunately, neither Section 4.4 [1] nor 5.6.17 [2] explain when which urgency should be used (so I just used the lowest one).
Is there documentation for this elsewhere?
> 14. I see you are ignoring .o files in debian/source/options. Since
> your clean target deletes .o files, why do you need this? If you don't
> need it, it would be less confusing if you just deleted it. Is it
> because upstream hasn't merged your fixed clean target patch yet? I
> think it would be more readable to the debian/clean file instead of
> debian/source/options.
No longer needed.
> 15. Any reason you haven't forwarded d/patches/fix-typos,
> d/patches/makefile-ldflags, and d/patches/makefile-version?
I didn't want to overwhelm Upstream with a dozen patches :O
He just merged my two other PRs, so I will forward these patches now :-)
> 16. Are you sure you need all the lines in your rules file? I think
> that with debhelper compat level 9, it is enough to export
> DEB_BUILD_MAINT_OPTIONS and the rest will happen automatically. Not
> sure, though.
I simply followed the Debian Wiki (and also manually exported LDFLAGS because Upstreams name is LDLIBS) [3]
> 17. You could probably add --parallel to the dh invocation.
Although I couldn't spot any consistent differences on my machine (~10s for complete build), I'll add it anyway :-)
> 18. You should install the README (patched to remove the installation
> instructions) and the screenshots to /usr/share/doc/yabar. It looks
> like useful documentation that a Debian user probably wants.
If you compare the README and the shipped man page you'll notice that the man page *is* the README stripped off the sections 'Screenshots' and 'Installation' ;-)
Yet again, huge thanks for your suggestions, tips and hints!
[0] https://www.debian.org/doc/debian-policy/ch-archive.html#s-priorities
[1] https://www.debian.org/doc/debian-policy/ch-source.html#s-dpkgchangelog
[2] https://www.debian.org/doc/debian-policy/ch-controlfields.html#s-f-Urgency
[3] https://wiki.debian.org/Hardening#dpkg-buildflags
Attachment:
signature.asc
Description: OpenPGP digital signature