Bug 209614

Summary: Review Request: wmmemload - windowmaker dock app
Product: [Fedora] Fedora Reporter: David Kovalsky <dkovalsk>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED WONTFIX QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: benl, pertusus
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2008-01-21 04:13:25 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 201449    

Description David Kovalsky 2006-10-06 08:40:42 EDT
Spec URL: http://www.kovalsky.cz/packages/wmmemload.spec
SRPM URL: http://www.kovalsky.cz/packages/wmmemload-0.1.6-1.src.rpm
Description: DockApp for window managers such as WindowMaker showing CPU memory load


Please note this is my first package in Extras -> need a sponsor
Comment 1 David Kovalsky 2006-10-06 08:45:54 EDT
ooops, s/CPU//
Comment 2 Patrice Dumas 2006-10-07 04:58:24 EDT
I don't have access to the .src.rpm.

I have many comments on the spec file (some are in fact blockers, some 
are really comments), though:

* I would personally have dropped 'for window managers such as WindowMaker'
  from the summary, since it will be shorter, and also those apps 
  are well suited to fluxbox, for example if I'm not wrong.
* the provide wmmemload is unusefull, it is automatically set by rpm
* The Epoch is not needed. In my opinion it is clearer if it is not 
  mentioned when set to 0
* the buildroot is not the preferred one
http://fedoraproject.org/wiki/Packaging/Guidelines#head-f196e7b2477c2f5dd97ef64e8eacddfb517f1aa1
  (although it has been agreed that %(%{__id_u} -n) could be removed).
* The Requires are not needed, there are picked up automatically by rpm
* BuildRequires: libX11-devel is optional since libX11-devel is required
  by libXext-devel or libXpm-devel
* export CFLAGS="$RPM_OPT_FLAGS" is unneeded, it is part of %configure
* if I'm not wrong, prefixing with %{_builddir}/%{name}-%{version}/ is 
  not needed in %install since it is what the current working directory 
  is set to.
* in %files, I think it is better to use
%{_mandir}/man1/wmmemload.1*
instead of
%{_mandir}/man1/wmmemload.1.gz
  to catch no compression and different compression schemes.
* in the changelog, I think the 0: corresponding with epoch is unneeded
Comment 3 David Kovalsky 2006-10-11 17:28:10 EDT
(In reply to comment #2)
> I don't have access to the .src.rpm.
Sorry, the permissions got messed up while copying


> * I would personally have dropped 'for window managers such as WindowMaker'
>   from the summary, since it will be shorter, and also those apps 
>   are well suited to fluxbox, for example if I'm not wrong.
Agreed, fixed


> * the provide wmmemload is unusefull, it is automatically set by rpm
removed


> * The Epoch is not needed. In my opinion it is clearer if it is not 
>   mentioned when set to 0
removed


> * the buildroot is not the preferred one
>
http://fedoraproject.org/wiki/Packaging/Guidelines#head-f196e7b2477c2f5dd97ef64e8eacddfb517f1aa1
>   (although it has been agreed that %(%{__id_u} -n) could be removed).
not much of a difference, but no proble to fix :-)



> * The Requires are not needed, there are picked up automatically by rpm
oh, didn't know about that, thanks for the info, fixed



> * BuildRequires: libX11-devel is optional since libX11-devel is required
>   by libXext-devel or libXpm-devel
aaah, ok, removed


> * export CFLAGS="$RPM_OPT_FLAGS" is unneeded, it is part of %configure
removed


> * if I'm not wrong, prefixing with %{_builddir}/%{name}-%{version}/ is 
>   not needed in %install since it is what the current working directory 
>   is set to.
ok, removed



> * in %files, I think it is better to use
> %{_mandir}/man1/wmmemload.1*
> instead of
> %{_mandir}/man1/wmmemload.1.gz
>   to catch no compression and different compression schemes.
looking good to me, modified


> * in the changelog, I think the 0: corresponding with epoch is unneeded
fixed :-)

still have to upload the files together with the other package, I'll add another
comment when I do that tomorrow morning
Comment 4 David Kovalsky 2006-10-12 10:30:37 EDT
Uploaded the fixed files:
http://www.kovalsky.cz/packages/wmmemload.spec
http://www.kovalsky.cz/packages/wmmemload-0.1.6-1.src.rpm

Thank you for the review notes!
Comment 5 Patrice Dumas 2006-10-12 15:49:36 EDT
Everytime you repost the package you are submitting after 
having made a change, you should bump the release and add
a changelog entry (just like how you would have done when
modifying a package), remake a src.rpm and repost it. There may
be exceptions for minor changes, but the rule is to repost a 
new srpm.

parallel make is preferred:
http://fedoraproject.org/wiki/Packaging/Guidelines#head-525c7d76890cb22df33b759c65c35c82bf434d2e

regarding the glob for the man pages
%{_mandir}/man1/wmmemload.1.*
won't catch the case when there is no compression, while
%{_mandir}/man1/wmmemload.1*
do. This is not a blocker.

During the build make triggers the whole autotools stuff. This is bad:

.......
+ make
cd . && aclocal
acinclude.m4:8: warning: underquoted definition of DA_CHECK_LIB
  run info '(automake)Extending aclocal'
  or see http://sources.redhat.com/automake/automake.html#Extending-aclocal
acinclude.m4:20: warning: underquoted definition of DA_CHECK_HEADER
cd . && automake --gnu --include-deps Makefile
cd . && autoconf
/bin/sh ./config.status --recheck
....
Comment 6 Patrice Dumas 2006-10-12 15:59:26 EDT
The files ChangeLog AUTHORS THANKS COPYING should go in %doc
Comment 7 Patrice Dumas 2007-04-25 04:22:54 EDT
What is the status of this submission?
Comment 8 David Kovalsky 2007-04-25 05:12:23 EDT
Hi Patrice, 

wmmemload uses internal libdockapp as well as wmcpuload. After a long time I've
about rewriten wmcpuload to work with libdockap 0.6 which is already in extras,
this will need the same work.

I'll get right on it once I submit wmcpuload again.

Comment 9 Patrice Dumas 2007-04-25 05:44:08 EDT
Ok. As a side note I am the libdockapp maintainer in fedora 
(was needed for wmacpi update). Be sure to use the libdockapp api
for command line args processing to have the -w option that
allows to run in a window. 
Comment 10 Jason Tibbitts 2007-07-04 11:31:43 EDT
Has there been any progress here?
Comment 11 Jason Tibbitts 2007-07-28 11:31:59 EDT
No progress in three months; setting NEEDINFO.
Comment 12 David Kovalsky 2007-08-07 17:00:24 EDT
Sorry, been swamped with other things over the last few months. I'll try to post
the work that I have done so far so if someone with spares cycles wants to
continue can do so. 
Comment 13 Jason Tibbitts 2008-01-20 17:26:25 EST
Any progress?  It's been another five months; perhaps this ticket should be closed.
Comment 14 David Kovalsky 2008-01-21 04:13:25 EST
Indeed, I can't find the time to invest the effort ATM.

Sorry.