Bug 579230 (upnp-inspector)

Hi Andrea, I'm very glad to review your package.

The review is mostly based on MUST/SHOULD checklist published in the ReviewGuidelines.

Notation:
[+]: OK
[-]: KO
[?]: doubt
[N]: Not applicable

Review:

[+]:rpmlint run (clean) on every package with the following output:
	rpmlint upnp-inspector.spec ../RPMS/noarch/upnp-inspector-0.2.2-1.fc12.noarch.rpm ../SRPMS/upnp-inspector-0.2.2-1.fc12.src.rpm 
	2 packages and 1 specfiles checked; 0 errors, 0 warnings.  
[?]:The package must be named according to the Package Naming Guidelines (see below)
[+]:The spec matches the base package %{name}
[+]:The package meets the Packaging Guidelines.
[+]:License is MIT (OK) .
[+]:The License field in the package spec matches the actual license.
[+]:LICENCE file contains text of "MIT, Modern Style with sublicense" and is included in %doc.
[+]:The spec file must be written in American English.
[+]:The spec file for the package MUST be legible.
[+]:The sources used to build the package matches the upstream source, as provided in the spec URL.
	md5sum ../SOURCES/UPnP-Inspector-0.2.2.tar.gz /tmp/UPnP-Inspector-0.2.2.tar.gz 
	6823a57f4501c8af6a7b8547b8133759  ../SOURCES/UPnP-Inspector-0.2.2.tar.gz
	6823a57f4501c8af6a7b8547b8133759  /tmp/UPnP-Inspector-0.2.2.tar.gz 
[+]:The package successfully compiles and builds into binary rpms on x86; considering that is a noarch package may be sufficient.
[N]:If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. [...] 
[+]:All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines.
[N]:The spec file MUST handle locales properly.
[N]:Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun.
[+]:Package doesn't bundle copies of system libraries.
[+]:The package is not relocatable
[+]:The package owns all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory.
[+]:A Fedora package must not list a file more than once in the spec file's %files listings.
[+]:Permissions on files are set properly. 
[+]:Use of macros is consistent
[+]:The package contains code and permissable content.
[+]:-doc package not needed: doc stuff is almost inexistent :-)
[+]:doc stuff doesn't affect the runtime of the application. 
[N]:Header files must be in a -devel package.
[N]:Static libraries must be in a -static package.
[N]:If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package.
[N]:[...] devel packages must require the base package using a fully versioned dependency [...]
[N]:Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.
[+]:Package includes upnp-inspector.desktop file properly installed with desktop-file-install
[+]:Packages does not own files or directories already owned by other packages.
[+]:rm -rf $RPM_BUILD_ROOT at the beginning of %install
[+]:all filenames in rpm packages are valid UTF-8.

SHOULD items

[N]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it.
[N]:The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available
[+]:Builds on mock 
[+]:The package build on all supported architectures [is noarch] (see http://koji.fedoraproject.org/koji/taskinfo?taskID=2093598) 
[+]:The program starts correctly  (unfortunately I don't have a DLNA so I can't check more in depth)
[+]:If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity.
[N]:Usually, subpackages other than devel should require the base package using a fully versioned dependency.
[+]:No pkgconfig(.pc) files (no -devel package needed) 
[+]:Package doesn't have file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin 
[-]:package doesn't contain man pages (see below)

Two issues (not blocking):

* Package name should be ok, considered that "[...] case should only be used where necessary."
Anyway guidelines say "Keep in mind to respect the wishes of the upstream maintainers. [...] However, if they do not express any preference of case, you should default to lowercase naming.".
It could be good to consider to check upstream's preference.

*Package doesn't contain man pages and there's no online help in the application: consider to work with upstream to add man pages (or doc in the help menu of the program)

Thanks for the review!

I guess this is not an official review because the bug isn't assigned to you and the fedora‑review flag is not set. If I'm wrong please set them :-)

