Bug 473046 - (miniupnpc) Review Request: miniupnpc - command line tool to control NAT in UPnP-enabled routers as Linksys, D-Link etc
Review Request: miniupnpc - command line tool to control NAT in UPnP-enabled ...
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-11-26 04:33 EST by Avi Alkalay
Modified: 2012-04-19 11:53 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-04-19 11:53:21 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Avi Alkalay 2008-11-26 04:33:47 EST
Spec URL: http://avi.alkalay.net/software/miniupnpc/
SRPM URL: http://avi.alkalay.net/software/miniupnpc/
Description: Your home router (Linksys, D-Link, Netgear, etc) can be remotely controlled through UPnP. MiniUPnP is a library and command line tool to help you execute this task.
It lets you list and define NATs in your router.
Comment 1 Felix Kaechele 2008-11-26 05:44:53 EST
Obviously you do not have a sponsor. Thus your Bug should block FE-NEEDSPONSOR. See http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored for more information.
Comment 2 Avi Alkalay 2008-11-26 06:25:53 EST
Both links:

http://fedoraproject.org/wiki/PackageMaintainers/Join
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Are not specific on how to get a sponsor.

The "Join" one says:

"When the package is APPROVED by the reviewer, you must separately obtain member sponsorship in order to check in and build your package. Sponsorship is not automatic and may require that you further participate in other ways in order to demonstrate your understanding of the packaging guidelines. Key to becoming sponsored is to convince an existing sponsor-level member that you understand and follow the project's guidelines and processes.

See PackageMaintainers/HowToGetSponsored for more information on the process of becoming sponsored."

And the HowToGetSponsored doc is purely philosophical and has no practical information that I could find.

Need guidance on getting sponsorship.

Thanks in advance
Comment 3 Felix Kaechele 2008-11-26 06:31:54 EST
Okay so one crucial step is to enter either 177841 or FE-NEEDSPONSOR into the "Blocks" field at the top of the bugreport and commit it. This will signalize that the contributer who filed this bug needs a sponsor. That makes it easier for sponsors to find your review.
Comment 4 Fabian Affolter 2008-11-26 08:31:23 EST
'Source0: %{name}-%{version}.tar.gz' should be changed to 'Source0: http://miniupnp.tuxfamily.org/files/%{name}-%{version}.tar.gz'

https://fedoraproject.org/wiki/Packaging/SourceURL
Comment 5 Avi Alkalay 2008-11-26 11:40:39 EST
OK, I'll change that. But just be aware that I copied this Source0 line format from the official Fedora rdesktop spec file.

Before I send a new version, I need some guidance with the group. The spec file I used as a base had "User Interface/Desktops" but this is not the case for this package.

Where I can find a list of valid groups ?

BTW, this FE-NEEDSPONSOR tag is documented somewhere? I couldn't find a single line in the documentation about it. I would like to update the documentation to include that, after this package is accepted.
Comment 6 Fabian Affolter 2008-11-26 13:40:51 EST
(In reply to comment #5)
> OK, I'll change that. But just be aware that I copied this Source0 line format
> from the official Fedora rdesktop spec file.

The rdesktop spec file is not reviewed so far.  The 'Merge Review' is pending.  Your Source0 is acceptable if there is no source code available from a online/upstream location or only out of a VCS.  But in this case it has to be documented in the spec file.  Basically all source must come from upstream.

> Before I send a new version, I need some guidance with the group. The spec file
> I used as a base had "User Interface/Desktops" but this is not the case for
> this package.
> Where I can find a list of valid groups ?

less /usr/share/doc/rpm-*/GROUPS

> BTW, this FE-NEEDSPONSOR tag is documented somewhere? I couldn't find a single
> line in the documentation about it. I would like to update the documentation to
> include that, after this package is accepted.

See https://fedoraproject.org/wiki/PackageMaintainers/Join#Create_Your_Review_Request
Comment 7 Avi Alkalay 2008-11-26 16:19:20 EST
Suggestions incorporated into spec.

Please recheck http://avi.alkalay.net/software/miniupnpc/
Comment 8 Fabian Affolter 2008-11-26 18:54:52 EST
You bumped the release of the spec file but there is no entry in the %changelog section. 

https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs

rpmlint is complaining about several things.

rpmlint output

[fab@laptop024 SRPMS]$ rpmlint miniupnpc*
miniupnpc.src:59: E: files-attr-not-set
miniupnpc.src:60: E: files-attr-not-set
miniupnpc.src:61: E: files-attr-not-set
miniupnpc.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 6)
miniupnpc.src: E: summary-too-long Library and command line tool to control NAT in UPnP-enabled routers as Linksys, D-Link etc
1 packages and 0 specfiles checked; 4 errors, 1 warnings.

