[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