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 196003 - Review Request: Kmenu-gnome
Summary: Review Request: Kmenu-gnome
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Rex Dieter
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-06-20 09:56 UTC by Chitlesh GOORAH
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-08-21 20:40:17 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
kmenu-gnome.spec without dangerous commands in %post and %preun (deleted)
2006-06-29 06:18 UTC, Ariszló
no flags Details
kmenu-gnome-0.5.1-3.src.rpm (deleted)
2006-06-29 06:21 UTC, Ariszló
no flags Details

Description Chitlesh GOORAH 2006-06-20 09:56:04 UTC
Spec URL: http://beta.glwb.info/kmenugnome/kmenu-gnome.spec
SRPM URL: http://beta.glwb.info/kmenugnome/kmenu-gnome-0.5-1.src.rpm
Description:
K Menu with Gnome folder and extra icons for Fedora Core 5.

Comment 1 Chitlesh GOORAH 2006-06-20 09:59:45 UTC
This spec file is rather particular, based on the project leader's spec file,
Ive tried to make it fit to FE policies, but still Its far from getting accept
to Fedora Extras.

rpmlint -i complains:
chitlesh(i386)[0]$rpmlint -i kmenu-gnome-0.5-1.i386.rpm
E: kmenu-gnome no-binary
The package should be of the noarch architecture because it doesn't contain
any binaries.

E: kmenu-gnome standard-dir-owned-by-package /usr/share
This package owns a directory that is part of the standard hierarchy, which
can lead to default directory permissions or ownerships being changed to
something non-standard.

E: kmenu-gnome standard-dir-owned-by-package /usr
This package owns a directory that is part of the standard hierarchy, which
can lead to default directory permissions or ownerships being changed to
something non-standard.

E: kmenu-gnome standard-dir-owned-by-package /etc
This package owns a directory that is part of the standard hierarchy, which
can lead to default directory permissions or ownerships being changed to
something non-standard.

E: kmenu-gnome standard-dir-owned-by-package /usr/share/icons
This package owns a directory that is part of the standard hierarchy, which
can lead to default directory permissions or ownerships being changed to
something non-standard.

W: kmenu-gnome non-conffile-in-etc
/etc/xdg/menus/applications-merged/kmenu-gnome.menu
A non-executable file in your package is being installed in /etc, but is not
a configuration file. All non-executable files in /etc should be configuration
files. Mark the file as %config in the spec file.

W: kmenu-gnome dangerous-command-in-%post rm
W: kmenu-gnome dangerous-command-in-%preun rm

Comment 2 Hans de Goede 2006-06-20 11:14:22 UTC
Chitlesh,

It seems that you misunderstand the sponsoring process. You've already been
sponsored by me for knetstats and you only need to be sponsored once. So now
this is a regular review request which can be done by any FE packager.

Resetting the blocker bug to FE-NEW.

I'll get back to you on your rpmlint issues when I find the time. You'll
probably have to find someone else todo the review though I'm rather busy at the
moment.



Comment 3 Parag AN(पराग) 2006-06-20 11:32:58 UTC
Not an official review as I'm not yet sponsored
Chitlesh,
    I also found that 
 rpmlint -i kmenu-gnome-0.5-1.src.rpm
E: kmenu-gnome invalid-spec-name kmenu-gnome-fc5.spec
Your spec filename must end with '.spec'. If it's not the case, rename your
file and rebuild your package.
   so rename spec file name to kmenu-gnome.spec
also try to list which files this package belongs under %files section


Comment 4 Parag AN(पराग) 2006-06-20 11:56:26 UTC
Ok let me try to solve rpmlint errors for you.

>rpmlint -i complains:
>chitlesh(i386)[0]$rpmlint -i kmenu-gnome-0.5-1.i386.rpm
>E: kmenu-gnome no-binary
>The package should be of the noarch architecture because it doesn't contain
>any binaries.

 Then you can add to SPEC file  BuildArchitectures: noarch

>E: kmenu-gnome standard-dir-owned-by-package /usr/share
>This package owns a directory that is part of the standard hierarchy, which
>can lead to default directory permissions or ownerships being changed to
>something non-standard.

>E: kmenu-gnome standard-dir-owned-by-package /usr
>This package owns a directory that is part of the standard hierarchy, which
>can lead to default directory permissions or ownerships being changed to
>something non-standard.

