Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 226322 - Merge Review: psmisc
Summary: Merge Review: psmisc
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Gwyn Ciesla
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: F9MergeReviewTarget
TreeView+ depends on / blocked
 
Reported: 2007-01-31 20:43 UTC by Nobody's working on this, feel free to take it
Modified: 2009-05-07 16:57 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-05-07 16:57:52 UTC
gwync: fedora-review+


Attachments (Terms of Use)

Description Nobody's working on this, feel free to take it 2007-01-31 20:43:51 UTC
Fedora Merge Review: psmisc

http://cvs.fedora.redhat.com/viewcvs/devel/psmisc/
Initial Owner: kzak@redhat.com

Comment 1 Andy Grimm 2007-02-03 21:24:51 UTC
I reviewed this one.  Just a few things:

rpmlint on ./psmisc-22.2-5.src.rpm
W: psmisc summary-ended-with-dot Utilities for managing processes on your system.
W: psmisc macro-in-%changelog _includedir

Please use the %{dist} in release

BuildRequires should require gettext rather than gettext-devel

Otherwise, it looks good.

Comment 2 Ruben Kerkhof 2007-02-04 18:15:57 UTC
Andy, please reassign tickets back to the requester and switch the fedora-review flag to
-, if you deny the request
+, if you approve it.

See http://www.fedoraproject.org/wiki/WarrenTogami/ReviewWithFlags for more info.

Comment 3 Andy Grimm 2007-02-05 17:07:25 UTC
Ruben, sorry for not following procedure.  I should have explained that I'm not
sponsored yet, so I do not have access to reassign tickets.

Comment 4 Karel Zak 2007-03-01 14:48:54 UTC
Fixed. Update to psmisc-22.3-1.fc7. Thanks for your review.

Comment 5 Gwyn Ciesla 2008-09-18 14:11:42 UTC
Adding current owner.

Karel, what is the status here?  I see you are the previous owner.  Any objection to my taking ownership of the bug, and completing the review?

Comment 6 Daniel Novotny 2008-09-19 10:55:13 UTC
Jon, I am the new maintainer of psmisc. You can take the ownership and complete the review.

Comment 7 Gwyn Ciesla 2008-09-19 12:35:31 UTC
Excellent.  I'll get right on it.

Comment 8 Gwyn Ciesla 2008-09-19 13:00:44 UTC
Full review is stellar.

One think I might ask, and it's not worth a rebuild, but could you comment in the spec on the purpose and upstream status of the patches?  Just got added to the Guidelines and I think it's a good practice.

Comment 9 Gwyn Ciesla 2008-12-04 19:35:34 UTC
Ping?

Comment 10 Gwyn Ciesla 2009-03-31 15:26:14 UTC
Ping again?

Comment 11 Michal Nowak 2009-04-08 16:41:51 UTC
Daniel, can you please follow up to Jon's proposals?

--

Also:

* Not necessary to define globally when used only for make

  export CFLAGS="$RPM_OPT_FLAGS -D_GNU_SOURCE"

also not sure whether is "-D_GNU_SOURCE" necessary, shouldn't $RPM_OPT_FLAGS take care of everything?

* don't mix variable and macro style 

https://fedoraproject.org/wiki/Packaging/Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS

* %defattr(-,root,root)

A little bit old-school to me. Use

  %defattr(-,root,root,-)

* point URL field to http://sourceforge.net/projects/psmisc/ the present one is useless (bogus content there)

Comment 12 Daniel Novotny 2009-05-06 13:58:21 UTC
hello,

I made the suggested .spec changes:

 * added purpose comments to all patches
 * fixed URL
 * fixed %defattr

as for the others:
> * Not necessary to define globally when used only for make
configure and autoreconf doesn't use those? I'm not sure...
> * don't mix variable and macro style 
as far as I can see, there's only variable style for
those two variables

output: 
http://people.fedoraproject.org/~dnovotny/psmisc.review.spec

Comment 13 Michal Nowak 2009-05-06 14:12:59 UTC
(In reply to comment #12)
> [...]
> output: 
> http://people.fedoraproject.org/~dnovotny/psmisc.review.spec  

Looks good to me. (Don't forget tu bump changelog entry when in CVS, please.)

Jon: Are you satisfied? Will you approve it, or should I?

Comment 14 Gwyn Ciesla 2009-05-07 16:57:52 UTC
I'm happy.  Commit.

APPROVED.

Thanks everyone!


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