This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours

Bug 525005

Summary: Review Request: libmxp - MUD eXtension protocol library
Product: [Fedora] Fedora Reporter: Ryan Rix <ry>
Component: Package ReviewAssignee: Jason Tibbitts <tibbs>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, mathstuf, notting
Target Milestone: ---Flags: tibbs: fedora‑review+
kevin: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-01-08 14:48:51 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:

Description Ryan Rix 2009-09-22 19:00:11 EDT
Spec URL: http://rrix.fedorapeople.org/libmxp.spec
SRPM URL: http://rrix.fedorapeople.org/libmxp-0.2.2-1.fc11.src.rpm
Description:
The MXP library implements the parser for the MUD eXtension protocol.

I need this included in Fedora so that I can get kmuddy (http://kmuddy.com) included.

Need a sponsor, will solicit Fedora KDE SIG for a sponsor.
Comment 1 Ryan Rix 2009-09-22 19:05:44 EDT
Ah, yes, rpmlint output:
[rrix@TheSwan rpmbuild]$ rpmlint RPMS/i586/libmxp-*
libmxp.i586: W: no-documentation
libmxp-devel.i586: W: no-documentation
3 packages and 0 specfiles checked; 0 errors, 2 warnings.

No documentation provided.
Comment 2 Ryan Rix 2009-09-22 20:14:32 EDT
Added Documentation...
Spec URL: http://rrix.fedorapeople.org/libmxp.spec
SRPM URL: http://rrix.fedorapeople.org/libmxp-0.2.2-2.fc11.src.rpm
Comment 3 Ben Boeckel 2009-09-22 23:43:43 EDT
I'll take this. Can't sponsor you myself though.
Comment 4 Jason Tibbitts 2009-09-22 23:58:10 EDT
If you can't sponsor, you can't do the review (at least if you wish to follow the rules we've all been following).
Comment 5 Ben Boeckel 2009-09-23 00:10:57 EDT
Alright, opening review again then.
Comment 6 Jason Tibbitts 2009-09-23 01:52:31 EDT
Well, I can at least take a look.  I'll need to go over both packages you've submitted; with luck I'll have some time tomorrow.

I note your rpmlint output doesn't match mine:

  libmxp.x86_64: W: incoherent-version-in-changelog 0.2.2-1
   ['0.2.2-2.fc12', '0.2.2-2']
The package is release 2 but your last changelog entry is for release 1.

  libmxp.x86_64: W: no-documentation
This is OK.

  libmxp.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libmxp.so.0.0.3 
   /lib64/libm.so.6
libmxp is linked against libm but doesn't call any functions in it.  This isn't really problematic because any running system is going to have libm in memory anyway.  There's a solution at http://fedoraproject.org/wiki/Common_Rpmlint_issues if you really care; just hack libtool to pass -Wl,--as-needed to the linker.

More review stuff to follow.
Comment 7 Jason Tibbitts 2009-09-23 02:24:09 EDT
You have "GPLv2" for the license but all of the source files seem to me to refer to the LGPL and include the "or (at your option) any  later version" language.  Can you indicate which files are under the GPL, or where the version is restricted to version 2 only?  Otherwise I'd say the license tag should be "LGPLv2+".

Generally it's a good idea to name patches for their function; in this case I guess that's a gcc44 compilation fix.  Using "foo-fedora.patch" for a patch in Fedora is stating the obvious.  Have you sent this patch upstream?  Is there an upstream bug number you can refer to?  See http://fedoraproject.org/wiki/Packaging:PatchUpstreamStatus for the guidelines on this.

You must include the license file(s) in the main package (which coincidentally will make the no-documentation rpmlint complaint go away).  There is no need to also include them in the devel package.

Obscuring your email address in the changelog is pointless.  Your choice, of course, but still pointless.

* source files match upstream.  sha256sum:                   
   54934b7db14683f5e9499bc3ac023c5e3bca443571963c1683e04fa742a27c7a
   libmxp-0.2.2.tar.gz
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.                                                              
* description is OK.                                                          
* dist tag is present.
* build root is OK.
X license field doesn't match the actual license.
* license is open source-compatible.
X license text included upstream, but not included in the main package.
* latest version is being packaged.
* BuildRequires are proper (none).
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* debuginfo package looks complete.
X rpmlint has a valid complaint (changelog).
* final provides and requires are sane:
   libmxp.so.0()(64bit)
   libmxp = 0.2.2-2.fc12
   libmxp(x86-64) = 0.2.2-2.fc12
  =
   /sbin/ldconfig
   libgcc_s.so.1()(64bit)
   libgcc_s.so.1(GCC_3.0)(64bit)
   libmxp.so.0()(64bit)
   libstdc++.so.6()(64bit)
   libstdc++.so.6(CXXABI_1.3)(64bit)
   libstdc++.so.6(GLIBCXX_3.4)(64bit)

  libmxp-devel-0.2.2-2.fc12.x86_64.rpm
   libmxp-devel = 0.2.2-2.fc12
   libmxp-devel(x86-64) = 0.2.2-2.fc12
  =
   libmxp = 0.2.2-2.fc12
   libmxp.so.0()(64bit)

* shared libraries are installed; ldconfig called properly.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no generically named files.
* scriptlets OK (ldconfig).
* code, not content.
* documentation is small, so no -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* headers are in the -devel package.
* no static libraries.
* no libtool .la files.
Comment 8 Ryan Rix 2009-09-23 10:33:55 EDT
[[You have "GPLv2" for the license but all of the source files seem to me to
refer to the LGPL and include the "or (at your option) any  later version"
language.]]
The COPYING file is GPLv2. That's what I based %{license} on.There is also a COPYING.LIB file which is LGPLv2+, so I'm not entirely sure which files are which, or if it's something dual licensed or odd like that. I've marked it LGPLv2+ in the spec.

changelog was a bad copy/paste movement.

There isn't really an 'upstream' bugtracker to push bugs, it's a one man project and is not a part of kde-extragear or anything like that. kmuddy AT kmuddy DOT com is the contact address. Currently trying to get in contact.

SRPM+SPEC updated this afternoon when I'm not on school network.
Comment 9 Jason Tibbitts 2009-09-23 12:33:03 EDT
The version of the COPYING file has nothing to do with the version of the GPL the code is licensed under; the COPYING file itself says that.  Similarly, the mere presence of a COPYING file doesn't actually tell you that the code is under GPL.  You have to actually look at the source code.  In this case, all of the source files have completely unambiguous licensing; you just need to look at them to see which license the code is under.  This is something you always have to do when building packages for submission to Fedora (because Fedora really cares about keeping its licensing information correct).
Comment 10 Ryan Rix 2009-09-24 01:52:14 EDT
Fixed license and changelog 
Spec URL: http://rrix.fedorapeople.org/libmxp.spec
SRPM URL: http://rrix.fedorapeople.org/libmxp-0.2.2-3.fc11.src.rpm
Comment 11 Ryan Rix 2009-10-07 21:16:00 EDT
Hi Jason, just seeing how this is coming along...
Comment 12 Jason Tibbitts 2009-10-07 21:29:58 EDT
I'm very low on time right now; let me see if I can carve some out over the next couple of days.
Comment 13 Ryan Rix 2009-10-07 21:44:05 EDT
Okay, it's no hurry, just want to make sure it didn't fall into a black hole. :-)
Comment 14 Jason Tibbitts 2009-10-09 13:00:02 EDT
Unfortunately the links in comment #10 are no longer valid.
Comment 15 Ryan Rix 2009-10-09 16:17:05 EDT
My apologies, I was reorganize my fp.o space. 
http://rrix.fedorapeople.org/libmxp/libmxp-0.2.2-3.fc11.src.rpm
http://rrix.fedorapeople.org/libmxp/libmxp.spec
Comment 16 Jason Tibbitts 2009-10-15 00:43:59 EDT
OK, I've finally managed to find some time.

It looks like, in the interim, someone else has offered to sponsor you.  Is that correct?  If so, it doesn't look as though that's happened yet and I guess this review will need to sit until that's finished.  In any case, I'd want to see the Ivan review get finished up as well before sponsoring you myself.

Anyway:

The License: tag is correct now.
The license file(s) are included as documentation in main package.
rpmlint complaints are OK.
And in addition you've given the patch a descriptive name.

So at this point I'd approve the package.  It would still be nice to have the upstream status of that patch, but it's not mandatory.

Since I'm apt to run out of time again, I'll go ahead and approve this and you'll be able to make a CVS request and check it in once you've been sponsored.

APPROVED
Comment 17 Ryan Rix 2009-10-15 01:02:05 EDT
[[It would still be nice to have the
upstream status of that patch, but it's not mandatory]]
I'll push it upstream.

[[In any case, I'd want to
see the Ivan review get finished up as well before sponsoring you myself.]]
in two weeks I'll have to time and resources to get that working. Until then it's just going to have to sit with the white board.

Kevin Kofler is sponsoring me.

Thanks Jason
Comment 18 Ryan Rix 2009-10-18 02:42:24 EDT
New Package CVS Request
=======================
Package Name: libmxp
Short Description: MUD eXtension protocol library
Owners: rrix
Branches: F-10 F-11 F-12
InitialCC:
Comment 19 Kevin Fenzi 2009-10-19 12:26:53 EDT
cvs done.
Comment 20 Jason Tibbitts 2010-01-07 21:37:06 EST
Any reason for this ticket to remain open?
Comment 21 Ryan Rix 2010-01-08 14:48:51 EST
No, I don't think so. I'm curious why it wasn't closed. Guess I didn't attach the bug to the builds in bodhi. Closing