Bug 1262552 - Review Request: python-pyhsm - Python module to talk to YubiHSM hardware
Review Request: python-pyhsm - Python module to talk to YubiHSM hardware
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Zbigniew Jędrzejewski-Szmek
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2015-09-12 13:33 EDT by Kevin Fenzi
Modified: 2016-04-03 12:56 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-12-24 19:27:39 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
zbyszek: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Kevin Fenzi 2015-09-12 13:33:15 EDT
Spec URL: https://www.scrye.com/~kevin/fedora/review/python-pyhsm/python-pyhsm.spec
SRPM URL: https://www.scrye.com/~kevin/fedora/review/python-pyhsm/python-pyhsm-1.0.4l-1.fc24.src.rpm
Description: 
PyHSM is a python module and set of utilities to manage a YubiHSM,
Yubico's Hardware Security Module.

Fedora Account System Username: kevin

koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=11065502

rpmlint says: 

2 packages and 0 specfiles checked; 0 errors, 0 warnings.
Comment 1 Zbigniew Jędrzejewski-Szmek 2015-11-30 12:08:33 EST
%py2_build
%py2_install

Remove rm -rf $RPMBUILDROOT.

Is it python2 only?

Looks OK otherwise.
Comment 2 Upstream Release Monitoring 2015-11-30 18:48:06 EST
kevin's scratch build of python-pyhsm-1.0.4l-2.fc24.src.rpm for f24 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12018984
Comment 3 Kevin Fenzi 2015-11-30 18:51:39 EST
Made those changes (%py2_build / %py2_install), removed rm -rf in install. 

Yes, it's python2 only at this time. 

Spec URL: https://www.scrye.com/~kevin/fedora/review/python-pyhsm/python-pyhsm.spec
SRPM URL: https://www.scrye.com/~kevin/fedora/review/python-pyhsm/python-pyhsm-1.0.4l-2.fc24.src.rpm
Comment 4 Zbigniew Jędrzejewski-Szmek 2015-11-30 20:08:04 EST
- license is OK
- license file is present, %license is used
- new python macros are used (mostly)
- rpmlint:

2 packages and 0 specfiles checked; 0 errors, 0 warnings.

- provides:
python-pyhsm = 1.0.4l-2.fc24

Please generate python2-pyhsm binary subpackage, and add %{?python_provide:%python_provide python2-%{srcname}}.

- requires:
/bin/sh
/usr/bin/env
/usr/bin/python
python(abi) = 2.7
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PartialHardlinkSets) <= 4.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsXz) <= 5.2-1

OK, except that /usr/bin/env is suspicious.

run.sh in Tests/ is completely broken. Should be either fixed or removed.

Some files in /bin/ use #!/usr/bin/env python, others use #!/usr/bin/python. Please fix them all to use #!%{__python2}.
Comment 5 Upstream Release Monitoring 2015-12-05 13:52:31 EST
kevin's scratch build of python-pyhsm-1.0.4l-3.fc24.src.rpm for f24 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12073282
Comment 6 Kevin Fenzi 2015-12-05 14:07:02 EST
ok. I created the python2 subpackage. 

Can you explain how run.sh in Tests is broken? It worked fine here, but note (as per the comment in the spec file) it's commented out because you have to have a YubiHSM to test against. 

Is there a guideline saying to replace all /usr/bin/env calls? I agree it's not great, but I would prefer to ask upstream to change that instead of carrying some large patch. 

