Bug 665853

*** Bug 509658 has been marked as a duplicate of this bug. ***

Review of h5py, 12th January:
https://bugzilla.redhat.com/show_bug.cgi?id=665853

- Package meets naming and packaging guidelines
YES: It does.
- Spec file matches base package name.
YES: It does.
- Spec has consistant macro usage.
YES: It does.
- Meets Packaging Guidelines.
YES: It does but see comments about eggs below.
- License
YES: BSD
- License field in spec matches
YES:
- License file included in package
YES: LICENSE.txt and licenses/*.txt
- Spec in American English
YES: It is.
- Spec is legible.
YES: It is.
- Sources match upstream md5sum:
YES: but see rpmlint error below.
$ md5sum h5py-1.3.1.tar.gz ../SOURCES/h5py-1.3.1.tar.gz 
cfef84992d33910a06371dc35becb71b  h5py-1.3.1.tar.gz
cfef84992d33910a06371dc35becb71b  ../SOURCES/h5py-1.3.1.tar.gz
- Package needs ExcludeArch
YES: Builds as is in koji.
- BuildRequires correct
YES: Builds in koji
- Spec handles locales/find_lang
YES: No locale.
- Package is relocatable and has a reason to be.
YES: Is not relocatable.
- Package has %defattr and permissions on files is good.
YES: It does.
- Package has a correct %clean section.
YES:
- Package has correct buildroot
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
NO:
It has %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)

- Package is code or permissible content.
YES:
- Doc subpackage needed/used.
YES: not needed.
- Packages %doc files don't affect runtime.
YES: Theyt don't.
- Headers/static libs in -devel subpackage.
Not relavent.
- Spec has needed ldconfig in post and postun
Not relavent.
- .pc files in -devel subpackage/requires pkgconfig
Not relavent.
- .so files in -devel subpackage.
Not relavent.
- -devel package Requires: %{name} = %{version}-%{release}
Not relavent.
- .la files are removed.
None created.

- Package is a GUI app and has a .desktop file
It's not.
- Package compiles and builds on at least one arch.
Koji.
- Package has no duplicate files in %files.
It does not.
- Package doesn't own any directories other packages own.]
It does not.
- Package owns all the directories it creates.
It does.
- No rpmlint output.
$ rpmlint SPECS/h5py.spec RPMS/x86_64/h5py-* SRPMS/h5py-1.3.1-1.fc14.src.rpm 
SPECS/h5py.spec: W: invalid-url Source0: http://h5py.googlecode.com/files/h5py-1.3.1.tar.gz HTTP Error 404: Not Found
h5py.x86_64: W: spelling-error %description -l en_US datasets -> data sets, data-sets, databases
h5py.x86_64: W: spelling-error %description -l en_US datatypes -> data types, data-types, databases
h5py.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/h5py/utils.so utils.so()(64bit)
h5py.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/h5py/_proxy.so _proxy.so()(64bit)
h5py.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/h5py/h5r.so h5r.so()(64bit)
h5py.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/h5py/_conv.so _conv.so()(64bit)
h5py.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/h5py/h5o.so h5o.so()(64bit)
h5py.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/h5py/h5.so h5.so()(64bit)
h5py.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/h5py/h5l.so h5l.so()(64bit)
h5py.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/h5py/h5a.so h5a.so()(64bit)
h5py.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/h5py/h5f.so h5f.so()(64bit)
h5py.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/h5py/h5s.so h5s.so()(64bit)
h5py.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/h5py/h5p.so h5p.so()(64bit)
h5py.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/h5py/h5g.so h5g.so()(64bit)
h5py.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/h5py/h5z.so h5z.so()(64bit)
h5py.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/h5py/h5t.so h5t.so()(64bit)
h5py.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/h5py/h5fd.so h5fd.so()(64bit)
h5py.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/h5py/h5e.so h5e.so()(64bit)
h5py.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/h5py/h5i.so h5i.so()(64bit)
h5py.x86_64: W: private-shared-object-provides /usr/lib64/python2.7/site-packages/h5py/h5d.so h5d.so()(64bit)
h5py.src: W: spelling-error %description -l en_US datasets -> data sets, data-sets, databases
h5py.src: W: spelling-error %description -l en_US datatypes -> data types, data-types, databases
h5py.src: W: invalid-url Source0: http://h5py.googlecode.com/files/h5py-1.3.1.tar.gz HTTP Error 404: Not Found
3 packages and 1 specfiles checked; 0 errors, 24 warnings.

You could split datasets and datatypes into two wordss but I believe both to be on common usage.

The "invalid-url" I do not understand, that URL works just fine? Do you see this with rpmlint?


Issues:

1. You have a buildroot of
BuildRoot:      %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
rather than the normal (?)
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

maybe what you have is now permitted? It looks better in someways for sure.
Of course it's not needed at all of new Fedoras.


2. Investigate the "invalid-url"

3. There are a lot of "private-shared-object-provides" , these can be removed with the use of 
%{?filter_provides_in: %filter_provides_in .*/h5py/.*\.so}
%{?filter_setup}

