Bug 226108 - Merge Review: lsof
Summary: Merge Review: lsof
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Michal Nowak
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: F9MergeReviewTarget
TreeView+ depends on / blocked
 
Reported: 2007-01-31 19:35 UTC by Nobody's working on this, feel free to take it
Modified: 2013-03-08 02:03 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-02-21 11:34:48 UTC
Type: ---
mnowak: fedora-review+


Attachments (Terms of Use)

Description Nobody's working on this, feel free to take it 2007-01-31 19:35:33 UTC
Fedora Merge Review: lsof

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

Comment 1 Ruben Kerkhof 2007-02-04 18:57:04 UTC
Hi Karel,

Review for release 3:
* RPM name is OK
* This is the latest version
* Builds fine in mock
* File list looks OK

Needs work:
* BuildRoot should be %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
  (wiki: PackagingGuidelines#BuildRoot)
* Missing SMP flags. If it doesn't build with it, please add a comment
  (wiki: PackagingGuidelines#parallelmake)
* Package is marked as relocatable, please check.
  (wiki: PackagingGuidelines#RelocatablePackages)
* No downloadable source. Please give the full URL in the Source tag.
* Preserve timestamps when installing files
* Use {?dist} in Release tag

Rpmlint is not silent:

Source RPM:
W: lsof summary-ended-with-dot A utility which lists open files on a Linux/UNIX system.
W: lsof invalid-license Free
W: lsof no-url-tag
W: lsof redundant-prefix-tag
W: lsof macro-in-%changelog install

rpmlint of lsof:
W: lsof summary-ended-with-dot A utility which lists open files on a Linux/UNIX system.
W: lsof invalid-license Free
W: lsof no-url-tag


Comment 2 Karel Zak 2007-02-07 21:42:45 UTC
Thanks for your enthusiasm. I'm going to fix/improve the package in next week(s).

Comment 3 Karel Zak 2007-03-01 16:45:13 UTC
(In reply to comment #1)

> * No downloadable source. Please give the full URL in the Source tag.

Not possible. We create our own tarball with linux dialect only.

The rest is fixed. Update to lsof-4.78-5.fc7.


Comment 4 Jason Tibbitts 2007-03-01 17:03:18 UTC
How is the source tarball created, then?  The procedure needs to be documented
in the specfile; see http://fedoraproject.org/wiki/Packaging/SourceURL

Or is this a case where Fedora has simply forked the upstream code completely?

Comment 5 Karel Zak 2007-03-01 18:29:37 UTC
No fork. The reason are licences -- we have to remove non-linux things from
original tarball. There is README.maintainer in CVS that describe the way. 

http://cvs.devel.redhat.com/cgi-bin/cvsweb.cgi/~checkout~/rpms/lsof/devel/README.maintainer?rev=1.1;content-type=text%2Fplain

Well, I'll add some notes to spec file (according to "When Upstream uses
Prohibited Code"
http://fedoraproject.org/wiki/Packaging/SourceURL#head-b46af5567f2338faafa13aff9855caa3c91f2c8c)

It's probably better than the README file.


Comment 6 Karel Zak 2007-10-02 23:02:42 UTC
I've added a note about tarball & script to the lsof.spec file. See lsof-4.78-6.fc8.

Comment 7 Michal Nowak 2009-04-08 15:06:54 UTC
Will do the Merge Review here, let's finish this one.
--

* `upstream2downstream.sh' -- could have license statement

* looks like there's new 4.82 version (not sure whether there are any Linux related stuff)

* %define lsofrh lsof_4.81-rh -- incorporate %{version} here

* 

[...]
# 184338 - allow lsof access nptl threads
Patch1: lsof_4.81-threads.patch
# 480694 - lsof manpage mismarked and formats badly
Patch2: lsof_4.81-man.patch
[...]
%patch1 -p1
%patch2 -p1
[...]

Patching starts from "Patch0" :)

* Don't mix macro and variable style 
  - $RPM_OPT_FLAGS -> %{optflags}
  - ${RPM_BUILD_ROOT} -> %{buildroot}

* %defattr(644,root,root,755) -> %defattr(-,root,root,-) it's the same and the later preferred

* %attr(0755,root,root) %{_sbindir}/lsof
`-------^^^^^---(not necessary)

--

The rest looks good.

Comment 8 Karel Zak 2009-04-29 10:21:19 UTC
Michal, thanks for your review. I'll fix all in the next rawhide update (probably together with some others issues).

Comment 9 Karel Zak 2010-02-19 13:12:10 UTC
Sorry for delay, I forgot...

(In reply to comment #7)
> * `upstream2downstream.sh' -- could have license statement

 well, "Public Domain" :-) -- note that the file is not distributed in srpm. It's in our CVS only.

> * %define lsofrh lsof_4.81-rh -- incorporate %{version} here

 Fixed.
 
> Patching starts from "Patch0" :)

 Fixed.

> * Don't mix macro and variable style 
>   - $RPM_OPT_FLAGS -> %{optflags}
>   - ${RPM_BUILD_ROOT} -> %{buildroot}

 Ah.. so pedentic ;-) Not fixed. I don't think it's more readable.

> * %defattr(644,root,root,755) -> %defattr(-,root,root,-) it's the same and the
> later preferred

 Fixed.

> * %attr(0755,root,root) %{_sbindir}/lsof
> `-------^^^^^---(not necessary)

 Fixed.

All changes are avaialable in lsof-4.83-2.fc14, Thanks  for review.

Comment 10 Michal Nowak 2010-02-19 13:55:26 UTC
Thanks for doing that. Approved (or whatever word is correct here).


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