Bug 193929 - Review Request: knetstats
Review Request: knetstats
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-06-02 19:59 EDT by Chitlesh GOORAH
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-06-17 15:28:59 EDT
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 Chitlesh GOORAH 2006-06-02 19:59:31 EDT
Spec URL: http://beta.glwb.info/knetstats/knetstats.spec
SRPM URL: http://beta.glwb.info/knetstats/knetstats-1.5-1.src.rpm
Description: A simple KDE network monitor that show rx/tx LEDs or numeric information about
the transfer rate of any network interface in a system tray icon.
Comment 1 Chitlesh GOORAH 2006-06-02 20:03:21 EDT
Hello, there is my first package to fedora extras :)

I have 2 issues with it though:

1. how can I add a link to the menu ?

2. I can't compile it with QA_RPATHS=$[ 0x0001|0x0010 ]. Any Workaround ?
Comment 2 Chitlesh GOORAH 2006-06-04 13:01:58 EDT
issue 1 : it automatically adds itself in Kmenu->Internet

issue 2 : rpmbuilding through QA_RPATHS=$[ 0x0001|0x0010 ] rpmbuild -ba
knetstats.spec

Updated:
Spec URL: http://beta.glwb.info/knetstats/knetstats.spec
SRPM URL: http://beta.glwb.info/knetstats/knetstats-1.5-2.src.rpm
Comment 3 Ian Chapman 2006-06-04 14:54:48 EDT
Not a formal review, but here's a few things I've noticed.

1. You should probably patch the SConstruct file to remove rpath from the
environment. See 'env.KDEuse("environ rpath")' in the file.

2. Add -q to the %setup macro

3. Since you're using %{version} in the source tag, consider using the %{name}
macro too.

4. Drop gcc-c++ from the buildrequires, see
http://fedoraproject.org/wiki/Packaging/Guidelines?highlight=%28CategoryExtras%29 

5. Perhaps a matter of preference, but consider using the scons supplied with
the source, ie drop the scons BR and use ./scons in the build section.

6. Use the %{?_smp_mflags} on the scons build line, to allow parallel building

7. The install process attempts to create files outside the buildroot:

"mkdir: cannot create directory `/usr/share/doc/HTML/en/knetstats': Permission
denied"

  This must be fixed.

8. The last changelog entry is missing its release number (im assuming it should
have been -1)

9. To fix the desktop file, one solultion might be to delete the .desktop from
the buildroot after scons install 'rm -f
$RPM_BUILD_ROOT%{_datadir}/applnk/Internet/knetstats.desktop', then use
desktop-file-install to install it yourself. Remember to add desktop-file-utils
as a BR

Comment 4 Chitlesh GOORAH 2006-06-07 17:16:23 EDT
Thanks for such a reply,

I have updated the spec file with respect to what have been documented above except:

No. 7.

Because I was unable to get scons root access/permissions while its execution.

No. 9.
Because It automatically adds itself to the kmenu->Internet. Do I have to use
desktop-file-utils nevertheless ?
Comment 6 Hans de Goede 2006-06-08 06:21:01 EDT
Hi Chitlesh,

I see (from google) that you're already active in other area's fo the Fedora
project, good! As such I'm willing to sponsor you, but first you must show some
more / better understanding of the FE packaging guidelines.

There are 2 ways to show this better understanding:
1) You review a couple of packages from others see bug 163776 for a list of
   Review Requests that need a Reviewer, don't worry about not being competent
   enough todo a review, just add me to the CC-list and I'll watch over your 
   back.
2) Create some more packages and put me in the CC for the Review Request

Or (probably the best) a combination of these 2.