Spec URL: https://www.scrye.com/~kevin/fedora/review/python-pyhsm/python-pyhsm.spec
SRPM URL: https://www.scrye.com/~kevin/fedora/review/python-pyhsm/python-pyhsm-1.0.4l-3.fc24.src.rpm
Comment 7 Zbigniew Jędrzejewski-Szmek 2015-12-05 17:50:47 EST
(In reply to Kevin Fenzi from comment #6)
> ok. I created the python2 subpackage. 
> 
> Can you explain how run.sh in Tests is broken? It worked fine here, but note
> (as per the comment in the spec file) it's commented out because you have to
> have a YubiHSM to test against. 

$ /usr/share/doc/python2-pyhsm/Tests/run.sh
python: can't open file '/usr/share/doc/python2-pyhsm/Tests/../setup.py': [Errno 2] No such file or directory

It also refers to ../Lib, which is not in the package at all.

> Is there a guideline saying to replace all /usr/bin/env calls? I agree it's
> not great, but I would prefer to ask upstream to change that instead of
> carrying some large patch. 
Some executables in the package in /usr/bin/ have /usr/bin/env python, others have /usr/bin/python. This is kind of inelegant. Also there's bigger chance of running a user-installed python by mistake. Changing it to /usr/bin/python2 everywhere makes things crystal clear. I don't think that there's an official guideline, but I think it's generally recommended.

> Spec URL:
> https://www.scrye.com/~kevin/fedora/review/python-pyhsm/python-pyhsm.spec
> SRPM URL:
> https://www.scrye.com/~kevin/fedora/review/python-pyhsm/python-pyhsm-1.0.4l-
> 3.fc24.src.rpm

OK, please fix (or not) the two small issues pointed out above as you see fit. Package is APPROVED.
Comment 8 Kevin Fenzi 2015-12-06 12:13:41 EST
(In reply to Zbigniew Jędrzejewski-Szmek from comment #7)
>
> $ /usr/share/doc/python2-pyhsm/Tests/run.sh
> python: can't open file '/usr/share/doc/python2-pyhsm/Tests/../setup.py':
> [Errno 2] No such file or directory
> 
> It also refers to ../Lib, which is not in the package at all.

Oh right. It does the tests from the source tree after build, but before install. 
That Lib is in the source checkout. 

> Some executables in the package in /usr/bin/ have /usr/bin/env python,
> others have /usr/bin/python. This is kind of inelegant. Also there's bigger
> chance of running a user-installed python by mistake. Changing it to
> /usr/bin/python2 everywhere makes things crystal clear. I don't think that
> there's an official guideline, but I think it's generally recommended.

ok. I'll check with upstream and if they don't respond or the like carry a patch. 

> OK, please fix (or not) the two small issues pointed out above as you see
> fit. Package is APPROVED.

Thank you very much for the review!
Comment 9 Zbigniew Jędrzejewski-Szmek 2015-12-06 12:30:50 EST
A patch would be overkill, imho:

sed -i '1s|^#!.*python.*$|#!%{__python2} -s|' %{buildroot}%{_bindir}/*

(untested)
Comment 10 Kevin Fenzi 2015-12-06 12:43:33 EST
Sure, I meant a local change in the spec, not a patch specifically, but sure.
Comment 11 Till Maas 2015-12-06 15:39:33 EST
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/python-pyhsm
Comment 12 Fedora Update System 2015-12-06 17:23:04 EST
python-pyhsm-1.0.4l-4.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2015-c7f5a96b03
Comment 13 Fedora Update System 2015-12-06 17:23:33 EST
python-pyhsm-1.0.4l-4.fc22 has been submitted as an update to Fedora 22. https://bodhi.fedoraproject.org/updates/FEDORA-2015-8ccbad579c
Comment 14 Fedora Update System 2015-12-06 17:23:54 EST
python-pyhsm-1.0.4l-4.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2015-a37053b711
Comment 15 Fedora Update System 2015-12-07 18:22:22 EST
python-pyhsm-1.0.4l-4.fc22 has been pushed to the Fedora 22 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'dnf --enablerepo=updates-testing update python-pyhsm'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-8ccbad579c
Comment 16 Fedora Update System 2015-12-07 23:32:41 EST
python-pyhsm-1.0.4l-4.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'dnf --enablerepo=updates-testing update python-pyhsm'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-c7f5a96b03
Comment 17 Fedora Update System 2015-12-08 02:21:53 EST
python-pyhsm-1.0.4l-4.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'yum --enablerepo=epel-testing update python-pyhsm'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2015-a37053b711
Comment 18 Fedora Update System 2015-12-24 19:27:37 EST
python-pyhsm-1.0.4l-4.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.
Comment 19 Fedora Update System 2015-12-28 18:54:30 EST
python-pyhsm-1.0.4l-4.fc22 has been pushed to the Fedora 22 stable repository. If problems still persist, please make note of it in this bug report.
Comment 20 Fedora Update System 2016-04-03 12:56:26 EDT
python-pyhsm-1.0.4l-4.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.

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