[fab@laptop024 i386]$ rpmlint mini*
miniupnpc.i386: W: devel-file-in-non-devel-package /usr/lib/libminiupnpc.so
miniupnpc.i386: E: summary-too-long Library and command line tool to control NAT in UPnP-enabled routers as Linksys, D-Link etc
miniupnpc.i386: W: incoherent-version-in-changelog 1.2 ['1.2-2.fc9', '1.2-2']
miniupnpc.i386: W: unstripped-binary-or-object /usr/lib/libminiupnpc.so.3
3 packages and 0 specfiles checked; 1 errors, 3 warnings.

https://fedoraproject.org/wiki/PackageMaintainers/Common_Rpmlint_Issues
Comment 9 Avi Alkalay 2008-11-28 07:32:54 EST
Suggestions incorporated into spec.

Please recheck http://avi.alkalay.net/software/miniupnpc/
Comment 10 manuel wolfshant 2008-11-29 09:31:20 EST
What is the purpose of the line
  mv LICENCE LICENSE
from the %build section ?

Please do not compress the manpages yourself. rpmbuild will do this for you (and might also accomodate other forms of compression than gzip)
Please use macros instead of explicit directories. This also goes for %{_mandir} which should be used instead of /usr/share/man in %install

Also, please do not manually strip the binaries. rpmbuild will use these pieces of info to build a separate package with debugging information (and strip them from the final binaries afterwards

Please make sure that Fedora's mandatory compilation flags are used (see https://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags ). Your spec uses "-fPIC -O -Wall -DNDEBUG" instead.

Mock build fails on x86_64:
  RPM build errors:
    File not found by glob: /builddir/build/BUILDROOT/miniupnpc-1.2-3.fc11.x86_64/usr/lib64/lib*.so.*
I have not examined the Makefile closely, but I suspect that the culprit is the INSTALLDIRLIB line, which makes the generated libs to always be installed in /usr/lib instead of using the correct path depending on architecture.

And last but not least, do not be shy and ADD changelog entries each time you modify the spec, leaving the older entries in place. For instance now, by looking at version "3" of the spec one could believe that this is the first attempt (because of the "initial build" line). But I know this is not true, because there exists a revision labeled "2".
Comment 11 Avi Alkalay 2008-11-29 16:22:31 EST
OK, I'll incorporate all suggestions.

I am also preparing an initscript and /etc/sysconfig configuration file to setup NATs on boot.

Just give me a few days.
Comment 12 Avi Alkalay 2008-11-30 08:10:59 EST
Suggestions incorporated. Please recheck http://avi.alkalay.net/software/miniupnpc/

Some notes:

- The LICENCE file is still being renamed due to wrong spelling.
- Package now includes an initscript and a sysconfig conf file.
- To avoid a patch and to make it install correctly on x86_64, I included the following on %install:
%ifarch x86_64
mv $RPM_BUILD_ROOT/usr/lib $RPM_BUILD_ROOT/%{_libdir}
%endif

Can't test this. I don't have a x86_64 build system.

rpmlint still outputs the following which I need some guidance on how to solve:
miniupnpc.i386: W: unstripped-binary-or-object /usr/lib/libminiupnpc.so.3
miniupnpc.i386: E: shlib-with-non-pic-code /usr/lib/libminiupnpc.so.3
miniupnpc.i386: W: conffile-without-noreplace-flag /etc/sysconfig/upnpnats
miniupnpc.i386: W: service-default-enabled /etc/rc.d/init.d/upnpnats
miniupnpc.i386: E: incoherent-subsys /etc/rc.d/init.d/upnpnats upnpcnats
miniupnpc.i386: W: service-default-enabled /etc/rc.d/init.d/upnpnats
miniupnpc.i386: W: incoherent-init-script-name upnpnats
Comment 13 manuel wolfshant 2008-11-30 09:39:23 EST
UNless you want to be sure the Americans are very very happy, you can remove the mv. "licence" is valid English (British English).See http://www.thefreedictionary.com/LICENCE

Your pseudo fix for the %{_libdir} problem does not work and you still need to address the problem of Fedora's mandatory compilation flags. Quoting from the end of the build log:
testigddescparse.c:51: warning: ignoring return value of 'fread', declared with attribute warn_unused_result
gcc   minixmlvalid.o minixml.o   -o minixmlvalid
gcc   testminixml.o minixml.o igd_desc_parse.o   -o testminixml
gcc   testupnpreplyparse.o minixml.o upnpreplyparse.o   -o testupnpreplyparse
gcc   testigddescparse.o igd_desc_parse.o minixml.o   -o testigddescparse
minixml validation test
./minixmlvalid
14 events
touch validateminixml
ar crs libminiupnpc.a miniwget.o minixml.o igd_desc_parse.o minisoap.o miniupnpc.o upnpreplyparse.o upnpcommands.o minissdpc.o upnperrors.o
gcc -shared -Wl,-soname,libminiupnpc.so.3 -o libminiupnpc.so miniwget.o minixml.o igd_desc_parse.o minisoap.o miniupnpc.o
upnpreplyparse.o upnpcommands.o minissdpc.o upnperrors.o
gcc -o upnpc-static upnpc.o libminiupnpc.a
/usr/bin/ld: miniwget.o: relocation R_X86_64_32 against `a local symbol' can not be used when making a shared object; recompile with -fPIC
miniwget.o: could not read symbols: Bad value
collect2: ld returned 1 exit status
make: *** [libminiupnpc.so] Error 1
make: *** Waiting for unfinished jobs....
error: Bad exit status from /var/tmp/rpm-tmp.jzb5MU (%build)

From the above you can see that gcc is called without any flags and that linking fails. 

I'll look at the other problems after you solve these two.
Comment 14 Avi Alkalay 2008-11-30 16:48:38 EST
I was passing the compilation flags on %build like this:

make %{?_smp_mflags} CFLAGS="%{optflags} -DNDEBUG"

But in some gcc calls it was not working. Apparently miniupnpc Makefile is not very robust. So I did this:

make %{?_smp_mflags} CFLAGS="%{optflags} -DNDEBUG" CC="gcc %{optflags} -DNDEBUG"

About the x86_64 issues, I can't really help because I don't access to this platform to test it. Hope you won't ask me to build it on an s390x too :-)

Seriously, how I can get access to a x86_64 machine to fix it? Does Fedora Project provides one for build purposes ?
Comment 15 manuel wolfshant 2008-12-01 08:43:52 EST
One can use koji scratch builds. 
As a sidenote, I fail to understand why do you keep trying to workaround instead of simply fixing the Makefile. And.. what's with that "DNDEBUG" flag ? What's wrong with Fedora's defaults ?
Comment 16 Avi Alkalay 2008-12-01 09:11:22 EST
Workaround is less intrusive, simpler, more readable, less patches, and provides exactly the same results.

DNDEBUG is needed by the source. It fails to compile if removed (a needed function declaration is enclosed inside an #ifdef). I tried without it at the first time but had to put it back.

Who can use koji? Is it mandatory for the acceptance of this package to have it built on ia32 AND x86_64? Which other platforms are mandatory ?

I think a x86_64 user can also use the ia32 binary package.

Please provide advice on what else can I do to have this simple package accepted ?
Comment 17 Avi Alkalay 2008-12-05 03:50:14 EST
So what is missing to get this done ?
What else can I do ?
Why the process stopped ?
Comment 18 manuel wolfshant 2008-12-05 06:53:04 EST
Access to do koji scratch builds is open to anyone with a FAS account, if I am not mistaken.

As of what is missing: you miss a proper fix of the package. Despite having being told several times to patch the makefile, you keep coming with incorrect workarounds.

gcc -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -DNDEBUG -o upnpc-sta
tic upnpc.o libminiupnpc.a
/usr/bin/ld: miniwget.o: relocation R_X86_64_32 against `a local symbol' can not be used when making a shared object; recompile with -fPIC
miniwget.o: could not read symbols: Bad value
collect2: ld returned 1 exit status
make: *** [libminiupnpc.so] Error 1
make: *** Waiting for unfinished jobs....
error: Bad exit status from /var/tmp/rpm-tmp.lhMd1F (%build)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.lhMd1F (%build)
EXCEPTION: Command failed. See logs for output.
 # bash -l -c 'rpmbuild -bb --target x86_64 --nodeps //builddir/build/SPECS/miniupnpc.spec'
Traceback (most recent call last):
  File "/usr/lib/python2.5/site-packages/mock/trace_decorator.py", line 70, in trace
    result = func(*args, **kw)
  File "/usr/lib/python2.5/site-packages/mock/util.py", line 286, in do
    raise mock.exception.Error, ("Command failed. See logs for output.\n # %s" % (command,), ret)
Error: Command failed. See logs for output.
 # bash -l -c 'rpmbuild -bb --target x86_64 --nodeps //builddir/build/SPECS/miniupnpc.spec'
LEAVE do --> EXCEPTION RAISED
Comment 19 Avi Alkalay 2008-12-05 09:05:57 EST
I can look for it, but do you have any fast link to how to use Koji ?

Thanks
Comment 21 Fabian Affolter 2008-12-19 12:15:10 EST
I would like to suggest to remove 'Linksys' from the summary since this is 	probably a trademark.  The same for the %description.  

Just my thought on the legal stuff...
Comment 23 Fabian Affolter 2009-04-18 08:11:48 EDT
Avi, any progress on this?
Comment 24 Mamoru TASAKA 2009-05-21 12:41:11 EDT
ping again?
Comment 25 Mamoru TASAKA 2009-06-07 12:48:45 EDT
Again ping?
Comment 26 Mamoru TASAKA 2009-06-14 12:41:50 EDT
I will close this bug as NOTABUG if no response is received from
the reporter within ONE WEEK.
Comment 27 Mamoru TASAKA 2009-06-22 13:12:53 EDT
Closing.

If someone wants to import this package into Fedora, please
file a new review request and mark this one as a duplicate
of the new one.

Thank you!
Comment 29 Jason Tibbitts 2010-12-14 13:08:50 EST
Not sure if I'll get flamed for nitpicking again, but here are some comments:

You should at least look at the rpmlint output and try to address what you can:

  miniupnpc.src: W: strange-permission upnpnats 0777L
The file in the srpm is mode 777; not really a good idea.

  miniupnpc.src:54: E: hardcoded-library-path in $RPM_BUILD_ROOT/usr/lib
I guess I understand that you're trying to avoid patching the makefile, but I'm curious why just doing 
  make PREFIX=$RPM_BUILD_ROOT INSTALLDIRLIB=$RPM_BUILD_ROOT/%{_libdir} install
doesn't work for you.

  miniupnpc.src:9: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 9)
