Bug 550143 - Review Request: webattery - Command line tool to display battery status
Summary: Review Request: webattery - Command line tool to display battery status
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-12-23 18:14 UTC by Alagunambi Welkin
Modified: 2010-02-04 17:49 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-02-04 17:49:22 UTC
Type: ---
Embargoed:
mtasaka: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Alagunambi Welkin 2009-12-23 18:14:10 UTC
Spec URL: http://download.sf.net/webattery/webattery.spec
SRPM URL: http://download.sf.net/webattery/webattery-1.1-1.fc10.src.rpm
Description: Webattery is a command line tool to display battery status with a cool battery like output. 
This my first package, need Sponsor.

Comment 1 Thomas Spura 2009-12-23 18:27:29 UTC
Just a few comments, because I'm no sponsor anyway:

- $ rpmlint webattery-1.1-1.fc12.src.rpm x86_64/webattery-*
webattery.src: W: no-cleaning-of-buildroot %install
webattery.src: W: no-buildroot-tag
webattery-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/webattery-1.1/src/webattery.c
3 packages and 0 specfiles checked; 0 errors, 3 warnings.

  * in %install needs to be rm -rf %{buildroot} as first line
  * no-buildroot-tag can be ignored, this is not needed anymore in fedora
    (only if you want to import it in EL-5, too...)
  * webattery.c needs to be 644, please change this in %install

