Bug 226322 - Merge Review: psmisc
Summary: Merge Review: psmisc
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:
Keywords:
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)

(edit)
Clone Of:
(edit)
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.