Bug 760033 - Review Request: kde-plasma-publictransport - Public Transport plasma applet
Review Request: kde-plasma-publictransport - Public Transport plasma applet
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Brendan Jones
Fedora Extras Quality Assurance
:
Depends On:
Blocks: kde-reviews
  Show dependency treegraph
 
Reported: 2011-12-05 04:57 EST by Gregor Tätzner
Modified: 2012-02-17 13:40 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-02-17 13:40:55 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
brendan.jones.it: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Gregor Tätzner 2011-12-05 04:57:55 EST
Spec URL: http://brummbq.fedorapeople.org/plasma-applet-publictransport.spec
SRPM URL: http://brummbq.fedorapeople.org/kde-plasma-publictransport-0.10-20111204git.fc16.0.src.rpm
Description: Hey dudes I created a publictransport-plasmoid package. I'm using master because it's easier to package :) (global cmake file). The package contains several apps/plasma-widgets. I'm unsure about the package splitting so I need your help there. 

PublicTransport is a plasma applet that shows a departure/arrival board 
for a given stop. It can also show journeys to or from the given "home stop".
Comment 1 Gregor Tätzner 2011-12-05 05:07:14 EST
source obtained via: git archive --format=tar --remote=git://anongit.kde.org/publictransport HEAD | gzip > publictransport_20111204git.tar.gz
Comment 3 Kevin Kofler 2011-12-05 12:32:54 EST
Great, I'd really like to see the publictransport plasmoid in Fedora!

Are you already sponsored?
Comment 4 Gregor Tätzner 2011-12-05 12:53:55 EST
yes! I'm the maintainer for semantik, very easy :D
Comment 5 Gregor Tätzner 2011-12-13 13:11:55 EST
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
Comment 6 Kevin Kofler 2011-12-20 19:15:43 EST
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.
Comment 7 Gregor Tätzner 2011-12-21 06:18:19 EST
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)
Comment 8 Kevin Kofler 2011-12-21 10:12:20 EST
> 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.)
Comment 9 Gregor Tätzner 2011-12-22 10:23:06 EST
(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.
Comment 10 Brendan Jones 2012-01-16 12:38:59 EST
I can take this review.
Comment 11 Brendan Jones 2012-01-17 13:09:55 EST
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.
Comment 13 Brendan Jones 2012-01-19 16:20:26 EST
Hi Greg, you'll need it on %posttrans as well. 

I'll try and finish this for you tomorrow.
Comment 15 Brendan Jones 2012-01-20 18:05:34 EST
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
Comment 16 Gregor Tätzner 2012-01-21 09:30:49 EST
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:
Comment 17 Jon Ciesla 2012-01-21 14:33:54 EST
Git done (by process-git-requests).
Comment 18 Gregor Tätzner 2012-02-17 13:40:55 EST
pushed to stable

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