Fedora Merge Review: lsof http://cvs.fedora.redhat.com/viewcvs/devel/lsof/ Initial Owner: kzak
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).