Spec URL: http://musuruan.fedorapeople.org/upnp-inspector.spec SRPM URL: http://musuruan.fedorapeople.org/upnp-inspector-0.2.2-1.fc12.src.rpm Description: The Inspector is an UPnP Device and Service analyzer, and a debugging tool, based on the Coherence DLNA/UPnP framework. It is a GUI to discover and explore UPnP devices on your network. Detected devices are displayed in a tree-view, where - amongst other things - actions can be called and state-variables be queried. Rpmlint output is clean.
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.