[Bug 806117] Review Request: Oplop - Generate account passwords based on account nicknames

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=806117

Michael Scherer <misc at zarb.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |misc at zarb.org

--- Comment #1 from Michael Scherer <misc at zarb.org> 2012-03-25 07:06:48 EDT ---
Hi,

a few notes :
- if you use python3, you should requires python3-devel, not python-devel
https://fedoraproject.org/wiki/Packaging:Python#BuildRequires
The same goes for python2 ( and the software seems to be in python2, so maybe
the requires is wrong )

- %doc PKG-INFO README build/* oplop/* bin/*
why is everything in %doc ?


- rm -rf %{buildroot} is no longer needed, so does %defattr and %clean, and the
BuildRoot tag, see :

https://fedoraproject.org/wiki/Packaging:Guidelines#.25clean
https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag
https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions

- %{__python} setup.py install -O1 --root=%{buildroot} --record=INSTALLED_FILES
since INSTALLED_FILES is not used, I think you should remove it

https://fedoraproject.org/wiki/Packaging:Python#Byte_compiling

- Requires:       python3 xclip
this is IMHO better to have 1 requires per line. This produce better diff , so
ease review of patches.

- I think the requires on python will be added automatically by rpm, so no need
to add it directly.

- unless you plan to use the rpm on EPEL 5, the definition of the macro is not
needed at the start of the spec, this is default since fedora 14 and can be
removed.

- the license should in %doc 
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text and
if not, you should contact upstream to add a license file.

- there seems to be test in the zip, they should be run in %check.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.