No reason not to clean that up.

  miniupnpc.x86_64: W: incoherent-version-in-changelog
   1.2-5.fc9 ['1.4-5.fc15', '1.4-5']
Don't include the dist tag in changelog entries.

  miniupnpc.x86_64: W: unstripped-binary-or-object /usr/lib64/libminiupnpc.so.4
This is odd; you'll need to investigate why this isn't being stripped properly.  It looks like the compiler flags are correct (although most of them seem to be listed twice, so maybe there's some sort issue relating to that).  Or maybe it has somethig to do with the static stuff.

  miniupnpc.x86_64: W: conffile-without-noreplace-flag /etc/sysconfig/upnpnats
Needs to be %config(noreplace), not just %config, else it will be overwritten when the package is updated.

  miniupnpc.x86_64: W: no-manual-page-for-binary upnpc
It is nice to have manual pages but it's not essential.

  miniupnpc.x86_64: W: service-default-enabled /etc/rc.d/init.d/upnpnats
  miniupnpc.x86_64: W: service-default-enabled /etc/rc.d/init.d/upnpnats
The services should definitely not be enabled by default.

  miniupnpc.x86_64: E: incoherent-subsys /etc/rc.d/init.d/upnpnats upnpcnats
  miniupnpc.x86_64: W: incoherent-init-script-name upnpnats
   ('miniupnpc', 'miniupnpcd')
These are OK; rpmlint is confused by the service name differing from the package name.

This package includes a static library.  If you really wish to include a static lib, you must follow http://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries

You are missing the the ldconfig dependencies for the scriptlets.

There are some bits of the spec you can remove if you're only planning to target Fedora and RHEL6 or newer: BuildRoot, the first line of %install, and the entire %clean section.
Comment 30 Ismael Olea 2012-01-16 07:27:19 EST
@Avi I can't sponsor you but I can review this package if you are still interested.
Comment 31 Avi Alkalay 2012-01-16 09:21:14 EST
I can't handle this anymore.

I think this may be a useful package for many people but this process kills me.

I appreciate if somebody can adopt and continue the work on this package. The sources, SPECs SRPMs etc can be found on links above.
Comment 32 Ismael Olea 2012-01-17 04:37:08 EST
I addopt it.
Comment 33 Michael Scherer 2012-03-19 14:28:41 EDT
Ismael, can you correct the error that Jason speak of in comment 29, and post a updated url ?
Comment 34 Jason Tibbitts 2012-04-19 11:53:21 EDT
It's been another month since the last comment and a full three months since this ticket was supposedly adopted.  I'm going to go ahead and close this.  If someone really wants to adopt this package, please submit your own review so the original submitter of this isn't subjected to mail on a ticket in which he is no longer interested.

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