Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 205023 - Review Request: filelight - cool diskspace use browser for kde
Summary: Review Request: filelight - cool diskspace use browser for kde
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Hans de Goede
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-09-02 15:32 UTC by Neal Becker
Modified: 2007-11-30 22:11 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-11-19 07:20:38 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Neal Becker 2006-09-02 15:32:24 UTC
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 15:58:47 UTC
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 10:08:27 UTC
{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 11:56:13 UTC
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 11:57:47 UTC
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 12:03:48 UTC
Re: comment #5
It's a reference to a runtime-loadable kpart (ie, KDE plugin).

Comment 7 Parag AN(पराग) 2006-09-05 12:07:35 UTC
ok

Comment 8 Neal Becker 2006-09-05 12:52:02 UTC
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 13:02:34 UTC
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 13:06:59 UTC
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 13:10:44 UTC
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 14:11:51 UTC
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 12:34:44 UTC
(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 12:45:27 UTC
OK, I'll be official reviewer.  Consider my suggestion a resolution. (:

Comment 16 Rex Dieter 2006-09-06 13:07:41 UTC
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 09:27:55 UTC
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 09:42:28 UTC
Nice way of Hijacking to sponsor anyone.

Comment 20 Rex Dieter 2006-09-09 12:24:59 UTC
> 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 12:55:26 UTC
(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-12 00:27:34 UTC
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 18:38:16 UTC
(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 07:13:31 UTC
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 07:20:38 UTC
Neal, looks like you forgot to close this bug after importing and building the
package, closing.


Comment 26 Christian Iseli 2007-01-03 00:25:05 UTC
Changed summary for tracking purposes.



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