Bug 1076186

I'll review this one.

Hi Ankur, do you have time to review the package? As it has been 1 month since your last comment, I will wait a week before resetting the review flag back to empty, following https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews#Reviewer_not_responding (but I would rather get the package reviewed, if you have the time). Thanks!

Hi David,

I'll review it by the end of the week. I seem to have forgotten to add it to my work list. Apologies.

Warm regards,
Ankur

[+] OK
[-] NA
[?] Issue

** Mandatory review guidelines: **
 [+] rpmlint output:
  
[asinha@ankur-laptop  SRPMS]$ rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/*.rpm ../SPECS/openstv.spec ./openstv-1.7-1.fc20.src.rpm
openstv.noarch: E: incorrect-fsf-address /usr/lib/python2.7/site-packages/openstv/License.html
openstv.src: W: strange-permission openstv-1.7-setup-package-data.patch 0600L
openstv.src: W: strange-permission openstv-1.7-setup-package-data.patch 0600L
3 packages and 1 specfiles checked; 1 errors, 2 warnings.
[asinha@ankur-laptop  SRPMS]$

Please inform upstream of the wrong address.

 [+] License is acceptable (...)
 [+] License field in spec is correct
 [?] License files included in package %docs if included in source package
The License isn't installed as a doc. It's installed with the rest of the
python files. Does it need to be placed there for functioning of the software?
Otherwise, please consider shifting it to docdir.

 [-] License files installed when any subpackage combination is installed
 [-] Spec written in American English
 [+] Spec is legible
 [-] Sources match upstream unless altered to fix permissibility issues
   Upstream SHA256: ...
   Your SHA256:     ...
 [+] Build succeeds on at least one primary arch
 [+] Build succeeds on all primary arches or has ExcludeArch + bugs filed
 [+] BuildRequires correct, justified where necessary
 [-] Locales handled with %find_lang, not %_datadir/locale/*
 [-] %post, %postun call ldconfig if package contains shared .so files
 [+] No bundled libs
 [-] Relocatability is justified
 [+] Package owns all directories it creates
 [+] Package requires others for directories it uses but does not own
 [+] No duplication in %files unless necessary for license files
 [+] File permissions are sane
 [+] Package contains permissible code or content
 [-] Large docs go in -doc subpackage
 [+] %doc files not required at runtime
 [-] Static libs go in -static package/virtual Provides
 [-] Development files go in -devel package
 [-] -devel packages Require base with fully-versioned dependency, %_isa
 [+] No .la files
 [+] GUI app uses .desktop file, installs it with desktop-file-install
 [+] File list does not conflict with other packages' without justification
 [+] File names are valid UTF-8

** Optional review guidelines: **
 [?] Query upstream about including license files
Already included. Should be fine. 

 [-] Translations of description, summary
 [+] Builds in mock
 [+] Builds on all arches
 [?] Functions as described (e.g. no crashes)
Not tested. Please ensure it works.

 [+] Scriptlets are sane
 [-] Subpackages require base with fully-versioned dependency if sensible
 [-] .pc file subpackage placement is sensible
 [+] No file deps outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin
 [+] Include man pages if available

Naming guidelines:
 [+] Package names use only a-zA-Z0-9-._+ subject to restrictions on -._+
 [+] Package names are sane
 [+] No naming conflicts
 [+] Spec file name matches base package name
 [+] Version is sane
 [+] Version does not contain ~
 [+] Release is sane
 [+] %dist tag
 [+] Case used only when necessary
 [-] Renaming handled correctly

Packaging guidelines:
 [+] Useful without external bits
 [+] No kmods
 [-] Pre-built binaries, libs removed in %prep
 [+] Sources contain only redistributable code or content
 [+] Spec format is sane
 [+] Package obeys FHS, except libexecdir, /run, /usr/target
 [+] No files in /bin, /sbin, /lib* on >= F17
 [+] Programs run before FS mounting use /run instead of /var/run
 [+] Binaries in /bin, /sbin do not depend on files in /usr on < F17
 [+] No files under /srv, /opt, /usr/local
 [+] Changelog in prescribed format
 [+] No Packager, Vendor, Copyright, PreReq tags
 [+] Summary does not end in a period
 [-] Correct BuildRoot tag on < EL6
 [-] Correct %clean section on < EL6
 [+] Requires correct, justified where necessary
 [+] Summary, description do not use trademarks incorrectly
 [+] All relevant documentation is packaged, appropriately marked with %doc
 [+] Doc files do not drag in extra dependencies (e.g. due to +x)
 [-] Code compilable with gcc is compiled with gcc
 [-] Build honors applicable compiler flags or justifies otherwise
 [-] PIE used for long-running/root daemons, setuid/filecap programs
 [-] Useful -debuginfo package or disabled and justified
 [-] Package with .pc files Requires pkgconfig on < EL6
 [+] No static executables
 [-] Rpath absent or only used for internal libs
 [-] Config files marked with %config(noreplace) or justified %config
 [+] No config files under /usr
 [-] Third party package manager configs acceptable, in %_docdir
 [+] .desktop files are sane
 [?] Spec uses macros consistently
You've used $RPM_BUILD_ROOT at one location. Consider changing it to buildroot,
for consistency. 

 [+] Spec uses macros instead of hard-coded names where appropriate
 [-] Spec uses macros for executables only when configurability is needed
 [-] %makeinstall used only when alternatives don't work
 [-] Macros in Summary, description are expandable at srpm build time
 [-] Spec uses %{SOURCE#} instead of $RPM_SOURCE_DIR and %sourcedir
 [-] No software collections (scl)
 [-] Macro files named /etc/rpm/macros.%name
 [+] Build uses only python/perl/shell+coreutils/lua/BuildRequired langs
 [+] %global, not %define
 [-] Package translating with gettext BuildRequires it
 [-] Package translating with Linguist BuildRequires qt-devel
 [-] File ops preserve timestamps
 [-] Parallel make
 [+] No Requires(pre,post) notation
 [-] User, group creation handled correctly (See Packaging:UsersAndGroups)
 [-] Web apps go in /usr/share/%name, not /var/www
 [-] Conflicts are justified
 [+] One project per package
 [+] No bundled fonts
 [?] Patches have appropriate commentary
Please comment the patch.

 [-] Available test suites executed in %check
 [-] tmpfiles.d used for /run, /run/lock on >= F15

 ** Python guidelines: **
 [+] Runtime Requires correct
 [-] Python macros declared on < EL6
 [+] All .py files packaged with .pyc, .pyo counterparts
 [+] Includes .egg-info files/directories when generated
 [-] Provides/Requires properly filtered
 [-] Code that invokes gtk.gdk.get_pixels_array() Requires numpy


Not many issues. Just cosmetics primarily. Can I please request you to include
and install an appdata file in the package for gnome-software?


Thanks,
Warm regards,
Ankur

(In reply to Ankur Sinha (FranciscoD) from comment #4)
>…
> Please inform upstream of the wrong address.

Sadly, upstream is no longer maintaining OpenSTV as Free Software. This is the last version using the GPL.

>  [?] License files included in package %docs if included in source package
> The License isn't installed as a doc. It's installed with the rest of the
> python files. Does it need to be placed there for functioning of the
> software?
> Otherwise, please consider shifting it to docdir.

There is a menu item in OpenSTV which shows the license: Help->License, so it needs to be installed with the rest of the Python files for the package to function correctly. Should I also add a symlink to /usr/shar/doc/openstv/COPYING (or similar)?

>  [?] Functions as described (e.g. no crashes)
> Not tested. Please ensure it works.

Yes, tested with the sample data and it works fine. :-)

>  [?] Spec uses macros consistently
> You've used $RPM_BUILD_ROOT at one location. Consider changing it to
> buildroot,
> for consistency. 

Thanks, fixed!

>  [?] Patches have appropriate commentary
> Please comment the patch.

The patch itself has a comment, but I have now added this to the spec file as well.

> Not many issues. Just cosmetics primarily. Can I please request you to
> include
> and install an appdata file in the package for gnome-software?

Thanks for the recommendation. I have included an AppData file along with the other changes.

Spec URL: http://amigadave.fedorapeople.org/openstv.spec
SRPM URL: http://amigadave.fedorapeople.org/openstv-1.7-1.fc21.src.rpm

Hi David,

A symlink should be fine, or you could just include a text version as another SOURCE. As long as the license is present in the package already, this isn't a blocker. 

The rest looks good to me. 


XXXXX APPROVED XXXXX

Thanks,
Warm regards,
Ankur

New Package SCM Request
=======================
Package Name: openstv
Short Description: single transferable vote and instant runoff voting software
Upstream URL: https://github.com/OpenTechStrategies/openstv
Owners: amigadave
Branches: f19 f20 epel7
InitialCC:

Hi Ankur, thanks for the review. I added a symlink for the license, and updated the srpm and spec file in place.

Git done (by process-git-requests).

openstv-1.7-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/openstv-1.7-1.fc20

openstv-1.7-1.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/openstv-1.7-1.fc19

openstv-1.7-1.fc20 has been pushed to the Fedora 20 testing repository.

openstv-1.7-1.fc20 has been pushed to the Fedora 20 stable repository.

openstv-1.7-1.fc19 has been pushed to the Fedora 19 stable repository.