Bug 477199 - Review Request: PolicyKit-kde - PolicyKit integration for the KDE desktop
Review Request: PolicyKit-kde - PolicyKit integration for the KDE desktop
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Kofler
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-12-19 11:40 EST by Ngo Than
Modified: 2009-07-26 16:26 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-07-26 16:26:02 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Ngo Than 2008-12-19 11:40:07 EST
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 14:40:17 EST
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 14:49:36 EST
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 Ngo Than 2008-12-22 16:33:44 EST
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 17:02:18 EST
> 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-23 19:18:07 EST
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 Ngo Than 2009-01-08 06:34:38 EST
>! 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 06:59:16 EST
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 00:36:41 EST
cvs done.

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