4. Do you need the "Requires: hdf" since this satisfied by the auto requires to "libhdf5.so.6()(64bit)"
   I would presume?

5. What's the reason for 
   rm -rf %{buildroot}/%{python_sitearch}/%{name}-%{version}-py2.*.egg-info/

6. Could you add a comment as to why "-fopenmp" has been added to the CFLAGS.

Comments:
I looked also at adding this to EPEL5. It seems while you can choose the 1.6 api to hdf you can't easily
get around the requirement for python 2.5?

However the package builds fine on EPEL6. Please could it be added to EPEL6 though that is not
a requirement for the review. If my python26-numpy review ever makes it to EPEL5 then I will
submit a patch for h5py to work with it. I have a user request for this on CentOS 5.

Thanks!

Updated package:

- fix buildroot
- add filter
- don't remove egg-info files
- remove explicit hdf5 req
- build and ship docs as html

Unsure about the -fopenp thingie.

There is a bug in rpmlint (or the google server is a bit funny): 
   https://bugzilla.redhat.com/show_bug.cgi?id=669339

python 2.5 seems to be a hard req. 
btw: I don't maintain EPEL packages.

spec: http://terjeros.fedorapeople.org/h5py/h5py.spec
srpm: http://terjeros.fedorapeople.org/h5py/h5py-1.3.1-3.fc14.src.rpm
koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=2719548

I was reading the old review in bug #509658  and there are a comment of items left which
I think still need comment.

Is the "open security issue" with lzf addressed?

You say the licensing issues is now resolved I guess because the LZF is an BSD but I still see
GPL stuff in lzf/lzf. 

Why is this only BSD now and not ""BSD and (BSD or GPLv2+)"?

Stve

I don't what the "open security issue" is, might have to ping Jason.

Hm, I must recheck the license issue, I did not see any GPL stuff.

Honestly I can't remember what it was.  I think there was an issue with decompression of large files, probably fixed with 1.4.

In any case, bundling libraries is expressly forbidden without an exemption these days, so that point is academic.  Bundling lzf is right out anyway.

 
> In any case, bundling libraries is expressly forbidden without an exemption
> these days, so that point is academic.  Bundling lzf is right out anyway.

http://oldhome.schmorp.de/marc/liblzf.html

looks to be the upstream.

Steve.

Created attachment 500306 [details]
Patch to h5py to use system provided liblzf.

Hi,
This is patch your last .spec file that allows the system liblzf to be
used rather than the included one.

Created attachment 500307 [details]
Patch to .spec to use system provided liblzf.

This is the patch to the .spec file. 
The previous patch was the patch the h5py tar ball its self.

Steve.

Thanks Steve, patches applied.

