Bug 666633 - Review Request: lastfmlib - library providing implementation of LastFm Submission Protocol
Summary: Review Request: lastfmlib - library providing implementation of LastFm Submi...
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Jochen Schmitt
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On:
Blocks: 667226
TreeView+ depends on / blocked
 
Reported: 2011-01-01 18:01 UTC by Dan Vratil
Modified: 2011-01-17 20:55 UTC (History)
4 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2011-01-17 20:55:17 UTC
jochen: fedora-review+
tibbs: fedora-cvs+


Attachments (Terms of Use)

Description Dan Vratil 2011-01-01 18:01:50 UTC
Spec URL: http://www.tepsonic.org/files/fedora/liblastfmlib.spec
SRPM URL: http://www.tepsonic.org/files/fedora/liblastfmlib-0.4.0-0.src.rpm

Description: a library providing implementation of LastFm Submission Protocol v1.2. It allows you to scrobble your tracks on Last.fm.

This is my first package, so I'd like to ask someone to sponsor me.

Comment 1 Jochen Schmitt 2011-01-04 19:02:14 UTC
Hallo,

here some pre-review complaints:

1.) I dont't like the package name liblastfmlib. It may be better to choich lastfmlib as an package name.

2.) you hve the buildRoot tag twice in your package. Current RPM releases doesn't need an BuildRoot tag. They are required only for EPEL-branches.

3.) the devel subpackage need a Requires to the main package in the form: Requires: %{name} = %{version}-%{release}

4.) Instead of '-n liblastfmlib-devel' you shoud prefer to write only 'devel' ot refer to the subpackage in the tags.

5.) It may be nice to insert a blank line between the %package and the %description stanza.

6.) Please us %{_includedir} instead of /usr/include

7.) Please use %{?_smp_mflags} to initiate a parallel build.

8:) Don't use the %makeinstall macro

9.) You don't need to specified the name of the main package in the %file stanza

10.) Please remove the *.la file from the devel package

11.) The license tag have to been GPLv2+. Please examinate the copyrith notes on the top of the sources files. This notes say, that you ca use the second version of the GPL or any later version, so you have to use GPLv2+ for the license tag.

12.) Upstream tar ball contains a verbatin copy of the license text which was not included in the package on the %doc stanza.

13 Pakcage has not %doc stanza with files like README which are provided by upstream authow

14.) You should use %{_includedir}/lastfm/ in the %files stanza to make sure, that this directory is onwed by the package. All Files and directories belong this directory will be included to the package, so no additional specification is required

15.) the specification of gcc-c++ as a BuildRequires is not required

Sorry for the long list of complaints, but if you are able to create a proper
package, I may will sponsor you.

Best Regards:

Jochen Schmitt

Comment 2 Rich Mattes 2011-01-04 20:00:10 UTC
I see in your changelog comments that you modified someone else’s specfile, which is probably why there are so many little issues.  You might want to use the command “rpmdev-newspec -t lib lastfmlib” to create a new skeleton specfile that conforms to Fedora guidelines, and work from there (rpmdev-newspec is in the rpmdevtools package).  Most of the things that Jochen addressed are laid out in the skeleton configuration, you'll just need to fill in the blanks.  I also found it very helpful to look at the packages already included in Fedora when I first started out to get familiar with the conventions people used.  Package sources are at http://pkgs.fedoraproject.org/gitweb

Comment 3 Dan Vratil 2011-01-04 20:25:08 UTC
Thank you both for your precious advices. 

I uploaded new files and renamed the library to lastfmlib.

Spec URL: http://www.tepsonic.org/files/fedora/lastfmlib.spec
SRPM URL: http://www.tepsonic.org/files/fedora/lastfmlib-0.4.0-1.fc14.src.rpm

Comment 4 Jochen Schmitt 2011-01-04 21:15:41 UTC
Thast is now an official review:

Good:
+ Package name fullfill naming guidelines
+ Package has consistantly rpm macro usage
+ Package contains URL tag to project homepage
+ Package contains recent version of the software
+ License tag contains a valid OSS license
+ License tag refer GPLv2+ as as valid license
+ Package contains verbatin copy of the license text
+ Package has a devel sub package
+ Source tag shows on proper dowload location
+ package tar ball matches with upstram one
(md5sum: 6f00882c15b8cc703718d22e1b1871f)
+ local build works fine
+ Parallel build is supported by the package
+ debuginfo rpm contains source files
+ Build on koji (rawhide) works fine
+ local install und uninstall works fine
+ All file and directories are owned by the package
+ There are not naming conflicts with other packages
+ %doc stanza is small, so we need no separate sub package.
+ Changelog entries has proper format

