Bug 477199

Summary: Review Request: PolicyKit-kde - PolicyKit integration for the KDE desktop
Product: [Fedora] Fedora Reporter: Than Ngo <than>
Component: Package ReviewAssignee: Kevin Kofler <kevin>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, kevin, notting, rdieter
Target Milestone: ---Flags: kevin: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-07-26 20:26:02 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Than Ngo 2008-12-19 16:40:07 UTC
Spec URL: http://than.fedorapeople.org/PolicyKit-kde.spec
SRPM URL: http://than.fedorapeople.org/PolicyKit-kde-0.20081219svn-1.fc11.src.rpm
Description: This package provides a PolicyKit Authentication Frontend for KDE

Comment 1 Kevin Kofler 2008-12-19 19:40:17 UTC
First thing I found (while building this so I have binary packages to run rpmlint on):
> BuildRequires:  PolicyKit-devel >= 0.9

This should be >= 0.8, as it's all the package needs and all we have in F9.

Comment 2 Kevin Kofler 2008-12-19 19:49:36 UTC
rpmlint output:
> PolicyKit-kde.i386: W: devel-file-in-non-devel-package /usr/lib/libpolkitkdeprivate.so

This library calls itself "private", is it really? If nothing should link against it, we should not ship this symlink at all. But if it is getting linked against, it should be in a -devel package (even if that's the only file in -devel).

The strange thing is: shouldn't there be some public API for the client library? Why is the only library in this package called "private"?

> PolicyKit-kde.i386: E: library-without-ldconfig-postin /usr/lib/libpolkitkdeprivate.so.4.1.0

Missing "%post -p /sbin/ldconfig".

> PolicyKit-kde.i386: E: library-without-ldconfig-postun /usr/lib/libpolkitkdeprivate.so.4.1.0

Missing "%postun -p /sbin/ldconfig".

> PolicyKit-kde.i386: W: incoherent-version-in-changelog 0.-0.20081219svn-1 ['0.20081219svn-1.fc9', '0.20081219svn-1']

I already mentioned this one in IRC, you said you uploaded a fixed specfile, but it's still broken in both the specfile and the SRPM which are now up.

Comment 3 Than Ngo 2008-12-22 21:33:44 UTC
http://than.fedorapeople.org/PolicyKit-kde-0.0-0.20081219svn.fc10.src.rpm
http://than.fedorapeople.org/PolicyKit-kde.spec

>This library calls itself "private", is it really? If nothing should link
>against it, we should not ship this symlink at all. But if it is getting linked
>against, it should be in a -devel package (even if that's the only file in
>-devel).
>The strange thing is: shouldn't there be some public API for the client
>library? Why is the only library in this package called "private"?

yes, it's private. There is no public API at the moment, but it will follow later as Dario even told me.

>Missing "%post -p /sbin/ldconfig
>Missing "%postun -p /sbin/ldconfig

it's fixed


>I already mentioned this one in IRC, you said you uploaded a fixed specfile,
>but it's still broken in both the specfile and the SRPM which are now

it's fixed.

PS: Dario will move it to extragear soon and it will follow the KDE version scheme in the future. our package will use 0.0 as version temporary, we will fix it later when PolicyKit-kde official released.

Comment 4 Kevin Kofler 2008-12-22 22:02:18 UTC
> PS: Dario will move it to extragear soon and it will follow the KDE version
> scheme in the future. our package will use 0.0 as version temporary, we will
> fix it later when PolicyKit-kde official released.

Makes sense.

rpmlint now comes up with only this harmless warning:
PolicyKit-kde-devel.i386: W: no-documentation

I'm going to go through the checklist ASAP.

Comment 5 Kevin Kofler 2008-12-24 00:18:07 UTC
MUST Items:
+ rpmlint output OK (see comment #4)
+ named and versioned according to the Package Naming Guidelines
+ spec file name matches base package name
+ Packaging Guidelines:
  ! License is listed as GPLv2+, should list "GPLv2+ and LGPLv2+" because the client library is LGPLv2+, otherwise OK
  + No known patent problems
  + No emulator, no firmware, no binary-only or prebuilt components
  + Complies with the FHS
  + proper changelog, tags, BuildRoot, BuildRequires, Summary, Description
  + no non-UTF-8 characters
  + the only relevant documentation is COPYING, included as %doc
  + RPM_OPT_FLAGS are used (%cmake_kde4 macro)
  + debuginfo package is valid
  + no static libraries nor .la files
  + no duplicated system libraries
  + no rpaths
  + no configuration files, so %config guideline doesn't apply
  + no init scripts, so init script guideline doesn't apply
  + no .desktop files needed: the only executable is polkit-kde-authorization and its functionality is available through systemsettings
  + ... and thus no desktop-file-install needed either
  + no timestamp-clobbering file commands
  + _smp_mflags used
  + scriptlets are valid
  + not a web application, so web application guideline doesn't apply
  + no conflicts
+ complies with all the legal guidelines
+ COPYING packaged as %doc
+ source compares identical to export of revision 898968 from upstream SVN
+ builds on at least one arch (F9 i386 mock)
+ no known non-working arches, so no ExcludeArch needed
+ no missing BuildRequires (builds in mock)
+ no translations, so translation/locale guidelines don't apply
+ ldconfig correctly called in %post and %postun
+ package not relocatable
+ ownership correct (owns package-specific directories, doesn't own directories owned by another package)
+ no duplicate files in %files
+ permissions correct, defattr used correctly
+ %clean section present and correct
+ macros used where possible
+ no non-code content
+ no large documentation files, so no -doc package needed
+ no %doc files required at runtime
+ no header files which would need to be in the -devel subpackage
+ no static libraries, so no -static package needed
+ no .pc files, so no Requires: pkgconfig needed
+ devel symlinks correctly in the -devel subpackage
+ plugin (KCM) in %{_kde4_libdir}/kde4/ is correctly NOT in a -devel subpackage
+ -devel package has "Requires: %{name} = %{version}-%{release}"
+ no .la files
+ no .desktop file needed
+ buildroot is deleted at the beginning of %install
+ all filenames are valid UTF-8

SHOULD Items:
+ license already included upstream
+ no translations for description and summary provided by upstream
+ package builds in mock (F9 i386)
+ passes basic functionality test (polkit-kde-authorization resp. the KCM look OK, service not tested yet)
+ scriptlets are sane
+ no subpackages other than -devel, so "Usually, subpackages other than devel should require the base package using a fully versioned dependency." is irrelevant
+ no .pc files, so "placement of .pc files" is irrelevant
+ no file dependencies

Questions (non-blockers):
* Should polkit-kde-authorization appear in the menu as a standalone app or is it enough to have it in systemsettings? (IMHO the latter, but there _is_ a standalone executable provided by upstream.)
* Do we really want to ship a -devel package right now when there are no installed headers yet? (Not that it will matter in the long term as a public API is planned.)

This package is APPROVED.
Please clarify the License tag from "GPLv2+" to "GPLv2+ and LGPLv2+" before, during or after import.

Comment 6 Than Ngo 2009-01-08 11:34:38 UTC
>! License is listed as GPLv2+, should list "GPLv2+ and LGPLv2+" because the
>client library is LGPLv2+, otherwise OK

it's fixed in new package

>* Should polkit-kde-authorization appear in the menu as a standalone app or is
>it enough to have it in systemsettings? (IMHO the latter, but there _is_ a
>standalone executable provided by upstream.)

it shouldn't appear in the menu. The polkit-kde-authorization should be started on demand. The another setting tool should be in systemsettings.

>* Do we really want to ship a -devel package right now when there are no
>installed headers yet? (Not that it will matter in the long term as a public
>API is planned.)

imo it doesn't make sense to ship the devel package that only has the symlink and there are installed headers yet. New package doesn't include -devel subpackage.

Comment 7 Kevin Kofler 2009-01-08 11:59:16 UTC
New Package CVS Request
=======================
Package Name: PolicyKit-kde
Short Description: PolicyKit integration for the KDE desktop
Owners: than rdieter kkofler ltinkl jreznik arbiter
Branches: F-9 F-10
InitialCC: tuxbrewr

Comment 8 Kevin Fenzi 2009-01-09 05:36:41 UTC
cvs done.