Hello, On Tue, Aug 02, 2016 at 12:19:55AM +0200, Jack Henschel wrote: > 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! No problem :) I noticed that in your changelog you thanked Michael Stapelberg for sponsoring the package. But I couldn't find it in the NEW queue. What's the current status? We might need to close this bug. > > 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. See the watch file in the source package ws-butler. > > 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. Okay, makes sense. > > 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. As I understand it, the current consensus is that the difference between extra and optional is only whether there are any file conflicts with other packages. The other stuff about specialised requirements, a "much larger system" etc. is ignored. When Debian had less than 1000 packages it meant something to talk about specialised requirements and divide packages between extra and optional based on that, but that division has broken down now we have 50,000 packages. Packages shouldn't depend on packages with a lower priority. So if you select priority extra, then anything that depends on yabar will also have to be priority extra. However, I guess yabar is a leaf package that it is unlikely anything will depend on. In conclusion, unless you expect file conflicts I think optional would be better, but it's not a big deal if you want to keep extra. > > 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? [breaking this discussion into a separate thread] > > 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 :-) Cool! > > 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] I encourage you to try removing some lines, and see if it works. I think that you only need the hardening=+all and LDLIBS lines. The reason this is useful is that the shorter d/rules is, the easier it is for other people to work with your package. From my experience of performing NMU/QA uploads and submitting patches, it makes a big difference to the amount of testing and verification one has to do. > > 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' ;-) Ah, sorry! -- Sean Whitton
Attachment:
signature.asc
Description: PGP signature