>E: kmenu-gnome standard-dir-owned-by-package /etc
>This package owns a directory that is part of the standard hierarchy, which
>can lead to default directory permissions or ownerships being changed to
>something non-standard.

>E: kmenu-gnome standard-dir-owned-by-package /usr/share/icons
>This package owns a directory that is part of the standard hierarchy, which
>can lead to default directory permissions or ownerships being changed to
>something non-standard.

   You should not make Systems standard direcory's to belong to your package.
thats what i ask you to add only those files that are installed by your package.

W: kmenu-gnome non-conffile-in-etc
/etc/xdg/menus/applications-merged/kmenu-gnome.menu
A non-executable file in your package is being installed in /etc, but is not
a configuration file. All non-executable files in /etc should be configuration
files. Mark the file as %config in the spec file.

  under %files section you can add
 %config /etc/xdg/menus/applications-merged/kmenu-gnome.menu

W: kmenu-gnome dangerous-command-in-%post rm
W: kmenu-gnome dangerous-command-in-%preun rm
   why the patch is needed to apply at post routine??
 

Comment 5 Rex Dieter 2006-06-21 01:34:56 UTC
I've gotta say this one does some absolutely nasty stuff in %post, including
symlinking icons, and patching xdg menu files, (almost?) all of which could be
implemented via an alternate icon theme and making a proper xdg menu and using
$XDG_CONFIG_DIRS.  IMO, definitely a MUSTFIX.

Comment 6 Chitlesh GOORAH 2006-06-21 08:24:40 UTC
True, It's the spec file from kmenu-gnome, I did my best to make it compatible
with FE policies, but Ill rather need help to complete this process. Ill send an
update soon.

