Bug 461011

Summary: Review Request: kde-plasma-lancelot - An alternative application launcher
Product: [Fedora] Fedora Reporter: Than Ngo <than>
Component: Package ReviewAssignee: Kevin Kofler <kevin>
Status: CLOSED CURRENTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, kevin, notting
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: 2008-11-12 00:07:52 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-09-03 14:01:05 UTC
Spec URL: http://than.fedorapeople.org/kde-plasma-lancelot.spec
SRPM URL: http://than.fedorapeople.org/kde-plasma-lancelot-1.0-1.fc9.src.rpm
Description: An alternative application launcher.

Comment 1 Kevin Kofler 2008-09-04 06:34:21 UTC
Reviewing this.

Comment 2 Kevin Kofler 2008-09-09 12:45:34 UTC
rpmlint output (reordered for clarity):

> kde-plasma-lancelot.src: W: summary-ended-with-dot An alternative application launcher.
> kde-plasma-lancelot.i386: W: summary-ended-with-dot An alternative application launcher.

Summary should not end with a dot, please remove the dot. :-)

> kde-plasma-lancelot.i386: W: devel-file-in-non-devel-package /usr/include/kde4/lancelot/widgets/HoverIcon.h
> kde-plasma-lancelot.i386: W: devel-file-in-non-devel-package /usr/include/kde4/lancelot/layouts/ColumnLayout.h
> kde-plasma-lancelot.i386: W: devel-file-in-non-devel-package /usr/include/kde4/lancelot/widgets/Widget.h
> kde-plasma-lancelot.i386: W: devel-file-in-non-devel-package /usr/include/kde4/lancelot/Global.h
> kde-plasma-lancelot.i386: W: devel-file-in-non-devel-package /usr/include/kde4/lancelot/models/PassagewayViewModels.h
> kde-plasma-lancelot.i386: W: devel-file-in-non-devel-package /usr/include/kde4/lancelot/widgets/ResizeBordersPanel.h
> kde-plasma-lancelot.i386: W: devel-file-in-non-devel-package /usr/include/kde4/lancelot/lancelot_export.h
> kde-plasma-lancelot.i386: W: devel-file-in-non-devel-package /usr/include/kde4/lancelot/widgets/Panel.h
> kde-plasma-lancelot.i386: W: devel-file-in-non-devel-package /usr/include/kde4/lancelot/widgets/ExtenderButton.h
> kde-plasma-lancelot.i386: W: devel-file-in-non-devel-package /usr/lib/liblancelot.so
> kde-plasma-lancelot.i386: W: devel-file-in-non-devel-package /usr/include/kde4/lancelot/widgets/WidgetPositioner.h
> kde-plasma-lancelot.i386: W: devel-file-in-non-devel-package /usr/include/kde4/lancelot/widgets/ActionListView.h
> kde-plasma-lancelot.i386: W: devel-file-in-non-devel-package /usr/include/kde4/lancelot/layouts/CardLayout.h
> kde-plasma-lancelot.i386: W: devel-file-in-non-devel-package /usr/include/kde4/lancelot/widgets/BasicWidget.h
> kde-plasma-lancelot.i386: W: devel-file-in-non-devel-package /usr/include/kde4/lancelot/widgets/PassagewayView.h
> kde-plasma-lancelot.i386: W: devel-file-in-non-devel-package /usr/include/kde4/lancelot/models/ActionListViewModels.h
> kde-plasma-lancelot.i386: W: devel-file-in-non-devel-package /usr/include/kde4/lancelot/lancelot.h

If we want to support building things against liblancelot, these need to go into a kde-plasma-lancelot-devel, otherwise we should just rm -rf them.

> kde-plasma-lancelot.i386: E: library-without-ldconfig-postin /usr/lib/liblancelot.so.0.5.0

Missing:
%post -p /sbin/ldconfig

> kde-plasma-lancelot.i386: E: library-without-ldconfig-postun /usr/lib/liblancelot.so.0.5.0

Missing:
%postun -p /sbin/ldconfig

Can you please fix these first, before I go through my manual checklist?

Comment 3 Kevin Kofler 2008-09-09 12:52:40 UTC
I'd also suggest pointing URL to http://www.kde-apps.org/content/show.php/Lancelot?content=87914 instead of the SVN branch.

Comment 4 Than Ngo 2008-09-09 13:12:01 UTC
i uploaded new spec and srpm that fixes the above issue.

