[Bug 806117] New: Review Request: Oplop - Generate account passwords based on account nicknames
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: Oplop - Generate account passwords based on account nicknames
https://bugzilla.redhat.com/show_bug.cgi?id=806117
Summary: Review Request: Oplop - Generate account passwords based on account nicknames Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: abdel.g.martinez.l@gmail.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, package-review@lists.fedoraproject.org Classification: Fedora Story Points: --- Type: --- Regression: --- Mount Type: --- Documentation: ---
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.
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@zarb.org changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |misc@zarb.org
--- Comment #1 from Michael Scherer misc@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.
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
--- Comment #2 from Abdel Gadiel Martínez Lassonde abdel.g.martinez.l@gmail.com 2012-03-27 06:55:52 EDT --- Thank you. I'm on it.
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
--- Comment #3 from Michael Scherer misc@zarb.org 2012-04-29 05:13:53 EDT --- Any news ?
https://bugzilla.redhat.com/show_bug.cgi?id=806117
--- Comment #4 from Abdel Gadiel Martínez Lassonde abdel.g.martinez.l@gmail.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=806117
Mario Blättermann mario.blaettermann@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |mario.blaettermann@gmail.co | |m
--- Comment #5 from Mario Blättermann mario.blaettermann@gmail.com --- To pull python2 as a runtime requirement is superfluous here, rpm picks it up automatically since python2-devel is a build dependency.
https://bugzilla.redhat.com/show_bug.cgi?id=806117
--- Comment #6 from Abdel Gadiel Martínez Lassonde abdel.g.martinez.l@gmail.com --- It is updated. Please check :)
https://bugzilla.redhat.com/show_bug.cgi?id=806117
--- Comment #7 from Mario Blättermann mario.blaettermann@gmail.com --- 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...
[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.
https://bugzilla.redhat.com/show_bug.cgi?id=806117
--- Comment #8 from Mario Blättermann mario.blaettermann@gmail.com --- Ping...?
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=806117
--- Comment #9 from Abdel Gadiel Martínez Lassonde abdel.g.martinez.l@gmail.com --- 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.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=806117
Eduardo Echeverria echevemaster@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |echevemaster@gmail.com
--- Comment #10 from Eduardo Echeverria echevemaster@gmail.com --- 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/%%7Bpkgname%7D/%%7Bpkgname%7D-%%7B... %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.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=806117
--- Comment #11 from Abdel Gadiel Martínez Lassonde abdel.g.martinez.l@gmail.com --- 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.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=806117
Eduardo Echeverria echevemaster@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Summary|Review Request: Oplop - |Review Request: |Generate account passwords |python-oplop - Generate |based on account nicknames |account passwords based on | |account nicknames
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=806117
Eduardo Echeverria echevemaster@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@fedoraproject.org |echevemaster@gmail.com Flags| |fedora-review?
--- Comment #12 from Eduardo Echeverria echevemaster@gmail.com --- Abdel, I'll do the review, tomorrow. I've been busy in this week
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=806117
--- Comment #13 from Eduardo Echeverria echevemaster@gmail.com --- 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
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=806117
--- Comment #14 from Abdel Gadiel Martínez Lassonde abdel.g.martinez.l@gmail.com --- I will upload the corrections.
Thanks for your help.
Regards.
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=806117
--- Comment #15 from Eduardo Echeverria echevemaster@gmail.com --- Ping, any update here?
Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=806117
--- Comment #16 from Eduardo Echeverria echevemaster@gmail.com --- anew, any update here?
https://bugzilla.redhat.com/show_bug.cgi?id=806117
--- Comment #17 from Abdel Gadiel Martínez Lassonde abdel.g.martinez.l@gmail.com --- 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.rp... SPEC: http://potty.fedorapeople.org/Oplop/1.6.1-4/python-oplop.spec
Hope this time is OK.
Regards, Abdel
https://bugzilla.redhat.com/show_bug.cgi?id=806117
Christopher Meng cickumqt@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |cickumqt@gmail.com
--- Comment #18 from Christopher Meng cickumqt@gmail.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=806117
--- Comment #19 from Eduardo Echeverria echevemaster@gmail.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=806117
--- Comment #20 from Abdel Gadiel Martínez Lassonde abdel.g.martinez.l@gmail.com --- Thanks for the information, @cicku and @echevemaster.
I would work on the package and upload the corrections.
Regards!
https://bugzilla.redhat.com/show_bug.cgi?id=806117
--- Comment #21 from Abdel Gadiel Martínez Lassonde abdel.g.martinez.l@gmail.com --- 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.rp...
Thanks in advance.
Merry Xmas!
https://bugzilla.redhat.com/show_bug.cgi?id=806117
Christopher Meng cickumqt@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC|cickumqt@gmail.com |
--- Comment #22 from Christopher Meng cickumqt@gmail.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=806117
--- Comment #23 from Eduardo Echeverria echevemaster@gmail.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=806117
Charalampos Stratakis cstratak@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |abdel.g.martinez.l@gmail.co | |m, cstratak@redhat.com Flags| |needinfo?(abdel.g.martinez. | |l@gmail.com)
--- Comment #24 from Charalampos Stratakis cstratak@redhat.com --- This project is now (not so) active on github: https://github.com/brettcannon/oplop
Is there still a desire to package that?
https://bugzilla.redhat.com/show_bug.cgi?id=806117
Elliott Sales de Andrade quantum.analyst@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED CC| |quantum.analyst@gmail.com Blocks| |201449 (FE-DEADREVIEW) Resolution|--- |WONTFIX Flags|fedora-review? | Last Closed| |2017-10-04 23:05:01
--- Comment #25 from Elliott Sales de Andrade quantum.analyst@gmail.com --- It seems not.
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=201449 [Bug 201449] FE-DEADREVIEW -- Reviews stalled due to lack of submitter response should be blocking this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=806117
Elliott Sales de Andrade quantum.analyst@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|WONTFIX |NOTABUG
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com -
Red Hat Bugzilla