[Bug 529496] Review Request: libmtag - An advanced C music tagging library with a simple API
bugzilla at redhat.com
bugzilla at redhat.com
Mon Oct 26 14:30:48 UTC 2009
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 at gmail.com> 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.
More information about the Fedora-package-review
mailing list