Spec URL: http://kenobi.mandriva.com/~pcpa/libminiupnpc.spec SRPM URL: http://kenobi.mandriva.com/~pcpa/libminiupnpc-1.6-1.fc16.src.rpm Description: Library and tool to control NAT in UPnP-enabled routers This and libircclient should be requiredto build yet another package I would like to contrib to fedora (megaglest). Already tested and it pass a koji scratch build (same for libircclient).
- ld-config is missing http://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries - devel doesn't require the base package Rest looks ok on the first sign. Will have a deeper look, when libircclient is through and I saw a few reviews of you (like discussed in the megaglest review).
I uploaded a new spec and srpm on top of the previous ones that now should fully comply with guidelines.
Two issues: * According to the Fedora packaing conventions, packages are supposed after their source-tarball. This source-tarball is named "miniupnpc" => This package should be named "miniupnpc" and not "libminiupnpc" * Please increment the Release each time you modify it. This helps reviewers to track progress in package submissions.
(In reply to comment #3) > * According to the Fedora packaing conventions, packages are supposed after > their source-tarball. Sorry, ugly typo, this should have been: ... packages are supposed to be named after their source-tarball ...
Thanks for the review. I also reviewed my work again and besides renaming the package I also did: o check that the license; I originally copied this package from Mageia to Mandriva, (acknowledging that), and the license actually was incorrect as the LICENSE file is a no extra clauses BSD license. o enable build of tests, what required a patch to link the tests, but there is no make target to run the tests, so, no %check added. o install a preformatted manual page. Update for spec and srpm locations: Spec URL: http://kenobi.mandriva.com/~pcpa/miniupnpc.spec SRPM URL: http://kenobi.mandriva.com/~pcpa/miniupnpc-1.6-2.fc16.src.rpm
libircclient is now packaged, but still needs miniupnpc reviewed and built to enable packaging megaglest. I added an entry to the "miniupnpc Bugs" forum listing the patches and packaging problems as well as request for any comments on how it packaged, and a back link to the review request. See http://miniupnp.tuxfamily.org/forum/viewtopic.php?t=1140 rpmlint output: $ ls SRPMS/miniupnpc-1.6-3.fc16.src.rpm RPMS/x86_64/miniupnpc-*.rpm RPMS/x86_64/miniupnpc-1.6-3.fc16.x86_64.rpm RPMS/x86_64/miniupnpc-debuginfo-1.6-3.fc16.x86_64.rpm RPMS/x86_64/miniupnpc-devel-1.6-3.fc16.x86_64.rpm SRPMS/miniupnpc-1.6-3.fc16.src.rpm $ rpmlint SRPMS/miniupnpc-1.6-3.fc16.src.rpm RPMS/x86_64/miniupnpc-*.rpm miniupnpc-devel.x86_64: W: spelling-error %description -l en_US libminiupnpc -> condominium 4 packages and 0 specfiles checked; 0 errors, 1 warnings. The package was also verified in a mock --scratch build. Actually, there was a bug corrected in the new spec and srpm, caused by enabling the build to compile some test programs, but that was being done before actually generating the library, so now it does not run "make -j" but runs as "make upnpc-shared all" to build the library first.
Update for spec and srpm locations: Spec URL: http://kenobi.mandriva.com/~pcpa/miniupnpc.spec SRPM URL: http://kenobi.mandriva.com/~pcpa/miniupnpc-1.6-3.fc16.src.rpm
I will review this package
==== C/C++ ==== [x]: MUST Package does not contain any libtool archives (.la) [x]: MUST Package does not contain kernel modules. [x]: MUST Package contains no static executables. [x]: MUST Rpath absent or only used for internal libs. [x]: MUST Package is not relocatable. ==== Generic ==== [x]: MUST Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: MUST Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: MUST %build honors applicable compiler flags or justifies otherwise. [x]: MUST All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [x]: MUST Buildroot is not present Note: Unless packager wants to package for EPEL5 this is fine [x]: MUST Package contains no bundled libraries. [x]: MUST Changelog in prescribed format. [x]: MUST Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) Note: Clean would be needed if support for EPEL is required [x]: MUST Sources contain only permissible code or content. [x]: MUST Each %files section contains %defattr if rpm < 4.4 Note: Note: defattr macros not found. They would be needed for EPEL5 [x]: MUST Macros in Summary, %description expandable at SRPM build time. [x]: MUST Package requires other packages for directories it uses. [x]: MUST Package uses nothing in %doc for runtime. [x]: MUST Package is not known to require ExcludeArch. [x]: MUST Permissions on files are set properly. [x]: MUST Package does not contain duplicates in %files. [x]: MUST Fully versioned dependency in subpackages, if present. [x]: MUST Spec file lacks Packager, Vendor, PreReq tags. [x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. Note: rm -rf would be needed if support for EPEL5 is required [-]: MUST Large documentation files are in a -doc subpackage, if required. [x]: MUST If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc. [x]: MUST License field in the package spec file matches the actual license. [x]: MUST License file installed when any subpackage combination is installed. [x]: MUST Package consistently uses macros (instead of hard-coded directory names). [x]: MUST Package is named according to the Package Naming Guidelines. [x]: MUST Package does not generate any conflict. [x]: MUST Package obeys FHS, except libexecdir and /usr/target. [x]: MUST Package must own all directories that it creates. [x]: MUST Package does not own files or directories owned by other packages. [x]: MUST Package installs properly. [x]: MUST Requires correct, justified where necessary. [x]: MUST Rpmlint output is silent. [x]: MUST Sources used to build the package match the upstream source, as provided in the spec URL. /home/slaanesh/Documents/fedora/817311/miniupnpc-1.6.tar.gz : MD5SUM this package : 88055f2d4a061cfd4cfe25a9eae22f67 MD5SUM upstream package : 88055f2d4a061cfd4cfe25a9eae22f67 [x]: MUST Spec file is legible and written in American English. [x]: MUST Spec file name must match the spec package %{name}, in the format %{name}.spec. [-]: MUST Package contains a SysV-style init script if in need of one. [x]: MUST File names are valid UTF-8. [x]: MUST Useful -debuginfo package or justification otherwise. [x]: SHOULD Reviewer should test that the package builds in mock. [-]: SHOULD If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: SHOULD Dist tag is present. [x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q --requires). [x]: SHOULD Package functions as described. [!]: SHOULD Latest version is packaged. [x]: SHOULD Package does not include license text files separate from upstream. [x]: SHOULD Patches link to upstream bugs/comments/lists or are otherwise justified. [x]: SHOULD Scriptlets must be sane, if used. [x]: SHOULD SourceX / PatchY prefixed with %{name}. Note: Source: http://miniupnp.free.fr/files/miniupnpc-%{version}.tar.gz (miniupnpc-%{version}.tar.gz) Patch0: miniupnpc-1.6-files.patch (miniupnpc-1.6-files.patch) Patch1: miniupnpc-1.6-version.patch (miniupnpc-1.6-version.patch) Patch2: miniupnpc-1.6-tests.patch (miniupnpc-1.6-tests.patch) [x]: SHOULD SourceX is a working URL. [x]: SHOULD Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: SHOULD Package should compile and build into binary rpms on all supported architectures. [!]: SHOULD %check is present and all tests pass. [x]: SHOULD Packages should try to preserve timestamps of original installed files. [x]: SHOULD Spec use %global instead of %define. Note: %define.
[!]: SHOULD Latest version is packaged. On the website I see the following: http://miniupnp.free.fr/files/download.php?file=miniupnpd-1.6.20120509.tar.gz If it's too new for megaglest than current version it's ok. [!]: SHOULD %check is present and all tests pass. There's a make check target in the makefiles. $ rpmlint *rpm miniupnpc-devel.x86_64: W: spelling-error %description -l en_US libminiupnpc -> condominium 4 packages and 0 specfiles checked; 0 errors, 1 warnings. Almost good!
(In reply to comment #10) > $ rpmlint *rpm > miniupnpc-devel.x86_64: W: spelling-error %description -l en_US libminiupnpc > -> condominium > 4 packages and 0 specfiles checked; 0 errors, 1 warnings. This is ok, you can ignore it.
(In reply to comment #10) > [!]: SHOULD Latest version is packaged. > > On the website I see the following: > http://miniupnp.free.fr/files/download.php?file=miniupnpd-1.6.20120509.tar.gz > > If it's too new for megaglest than current version it's ok. I preferred to package the release version, not snapshots. > [!]: SHOULD %check is present and all tests pass. > > There's a make check target in the makefiles. Thanks, I overlooked it, and did only check the Makefile generated by cmake, that should not have overwritten the toplevel one due to making the build in a subdirectory. Now added a proper %check. Also used %{name} and %{version} were sane, for source and patches. > $ rpmlint *rpm > miniupnpc-devel.x86_64: W: spelling-error %description -l en_US libminiupnpc > -> condominium > 4 packages and 0 specfiles checked; 0 errors, 1 warnings. > > Almost good! New package: Spec URL: http://fedorapeople.org/~pcpa/miniupnpc.spec SRPM URL: http://fedorapeople.org/~pcpa/miniupnpc-1.6-4.fc18.src.rpm
Approved for me.
Excuse me, I just stumbled on this review. I've been building miniupnpc by myself since now, so I'm really glad someone is packaging it! However, I noticed tht at least in the scratch build, your package is missing the python bindings. Is this intentional? Because at least nicotine+ needs them. The original tarball includes a script to generate them (see README) but I'm not sue if that can be used in a Fedora package.
(In reply to comment #14) > Excuse me, I just stumbled on this review. I've been building miniupnpc by > myself since now, so I'm really glad someone is packaging it! > However, I noticed tht at least in the scratch build, your package is > missing the python bindings. Is this intentional? Because at least nicotine+ > needs them. I did not add it because it is not built by default and not required by the other package I have a review request, that requires miniupnpc, but I can add it, as well as the java interface if there is interest. > The original tarball includes a script to generate them (see README) but I'm > not sue if that can be used in a Fedora package. I will make a newer package with it enabled for further review.
New package with python module enabled: Spec URL: http://fedorapeople.org/~pcpa/miniupnpc.spec SRPM URL: http://fedorapeople.org/~pcpa/miniupnpc-1.6-5.fc18.src.rpm
Thank you very much, works like a charm here!
Thanks,I will wait a bit in case someone wants to make further comments about the python interface, and then, since the package is "reviewed +", make a "fedora-cvs ?" request. Mostly because this is actually the first python (sub)package I make for fedora, so I may be missing something, but I followed all steps and checked the specs of a few other python-* packages, so should be already ok.
$ rpmlint /home/tomspur/rpmbuild/RPMS/x86_64/python-miniupnpc-1.6-5.fc16.x86_64.rpm python-miniupnpc.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/miniupnpc.so miniupnpc.so()(64bit) See here: http://fedoraproject.org/wiki/Common_Rpmlint_issues#private-shared-object-provides http://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering python-miniupnpc.x86_64: W: no-documentation This package R the main package, so is fine LICENSE-wise -> ignorable When %files is more explicit, you directly see, when the egg cannot be build, e.g.: %{python_sitearch}/miniupnpc-%{version}-py?.?.egg-info %{python_sitearch}/miniupnpc.so
Thanks, I remade the package addressing python module issues: o Added a verbose listing of files, and found out that there is an upstream bug with not updating the version of the python module. o Aded a filter for the private shared object. o Changed the regex in the spec to use "$(DESTDIR)/" as the --root argument to setup.py, to avoid another rpmlint warning about use of $RPM_BUILD_ROOT. o Also added Changelog.txt to %doc. New package: Spec URL: http://fedorapeople.org/~pcpa/miniupnpc.spec SRPM URL: http://fedorapeople.org/~pcpa/miniupnpc-1.6-6.fc18.src.rpm
The package was already reviewed, and after adding support to build the python module, I did extra corrections after comments about it. I also informed upstream about the regex I did need to add to %prep of miniupnpc.spec at http://miniupnp.tuxfamily.org/forum/viewtopic.php?p=2971#2971
Please include an SCM request and re-set the cvs flag. Thanks! https://fedoraproject.org/wiki/Package_SCM_admin_requests
New Package SCM Request ======================= Package Name: miniupnpc Short Description: Library and tool to control NAT in UPnP-enabled routers Owners: pcpa Branches: f16 f17 InitialCC: pcpa
Git done (by process-git-requests). Simone, please take ownership of review BZs, thanks!
Oops, sorry I thought fedora-review would have set the flag. Done. --Simone
Package already available for rawhide. Initially I was planning to have megaglest for fc16 and newer, but am ok if it is done for fc18.
Reopened as package was not made an update for f16 and f17. BTW, during search for bug I found this (like libircclient) also had a previous review request https://bugzilla.redhat.com/show_bug.cgi?id=473046
miniupnpc-1.6-6.fc16 has been submitted as an update for Fedora 16. https://admin.fedoraproject.org/updates/miniupnpc-1.6-6.fc16
miniupnpc-1.6-6.fc17 has been submitted as an update for Fedora 17. https://admin.fedoraproject.org/updates/miniupnpc-1.6-6.fc17
miniupnpc-1.6-6.fc17 has been pushed to the Fedora 17 testing repository.
miniupnpc-1.6-6.fc17 has been pushed to the Fedora 17 stable repository.
miniupnpc-1.6-6.fc16 has been pushed to the Fedora 16 stable repository.