Now some remarks related to this review:
(In reply to comment #4)
> Thanks for such a reply,
> 
> I have updated the spec file with respect to what have been documented above
except:
> 
> No. 7.
> 
> Because I was unable to get scons root access/permissions while its execution.
> 
You really _must_ fix this, the trick is not to give scons root permission, but
to change the build so that it doesnot try to install files outside of the buildroot

> No. 9.
> Because It automatically adds itself to the kmenu->Internet. Do I have to use
> desktop-file-utils nevertheless ?

Yes, and since it already puts the .desktop file under /usr/share/applicatiuons
you should use the --delete-original option to desktop-file-install.
Comment 7 Ian Chapman 2006-06-08 07:27:05 EDT
Couple of other points, it might be better to leave a blank line between
different %changelog entries as it's easier on the eye and perhaps be a little
more verbose as there's no mention of the other changes you made such as
dropping gcc-c++ etc. 

I read somewhere on the wiki (although I can't find it at the mo), that patches
should be named in the following way to avoid name collisions with other packages:

name-version-description.patch

For example:
knetstats-1.5-rpathfix.patch

It also advised against using the %{name} and %{version} macros here,
particularly because the version indicates the version of the package this patch
was introduced in, so for example if you were to upgrade to version 1.6, the
patch would still be knetstats-1.5-rpathfix.patch, if it was still applicable.
Anyway Hans will keep you right, he sponsored me too :)

 
Comment 8 Rex Dieter 2006-06-08 07:49:09 EDT
NEEDSWORK:

* use icon cache scriptlets, see "GTK+ icon cache" on 
http://www.fedoraproject.org/wiki/ScriptletSnippets

* use "desktop-file-install" for .desktop file, see 
"desktop files" on 
http://www.fedoraproject.org/wiki/Packaging/Guidelines

Since this is your first package submissino, it's a good idea to look over all 
of those ScriptletSnippets and Packaging Guidelines pages, if you haven't done 
so already.
Comment 9 Chitlesh GOORAH 2006-06-14 00:23:28 EDT
------- Additional Comments From cgoorah AT yahoo com au  2006-06-11 08:19 EST
-------
Updated:
Spec URL: http://beta.glwb.info/knetstats/knetstats.spec
SRPM URL: http://beta.glwb.info/knetstats/knetstats-1.5-4.src.rpm

I have made the necessary modifications and read the guidelines as well :)
Now I need to put that into action :)

Nevertheless, I was unable to fix the

mkdir -p /usr/share/doc/HTML/en/knetstats && cd /usr/share/doc/HTML/en/knetstats
&& rm -f common && ln -s ../common commonln: creating symbolic link `common' to
`../common': Permission denied

How can I change the build so that it doesnot try to install files outside of
the buildroot ?
Comment 10 Chitlesh GOORAH 2006-06-14 00:24:49 EDT
------- Additional Comments From j.w.r.degoede AT hhs nl  2006-06-11 16:08 EST
-------
(In reply to comment #9)
> Updated:
> Spec URL: http://beta.glwb.info/knetstats/knetstats.spec
> SRPM URL: http://beta.glwb.info/knetstats/knetstats-1.5-4.src.rpm
>
> I have made the necessary modifications and read the guidelines as well :)
> Now I need to put that into action :)
>

Okay,

Some (semi) initial remarks:
-You BuildRequire qt-devel and you also BuildRequire kdelibs-devel, however
 kdelibs-devel Requires qt-devel itself, so the BR qt-devel is redundant remove
 please.
-update-desktop-database is deprecated, remove please as well as the belongen
 Requires(%post[un])
-as already mentioned in comment #8 you must properly update the icon cache in
 %post[un] see the wiki scriptlets page
-don't call /sbin/ldconfig in %post[un] this package does not seem to contain
 any libs
-instead of:
 ###
 rm -rf $RPM_BUILD_ROOT%{_datadir}/applnk/Internet/%{name}.desktop

 desktop-file-install --vendor fedora \
    --add-category Network \
    --dir $RPM_BUILD_ROOT%{_datadir}/applnk/Internet/ \
    %{name}.desktop
 ###
 use:
 ###
 desktop-file-install --vendor fedora \
    --add-category X-Fedora \
    --add-category Network \
    --delete-original \
    --dir $RPM_BUILD_ROOT%{_datadir}/applications/ \
    $RPM_BUILD_ROOT%{_datadir}/applnk/Internet/%{name}.desktop
 ###
 Notice that the changed --dir, desktop files should be installed in
 %{_datadir}/applications/ nowadays. Also notice the --delete-original
 replacing the seperate rm command and last notice the additional
 "--add-category X-Fedora" param which all fedora packages should use.
 Also don't forget to update %files for the changed dir.




