Bug 209614 - Review Request: wmmemload - windowmaker dock app
Review Request: wmmemload - windowmaker dock app
Status: CLOSED WONTFIX
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Package Reviews List
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2006-10-06 08:40 EDT by David Kovalsky
Modified: 2014-03-31 19:44 EDT (History)
2 users (show)

See Also:
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:


Attachments (Terms of Use)

  None (edit)
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.

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