Bug 205023

You might want to review the Packaging Guidelines for FE.  Just a quick look at
your spec, and I can see some things that obviously need to be fixed before this
can be approved. For example, the desktop file is handled incorrectly, and your
using the Vendor & Packager tags.

http://fedoraproject.org/wiki/Packaging/Guidelines

{Not official reviewer}
+ Mockbuild is successfull for i386 FC6 with messages
No translations found for filelight in /var/tmp/filelight-1.0-1-root
don't use %files -f %{name}.lang
use only %files in SPEC file

- rpmlint on SOURCE rpm is NOT silent
I: filelight checking
W: filelight hardcoded-packager-tag Dag
The Packager tag is hardcoded in your spec file. It should be removed, so
as to use rebuilder's own defaults.
=> Remove packager tag

W: filelight setup-not-quiet
You should use -q to have a quiet extraction of the source tarball, as this
generate useless lines of log ( for buildbot, for example )
packaging looks ok.
=> use setup -q instead of setup only
   You need to replace %prep section to
   %prep 
   %setup -q -n %{name}-%{version}

+ dist tag is NOT present
- Buildroot is Must be 
   %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
+ source URL is correct
+ BR is correct
+ License used is GPL
+ License file COPYING is included
- Desktop files are NOT handled correctly.
   Check
http://fedoraproject.org/wiki/Packaging/Guidelines#head-254ddf07aae20a23ced8cecc219d8f73926e9755

+ MD5 sum on tarball is matching upstream tarball
aa885e53e09f40e7fdd371395140b957  filelight-1.0.tar.bz2

- rpmlint on Binary RPM is NOT silent
E: filelight file-in-usr-marked-as-conffile /usr/share/config/filelightrc
A file in /usr is marked as being a configuration file.
Store your conf files in /etc/ instead.

W: filelight conffile-without-noreplace-flag /usr/share/config/filelightrc
A configuration file is stored in your package without the noreplace flag.
A way to resolve this is to put the following in your SPEC file:

%config(noreplace) /etc/your_config_file_here

Follow comment #1

All fixed (hopefully).   Please try:
http://nbecker.dyndns.org:8080/RPM/filelight-1.0-2.src.rpm
http://nbecker.dyndns.org:8080/RPM/filelight.spec

More to do in SPEC
Add %{?dist} to Release tag
Release: 2%{?dist}

Remove macros used in %chnagelog section otherwise rpmlint on src.rpm will NOT
be silent
I: filelight checking
W: filelight macro-in-%changelog files
Macros are expanded in %changelog too, which can in unfortunate cases lead
to the package not building at all, or other subtle unexpected conditions that
affect the build.  Even when that doesn't happen, the expansion results in
possibly "rewriting history" on subsequent package revisions and generally
odd entries eg. in source rpms, which is rarely wanted.  Avoid use of macros
in %changelog altogether, or use two '%'s to escape them, like '%%foo'.

W: filelight macro-in-%changelog _sysconfdir
Macros are expanded in %changelog too, which can in unfortunate cases lead
to the package not building at all, or other subtle unexpected conditions that
affect the build.  Even when that doesn't happen, the expansion results in
possibly "rewriting history" on subsequent package revisions and generally
odd entries eg. in source rpms, which is rarely wanted.  Avoid use of macros
in %changelog altogether, or use two '%'s to escape them, like '%%foo'.

On Binary RPM rpmlint is silent

Still desktop file is not handled properly
add --delete-original option to desktop-file-install
what is second filelight_part.desktop ?

Also tell me what is use of placing filelight_part.desktop in
%{_datadir}/services/filelight_part.desktop
path?

Re: comment #5
It's a reference to a runtime-loadable kpart (ie, KDE plugin).

ok

All fixed, but:
"add --delete-original option to desktop-file-install"
doesn't seem to be mentioned here:
http://fedoraproject.org/wiki/Packaging/Guidelines

http://nbecker.dyndns.org:8080/RPM/filelight-1.0-3.src.rpm
http://nbecker.dyndns.org:8080/RPM/filelight.spec

this option is ised only when final binary rpm will contains duplicate .desktop
files
Like in your case before using desktop-file-install final binary rpm was
installing desktop file to location
/usr/share/applications/kde/filelight.desktop as its part of make install
but when you used desktop-file-install same desktop file will get installed
again with path
/usr/share/applications/fedora-filelight.desktop

Now to remove duplicate files use --delete-original that will delete file
/usr/share/applications/kde/filelight.desktop
so final binary RPM will contain only
/usr/share/applications/fedora-filelight.desktop

OTOH, the original rationale for adding --vendor=fedora is when there isn't an
upstream .desktop file, but that's not true in this case.  I'd recommend instead
using something (simpler) like this instead:
desktop-file-install  \
  --dir ${RPM_BUILD_ROOT}%{_datadir}/applications/kde \
  --add-category="X-Fedora" --vendor=""               \
  ${RPM_BUILD_ROOT}%{_datadir}/applications/kde/filelight.desktop

I think moving filelightrc from %{_datadir}/config to {_sysconfdir} was a 
mistake.  I think it should be put back.  But without %config.

Right, kde *expects* %{_datadir}/config, and, imo, it is still appropriate to
mark it %config, despite what rpmlint says.