> Nevertheless, I was unable to fix the
>
> mkdir -p /usr/share/doc/HTML/en/knetstats && cd /usr/share/doc/HTML/en/knetstats
> && rm -f common && ln -s ../common commonln: creating symbolic link `common' to
> `../common': Permission denied
>
> change the build so that it doesnot try to install files outside of the
buildroot ?
> How can I

Okay, this is because of a dirty hack in upstreams sources, there are 2 possible
fixes:
1) ignore the error (scons already does this) and add the following at the end
  of %install:
  ln -s ../common $RPM_BUILD_ROOT%{_datadir}/doc/HTML/en/knetstats/common
2) patch admin/kde.py to properly honor DESTDIR.
Comment 11 Chitlesh GOORAH 2006-06-14 00:25:42 EDT
------- Additional Comments From cgoorah AT yahoo com au  2006-06-11 16:55 EST
-------
Hello Hans,
thanks for your remarks,

> Some (semi) initial remarks:
> -You BuildRequire qt-devel and you also BuildRequire kdelibs-devel, however
>  kdelibs-devel Requires qt-devel itself, so the BR qt-devel is redundant remove

Removed.

>  please.
> -update-desktop-database is deprecated, remove please as well as the belongen
>  Requires(%post[un])
> -as already mentioned in comment #8 you must properly update the icon cache in
>  %post[un] see the wiki scriptlets page
> -don't call /sbin/ldconfig in %post[un] this package does not seem to contain
>  any libs

Updated to

%post
touch --no-create %{_datadir}/icons/hicolor || :

%postun
touch --no-create %{_datadir}/icons/hicolor || :

> -instead of:
>  ###
>  rm -rf $RPM_BUILD_ROOT%{_datadir}/applnk/Internet/%{name}.desktop
>
>  desktop-file-install --vendor fedora \
>      --add-category Network \
>      --dir $RPM_BUILD_ROOT%{_datadir}/applnk/Internet/ \
>      %{name}.desktop
>  ###
>  use:
>  ###
>  desktop-file-install --vendor fedora \
>      --add-category X-Fedora \
>      --add-category Network \
>      --delete-original \
>      --dir $RPM_BUILD_ROOT%{_datadir}/applications/ \
>      $RPM_BUILD_ROOT%{_datadir}/applnk/Internet/%{name}.desktop
>  ###
>  Notice that the changed --dir, desktop files should be installed in
>  %{_datadir}/applications/ nowadays. Also notice the --delete-original
>  replacing the seperate rm command and last notice the additional
>  "--add-category X-Fedora" param which all fedora packages should use.
>  Also don't forget to update %files for the changed dir.
>

done .

>
>
> > Nevertheless, I was unable to fix the
> >
> > mkdir -p /usr/share/doc/HTML/en/knetstats && cd /usr/share/doc/HTML/en/knetstats
> > && rm -f common && ln -s ../common commonln: creating symbolic link `common' to
> > `../common': Permission denied
> >
> > change the build so that it doesnot try to install files outside of the
> buildroot ?
> > How can I
>
> Okay, this is because of a dirty hack in upstreams sources, there are 2 possible
> fixes:
> 1) ignore the error (scons already does this) and add the following at the end
>    of %install:
>    ln -s ../common $RPM_BUILD_ROOT%{_datadir}/doc/HTML/en/knetstats/common
> 2) patch admin/kde.py to properly honor DESTDIR.
>

I was able to overcome that Permission Denied issue with
%install
rm -fr $RPM_BUILD_ROOT
scons prefix=$RPM_BUILD_ROOT%{_prefix} install

