source obtained via: git archive --format=tar --remote=git://anongit.kde.org/publictransport HEAD | gzip > publictransport_20111204git.tar.gz

sry guys wrong spec in the first comment
Spec URL: http://brummbq.fedorapeople.org/kde-plasma-publictransport.spec
SRPM Url: http://brummbq.fedorapeople.org/kde-plasma-publictransport-0.10-20111204git.fc16.0.src.rpm

Great, I'd really like to see the publictransport plasmoid in Fedora!

Are you already sponsored?

yes! I'm the maintainer for semantik, very easy :D

Spec URL: http://brummbq.fedorapeople.org/kde-plasma-publictransport.spec
SRPM Url:
http://brummbq.fedorapeople.org/kde-plasma-publictransport-0.10-20111204git.fc16.1.src.rpm

minimal changes. Still not sure about package splitting. The guys at suse have one subpackage for every plasmoid/krunner. I don't like that...it feels too scattered.

https://build.opensuse.org/package/view_file?file=plasmoid-publictransport.spec&package=plasmoid-publictransport&project=KDE%3AExtra&rev=306504949ccff771fe2c41b9f1cd2571

IMHO, the subpackages are fine as is, the openSUSE version is excessively split.

Some comments (not a complete review yet):

> Release:        %{snapshot}%{?dist}.1
should be:
Release:        0.1.%{snapshot}%{?dist}
The next builds should then be 0.2.%{snapshot}%{?dist}, 0.3.%{snapshot}%{?dist} etc.

BuildRequires are global, so I'd list them all together at the beginning (but putting them where you put them works, too).

> #remove executable bit
> chmod 644 %{buildroot}/%{_kde4_datadir}/applications/kde4/timetablemate.desktop

Not sure about that one… We don't normally do this, i.e. we keep the +x bit upstream sets, but some non-KDE folks want us to do what you did everywhere. In practice, it will work either way because KDE Plasma does not require the +x bit in the standard prefix.

