Bug 806117 - Review Request: python-oplop - Generate account passwords based on account nicknames [NEEDINFO]
Review Request: python-oplop - Generate account passwords based on account ni...
Status: CLOSED NOTABUG
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Eduardo Echeverria
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2012-03-22 19:12 EDT by Abdel Gadiel Martínez Lassonde
Modified: 2017-10-04 23:06 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2017-10-04 23:05:01 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
cstratak: needinfo? (abdel.g.martinez.l)


Attachments (Terms of Use)

  None (edit)
Description Abdel Gadiel Martínez Lassonde 2012-03-22 19:12:45 EDT
Spec URL: http://potty.fedorapeople.org/Oplop/1.6-1/Oplop.spec
SRPM URL: http://potty.fedorapeople.org/Oplop/1.6-1/Oplop-1.6-1.fc16.src.rpm
Description: Using a single master password and various nicknames, one can create an infinite number of unique account passwords. These unique account passwords are commonly called password hashes, domain-specific passwords, or per-site passwords.
Comment 1 Michael Scherer 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.
Comment 2 Abdel Gadiel Martínez Lassonde 2012-03-27 06:55:52 EDT
Thank you. I'm on it.
Comment 3 Michael Scherer 2012-04-29 05:13:53 EDT
Any news ?
Comment 4 Abdel Gadiel Martínez Lassonde 2012-08-24 15:01:00 EDT
I have updated the package with the suggestions you did. Here is the spec and src.rpm files link:
http://potty.fedorapeople.org/Oplop/1.6-1/Oplop-1.6-1.fc17.1.src.rpm
http://potty.fedorapeople.org/Oplop/1.6-1/Oplop.spec
Comment 5 Mario Blättermann 2012-09-22 13:56:50 EDT
To pull python2 as a runtime requirement is superfluous here, rpm picks it up automatically since python2-devel is a build dependency.
Comment 6 Abdel Gadiel Martínez Lassonde 2012-09-22 18:44:44 EDT
It is updated. Please check :)
Comment 7 Mario Blättermann 2012-09-23 04:43:22 EDT
Release:        1%{?dist}.2

This is wrong, it is only applicable in some special cases [1]. Each time you change something in your spec, you have to bump the release tag. But this doesn't mean, you have to add a number after, rather you have to bump the first number:

Release:        2%{?dist}

See [2] for more information.


[1] http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Minor_release_bumps_for_old_branches

[2] http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Release_Tag


BTW, your srpm download link from comment #4 doesn't point to the newest version. It is http://potty.fedorapeople.org/Oplop/1.6-1/Oplop-1.6-1.fc17.2.src.rpm. Please check the links once you have build a new srpm.
Comment 8 Mario Blättermann 2012-10-31 10:08:14 EDT
Ping...?
Comment 9 Abdel Gadiel Martínez Lassonde 2013-03-04 14:15:37 EST
I have updated the packages with the suggestions Mario did. Here are the links:
http://potty.fedorapeople.org/Oplop/1.6-2/Oplop.spec
http://potty.fedorapeople.org/Oplop/1.6-2/Oplop-1.6-2.fc18.src.rpm

I'm sorry for the delay.

Regards.
Comment 10 Eduardo Echeverria 2013-03-07 23:15:31 EST
Abdel, I suggest you rename this package to python-oplop instead of Oplop, in the future this package will eventually have support for python3 (
as can be seen in setup.py) and you will have to provide support to upstream, and do a sub-package python3-oplop. One way would be as follows, is better do right now and not worry about having headaches after


%global pkgname Oplop
Name:		python-oplop
Source0: https://pypi.python.org/packages/source/O/%{pkgname}/%{pkgname}-%{version}.zip
%prep
%setup -qn %{pkgname}-%{version}

if you have plan to ship this package to el5 must be define python_sitelib and python_sitearch macros
i.e. 