but with rpmlint -i knetstats-1.5-5.i386.rpm, Ive fallen on
W: knetstats dangling-relative-symlink /usr/share/doc/HTML/en/knetstats/common
../common
The relative symbolic link points nowhere.

With f13's advice I've patched admin/kde.py accordingly.

Again with f13's advise, I've included scons as BR.

Updated:
Spec URL: http://beta.glwb.info/knetstats/knetstats.spec
SRPM URL: http://beta.glwb.info/knetstats/knetstats-1.5-5.src.rpm
Comment 12 Chitlesh GOORAH 2006-06-14 00:26:40 EDT
------- Additional Comments From rdieter AT math unl edu  2006-06-11 18:23 EST
-------
Re: comment #10
> -update-desktop-database is deprecated

It certainly is not deprecated (what makes you say that?).

If the app's .desktop file contains MimeType= entries, ScriptletSnippets are
clear that use of update-desktop-database in %post/%postun is required, (though
IMO, there shouldn't be need for additional Requires for this, just like the
icon cache case).
Comment 13 Chitlesh GOORAH 2006-06-14 00:27:26 EDT
------- Additional Comments From cgoorah AT yahoo com au  2006-06-12 01:42 EST
-------
(In reply to comment #12)
> Re: comment #10
> > -update-desktop-database is deprecated
>
> It certainly is not deprecated (what makes you say that?).

Hans, you want to comment on this ?

Anyone want to sponsor me for knetstats ? Hans ?
Comment 14 Chitlesh GOORAH 2006-06-14 00:28:06 EDT
------- Additional Comments From j.w.r.degoede hhs nl  2006-06-12 02:22 EST -------
(In reply to comment #12)
> Re: comment #10
> > -update-desktop-database is deprecated
>
> It certainly is not deprecated (what makes you say that?).
>

Sorry, deprected in deed is the wrong way to formulate this, what I meant is
usually calling update-desktop-database is no longer needed unless:

> the app's .desktop file contains MimeType= entries, ScriptletSnippets are
> clear that use of update-desktop-database in %post/%postun is required.

---

(In reply to comment #13)
> (In reply to comment #12)
> > Re: comment #10
> > > -update-desktop-database is deprecated
> >
> > It certainly is not deprecated (what makes you say that?).
>
> Hans, you want to comment on this ?
>

See above, the knetstats does not contain any MimeType's, so the call to
update-desktop-database should be removed (and the Requires).

> Anyone want to sponsor me for knetstats ? Hans ?
>

Quoting myself from comment #6:

I see (from google) that you're already active in other area's of the Fedora
project, good! As such I'm willing to sponsor you, but first you must show some
more / better understanding of the FE packaging guidelines.

There are 2 ways to show this better understanding:
1) You review a couple of packages from others see FE-NEW for a list of
  Review Requests that need a Reviewer, don't worry about not being competent
  enough todo a review, just add me to the CC-list and I'll watch over your
  back.
2) Create some more packages and put me in the CC for the Review Request

So yes, I'm willing to sponsor you (eventually) provided that you show a
somewhat better understanding of the packaging guidelines then you've done
sofar. (Sofar is ok, you're still learning I understand!).

So lets first get this package approved (but not yet imported as your not
sponsored), and then I think it would be a good idea for you to package
something else (maybe you already have) and get that reviewed too, usually after
2 packages most people are beginning to understand the guidelines good enough to
get sponsored (good enough for my taste atleast).
Comment 15 Hans de Goede 2006-06-14 03:44:04 EDT
Good job on restoring those lost comments!

About the review, I've taken a quick glance at:
Spec URL: http://beta.glwb.info/knetstats/knetstats.spec

And you don't properly update the icon cache, the correctway is:
touch --no-create %{_datadir}/icons/hicolor || :
if [ -x %{_bindir}/gtk-update-icon-cache ]; then
   %{_bindir}/gtk-update-icon-cache --quiet %{_datadir}/icons/hicolor || :
fi


And yes you need the gtk-update-icon-cache for KDE-apps too, because you want
the icon to show properly in the gnome applications menu.

About the dangling symlink, that dir is provided by kdelibs:
[hans@shalem ~]$ rpm -qf /usr/share/doc/HTML/en/common
kdelibs-3.5.3-4.x86_64

Your package should automaticly require kdelibs because of .so dependencies, but
in this case you could explicitly Require it, or better perhaps you could add a:
Requires: /usr/share/doc/HTML/en/common

Please post a new version with these things fixed and then I'll do a full review
(and sponsor you once this package is approved).
Comment 16 Chitlesh GOORAH 2006-06-16 02:22:11 EDT
Here you go :)

Spec URL: http://beta.glwb.info/knetstats/knetstats.spec
SRPM URL: http://beta.glwb.info/knetstats/knetstats-1.5-6.src.rpm
Comment 17 Rex Dieter 2006-06-16 09:02:30 EDT
Re: comment #15
> And yes you need the gtk-update-icon-cache for KDE-apps too, because 
>you want the icon to show properly in the gnome applications menu.

FYI, only the 'touch' is strictly required for proper function (and adherance to
the fdo icon spec).  gtk-update-icon-cache only improves gtk2's icon loading
performance.
Comment 18 Hans de Goede 2006-06-16 14:12:26 EDT
MUST:
=====
* rpmlint output is clean
* Package and spec file named appropriately
* Packaged according to packaging guidelines
* License (GPL) ok, license file included
* spec file is legible and in Am. English.
* Source matches upstream
* Compiles and builds on devel-x86_64
* BR: ok
* ocales properly handled
* 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.
* .desktop file as required and properly installed


MUST fix:
=========
* Source0 is wrong (my fault) it should be:
 Source0:   http://dl.sf.net/sourceforge/%{name}/%{name}-%{version}.tar.bz2
* the common symlink is broken now due to your changes it now points to common,
 whereas it should point to ../common


(In reply to comment #17)
> Re: comment #15
> > And yes you need the gtk-update-icon-cache for KDE-apps too, because 
> >you want the icon to show properly in the gnome applications menu.
> 
> FYI, only the 'touch' is strictly required for proper function (and adherance to
> the fdo icon spec).  gtk-update-icon-cache only improves gtk2's icon loading
> performance.


Nope, when you install a package while running gnome and it doesn't properly
call gtk-update-icon-cache the icon doesnot show in the menu, atleast not during
that session. I've made that mistake myself enough times to know this.
Comment 19 Rex Dieter 2006-06-16 14:19:07 EDT
> Nope, when you install a package while running gnome and it doesn't properly
> call gtk-update-icon-cache the icon doesnot show in the menu, atleast not 
> during that session. I've made that mistake myself enough times to 
> know this.

Sounds more like a gtk2 bug that it doesn't validate it's icon cache.
Comment 20 Chitlesh GOORAH 2006-06-16 15:08:53 EDT
(In reply to comment #18)

Hello,

> MUST fix:
> =========
> * Source0 is wrong (my fault) it should be:
>  Source0:   http://dl.sf.net/sourceforge/%{name}/%{name}-%{version}.tar.bz2
> * the common symlink is broken now due to your changes it now points to common,
>  whereas it should point to ../common
> 

I've updated it and dropped the knetstats-1.5-kde.patch

Spec URL: http://beta.glwb.info/knetstats/knetstats.spec
SRPM URL: http://beta.glwb.info/knetstats/knetstats-1.5-7.src.rpm
Comment 21 Hans de Goede 2006-06-16 16:50:03 EDT
All blockers fixed -> Approved!

If you create an account in the account system and sign the CLA as described here:
http://fedoraproject.org/wiki/Extras/Contributors

Then I'll sponsor you, after which you can import the package into CVS as
described on the same page. It could be that you already have an account since
you contribute to other areas of Fedora, in that case add yourself to the
cvsextras group.

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