(In reply to comment #1)

> Two issues (not blocking):
> 
> * Package name should be ok, considered that "[...] case should only be used
> where necessary."
> Anyway guidelines say "Keep in mind to respect the wishes of the upstream
> maintainers. [...] However, if they do not express any preference of case, you
> should default to lowercase naming.".
> It could be good to consider to check upstream's preference.

I chose this name to be consistent to what other distributions already did. Mandriva, Debian and Ubuntu call this package upnp-inspector (lower-case). Moreover, even their binary file is called upnp-inspector (lower-case).

They seems to write upnp-inspector in all combination they can: UPnP-Inspector, UPnP_Inspector, upnp-inspector, upnp_inspector. So I guess they don't care very much.

> *Package doesn't contain man pages and there's no online help in the
> application: consider to work with upstream to add man pages (or doc in the
> help menu of the program)    

Debian has a manpage available:
http://patch-tracker.debian.org/package/upnp-inspector/0.2.2+dfsg-2

I can use that and nag upstream about providing it themselves :-)

(In reply to comment #2)
> Thanks for the review!
> 
> I guess this is not an official review because the bug isn't assigned to you
> and the fedora‑review flag is not set. If I'm wrong please set them :-)


In my first intention, it would just be an informal review but considering that I spent a lot of time to do it at my best, there is no good reason to not consider this review as an official one :-)

So I assign the bug to myself and I set the fedora-review to "?" 
Just let me know if it is worth to proceed with the doc task before the approval (see below).


> (In reply to comment #1)
> 
> > Two issues (not blocking):
> > 
> >  [...]
> > It could be good to consider to check upstream's preference.
> 
> I chose this name to be consistent to what other distributions already did.
> Mandriva, Debian and Ubuntu call this package upnp-inspector (lower-case).
> Moreover, even their binary file is called upnp-inspector (lower-case).
> 
> They seems to write upnp-inspector in all combination they can: UPnP-Inspector,
> UPnP_Inspector, upnp-inspector, upnp_inspector. So I guess they don't care very much.

Yes, this is absolutely reasonable.
I just wanted to consider this aspect in little more depth.
It's OK.

> > *Package doesn't contain man pages and there's no online help in the
> > application: consider to work with upstream to add man pages (or doc in the
> > help menu of the program)    
> 
> Debian has a manpage available:
> http://patch-tracker.debian.org/package/upnp-inspector/0.2.2+dfsg-2
> 
> I can use that and nag upstream about providing it themselves :-)

This is just a SHOULD item, so I think the package could be approved without it.
Do you think it is worth to include that doc in this package?

(In reply to comment #1)
> [+]:The program starts correctly  (unfortunately I don't have a DLNA so I can't
> check more in depth)

Fedora already contains UPnP servers, controllers and renderers. You can check if upnp-inspector is working fine using, for example, mediatomb (a UPnP server) and gstreamer (a UPnP renderer and a server).

(In reply to comment #3)
> > > *Package doesn't contain man pages and there's no online help in the
> > > application: consider to work with upstream to add man pages (or doc in the
> > > help menu of the program)    
> > 
> > Debian has a manpage available:
> > http://patch-tracker.debian.org/package/upnp-inspector/0.2.2+dfsg-2
> > 
> > I can use that and nag upstream about providing it themselves :-)
> 
> This is just a SHOULD item, so I think the package could be approved without
> it.
> Do you think it is worth to include that doc in this package?    

I think that that man page is better than nothing. I'll add it to the package but I think I won't have the required time until next weekend.

Spec URL: http://musuruan.fedorapeople.org/upnp-inspector.spec
SRPM URL: http://musuruan.fedorapeople.org/upnp-inspector-0.2.2-2.fc12.src.rpm

Changelog:
- Added man page from Debian

(In reply to comment #5)
> Spec URL: http://musuruan.fedorapeople.org/upnp-inspector.spec
> SRPM URL: http://musuruan.fedorapeople.org/upnp-inspector-0.2.2-2.fc12.src.rpm
> 
> Changelog:
> - Added man page from Debian

rpmlint still run cleanly.
src.rpm still build successfully on mock.
rpm installs/uninstalls successfully.
man pages available after package installation.

As far as I can see, everything seems to be OK so this package is APPROVED.

New Package CVS Request
=======================
Package Name: upnp-inspector
Short Description: UPnP Device and Service analyzer
Owners: musuruan
Branches: F-12 F-13
InitialCC:

CVS done (by process-cvs-requests.py).

Added to the repository and built. Closing.