spec: http://terjeros.fedorapeople.org/h5py/h5py.spec
srpm: http://terjeros.fedorapeople.org/h5py/h5py-1.3.1-4.fc14.src.rpm
koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=3087314

Builds in mock f15 x86_64.

rpmlint results:
$ rpmlint ./h5py.spec /var/lib/mock/fedora-15-x86_64/result/*.rpm
./h5py.spec: W: invalid-url Source0: http://h5py.googlecode.com/files/h5py-1.3.1.tar.gz HTTP Error 404: Not Found
h5py.src: W: invalid-url Source0: http://h5py.googlecode.com/files/h5py-1.3.1.tar.gz HTTP Error 404: Not Found

this is clean, known problem with googlecode.


- Package meets naming and packaging guidelines

- Spec file matches base package name.
Yes named after tar ball.
- Spec has consistant macro usage.
They are.
- Meets Packaging Guidelines.
Yes.
- License
BSD
- License field in spec matches
License looks to be consistantly BSD
- License file included in package
Yes license directory included.
- Spec in American English
yes
- Spec is legible.
yes
- Sources match upstream md5sum:
$ md5sum h5py-1.3.1.tar.gz ../SOURCES/h5py-1.3.1.tar.gz 
cfef84992d33910a06371dc35becb71b  h5py-1.3.1.tar.gz
cfef84992d33910a06371dc35becb71b  ../SOURCES/h5py-1.3.1.tar.gz

- Package needs ExcludeArch
It  odes not
- BuildRequires correct
Look good and passes mock
- Spec handles locales/find_lang
Not important.
- Package is relocatable and has a reason to be.
Not relocatable.
- Package has %defattr and permissions on files is good.
Fine
- Package has a correct %clean section.
Fine
- Package has correct buildroot
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
- Package is code or permissible content.
Yes
- Doc subpackage needed/used.
Not needed
- Packages %doc files don't affect runtime.
Thet don't

- Headers/static libs in -devel subpackage.
Not needed.
- Spec has needed ldconfig in post and postun
Not Needed.
- .pc files in -devel subpackage/requires pkgconfig
Not Needed.
- .so files in -devel subpackage.
Not Needed.
- -devel package Requires: %{name} = %{version}-%{release}
Not Needed.
- .la files are removed.
Not Needed.

- Package is a GUI app and has a .desktop file
Not Needed.
- Package compiles and builds on at least one arch.
Mock
- Package has no duplicate files in %files.
No
- Package doesn't own any directories other packages own.
It does not
- Package owns all the directories it creates.
It does.
- No rpmlint output.
See above.

- final provides and requires are sane:
They are indeed.

SHOULD Items:

- Should build in mock.
Yes
- Should build on all supported archs
Not checked but probablyu
- Should function as described.
%checks pass
- Should have sane scriptlets.
None
- Should have subpackages require base package with fully versioned depend.
Not relavent.

- Should package latest version
1.3.1 is  newest except for a beta.


Issues:
None

Package APPROVED.

Thanks!

New Package SCM Request
=======================
Package Name: h5py
Short Description: A Python interface to the HDF5 library
Owners: terjeros
Branches: f14 f15
InitialCC:

Git done (by process-git-requests).

Steve, in the future, please take ownership of review bugs you're working
on.  Thanks!

>Steve, in the future, please take ownership of review bugs you're working
>on.  Thanks!

Yes of course, just an omission , thanks.

Steve

(In reply to comment #3)
> btw: I don't maintain EPEL packages.
> 

Package Change Request
======================
Package Name: h5py
New Branches: el6
Owners: stevetraylen

Git done (by process-git-requests).

h5py-1.3.1-6.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/h5py-1.3.1-6.el6

h5py-1.3.1-6.el6 has been pushed to the Fedora EPEL 6 testing repository.

h5py-1.3.1-6.el6 has been pushed to the Fedora EPEL 6 stable repository.