Comment 2 Alagunambi Welkin 2009-12-23 20:08:02 UTC
(In reply to comment #1)
> 
>   * in %install needs to be rm -rf %{buildroot} as first line
>   * no-buildroot-tag can be ignored, this is not needed anymore in fedora
>     (only if you want to import it in EL-5, too...)
>   * webattery.c needs to be 644, please change this in %install  

Cleared all, should I replace the given src.rpm and spec file with the new one or do I need to add them separately along with the old src.rpm and sepc file?

Comment 3 Alagunambi Welkin 2009-12-24 05:37:44 UTC
(In reply to comment #1)

>   * in %install needs to be rm -rf %{buildroot} as first line

I was recommend to leave this part since it is no need[1], thanks to mether :-D

my new spec URL: https://sourceforge.net/projects/webattery/files/webattery-rpm/webattery.spec/download
my new src.rpm URL: https://sourceforge.net/projects/webattery/files/webattery-rpm/webattery-1.1-2.fc10.src.rpm/download

Waiting for a Sponsor :-)

Comment 4 Alagunambi Welkin 2009-12-24 05:41:34 UTC
> I was recommend to leave this part since it is no need[1], thanks to mether :-D

[1] https://fedoraproject.org/wiki/Packaging/Guidelines#Prepping_BuildRoot_For_.25install

Comment 5 Christoph Wickert 2009-12-24 10:19:41 UTC
I'd leave both

BuildRoot:

and 

%install
rm -rf $RPM_BUILD_ROOT

Without it, the package will build on Fedora >= 10 only but not on EPEL or alike.


The license tag is wrong. COPYING is GPLv3 and the source states "version 3 or any later version", so the tag should be GPLv3+


Please change

%{_mandir}/man1/webattery.1.gz

to

%{_mandir}/man1/webattery.1.*

because the compression of the manpages is applied automatically to the package and we might switch to something different than gz.

Comment 6 Alagunambi Welkin 2009-12-24 12:49:57 UTC
(In reply to comment #5)

> The license tag is wrong. COPYING is GPLv3 and the source states "version 3 or
> any later version", so the tag should be GPLv3+

Changed

> %{_mandir}/man1/webattery.1.*

Changed

Thanks for correcting me

Here are the new modified spec and src.rpm files

SPEC URL: https://sourceforge.net/projects/webattery/files/webattery-rpm/webattery.spec-1.1-3/download
SRPM URL: https://sourceforge.net/projects/webattery/files/webattery-rpm/webattery-1.1-3.fc10.src.rpm/download

Comment 9 Till Maas 2010-01-04 12:38:07 UTC
(In reply to comment #5)

> Please change
> 
> %{_mandir}/man1/webattery.1.gz
> 
> to
> 
> %{_mandir}/man1/webattery.1.*
> 
> because the compression of the manpages is applied automatically to the package
> and we might switch to something different than gz.  

It might also not be compressed at all, therefore this is even better (the last "." is removed):

%{_mandir}/man1/webattery.1*

Comment 10 Alagunambi Welkin 2010-01-05 15:21:14 UTC
(In reply to comment #9)

Thanks for your comment

New SPEC and SRPM can be found here,

SPEC: http://downloads.sourceforge.net/webattery/webattery-spec-1.1-5
SRPM: http://downloads.sourceforge.net/webattery/webattery-1.1-5.fc10.src.rpm

Comment 11 Mamoru TASAKA 2010-01-16 17:58:56 UTC
Some notes:

* Source
  - Source tarball contained in your srpm does not coincide with
    what I could download from the URL written in your spec file:
-----------------------------------------------------------
181106 2009-12-22 18:07 downloaded/webattery-1.1.tar.gz
180043 2009-12-24 04:59 webattery-1.1-5.fc10.src/webattery-1.1.tar.gz
-----------------------------------------------------------

* CFLAGS
  - 'CFLAGS="%{optflags}"' on "make" line is not needed as
    %configure correctly sets this value
    ( see what %configure actually does by
      $ rpm --eval %configure )

* Timestamp
  - Please consider to use
-----------------------------------------------------------
make install DESTDIR=%{buildroot} INSTALL="install -p"
-----------------------------------------------------------
    to keep timestamps on installed files. This method
    usually works for Makefiles generated by recent
    autotools.

! warnings
  - By the way, would you take care of these warnings?
-----------------------------------------------------------
    92  webattery.c:266: warning: format '%s' expects type 'char *', but argument 4 has type 'const char (*)[4]'
    93  webattery.c:270: warning: format '%s' expects type 'char *', but argument 3 has type 'const char (*)[15]'
    94  webattery.c:276: warning: format '%s' expects type 'char *', but argument 4 has type 'const char (*)[4]'
    95  webattery.c:282: warning: format '%s' expects type 'char *', but argument 3 has type 'const char (*)[30]'
    96  webattery.c:282: warning: format '%s' expects type 'char *', but argument 4 has type 'const char (*)[4]'
    97  webattery.c:287: warning: format '%s' expects type 'char *', but argument 3 has type 'const char (*)[20]'
    98  webattery.c:293: warning: format '%s' expects type 'char *', but argument 3 has type 'const char (*)[40]'
    99  webattery.c:293: warning: format '%s' expects type 'char *', but argument 4 has type 'const char (*)[3]'
   100  webattery.c:299: warning: format '%s' expects type 'char *', but argument 3 has type 'const char (*)[20]'
   101  webattery.c:299: warning: format '%s' expects type 'char *', but argument 4 has type 'const char (*)[4]'
   102  webattery.c:305: warning: format '%s' expects type 'char *', but argument 3 has type 'const char (*)[20]'
   103  webattery.c:305: warning: format '%s' expects type 'char *', but argument 4 has type 'const char (*)[4]'
   104  webattery.c:309: warning: format '%s' expects type 'char *', but argument 3 has type 'const char (*)[20]'
   105  webattery.c:315: warning: format '%s' expects type 'char *', but argument 3 has type 'const char (*)[20]'
   106  webattery.c:315: warning: format '%s' expects type 'char *', but argument 4 has type 'const char (*)[20]'
   107  webattery.c:319: warning: format '%s' expects type 'char *', but argument 3 has type 'const char (*)[20]'
   108  webattery.c:323: warning: format '%s' expects type 'char *', but argument 3 has type 'const char (*)[20]'
   109  webattery.c:327: warning: format '%s' expects type 'char *', but argument 3 has type 'const char (*)[20]'
   110  webattery.c:333: warning: format '%s' expects type 'char *', but argument 4 has type 'const char (*)[3]'
   111  webattery.c:339: warning: format '%s' expects type 'char *', but argument 3 has type 'const char (*)[20]'
   112  webattery.c:339: warning: format '%s' expects type 'char *', but argument 4 has type 'const char (*)[3]'
-----------------------------------------------------------

Comment 12 Alagunambi Welkin 2010-01-24 19:36:02 UTC
(In reply to comment #11)

Thanks for your valuable comment

Here is the new spec file and SRPM

SPEC URL: http://downloads.sourceforge.net/webattery/webattery-spec-1.2-6
SRPM URL: http://downloads.sourceforge.net/webattery/webattery-1.2-6.fc10.src.rpm

Comment 13 Mamoru TASAKA 2010-01-25 15:56:09 UTC
Well, this package itself is now okay, so:

-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few (or no) work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on my wiki page:
http://fedoraproject.org/wiki/User:Mtasaka#B._Review_request_tickets
(Check "No one is reviewing")

Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------

Comment 14 Alagunambi Welkin 2010-01-27 15:20:44 UTC
(In reply to comment #13)

Thanks for your time,

Here are my non-formal reviews

1) https://bugzilla.redhat.com/show_bug.cgi?id=549366
2) https://bugzilla.redhat.com/show_bug.cgi?id=550104
3) https://bugzilla.redhat.com/show_bug.cgi?id=542458

Comment 15 Mamoru TASAKA 2010-01-27 17:02:09 UTC
Well, all of the spec files in the links you pre-reviewed
were already modified after your comments, however your comments
themselves seem good.

----------------------------------------------------------
  This package (webattery) is APPROVED by mtasaka
----------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Install the Client Tools (Koji)".

Now I am sponsoring you.

If you want to import this package into Fedora 11/12, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

Removing NEEDSPONSOR.

Comment 16 Alagunambi Welkin 2010-01-27 23:41:44 UTC
Hi, 

Thanks and please create CVS. and please let me know your IRC Nick. Myne is SkyKnight @ freenode

New Package CVS Request
=======================
Package Name: webattery
Short Description: Command line tool to display battery power status
Owner: Alagunambi Welkin
Branches: F-11 F-12 EL-5
InitialCC:

Comment 17 Jason Tibbitts 2010-01-27 23:52:17 UTC
Can't process that CVS request; "Alagunambi Welkin" is not a valid Fedora account.  Please submit a corrected request and re-set the fedora-cvs flag.

Comment 18 Alagunambi Welkin 2010-01-28 05:24:24 UTC
New Package CVS Request
=======================
Package Name: webattery
Short Description: Command line tool to display battery power status
Owner: alagunambi
Branches: F-11 F-12 EL-5
InitialCC:

Comment 19 Jason Tibbitts 2010-01-31 18:31:35 UTC
These are processed by a script.  I'll adjust it so that it accepts "Owner" instead of "Owners" but there really shouldn't be any reason to alter the template.

Comment 20 Alagunambi Welkin 2010-01-31 19:04:27 UTC
New Package CVS Request
=======================
Package Name: webattery
Short Description: Command line tool to display battery power status
Owners: alagunambi
Branches: F-11 F-12 EL-5
InitialCC:

Comment 21 Kevin Fenzi 2010-01-31 19:17:53 UTC
CVS done (by process-cvs-requests.py).


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