(In reply to comment #10)
> OTOH, the original rationale for adding --vendor=fedora is when there isn't 
an
> upstream .desktop file, but that's not true in this case.  I'd recommend 
instead
> using something (simpler) like this instead:
> desktop-file-install  \
>   --dir ${RPM_BUILD_ROOT}%{_datadir}/applications/kde \
>   --add-category="X-Fedora" --vendor=""               \
>   ${RPM_BUILD_ROOT}%{_datadir}/applications/kde/filelight.desktop

I'm ready to submit the package, but I'm waiting to hear a resolution on this 
question.

OK, I'll be official reviewer.  Consider my suggestion a resolution. (:

OK.  Please see

http://nbecker.dyndns.org:8080/RPM/filelight.spec
http://nbecker.dyndns.org:8080/RPM/filelight-1.0-5.src.rpm

I think you meant:
%config %{_datadir}/config/filelightrc
%{_datadir}/applications/kde/filelight.desktop
not
%{_datadir}/config/filelightrc
%config %{_datadir}/applications/kde/filelight.desktop
right?  (:

Yes, thanks.

http://nbecker.dyndns.og:8080/RPM/filelight.spec
http://nbecker.dyndns.org:8080/RPM/filelight-1.0-6.src.rpmr

Rex,

Sorry for hijacking this review, but Neal needs a sponsor (and forgot to set the
FE_NEEDSPONSOR flag on this) and I'm currently in the process of sponsering him.

Also I know /realize that you know much more about KDE then may so feel free to
jump in.


MUST:
=====
* rpmlint output is:
E: filelight file-in-usr-marked-as-conffile /usr/share/config/filelightrc
W: filelight conffile-without-noreplace-flag /usr/share/config/filelightrc
This is normal for KDE packages and can both be ignored
* Package and spec file named appropriately
* Packaged according to packaging guidelines
* License GPL ok and included
* spec file is legible and in Am. English.
* Source matches upstream
* Compiles and builds on devel x86_64
0 BR, some are redundant see must fix
0 No locales, but still %find_lang is used, remove it!
* No shared libraries
* Not relocatable
* Package owns / or requires all dirs
* No duplicate files & Permissions ok
* %clean & macro usage OK
* Contains code only
* %doc does not affect runtime, and isn't large enough to warrent a sub package
* no -devel package needed, no libs
* .la files, but this is ok (KDE exception)
O .desktop file as required, but not properly installed


Must Fix
========
* Drop the "Vendor: Dag Apt Repository, http://dag.wieers.com/apt/" line
* The qt-devel BR is redundant and should be removed as kdelibs-devel already
  requires it
* Drop this line "%find_lang %{name} || touch %{name}.lang" I don't see a 
  -f arg to %files, so clearly this is not needed
* Use desktop-file-install as documented in the Scriptles page of the wiki,
  this is in the review guidelines! If you disagree with the scriptlet page 
  discuss this on the extras mailing list instead of deviating on your own.
* You install files under /usr/share/icons, you must add the nescesarry post 
  postun scriptlets to update the icon-cache, and please use the scriplets 
  exactly as documented. Again if you (or Rex) disagree discuss this on the 
  list.
* You install files under /usr/share/icons/hicolor, so you must Require
  hicolor-icon-theme, which is the "filesystem" equivalent for the 
  /usr/share/icons/hicolor dir hierarchy (I just learned this myself recently)


Should Fix
==========
* Drop the following lines:
# $Id$
# Authority: dag
# Upstream: Max Howell <filelight$methylblue,com>
* "%defattr(-, root, root, 0755)" the FE default for this is
  "%defattr(-,root,root,-)"

Nice way of Hijacking to sponsor anyone.

> Sorry for hijacking this review

No prob.  

>  Use desktop-file-install as documented 

The most important thing is that .desktop files *not* be renamed, and adding
--vendor-fedora does that.  --vendor=fedora is only appropriate if upstream
doesn't (already) provide a .desktop file.

*sigh*, OK, I'll bite the bullet and take this to the Packaging comittee to get
the guidelines updated to reflect reality.

(In reply to comment #20)
> The most important thing is that .desktop files *not* be renamed, and adding
> --vendor-fedora does that.  --vendor=fedora is only appropriate if upstream
> doesn't (already) provide a .desktop file.
> 
> *sigh*, OK, I'll bite the bullet and take this to the Packaging comittee to get
> the guidelines updated to reflect reality.
>

Ah I see, the desktop file is already installed, in that case Neal instead of
not renaming you should add --delete-original to the desktop-file-install
cmdline. Atleast until the packaging comittee changes the guidelines on this.

Please see

http://nbecker.dyndns.org:8080/RPM/filelight.spec
http://nbecker.dyndns.org:8080/RPM/filelight-1.0-8.src.rpm

Also, FE_NEEDSPONSOR is now correctly set?

(Neal, FE_NEEDSPONSOR is indeed set as a blocker).

FYI, here's the packaging guidelines update proposal I'll take to the 
committee:
http://fedoraproject.org/wiki/RexDieter/desktop_files

Looks good, I'll sponsor you now, if you create an account I'll sponsor you and
then you can import and build this and kdiff3 as documented in the wiki.

Neal, looks like you forgot to close this bug after importing and building the
package, closing.

Changed summary for tracking purposes.