Comment 7 Ariszló 2006-06-22 16:29:33 UTC
(In reply to comment #5)
> I've gotta say this one does some absolutely nasty stuff in %post, including
> symlinking icons, and patching xdg menu files, (almost?) all of which could be
> implemented via an alternate icon theme and making a proper xdg menu and using
> $XDG_CONFIG_DIRS.  IMO, definitely a MUSTFIX.

Symlinking the icons is not essential so I don't mind removing it from
kmenu-gnome.spec but I don't see how symlinking _these specific icons_ would be
harmful. It is unlikely that they would ever be included in Bluecurve or Crystal
SVG.

However, patching xdg merge files like /etc/xdg/menus/system-settings.menu is a
must if you put anything into /etc/xdg/menus/applications-merged to suppress a
bug caused by a harmful piece of code found in (almost?) all of the xdg merge
files installed with redhat-menus.

See https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=196275 for details.

The patch in %post temporarily fixes the bug caused by redhat-menus while the
patch in %preun is a reverse patch to undo what the patch in %post did.

As for making a proper xdg menu and using $XDG_CONFIG_DIRS, I don't understand
how one could do it less intrusively than kmenu-gnome does.


Comment 8 Rex Dieter 2006-06-22 16:37:09 UTC
> I don't see how symlinking _these specific icons_ would be
> harmful.

symlinking icons in %post will make unowned files -> bad.  
Making symlinks in %install, and having the rpm *own* them is ok.

Comment 9 Rex Dieter 2006-06-22 16:40:01 UTC
> As for making a proper xdg menu and using $XDG_CONFIG_DIRS, I don't understand
> how one could do it less intrusively than kmenu-gnome does.

See bug #178320 for one example (of how kde uses a different menu than gnome)
using $XDG_CONFIG_DIRS

Comment 10 Ariszló 2006-06-22 17:07:29 UTC
(In reply to comment #8)
> symlinking icons in %post will make unowned files -> bad.  
> Making symlinks in %install, and having the rpm *own* them is ok.

I see. I will do so.

(In reply to comment #9)
> See bug #178320 for one example (of how kde uses a different menu than gnome)
> using $XDG_CONFIG_DIRS

Yes, it is another solution to keep K Menu and Gnome's Applications menu apart
but it does not fix bug #196275

Going back to the ownership of directories that are part of the standard
hierarchy, it is already fixed in the upcoming release of kmenu-gnome-0.5.1.


Comment 11 Ariszló 2006-06-29 06:18:39 UTC
Created attachment 131715 [details]
kmenu-gnome.spec without dangerous commands in %post and %preun

Comment 12 Ariszló 2006-06-29 06:21:20 UTC
Created attachment 131716 [details]
kmenu-gnome-0.5.1-3.src.rpm

Comment 13 Ariszló 2006-06-29 06:30:10 UTC
Uploaded new spec. It no longer fixes bug #196275 - it only hides it using a
Fedora-specific additional merge file. While fixing a bug is preferable to
hiding it, I accept the criticism that %post is not the best place for patches.

The symlinks are moved under %install so they all are owned by the package.
rpmlint warns about symbolic links pointing nowhere but that's normal:
http://qa.mandriva.com/twiki/bin/view/Main/PackagingProblems#RpmLint_Warnings


Comment 14 Chitlesh GOORAH 2006-06-29 15:29:03 UTC
updated:
Spec URL: http://beta.glwb.info/kmenugnome/kmenu-gnome.spec
SRPM URL: http://beta.glwb.info/kmenugnome/kmenu-gnome-0.5.1-2pre3.src.rpm

%changelog

- dropped gtk-update-icon-cache from %%post and %preun
- corrected the top-level icon directory in %%post and %preun

Comment 15 Hans de Goede 2006-06-29 15:51:29 UTC
(In reply to comment #14)
> %changelog
> 
> - dropped gtk-update-icon-cache from %%post and %preun

Erm, why? You make changes to the icon dirs, thus the cache needs updating.



Comment 16 Chitlesh GOORAH 2006-06-29 16:00:50 UTC
from http://fedoraproject.org/wiki/ScriptletSnippets:
For KDE, just 'touch'ing the top-level icon directory is enough.

Also since kmenu-gnome does not affect on gnome menus, its useless to have
gtk-update-icon-cache.

Comment 17 Hans de Goede 2006-06-29 16:04:32 UTC
I see, could you add a comment to explain this to the spec-file then? This is
sorta in direct contradiction with the scriptlets wiki page. But I guess thats
ok in this case.



Comment 18 Chitlesh GOORAH 2006-06-29 16:12:44 UTC
updated:
Spec URL: http://beta.glwb.info/kmenugnome/kmenu-gnome.spec
SRPM URL: http://beta.glwb.info/kmenugnome/kmenu-gnome-0.5.1-3pre3.src.rpm

Hans, can you do an official review taking into consideration the comment #13 ?

Ariszló is the kmenu-gnome developper.

Comment 19 Hans de Goede 2006-06-29 16:18:27 UTC
Nope,

Sorry Chitlesh, but I'm both rather busy and not a KDE user and this package
looks like it needs a thorough review preferably by someone intimate with KDE.

Rex, maybe this is something for you?


Comment 21 Rex Dieter 2006-07-25 20:54:20 UTC
I'll see if I can bang this one out...

Comment 22 Rex Dieter 2006-07-25 21:18:42 UTC
* upstream source checks out:
9b36e22fc02f022e35fae3241ec9d622  31025-kmenu-gnome-0.6.tar.gz

1.  missing %build section.  Though this is mostly harmless...

2.  drop the use of $RPM_BUILD_DIR.

3.  rm -rf %{buildroot} should be at beginning of %install section, not %prep

4.  scriptlets should (instead) be
touch --no-create %{_datadir}/icons/Bluecurve ||:
touch --no-create %{_datadir}/icons/crystalsvg ||:

similarly, I'd recommend replacing %files
%{_icondir}/*/*/*/*
with
%{_datadir}/icons/Bluecurve/*/*/*
%{_datadir}/icons/crystalsvg/*/*/*

5.  Due to ownership of stuff under %{_datadir}/icons/Bluecurve, to avoid
possible unowned directory, you should
Requires: redhat-artwork
(yucky yes, but unowned dirs are even worse)

6. License.  Should probably be
License: GPL/LGPL
since there are bits of both in this package.

Fix (at least) 2-6, and I'll approve this.

Comment 23 Ariszló 2006-07-27 14:41:17 UTC
> NEEDINFO requested of cgoorah.au

Chitlesh writes at clunixchit.blogspot.com that he will be on vacation until
about the middle of August.

Comment 24 Rex Dieter 2006-07-27 14:45:36 UTC
OK, we can wait. (:

Comment 25 Chitlesh GOORAH 2006-08-19 18:55:42 UTC
I'm back :)

Updated:
SRPM: http://chitlesh.funpic.de/rpm/kmenu-gnome-0.6-3.src.rpm
SPEC: http://chitlesh.funpic.de/rpm/kmenu-gnome.spec

Comment 26 Rex Dieter 2006-08-21 14:34:23 UTC
Excellent, APPROVED.


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