Bug 1259061 - Review request: python-securepass - SecurePass Python library & tools
Review request: python-securepass - SecurePass Python library & tools
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Parag AN(पराग)
Fedora Extras Quality Assurance
Depends On:
  Show dependency treegraph
Reported: 2015-09-01 17:11 EDT by Giuseppe Paterno'
Modified: 2016-08-01 03:33 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2016-08-01 03:33:03 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Attachments (Terms of Use)

  None (edit)
Description Giuseppe Paterno' 2015-09-01 17:11:46 EDT
Spec URL: https://gpaterno.fedorapeople.org/python-securepass.spec
SRPM URL: https://gpaterno.fedorapeople.org/python-securepass-0.4.2-1.el6.src.rpm

The tools and python libraries for accessing SecurePass platform.
It uses the SecurePass public APIs.

Fedora Account System Username: gpaterno

The package is linked somehow to the NSS/PAM modules that are filed in the bugzilla request 1162234.

The SPEC file defines a subpackage, as a matter of fact the binaries that comes out of the python egg are split in a separate package.

Please help me reviewing the package.

Note: the subpackage "securepass-tools" produces an rpmlint warning because " devel-file-in-non-devel-package /usr/bin/sp-config". sp-config is not actually a development package, but rather a config facility for kickstart or cloud-init.
Comment 1 Parag AN(पराग) 2015-09-07 02:28:32 EDT
Sorry I am busy very much and will take some more time to check this package.
you can till then do some package reviews.
Comment 2 Giuseppe Paterno' 2015-09-13 15:26:07 EDT
Thanks Parag!
In the meanwhile, new SPEC/SRPMS:

SPEC URL: https://gpaterno.fedorapeople.org/python-securepass.spec
SRPMS URL: https://gpaterno.fedorapeople.org/python-securepass-0.4.3-2.el6.src.rpm
Comment 3 Parag AN(पराग) 2015-09-16 01:35:17 EDT
Hi, If you run fedora-review tool on this review bugzilla you will see review.txt output from it showing

- Package contains BR: python2-devel or python3-devel

=> You need to specify for which version of python you are building package. I see on upstream that there is no information if the module is dual compatible that mean same code can be run under python2 or python3 environment. So, Let's take default that upstream code is python2 code. You may want to ask upstream if existing code supports python3 as well.

We are soon moving to python3 as default so all python module should start providing python3 subpackages as well.

You can fix this for now as
BuildRequires: python2-devel

- Package uses either %{buildroot} or $RPM_BUILD_ROOT
  Note: Using both %{buildroot} and $RPM_BUILD_ROOT
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#macros

=> Use only one style and not to mix it.

- If (and only if) the source package includes the text of the license(s)
  in its own file, then that file, containing the text of the license(s)
  for the package is included in %license.
  Note: License file LICENSE is marked as %doc instead of %license

==> This is okay as it looks you want your package to be built for EPEL6 also.

There are other issues in the spec that need to be cleaned. See more information for those on  https://fedoraproject.org/wiki/EPEL:Packaging

1) Spec file contains Buildroot tag which is not needed now. See https://fedoraproject.org/wiki/EPEL:Packaging#BuildRoot_tag , remove it form spec

2) You also don;t need Group tag. See https://fedoraproject.org/wiki/EPEL:Packaging#Group_tag
remove it from spec file.

3) You also don't need to specify in spec file
[ "%{buildroot}" != "/" ] && rm -rf $RPM_BUILD_ROOT

remove this line. See https://fedoraproject.org/wiki/EPEL:Packaging#Prepping_BuildRoot_For_.25install

4) prefix macro is not needed as install already using it as /usr

5) You don't need %clean section at all, its optional, see https://fedoraproject.org/wiki/EPEL:Packaging#Cleaning_BuildRoot_in_.25clean

6) Then you created subpackage securepass-tools but written it below %build and %install. You should have written all subpackages before %prep section

Move following lines just before %prep section
%package -n securepass-tools
Requires:   python-securepass
Summary:    SecurePass Tools

%description -n securepass-tools
The official tools for accessing SecurePass platform.

It uses the SecurePass public APIs.