You put the icon scriptlets into the main package and the actual icons
into -libs. They should be in the same package. (IMHO in the main package. Those icons shouldn't be multilibbed.)

That's all I can spot at first glance, I haven't done a complete systematic review yet though.

Spec URL: http://brummbq.fedorapeople.org/kde-plasma-publictransport.spec
SRPM Url:
http://brummbq.fedorapeople.org/kde-plasma-publictransport-0.10-0.2.20111204git.fc16.src.rpm

Thank you for your feedback.

(In reply to comment #6)
> > Release:        %{snapshot}%{?dist}.1
> should be:
> Release:        0.1.%{snapshot}%{?dist}
> The next builds should then be 0.2.%{snapshot}%{?dist}, 0.3.%{snapshot}%{?dist}
> etc.
done

> BuildRequires are global, so I'd list them all together at the beginning (but
> putting them where you put them works, too).
done
 
> > #remove executable bit
> > chmod 644 %{buildroot}/%{_kde4_datadir}/applications/kde4/timetablemate.desktop
> 
> Not sure about that one… We don't normally do this, i.e. we keep the +x bit
> upstream sets, but some non-KDE folks want us to do what you did everywhere. In
> practice, it will work either way because KDE Plasma does not require the +x
> bit in the standard prefix.
So, I'll leave it as it is. Otherwise rpmlint is complaining.

> You put the icon scriptlets into the main package and the actual icons
> into -libs. They should be in the same package. (IMHO in the main package.
> Those icons shouldn't be multilibbed.)
The problem is, in libs subpackage are also icons for timetablemate included. This means I can't move them all together into the main package because timetable doesn't need the rest of the main package. Well, and I was too lazy to split up the icons suitably :) (Probably I would make horrible mistakes)

> The problem is, in libs subpackage are also icons for timetablemate included.
> This means I can't move them all together into the main package because
> timetable doesn't need the rest of the main package.

Well, right now it does because you have a Requires…

To achieve what you want, please:
* remove the Requires: %{name} = %{version}-%{release} from -libs and
* make the icon scriptlets apply to the -libs subpackage where the icons actually are. (You'll have to remove the -p /sbin/ldconfig and write /sbin/ldconfig as a scriptlet line instead, so that the other stuff can be added.)

(In reply to comment #8)
> > The problem is, in libs subpackage are also icons for timetablemate included.
> > This means I can't move them all together into the main package because
> > timetable doesn't need the rest of the main package.
> 
> Well, right now it does because you have a Requires…
> 
> To achieve what you want, please:
> * remove the Requires: %{name} = %{version}-%{release} from -libs and
check

> * make the icon scriptlets apply to the -libs subpackage where the icons
> actually are. (You'll have to remove the -p /sbin/ldconfig and write
> /sbin/ldconfig as a scriptlet line instead, so that the other stuff can be
> added.)
What do you mean exactly by "make the icon scriptlets apply to the -libs subpackage" I'm a little bit confused.

Another point: Atm my package is missing some doc files (changelogs from various plasmoids in the main package) How can I include them? They have all the same file name.

I can take this review.

I haven't really looked yet, but what Kevin is saying is that you need to put your icons scriplets under the correct subpackage. ie in 

%post(un) libs
.
.
.
.
/usr/bin/ldconfig

Make sure the ldconfig call is last.

As they stand at the moment your icon scriplets apply to the main package.

Doh...now I have got it, thanks ;)

Spec URL: http://brummbq.fedorapeople.org/kde-plasma-publictransport.spec
SRPM Url: http://brummbq.fedorapeople.org/kde-plasma-publictransport-0.10-0.3.20111204git.fc16.src.rpm

Hi Greg, you'll need it on %posttrans as well. 

I'll try and finish this for you tomorrow.

Cool

Spec URL: http://brummbq.fedorapeople.org/kde-plasma-publictransport.spec
SRPM Url:
http://brummbq.fedorapeople.org/kde-plasma-publictransport-0.10-0.4.20111204git.fc16.src.rpm

Looks good Greg, just the #incomplete comment and a question on the license. 

Each sub-dir has its own COPYING file, I noticed you've %doc the file from applet/ directory. (I've checked that its the same file in every directory) You could ask upstream to drop the COPYING file in the root directory (next to the HINTS_FOR_PACKAGE_MAINTAINERS file, seeing as they're being so helpful)

This package is APPROVED

Required
========
+ - OK
- - N/A
X - attention
? - comment please

[+] named according to the Package Naming Guidelines 
[+] The spec file name must match the base package %{name}, in the format
%{name}.spec 
[X] Meet the Packaging Guidelines
# extraneous comment - I think I understand why - see below
[+] Be licensed with a Fedora approved license and meet the Licensing
Guidelines 
[+] The License field in the package spec file must match the actual license 
[+] License file must be included in %doc
[+] The spec file must be written in American English
[+] The spec file for the package MUST be legible
[+] The sources used to build the package must match the upstream source
# git
[+] Successfully compile and build into binary rpms on at least one primary
architecture
[+] Proper use of ExcludeArch 
[+] All build dependencies must be listed in BuildRequires
[+] The spec file MUST handle locales properly
[+] Shared library files (not just symlinks) in any of the dynamic linker's
default paths, must call ldconfig in %post and %postun
[+] Packages must NOT bundle copies of system libraries
[-] If the package is designed to be relocatable, the packager must state this
fact in the request for review, along with the rationalization for relocation
of that specific package
[+] A package must own all directories that it creates
directories under this
[+] A Fedora package must not list a file more than once in the spec file's
%files listings
[+] Permissions on files must be set properly.
[+] Each package must consistently use macros
[+] The package must contain code, or permissable content
[-] Large documentation files must go in a -doc subpackage
[+] If a package includes something as %doc, it must not affect the runtime of
the application
[+] Header files must be in a -devel package
[-] Static libraries must be in a -static package
[+] library files that end in .so (without suffix) must go in a -devel package
You have separated the *_debug.so files out into a separate package?
[+] devel packages must require the base package using a fully versioned
dependency
[+] Packages must NOT contain any .la libtool archives
[+] GUI apps must include a %{name}.desktop file, properly installed with
desktop-file-install in the %install section 
[+] Packages must not own files or directories already owned by other packages
[+] All filenames in rpm packages must be valid UTF-8

[+] Packaged using KDE macros

Should Items
============
[-] the packager SHOULD query upstream for any missing license text files to
include it
[-] Non-English language support for description and summary sections in the
package spec if available

[+] The reviewer should test that the package builds in mock
[-] The package should compile and build into binary rpms on all supported
architectures
[+] The reviewer should test that the package functions as described
[+] If scriptlets are used, those scriptlets must be sane
[+] Usually, subpackages other than devel should require the base package using
a fully versioned dependency

[-] The placement of pkgconfig(.pc) should usually be placed in a -devel pkg
[-] If the package has file dependencies outside of /etc, /bin, /sbin,
/usr/bin, or /usr/sbin consider requiring the package which provides the file
instead of the file itself
[-] Should contain man pages for binaries/scripts

Hurray, thanks Brendan. I have already contacted upstream.

New Package SCM Request
=======================
Package Name: kde-plasma-publictransport
Short Description: Public Transport plasma applet
Owners: brummbq
Branches: f15 f16
InitialCC:

Git done (by process-git-requests).

pushed to stable