Many thanks James! Valid points Ross, please also consider those comments. Especially please fix the build system. I missed that during my review, sorry, but I will file a bug for that.
(Also, please send your patches upstream.) -- tobi Am Montag, den 20.10.2014, 21:59 +0100 schrieb James Cowgill: > On Mon, 2014-10-20 at 15:59 +0200, Ross Gammon wrote: > > Hi All, > > > > I know everyone is busy with the Jessie Release Freeze, but I would be > > grateful if somebody could take a look at abcmidi (and sponsor if > > happy). Abcmidi has been sitting unloved for a while now (since 2007). > > It would be great to get the latest version into Jessie. > > Hi, > > Here's a review (I'm not a DD so can't sponsor you however). > > General > * There is a new upstream version (16th October 2014). > * #764998 abcmidi: binary-without-manpage usr/bin/abcmatch > Obviously you know this, but it would be good if a manpage was added. > * The file "/usr/share/doc/abcmidi/VERSION" seems redundant and can > probably be removed. Also ÁUTHORS should not be installed. > debian/copyright > * You don't need to list abc.h, sizes.h, structs.h manually in the first > section since they're already included when you say "Files: *". > * There seems to be some confusion about whether the code is GPL-2 or > GPL-2+. Are you sure what you've put is correct? I see files with no > copyright headers but nothing with "GPL 2 only" in them. > * You don't need to repeat the GPL header lots of times. I'd also be > tempted to merge all the GPL sections together and just have a large > "Copyright:" block. > > debian/rules > * I don't think you need to use autotools-dev in this package (I don't > know a huge amount about this though). > * The clean target doesn't work because you disabled it. This is a > violation of debian policy (4.9) "clean (required): This must undo any > effects that the build and binary targets may have had" > > debian/patches: > * Make sure you send these patches upstream (sorry if you've already > done this - they're not in the new version though). > * hardening.patch: Only LDFLAGS should be passed during the link stage. > Remove your CFLAGS and CPPFLAGS additions. > > Build > There are lots of bad warnings printed when building this > Examples: > * parseabc.c:1701:3: warning: format ‘%s’ expects argument of type ‘char *’, > but argument 3 has type ‘char **’ [-Wformat=] > success = sscanf (s, "%%abc-version %s", &abcversion); /* [SS] 2014-08-11 > */ > > Isn't this a buffer overflow?! > > * toabc.c:1490:8: warning: iteration 7u invokes undefined behavior > [-Waggressive-loop-optimizations] > semi = convertnote[i]; > > It's not too difficult to use these to make abc2midi segfault - please > try and fix them if you have time. > > James > > -- To UNSUBSCRIBE, email to debian-mentors-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: https://lists.debian.org/1414192702.31439.5.ca...@debian.org