Please check:
? Rpmlist shows folling warning on source rpm:
lastfmlib.src: W: spelling-error Summary(en_US) scrobbling -> scribbling, scrabbling
lastfmlib.src: W: spelling-error %description -l en_US scrobble -> scribble, scrabble
lastfmlib.src: W: strange-permission lastfmlib-0.4.0.tar.gz 0600L
1 packages and 0 specfiles checked; 0 errors, 3 warnings.
? If you have following the suggestion of Michale Schwend you should remove
the reference to Petr Vanek from your Changelog entry

Bad:
- Rpmlint has the following cmplaints to the binary rpms:
rpmlint lastfmlib-0.4.0-1.fc14.x86_64.rpm 
lastfmlib.x86_64: W: spelling-error Summary(en_US) scrobbling -> scribbling, scrabbling
lastfmlib.x86_64: W: spelling-error %description -l en_US scrobble -> scribble, scrabble
lastfmlib.x86_64: E: non-standard-dir-perm /usr/share/doc/lastfmlib-0.4.0 0644L
lastfmlib.x86_64: E: zero-length /usr/share/doc/lastfmlib-0.4.0/TODO
1 packages and 0 specfiles checked; 2 errors, 2 warnings.
Please remove the empty TODO file and change the permissions of the documentation 
directory because traversal into this directory will be denied for ono-root users

Comment 5 Dan Vratil 2011-01-04 21:36:06 UTC
I uploaded fixed SPEC and SRPM. I removed the reference to original spec from changelog, fixed source tarball permissions, removed the empty TODO and changed documentation directory permissions to 755. 
The spelling of "scrobbling/scrobble" is correct.

Spec URL: http://www.tepsonic.org/files/fedora/lastfmlib.spec
SRPM URL: http://www.tepsonic.org/files/fedora/lastfmlib-0.4.0-1.fc14.src.rpm

Comment 6 Jochen Schmitt 2011-01-05 15:44:12 UTC
Good:
+ remove empty TODO file
* Local build works fine
* Package contains no patches
* Files permissions are ok on %files and %doc stanza

Bad:
- Please remove the BRs to automake and autoconf. Automake is only requires, if you make a patch to the Makefile.am file and want to recreate the makefile.in file. autoconf is only required, if you want to patch the configure.ac or configure.in file and recreating the configure script.

Please bump the release number of your package, if you are uploading a new release of your package for review.

Comment 7 Dan Vratil 2011-01-05 18:37:39 UTC
Fixed and version bumped. 

Spec URL: http://www.tepsonic.org/files/fedora/lastfmlib.spec
SRPM URL: http://www.tepsonic.org/files/fedora/lastfmlib-0.4.0-2.fc14.src.rpm

Comment 8 Jochen Schmitt 2011-01-05 19:42:05 UTC
Good:
- BR automake and autoconf was remove from SPEC file
+ Local build works fine
+ Koji build works fine.

*** APPROVED ***

Please let me know your FAS userid, so I can sponsor your.

If you have any issues don't hastle to contact me.

Comment 9 Dan Vratil 2011-01-05 20:03:44 UTC
My FAS username is progdan.

Thank you!

Comment 10 Jochen Schmitt 2011-01-05 20:42:53 UTC
You may create a SCM-Admin request for your package now.

Comment 11 Dan Vratil 2011-01-06 15:17:54 UTC
New Package SCM Request
=======================
Package Name: lastfmlib
Short Description: a library providing implementation of LastFm Submission Protocol v1.2. It allows you to scrobble your tracks on Last.fm.
Owners: progdan
Branches: f14
InitialCC:

Comment 12 Jason Tibbitts 2011-01-07 17:32:16 UTC
This ticket is not assigned to anyone.  Review tickets should be assigned to
the reviewer.  Please fix and re-set the fedora-cvs flag.

Comment 13 Jochen Schmitt 2011-01-07 17:50:36 UTC
Sorry, I have forgotten it.

Comment 14 Jason Tibbitts 2011-01-08 02:47:50 UTC
Git done (by process-git-requests).

Comment 15 Fedora Update System 2011-01-08 13:49:49 UTC
lastfmlib-0.4.0-2.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/lastfmlib-0.4.0-2.fc14

Comment 16 Fedora Update System 2011-01-08 21:29:56 UTC
lastfmlib-0.4.0-2.fc14 has been pushed to the Fedora 14 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update lastfmlib'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/lastfmlib-0.4.0-2.fc14

Comment 17 Fedora Update System 2011-01-17 20:55:12 UTC
lastfmlib-0.4.0-2.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.