Bug 461571 - Review Request: sugar-log -- Log activity for sugar
Review Request: sugar-log -- Log activity for sugar
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jeremy Katz
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-09 04:31 EDT by Simon Schampijer
Modified: 2010-06-18 11:51 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-09-18 13:53:27 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
katzj: fedora‑review+


Attachments (Terms of Use)
SPEC URL (updated) (44 bytes, text/plain)
2008-09-09 04:31 EDT, Simon Schampijer
no flags Details
The source rpm (57 bytes, text/plain)
2008-09-12 08:31 EDT, Simon Schampijer
no flags Details
new SRPM (57 bytes, text/plain)
2008-09-13 06:44 EDT, Simon Schampijer
no flags Details

  None (edit)
Description Simon Schampijer 2008-09-09 04:31:53 EDT
Created attachment 316157 [details]
SPEC URL (updated)

Log activity for sugar
Comment 1 Jeremy Katz 2008-09-09 15:56:25 EDT
A couple of things from a quick look
1) Need src.rpm link so that source can be verified against upstream
2) Shouldn't need gettext buildrequires anymore as I added the requires to sugar-toolkit so that it isn't needed in every package
Comment 2 Simon Schampijer 2008-09-12 08:31:18 EDT
Created attachment 316565 [details]
The source rpm
Comment 3 Simon Schampijer 2008-09-12 08:33:27 EDT
Jeremy, thanks for the review

1) i added the src.rpm
2) removed the gettext buildrequires
3) removed the psuedo.po check since we do remove it in the upcoming sugar-toolkit rpm
Comment 4 Jeremy Katz 2008-09-12 09:33:22 EDT
Okay, going through to do a more complete review.

* NEEDSWORK: The URL given for the tarball doesn't exist, it looks like Log-15 never got uploaded?  So can't verify against upstream source
* FYI: Please bump the release in the future when making changes, even as the result of review comments 
* FYI: Please request that upstream include a version of the GPL as COPYING within their source tarball


Builds fine in mock, rpmlint output has a few things which need fixing

* sugar-log.noarch: E: zero-length /usr/share/sugar/activities/Log.activity/README
sugar-log.noarch: E: zero-length /usr/share/doc/sugar-log-15/README

If it's empty, it's probably not worth including

* sugar-log.noarch: E: non-executable-script /usr/share/sugar/activities/Log.activity/logviewer.py 0644
sugar-log.noarch: E: non-executable-script /usr/share/sugar/activities/Log.activity/logcollect.py 0644

These should have the #!/usr/bin/python removed from the top if they're not meant to be directly executed.

* sugar-log.noarch: E: description-line-too-long Log is an activity for developers to examine the log files that are generated by

The description lines are supposed to be limited to 72 (iirc) characters

* sugar-log.noarch: W: non-standard-group Sugar/Activities

There's a bug tracking adding this as acceptable, so fine

* sugar-log.noarch: E: incorrect-locale-subdir /usr/share/locale/pseudo/LC_MESSAGES/org.laptop.Log.mo

If this will be getting fixed up in a future sugar-toolkit, then I'm fine with leaving the problem for now and it'll just get fixed up as things rebuild
Comment 5 Simon Schampijer 2008-09-13 06:42:08 EDT
(In reply to comment #4)
> Okay, going through to do a more complete review.
> 
> * NEEDSWORK: The URL given for the tarball doesn't exist, it looks like Log-15
> never got uploaded?  So can't verify against upstream source

Sorry they are uploaded now.

> * FYI: Please bump the release in the future when making changes, even as the
> result of review comments 

Sure.

> * FYI: Please request that upstream include a version of the GPL as COPYING
> within their source tarball

Done. 

> Builds fine in mock, rpmlint output has a few things which need fixing
> 
> * sugar-log.noarch: E: zero-length

Done.

> /usr/share/sugar/activities/Log.activity/README
> sugar-log.noarch: E: zero-length /usr/share/doc/sugar-log-15/README
> 
> If it's empty, it's probably not worth including

Removed from the sources.

> * sugar-log.noarch: E: non-executable-script
> /usr/share/sugar/activities/Log.activity/logviewer.py 0644
> sugar-log.noarch: E: non-executable-script
> /usr/share/sugar/activities/Log.activity/logcollect.py 0644
> These should have the #!/usr/bin/python removed from the top if they're not
> meant to be directly executed.

Fixed.
 
> * sugar-log.noarch: E: description-line-too-long Log is an activity for
> developers to examine the log files that are generated by

Done.
 
> The description lines are supposed to be limited to 72 (iirc) characters
> 
> * sugar-log.noarch: W: non-standard-group Sugar/Activities
> 
> There's a bug tracking adding this as acceptable, so fine
> 
> * sugar-log.noarch: E: incorrect-locale-subdir
> /usr/share/locale/pseudo/LC_MESSAGES/org.laptop.Log.mo
> 
> If this will be getting fixed up in a future sugar-toolkit, then I'm fine with
> leaving the problem for now and it'll just get fixed up as things rebuild

Yeah we just wait for another fix to get in.
Comment 6 Simon Schampijer 2008-09-13 06:44:56 EDT
Created attachment 316664 [details]
new SRPM
Comment 7 Rakesh Pandit 2008-09-13 06:47:04 EDT
Changed status from NEW to ASSIGNED
Comment 8 Jeremy Katz 2008-09-14 15:19:19 EDT
Great, looks good.

APPROVED.
Comment 9 Huzaifa S. Sidhpurwala 2008-09-16 23:11:59 EDT
Simon, 
Can you please add the CVS request in the specified format please.
It helps to cvs admin to determine a lot of things

---- EXAMPLE ----

New Package CVS Request
=======================
Package Name: foo
Short Description: foo bar foo
Owners: bar
Branches: F-8 F-9
InitialCC:
Comment 10 Simon Schampijer 2008-09-17 03:02:17 EDT
Sorry - missed this :/

New Package CVS Request
=======================
Package Name: sugar-log
Short Description: Tool to examine sugar log files 
Owners: erikos
Branches: F-9
InitialCC: mpg

Thanks!
Comment 11 Kevin Fenzi 2008-09-17 15:00:54 EDT
cvs done.
Comment 12 Jeremy Katz 2008-09-18 13:53:27 EDT
And this is built
Comment 13 Peter Robinson 2010-06-10 16:50:00 EDT
Package Change Request
======================
Package Name: sugar-log
New Branches: EL-6
Owners: pbrobinson sdz
Comment 14 Kevin Fenzi 2010-06-11 00:37:00 EDT
Have you checked with erikos to see if he would like to maintain in EPEL?

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