Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=529496 --- Comment #3 from Felipe Contreras <felipe.contreras@xxxxxxxxx> 2009-10-26 10:30:47 EDT --- (In reply to comment #2) > > Summary: An advanced C music tagging library with a simple API > > It's widely accepted practise to omit leading articles, such as "An" and "A" to > shorten summaries even further: > > Summary: Advanced C music tagging library with a simple API > > That also looks better when displayed during installation. Ok. > > Source0: http://libmtag.googlecode.com/files/%{name}-%{version}.tar.gz > > 0.3.2 is not available there. 404 Not Found. Yes, I want to release 0.3.2 after fixing all the issues mentioned here. > * Spelling errors: sed -i 's!excercise!exercise!g' libmtag.spec Ooops. Fixed. > > License: LGPLv2 > > You should include the LGPL text in your tarball and confirm the licensing in > the source files: > https://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text Sure, I can put the license in the COPYING file. But I don't see anything in the guidelines regarding the source files. > > Requires: taglib > > Found in the pkgconfig file. And this dependency is not true. It would only be > true if you had a build-time dependency on taglib headers or static library. Strange, I don't have that in my .spec file. Maybe I send an outdated one. > > mtag.cpp > > Is there any reason why you include C headers instead of their Standard C++ > Library counterparts, such as <cstdio> and <cstring>? No. I'm not familiar with cstdio and cstring so I don't know why I should include them. libmtag is a C library, and C++ is used only when necessary. > * Please also take a look at the compiler warnings related to mtag.cpp and > mtag.c. Currently: > > mtag.cpp:38: warning: suggest parentheses around assignment used as truth value > mtag.cpp:79: warning: suggest parentheses around assignment used as truth value > mtag.cpp:95: warning: suggest parentheses around assignment used as truth value > mtag.cpp:113: warning: suggest parentheses around assignment used as truth > value > mtag.cpp:126: warning: no return statement in function returning non-void > mtag.cpp:136: warning: suggest parentheses around assignment used as truth > value > mtag.cpp:143: warning: suggest parentheses around assignment used as truth > value > mtag.cpp:150: warning: suggest parentheses around assignment used as truth > value > mtag.cpp:310: warning: suggest parentheses around assignment used as truth > value > mtag.cpp:331: warning: suggest parentheses around assignment used as truth > value > mtag.c:179: warning: implicit declaration of function 'basename' > mtag.c:179: warning: format '%s' expects type 'char *', but argument 2 has type > 'int' I didn't get those warnings, but I'll changes the CFLAGS to catch them in the future, and I'll fix them. > > libmtag-tools > > $ mtag > Error: Bad arguments: No filename specified > > $ mtag --help > mtag: unrecognized option '--help' > Error: Bad arguments: No filename specified > > $ mtag test1.mp3 > Error: Bad arguments: No filename specified > > $ mtag test1.ogg > Error: Bad arguments: No filename specified I'll improve the help for that app. > Without the README (which you don't include in the package), the mtag tool is > not very helpful about telling how to use it. > > Run "rpmlint" on all the packages (src.rpm and built rpms). > https://fedoraproject.org/wiki/Packaging/Guidelines#rpmlint I did. > It would have warned about lack of documentation files. ;) Warnings not always need to be fixed. Thanks for the review. I'll come up with a second version soon. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review