Bug 226322 - Merge Review: psmisc
Merge Review: psmisc
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jon Ciesla
Fedora Package Reviews List
:
Depends On:
Blocks: F9MergeReviewTarget
  Show dependency treegraph
 
Reported: 2007-01-31 15:43 EST by Nobody's working on this, feel free to take it
Modified: 2009-05-07 12:57 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-05-07 12:57:52 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
limburgher: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 15:43:51 EST
Fedora Merge Review: psmisc

http://cvs.fedora.redhat.com/viewcvs/devel/psmisc/
Initial Owner: kzak@redhat.com
Comment 1 Andy Grimm 2007-02-03 16:24:51 EST
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 13:15:57 EST
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 12:07:25 EST
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 09:48:54 EST
Fixed. Update to psmisc-22.3-1.fc7. Thanks for your review.
Comment 5 Jon Ciesla 2008-09-18 10:11:42 EDT
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 06:55:13 EDT
Jon, I am the new maintainer of psmisc. You can take the ownership and complete the review.
Comment 7 Jon Ciesla 2008-09-19 08:35:31 EDT
Excellent.  I'll get right on it.
Comment 8 Jon Ciesla 2008-09-19 09:00:44 EDT
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 Jon Ciesla 2008-12-04 14:35:34 EST
Ping?
Comment 10 Jon Ciesla 2009-03-31 11:26:14 EDT
Ping again?
Comment 11 Michal Nowak 2009-04-08 12:41:51 EDT
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 09:58:21 EDT
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 10:12:59 EDT
(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 Jon Ciesla 2009-05-07 12:57:52 EDT
I'm happy.  Commit.

APPROVED.

Thanks everyone!

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