Bug 226108

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

Thanks for your enthusiasm. I'm going to fix/improve the package in next week(s).

(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.

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?

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.

I've added a note about tarball & script to the lsof.spec file. See lsof-4.78-6.fc8.

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.

Michal, thanks for your review. I'll fix all in the next rawhide update (probably together with some others issues).

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.

Thanks for doing that. Approved (or whatever word is correct here).