Comment 5 Kevin Kofler 2008-09-09 16:04:41 UTC
MUST Items:
+ rpmlint output OK (no warnings or errors)
+ named and versioned according to the Package Naming Guidelines
+ spec file name matches base package name
+ Packaging Guidelines:
  ! License is listed as GPLv3+, is actually GPLv2+
  + 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
  ! Requires: kdelibs4 >= 4 should be >= 4.1 if mentioned, but it isn't needed at all, as the autoreqs take care of it (libplasma.so.2, i.e. kdebase-workspace-libs 4.1 is automatically dragged in, and that Requires a kdelibs4 >= 4.1 already)
  + no non-UTF-8 characters
  + upstream TODO is included, no other documentation
  + 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 GUI executables (in the proper sense), so no .desktop file needed
  + ... and thus no desktop-file-install needed either
  + no timestamp-clobbering file commands
  + _smp_mflags used
  ! scriptlets:
    mkdir %{buildroot} fails in Rawhide because the parent directory of the new RPM's new buildroot is not getting created by mock (unless that has been fixed), it's probably easiest to just omit it and let make install create the directories recursively. (The buildroot used by the Rawhide RPM is secure against race conditions, by the way, it's under the user's home directory.)
  + not a web application, so web application guideline doesn't apply
  + no conflicts
+ complies with all the legal guidelines
! COPYING not packaged as %doc
! source does not match upstream 1.0 release at:
  http://sourceforge.net/project/showfiles.php?group_id=238004&package_id=289266&release_id=622496
  The source tarball is polluted with .svn directories, probably a checkout rather than an export. Please use the upstream tarball instead!
! source tarball should be specified using full URL according to the guidelines for SourceForge-hosted packages
  https://fedoraproject.org/wiki/Packaging/SourceURL
  -> http://downloads.sourceforge.net/lancelot-menu/lancelot-1.0.tar.bz2
+ builds on at least one arch (F9 Koji scratch build on all arches)
+ no non-working arches, so no ExcludeArch needed
+ no missing BuildRequires (builds in Koji)
+ no translations, so translation/locale guidelines don't apply
+ ldconfig correctly called in %post and %postun
+ package not relocatable
! unowned directory %{_kde4_appsdir}/desktoptheme/default/lancelot/
  -> please remove the * from %{_kde4_appsdir}/desktoptheme/default/lancelot/*
  (other than that, no package-specific directories which would have to be owned, 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 a -devel subpackage
+ no static libraries, so no -static package needed
+ no .pc files, so no Requires: pkgconfig needed
+ no devel symlinks which would need to be in a -devel subpackage
+ plugins in %{_kde4_libdir}/kde4/ are correctly NOT in a -devel subpackage
+ no -devel package, so "-devel should require %{name} = %{version}-%{release}" is irrelevant
+ no .la files
+ no GUI executables, so 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, on all architectures (F9 Koji scratch build)
* not tested functionality (will do once the other stuff is fixed)
+ 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


Please fix the following MUST FIX items:
* License is listed as GPLv3+, is actually GPLv2+
* remove unneeded Requires: kdelibs4 >= 4, a more precise dependency is already implied automatically
* remove the explicit mkdir %{buildroot} (at least until the mock used by Koji gets fixed)
* add COPYING as %doc
* use the upstream tarball from: http://sourceforge.net/project/showfiles.php?group_id=238004&package_id=289266&release_id=622496
* Source: should have full URL: http://downloads.sourceforge.net/lancelot-menu/lancelot-1.0.tar.bz2
* remove the * from %{_kde4_appsdir}/desktoptheme/default/lancelot/* (to own the directory)

Once these are fixed, I'll do a short test to make sure it actually works and then I'll approve it.

Comment 6 Than Ngo 2008-09-09 20:05:20 UTC
it's fixed now. Please take a look at new specfile and srpms. Thanks

Comment 7 Kevin Kofler 2008-09-09 20:23:44 UTC
All objections addressed. Tarball checksums (matching upstream):
MD5SUM: 20ad6b20817ac0f0e6903930c27e7d85
SHA1SUM: e1dc6ddee25de98ba7ba81b953486c60406d1362

I tested this quickly, appears to work fine.

APPROVED

Comment 8 Than Ngo 2008-09-09 20:38:00 UTC
New Package CVS Request
=======================
Package Name: kde-plasma-lancelot
Short Description: An alternative application launcher
Owners: than
Branches: F-9, F-10
InitialCC: than

Comment 9 Kevin Fenzi 2008-09-09 23:42:11 UTC
cvs done.

Comment 10 Than Ngo 2008-09-10 14:06:00 UTC
packages are already commited in CVS.

Comment 11 Kevin Kofler 2008-11-12 00:07:52 UTC
That stuff has been built and pushed for a while.