%if 0%{?rhel} && 0%{?rhel} <= 5
%{!?python_sitelib: %global python_sitelib %(%{__python} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib())")}
%{!?python_sitearch: %global python_sitearch %(%{__python} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib(1))")}
%endif

See https://fedoraproject.org/wiki/Packaging:Python#Macros

Please add the LICENSE file.
Comment 11 Abdel Gadiel Martínez Lassonde 2013-03-12 17:30:20 EDT
I would like to change the package name to python-oplop instead of Oplop.

Here are the updated links:
SRPM: http://potty.fedorapeople.org/Oplop/1.6-3/python-oplop-1.6-3.fc18.src.rpm
SPEC: http://potty.fedorapeople.org/Oplop/1.6-3/python-oplop.spec

Regards.
Comment 12 Eduardo Echeverria 2013-03-14 21:46:00 EDT
Abdel, I'll do the review, tomorrow.   
I've been busy in this week
Comment 13 Eduardo Echeverria 2013-03-16 02:19:30 EDT
Hi Abdel

== TODO == 
- Don't need do the duplicate declaration of %doc, easily you can include the files in one only declaration, please fix

- Please add the boiler plate of the license in %Source1, upstream explain explicitely in the LICENSE file  that the package is ASL2.0 and invite to obtain a copy, the copy should be obtained from http://www.apache.org/licenses/LICENSE-2.0.txt

- tests should be run at some point, when the package is approved ,please seek work soon with upstream for provide tests that work well in python3 and python2, also provide the opportunity to build the package correctly on both stacks, Remember that we are not allowed to build python3 packages if upstream does not give full support (and in this case, upstream not given this support)

- From what I see, you plan to ship this package to epel5 therefore need to follow appropriate guidelines, see http://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#EL5, i.e. buildroot present, clean of the buildroot,and %defattr, but there are a problem,  the build fails on epel5.If you ask me, I am among those who believe that we should gradually be left support to el5, and moving on to el6 directly. Why? make things easier for packager and el6 also shares the same Fedora packaging guidelines

- I copy and paste the el5 build error here:

+ /usr/bin/unzip -qq /builddir/build/SOURCES/Oplop-1.6.zip
+ STATUS=0
+ '[' 0 -ne 0 ']'
+ cd Oplop-1.6
++ /usr/bin/id -u
+ '[' 1001 = 0 ']'
++ /usr/bin/id -u
+ '[' 1001 = 0 ']'
+ /bin/chmod -Rf a+rX,u+w,g-w,o-w .
+ exit 0
Ejecutando(%build): /bin/sh -e /var/tmp/rpm-tmp.45353
+ umask 022
+ cd /builddir/build/BUILD
+ cd Oplop-1.6
+ LANG=C
+ export LANG
+ unset DISPLAY
+ /usr/bin/python setup.py build
  File "setup.py", line 15
    with open('README', 'r') as file:
            ^
SyntaxError: invalid syntax
====================================================================

afaik, the python binary on el5 is Python 2.4, "with statement"  was introduced in python 2.5, (understand because I say it is easier to move on to epel 6?) 


== EPEL6 ==
the package build fails too, due to small error:

* File not found: /builddir/build/BUILDROOT/python-oplop-1.6-3.el6.x86_64/usr/lib/python2.6/site-packages/Oplop-1.6-py2.7.egg-info

in the spec you have this line: 
%{python_sitelib}/%{zip_name}-%{version}-py2.7.egg-info 
this line should be
%{python_sitelib}/%{zip_name}-%{version}-*.egg-info

Regards
Comment 14 Abdel Gadiel Martínez Lassonde 2013-03-19 14:49:34 EDT
I will upload the corrections.

Thanks for your help.

Regards.
Comment 15 Eduardo Echeverria 2013-03-31 23:25:31 EDT
Ping, any update here?
Comment 16 Eduardo Echeverria 2013-05-18 17:52:43 EDT
anew, any update here?
Comment 17 Abdel Gadiel Martínez Lassonde 2013-10-03 18:16:08 EDT
Hi Eduardo!

Here are my updates for this packages:
SRPM: http://potty.fedorapeople.org/Oplop/1.6.1-4/python-oplop-1.6.1-4.fc19.src.rpm
SPEC: http://potty.fedorapeople.org/Oplop/1.6.1-4/python-oplop.spec

Hope this time is OK.

Regards,
Abdel
Comment 18 Christopher Meng 2013-10-08 02:31:03 EDT
It's nonsense to add support for EL5 as el5 only has py2.4

http://pkgs.org/search/?keyword=python-devel&search_on=name&distro=2

And looking into

https://github.com/brettcannon/oplop/blob/master/Python/setup.py

We can see it needs at least 2.6.

So please drop el5 support and remove all unneeded fields.

Another note is that 

https://github.com/brettcannon/oplop

should be the URL of this package.
Comment 19 Eduardo Echeverria 2013-10-08 02:51:26 EDT
I agree with @cicku

As I said above the package don't have support for python =< 2.4; so do you must remove the epel5 stuff

- %clean is not needed
- BuildRoot is not needed

and remove these lines:

%if 0%{?rhel} && 0%{?rhel} <= 5
%{!?python_sitelib: %global python_sitelib %(%{__python} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib())")}
%{!?python_sitearch: %global python_sitearch %(%{__python} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib(1))")}
%endif

btw, I don't know why always the pipy tarballs never contains the boilerplate of the licenses (thing that annoying me), but in the package's github we can see the license , please add of local way or build all the package directly from the github's source => https://github.com/brettcannon/oplop/blob/master/LICENSE


if you decide build from github's sources please handle the url following the guidelines 

https://fedoraproject.org/wiki/Packaging:SourceURL#Github
Comment 20 Abdel Gadiel Martínez Lassonde 2013-10-08 10:55:57 EDT
Thanks for the information, @cicku and @echevemaster.

I would work on the package and upload the corrections.

Regards!
Comment 21 Abdel Gadiel Martínez Lassonde 2013-12-26 16:35:07 EST
Hi Eduardo and Christopher.

I follow your previous advices. Please review this package:
SPEC -> http://potty.fedorapeople.org/Oplop/1.6.1-5/python-oplop.spec
SRPM -> http://potty.fedorapeople.org/Oplop/1.6.1-5/python-oplop-1.6.1-5.fc20.src.rpm

Thanks in advance.

Merry Xmas!
Comment 22 Christopher Meng 2013-12-26 22:51:04 EST
Thank you.

New thoughts only stand by me:

1. It's packager's choice to choose which kind of sources should they use for packaging, however I always choose the smallest one.

In this case, I would choose tar.gz sources instead of zipball as RPM Source0 tag.

But, since only differences is 7 kb, I think it's trivial to let you change.

2. I can see that this package support python3, as many reviewer have said, you should enable python3 subpackage, and Fedora 22(maybe higher) will set python3 as default stack so you'd better do that. Currently the way of supporting python3 in your spec is invalid, checkout a template or do it by yourself:

http://cicku.me/python-pygit2.spec

3. In pace with python3's moving on, you should set old python2 macro from unversioned to versioned:

%{__python} --> %{__python2}

%{python_sitelib} --> %{python2_sitelib}

4. Upstream has clearly told you that this package supports testing:

"
Run python3 test_main.py. Do note that Python 3 is required to run the test suite."

So you should add check section for it.

5. I don't know which name is better, Oplop or oplop? Because Oplop is the tarball name, but I'm not sure about naming the package now for python packages,   maybe Eduardo can answer that. ;)

------------
I will let Eduardo finish this review and quit now.
Comment 23 Eduardo Echeverria 2013-12-29 20:12:08 EST
with respect to naming,  guidelines can help to clarify .

https://fedoraproject.org/wiki/Packaging:NamingGuidelines#General_Naming

" When naming a package you can take some cues from the name of the upstream tarball, project name from which this software came, and what has been used for this package by other distributions/packagers in the past. Do not just blindly follow those examples, however, as package names should strive to be consistent within Fedora more than consistent between distros. You should generally use lowercase and turn underscores into dashes unless there's a compelling reason to follow a different upstream convention." 

Therefore, guidelines advise the use of lowercase, and in other distros (like Arch linux), oplop is named in lowercase => https://aur.archlinux.org/packages/oplop/?setlang=es

Please if you will to support py3 and py2, the correct BR are python2-devel and python3-devel and as said @cicku, use the appropiate macros. 

Best Regards and happy holidays
Comment 24 Charalampos Stratakis 2016-09-26 11:26:13 EDT
This project is now (not so) active on github:
https://github.com/brettcannon/oplop

Is there still a desire to package that?
Comment 25 Elliott Sales de Andrade 2017-10-04 23:05:01 EDT
It seems not.

Note You need to log in before you can comment on or make changes to this bug.