7) we have macros for /usr/bin why not to use it?

See more on this https://fedoraproject.org/wiki/Packaging:RPMMacros

8) securepass-tools subpackage needs securepass library at runtime so add following to %package section of securepass-tools package

Requires: python-securepass

9) You need to follow python packaging guidelines as well. 

a) For that you should use specific python version macros. e.g.
%{__python} setup.py build

%{__python} setup.py install --skip-build --root="%{buildroot}"

Should be written as
%{__python2} setup.py build

%{__python2} setup.py install --skip-build --root %{buildroot}

b) If you want to build this package for EPEL6 as well then add following lines at top of spec file
%if 0%{?rhel} && 0%{?rhel} <= 6
%{!?__python2: %global __python2 /usr/bin/python2}
%{!?python2_sitelib: %global python2_sitelib %(%{__python2} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib())")}
%{!?python2_sitearch: %global python2_sitearch %(%{__python2} -c "from distutils.sysconfig import get_python_lib; print(get_python_lib(1))")}

This is as per given on https://fedoraproject.org/wiki/Packaging:Python_Old#Macros

I suppose there can be few more issues in this package but for now please fix above and submit a new SPEC and SRPM by increasing release number and adding relevant changelog.

You should also read all requires Guidelines pages like 
1) https://fedoraproject.org/wiki/EPEL:Packaging
2) https://fedoraproject.org/wiki/Packaging:Python_Old
3) https://fedoraproject.org/wiki/Packaging:Guidelines
Comment 4 Giuseppe Paterno' 2015-09-16 16:26:00 EDT
Yes, you're right, we're also building for EPEL6 as most of the cloud instances are still on CentOS/RHEL 6.

I tried to fix what you requested.
Sources are not fully ready for python3, but we are working on that, we will stick to python2 until ready to do so.

SPEC URL: https://gpaterno.fedorapeople.org/python-securepass.spec
SRPMS URL: https://gpaterno.fedorapeople.org/python-securepass-0.4.3-3.el6.src.rpm

Comment 5 Giuseppe Paterno' 2015-09-16 17:10:55 EDT
Apologies, I ran fedora-review and I noticed some errors.

SPEC URL: https://gpaterno.fedorapeople.org/python-securepass.spec
SRPMS URL: https://gpaterno.fedorapeople.org/python-securepass-0.4.3-4.el6.src.rpm

Comment 6 Parag AN(पराग) 2015-09-23 04:34:39 EDT
1) I see scratch build for EPEL6 succeed after removing defattr(-,root,root,-)
so remove that also.
See http://koji.fedoraproject.org/koji/taskinfo?taskID=11191125

2) As tools are sub-packaged, you can remove tools word from description of main package that is
The tools and python libraries for accessing SecurePass platform.
The python library for accessing SecurePass platform.

3) BuildRequires: python-pycurl is not needed as its runtime dependency only.

4) Last, this package need to follow https://fedoraproject.org/wiki/Packaging:SourceURL#Commit_Revision

Add following just before Name: tag
%global commit0 5fe6a702d5b5f66c1dae4e240bd4c823e82ff5bc
%global shortcommit0 %(c=%{commit0}; echo ${c:0:7})

Then your Source0 will become as
Source0:        https://github.com/garlsecurity/securepass-tools/archive/%{commit0}.tar.gz#/%{name}-%{shortcommit0}.tar.gz

then last change required is in %prep
%setup -qn securepass-tools-%{commit0}

For your easiness I have fixed above issues and new spec is at
just add changelog.
Comment 7 Parag AN(पराग) 2015-12-22 00:06:18 EST
Please ignore point 4 above and fix remaining 3 issues. The reason I am asking to ignore point 4 above is based on guideline "If the upstream does create tarballs you should use them as tarballs provide an easier trail for people auditing the packages." in https://fedoraproject.org/wiki/Packaging:SourceURL#Git_Hosting_Services

Also I found new upstream release 0.4.4 , update your package to this release.
Comment 8 Parag AN(पराग) 2016-08-01 03:33:03 EDT
No progress since last 10 months. Closing, reopen when you back with the updated srpm here.

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