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 165485 - Review Request: wmacpi - This is a port of WMApm 1.1 with ACPI support
Summary: Review Request: wmacpi - This is a port of WMApm 1.1 with ACPI support
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: José Matos
QA Contact: David Lawrence
URL: http://www.ne.jp/asahi/linux/timecop/
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2005-08-09 19:06 UTC by Andreas Bierfert
Modified: 2008-05-24 17:56 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-08-29 09:01:04 UTC
Type: ---
Embargoed:
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Andreas Bierfert 2005-08-09 19:06:11 UTC
Spec Name or Url: http://fedora.lowlatency.de/review/wmacpi.spec
SRPM Name or Url: http://fedora.lowlatency.de/review/wmacpi-1.34-1.src.rpm
Description:
This is a port of WMApm 1.1 with ACPI support

Comment 1 José Matos 2005-08-19 19:18:53 UTC
Could you please improve the summary and description of the packages?    
    
It was very difficult to me to know what it did other than reading the    
included files.   
    
Believe me or not that is my main complain with the spec file. :-)    
    
One other nitpick, please add a blank line over %clean, just to increase the   
readability of the spec file.   
   
You could also replace the version number by %{version} in the %{source} line  
as well as using %{?dist} in the release file. 
 

Comment 2 José Matos 2005-08-19 21:22:56 UTC
The spec file is missing one BR: xorg-x11-libs or else you cann't link with 
X11. 

Comment 3 Andreas Bierfert 2005-08-19 22:43:57 UTC
I will add dist later after importing. xorg-x11-libs is not needed because it
gets pulled in from BR: xorg-x11-devel. Other than that:

http://fedora.lowlatency.de/review/wmacpi-1.34-2.src.rpm
http://fedora.lowlatency.de/review/wmacpi.spec

Comment 4 José Matos 2005-08-20 07:20:07 UTC
You are right about the building requirement, it was my mistake I 
misunderstood a problem I had using mock to generate the package in x86_64. 
 
Mock fails because in the makefile we have: 
 
LDFLAGS = $(OPT) -L/usr/X11R6/lib -lX11 -lXpm -lXext 
 
As you can see this is will fail for 64 bits where the right path ends with 
lib64. 
 
Notice also that OPT is used here, this is a bug from the original package. We 
do not optimize when linking, do we? That is an honest question, but even if 
you do I would expect it to be the same as the remaining Makefile. 
 

Comment 6 José Matos 2005-08-20 10:39:22 UTC
+ Builds inside mock on x86_64 
+ rpmlint output: 
W: wmacpi no-version-in-last-changelog 
 
  I guess that is due to the version being in the next line. Easy to fix. 
 
+ package name follows rules 
+ the spec file is named correctly, it is written in English and it is legible 
+ the license is correct (GPL), the same as upstream and it is included in 
%doc 
+ The source file matches upstream. (sha1sum) 
+ BR are correct 
+ there are no locales, %post*, devel or doc subpackages to worry about 
+ the package is not relocatable 
+ there is no directory to be owned, and file permissions are correct 
 
I still do not like the description, I would expect something like (I took 
this from the web page with some small changes): 
 
This is a typical laptop ACPI dockapp. One interesting feature is the "timer" 
mode, where you can keep track of how long the laptop has been "on battery". 
This is opposite of the information usually provided by the BIOS, which is 
"time remaining", and in many cases wrong. This option can be toggled at 
run-time. System messages scroll on the bottom of the window, AC plug flashes 
when battery is charging, and green LED inside the big button flashes red if 
battery level is critical low. 
 
I think that the word laptop is also missing in the the Summary field, I think 
that you agree that this is only usefull for laptops. I am not aware of any 
desktop with an included battery. ;-) UPS does not count as such, at least for 
this program scope :-). 
 
I noticed that this program does not give any reliable information if you have 
two battery plugs and only the second one is present. But this is an upstream 
issue. 
 
The package is APPROVED. 

Comment 7 Andreas Bierfert 2005-08-20 10:52:05 UTC
Thx for the review... issues fixed... :)

Comment 8 José Matos 2006-08-29 08:55:44 UTC
Reopening bug to fix assignee.



Comment 9 José Matos 2006-08-29 09:01:04 UTC
Assignee fixed, closing again.

Comment 10 Christian Iseli 2006-10-18 09:01:57 UTC
Normalize summary field for easy parsing

Comment 11 Patrice Dumas 2008-05-23 14:53:46 UTC
I would like to have that package in EL-4 and EL-5. Would you like
to maintain it in EL, and if not could I do it?

Comment 12 Andreas Bierfert 2008-05-23 17:14:41 UTC
Fine by me. Feel free to maintain it and add me as a co-maintainer, should be
very low profile...

Comment 13 Patrice Dumas 2008-05-23 19:12:02 UTC
Package Change Request
======================
Package Name: wmacpi
New Branches: EL-4 EL-5
Updated EPEL Owners: pertusus, awjb

Comment 14 Kevin Fenzi 2008-05-24 17:56:39 UTC
cvs done.


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