This service will be undergoing maintenance at 00:00 UTC, 2017-10-23 It is expected to last about 30 minutes
Bug 817311 - Review Request: miniupnpc - Library and tool to control NAT in UPnP-enabled routers
Review Request: miniupnpc - Library and tool to control NAT in UPnP-enabled r...
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Paulo Andrade
Fedora Extras Quality Assurance
: Reopened
Depends On:
Blocks: 817315
  Show dependency treegraph
 
Reported: 2012-04-28 17:01 EDT by Paulo Andrade
Modified: 2012-07-26 18:34 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-07-26 18:25:38 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
negativo17: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Paulo Andrade 2012-04-28 17:01:45 EDT
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).
Comment 1 Thomas Spura 2012-04-29 11:15:09 EDT
- 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).
Comment 2 Paulo Andrade 2012-05-01 12:54:17 EDT
I uploaded a new spec and srpm on top of the previous
ones that now should fully comply with guidelines.
Comment 3 Ralf Corsepius 2012-05-02 01:40:00 EDT
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.
Comment 4 Ralf Corsepius 2012-05-02 01:46:16 EDT
(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 ...
Comment 5 Paulo Andrade 2012-05-02 20:27:55 EDT
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
Comment 6 Paulo Andrade 2012-05-07 22:09:18 EDT
  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.
Comment 7 Paulo Andrade 2012-05-07 22:10:05 EDT
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
Comment 8 Simone Caronni 2012-05-21 03:37:11 EDT
I will review this package
Comment 9 Simone Caronni 2012-05-21 03:52:31 EDT
==== 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.
Comment 10 Simone Caronni 2012-05-21 03:52:49 EDT
[!]: 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!
Comment 11 Simone Caronni 2012-05-21 03:53:37 EDT
(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.
Comment 12 Paulo Andrade 2012-05-21 22:07:58 EDT
(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
Comment 13 Simone Caronni 2012-05-22 02:41:23 EDT
Approved for me.
Comment 14 Dario Castellarin 2012-05-24 00:09:08 EDT
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.
Comment 15 Paulo Andrade 2012-05-24 16:04:01 EDT
(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.
Comment 16 Paulo Andrade 2012-05-26 01:59:08 EDT
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
Comment 17 Dario Castellarin 2012-05-29 19:25:37 EDT
Thank you very much, works like a charm here!
Comment 18 Paulo Andrade 2012-05-30 10:33:09 EDT
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.
Comment 19 Thomas Spura 2012-05-30 11:17:50 EDT
$ 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
Comment 20 Paulo Andrade 2012-05-30 18:23:19 EDT
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
Comment 21 Paulo Andrade 2012-05-31 19:26:45 EDT
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
Comment 22 Gwyn Ciesla 2012-05-31 21:35:21 EDT
Please include an SCM request and re-set the cvs flag.  Thanks!

https://fedoraproject.org/wiki/Package_SCM_admin_requests
Comment 23 Paulo Andrade 2012-06-01 09:33:55 EDT
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
Comment 24 Gwyn Ciesla 2012-06-01 09:51:40 EDT
Git done (by process-git-requests).

Simone, please take ownership of review BZs, thanks!
Comment 25 Simone Caronni 2012-06-01 09:59:05 EDT
Oops, sorry I thought fedora-review would have set the flag.

Done.
--Simone
Comment 26 Paulo Andrade 2012-07-02 14:59:53 EDT
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.
Comment 27 Paulo Andrade 2012-07-18 12:28:00 EDT
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
Comment 28 Fedora Update System 2012-07-18 12:30:17 EDT
miniupnpc-1.6-6.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/miniupnpc-1.6-6.fc16
Comment 29 Fedora Update System 2012-07-18 12:31:47 EDT
miniupnpc-1.6-6.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/miniupnpc-1.6-6.fc17
Comment 30 Fedora Update System 2012-07-19 05:07:27 EDT
miniupnpc-1.6-6.fc17 has been pushed to the Fedora 17 testing repository.
Comment 31 Fedora Update System 2012-07-26 18:25:38 EDT
miniupnpc-1.6-6.fc17 has been pushed to the Fedora 17 stable repository.
Comment 32 Fedora Update System 2012-07-26 18:34:02 EDT
miniupnpc-1.6-6.fc16 has been pushed to the Fedora 16 stable repository.

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