Bug 205023 - Review Request: filelight - cool diskspace use browser for kde
Review Request: filelight - cool diskspace use browser for kde
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-09-02 11:32 EDT by Neal Becker
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-11-19 02:20:38 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Neal Becker 2006-09-02 11:32:24 EDT
Spec URL: http://nbecker.dyndns.org:8080/RPM/filelight.spec
SRPM URL: http://nbecker.dyndns.org:8080/RPM/filelight-1.0-1.src.rpm
Description: Filelight graphically represents a file system as a set of concentric
segmented-rings, indicating where diskspace is being used. Segments
expanding from the center represent files (including directories),
with each segment's size being proportional to the file's size and
directories having child segments.
Comment 1 Brian Pepple 2006-09-02 11:58:47 EDT
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
Comment 2 Parag AN(पराग) 2006-09-05 06:08:27 EDT
{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
Comment 4 Parag AN(पराग) 2006-09-05 07:56:13 EDT
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 ?
Comment 5 Parag AN(पराग) 2006-09-05 07:57:47 EDT
Also tell me what is use of placing filelight_part.desktop in
%{_datadir}/services/filelight_part.desktop
path?
Comment 6 Rex Dieter 2006-09-05 08:03:48 EDT
Re: comment #5
It's a reference to a runtime-loadable kpart (ie, KDE plugin).
Comment 7 Parag AN(पराग) 2006-09-05 08:07:35 EDT
ok
Comment 8 Neal Becker 2006-09-05 08:52:02 EDT
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
Comment 9 Parag AN(पराग) 2006-09-05 09:02:34 EDT
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
Comment 10 Rex Dieter 2006-09-05 09:06:59 EDT
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
Comment 11 Neal Becker 2006-09-05 09:10:44 EDT
I think moving filelightrc from %{_datadir}/config to {_sysconfdir} was a 
mistake.  I think it should be put back.  But without %config.
Comment 12 Rex Dieter 2006-09-05 10:11:51 EDT
Right, kde *expects* %{_datadir}/config, and, imo, it is still appropriate to
mark it %config, despite what rpmlint says.
Comment 13 Neal Becker 2006-09-06 08:34:44 EDT
(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.
Comment 14 Rex Dieter 2006-09-06 08:45:27 EDT
OK, I'll be official reviewer.  Consider my suggestion a resolution. (:
Comment 16 Rex Dieter 2006-09-06 09:07:41 EDT
I think you meant:
%config %{_datadir}/config/filelightrc
%{_datadir}/applications/kde/filelight.desktop
not
%{_datadir}/config/filelightrc
%config %{_datadir}/applications/kde/filelight.desktop
right?  (:
Comment 18 Hans de Goede 2006-09-09 05:27:55 EDT
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,-)"
Comment 19 Parag AN(पराग) 2006-09-09 05:42:28 EDT
Nice way of Hijacking to sponsor anyone.
Comment 20 Rex Dieter 2006-09-09 08:24:59 EDT
> 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.
Comment 21 Hans de Goede 2006-09-09 08:55:26 EDT
(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.
Comment 22 Neal Becker 2006-09-11 20:27:34 EDT
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?
Comment 23 Rex Dieter 2006-09-12 14:38:16 EDT
(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
Comment 24 Hans de Goede 2006-09-15 03:13:31 EDT
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.
Comment 25 Hans de Goede 2006-11-19 02:20:38 EST
Neal, looks like you forgot to close this bug after importing and building the
package, closing.
Comment 26 Christian Iseli 2007-01-02 19:25:05 EST
Changed summary for tracking purposes.

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