Bug 1020088 (blosc) - Review Request: blosc - A high performance compressor optimized for binary data
Summary: Review Request: blosc - A high performance compressor optimized for binary data
Keywords:
Status: CLOSED ERRATA
Alias: blosc
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: python-blosc
TreeView+ depends on / blocked
 
Reported: 2013-10-17 01:58 UTC by Thibault North
Modified: 2015-05-28 15:27 UTC (History)
5 users (show)

Fixed In Version: blosc-1.2.3-9.fc20
Clone Of:
Environment:
Last Closed: 2013-11-05 03:01:00 UTC
Type: ---
Embargoed:
zbyszek: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Thibault North 2013-10-17 01:58:24 UTC
Spec URL: http://tnorth.fedorapeople.org/rev/blosc.spec
SRPM URL: http://tnorth.fedorapeople.org/rev/blosc-1.2.3-2.fc19.src.rpm
Description: A high performance compressor optimized for binary data
Fedora Account System Username: tnorth

Comment 1 Zbigniew Jędrzejewski-Szmek 2013-10-17 21:15:13 UTC
There's a typo in sed:
sed -i 's|BLOSC_VERSION_PATCH 6|BLOSC_VERSION_PATCH 6|' CMakeLists.txt
                                                    ^ should be 3


I think the description is still awkward... Maybe something like this:

Blosc is a compression library designed to transmit data to the processor cache 
faster than the traditional non-compressed memory fetch. Compression ratios are not very high, but the decompression is very fast. Blosc is meant not only to reduce the size of large datasets on-disk or in-memory, but also to accelerate 
memory-bound computations.


There's a problem with the -devel package's docs (under F20 at least): the files are installed into /usr/share/doc/bench, and should be in /usr/share/doc/blosc/bench... I'm not sure what's the best way. One approach:

--->8----------------------------------------------------------------
--- /home/zbyszek/rpmbuild/SPECS/blosc.spec~    2013-10-16 21:52:48.000000000 -0400
+++ /home/zbyszek/rpmbuild/SPECS/blosc.spec     2013-10-17 16:45:58.895419994 -0400
@@ -55,10 +55,6 @@
 
 make install DESTDIR=${RPM_BUILD_ROOT}
 
-mkdir -p ${RPM_BUILD_ROOT}/%{_docdir}/bench
-cp -pr bench/plot-speeds.py* ${RPM_BUILD_ROOT}/%{_docdir}/bench
-cp -pr bench/*.c ${RPM_BUILD_ROOT}/%{_docdir}/bench
-
 %clean
 rm -rf ${RPM_BUILD_ROOT}
 
@@ -70,11 +66,13 @@
 %doc README.rst ANNOUNCE.rst RELEASE_NOTES.rst README_HEADER.rst README_THREADED.rst RELEASING.rst
 %{_libdir}/libblosc.so.*
 
+%global _docdir_fmt %{name}
+
 %files devel
 %{_libdir}/libblosc.so
 %{_includedir}/blosc.h
-%{_docdir}/bench/plot-speeds.py*
-%{_docdir}/bench/*.c
+%doc bench/plot-speeds.py
+%doc bench/*.c
 
 
 %changelog
--->8----------------------------------------------------------------

If has the disadvantage that "/bench/" part of the path is gone, all files are in /usr/share/doc/blosc, and also that the -devel package also has the docs from the main package (the packages install fine because the files are indentical). Maybe you can come up with something better.
See https://fedoraproject.org/wiki/Changes/UnversionedDocdirs for more info.


The -devel package requires:

blosc = 1.2.3-2.fc20
libblosc.so.1.2.3()(64bit)

Those are duplicates, so 
Requires: %{name} = %{version}-%{release}
can be removed.


%clean section can be removed.

Comment 2 Thibault North 2013-10-19 00:40:03 UTC
Thanks for the comments!

(In reply to Zbigniew Jędrzejewski-Szmek from comment #1)
> There's a typo in sed:
> sed -i 's|BLOSC_VERSION_PATCH 6|BLOSC_VERSION_PATCH 6|' CMakeLists.txt
>                                                     ^ should be 3
Thanks, fixed.

> I think the description is still awkward... Maybe something like this:
> 
> Blosc is a compression library designed to transmit data to the processor
> cache 
> faster than the traditional non-compressed memory fetch. Compression ratios
> are not very high, but the decompression is very fast. Blosc is meant not
> only to reduce the size of large datasets on-disk or in-memory, but also to
> accelerate 
> memory-bound computations.

Yes, this is better.

> 
> There's a problem with the -devel package's docs (under F20 at least): the
> files are installed into /usr/share/doc/bench, and should be in
> /usr/share/doc/blosc/bench... I'm not sure what's the best way. One approach:
> 
> --->8----------------------------------------------------------------
> --- /home/zbyszek/rpmbuild/SPECS/blosc.spec~    2013-10-16
> 21:52:48.000000000 -0400
> +++ /home/zbyszek/rpmbuild/SPECS/blosc.spec     2013-10-17
> 16:45:58.895419994 -0400
> @@ -55,10 +55,6 @@
>  
>  make install DESTDIR=${RPM_BUILD_ROOT}
>  
> -mkdir -p ${RPM_BUILD_ROOT}/%{_docdir}/bench
> -cp -pr bench/plot-speeds.py* ${RPM_BUILD_ROOT}/%{_docdir}/bench
> -cp -pr bench/*.c ${RPM_BUILD_ROOT}/%{_docdir}/bench
> -
>  %clean
>  rm -rf ${RPM_BUILD_ROOT}
>  
> @@ -70,11 +66,13 @@
>  %doc README.rst ANNOUNCE.rst RELEASE_NOTES.rst README_HEADER.rst
> README_THREADED.rst RELEASING.rst
>  %{_libdir}/libblosc.so.*
>  
> +%global _docdir_fmt %{name}
> +
>  %files devel
>  %{_libdir}/libblosc.so
>  %{_includedir}/blosc.h
> -%{_docdir}/bench/plot-speeds.py*
> -%{_docdir}/bench/*.c
> +%doc bench/plot-speeds.py
> +%doc bench/*.c
>  
>  
>  %changelog
> --->8----------------------------------------------------------------
> 
> If has the disadvantage that "/bench/" part of the path is gone, all files
> are in /usr/share/doc/blosc, and also that the -devel package also has the
> docs from the main package (the packages install fine because the files are
> indentical). Maybe you can come up with something better.
> See https://fedoraproject.org/wiki/Changes/UnversionedDocdirs for more info.

About that, why are the files duplicated in this case? I don't remember having met this issue in the past.
I also tried this:

[...]
mkdir -p ${RPM_BUILD_ROOT}/%{_docdir}/%{name}/bench
cp -pr bench/plot-speeds.py* ${RPM_BUILD_ROOT}/%{_docdir}/%{name}/bench
cp -pr bench/*.c ${RPM_BUILD_ROOT}/%{_docdir}/%{name}/bench
[...]

%global _docdir_fmt %{name}

%files
%doc README.rst ANNOUNCE.rst RELEASE_NOTES.rst README_HEADER.rst README_THREADED.rst RELEASING.rst
%{_libdir}/libblosc.so.*

%files devel
%{_libdir}/libblosc.so
%{_includedir}/blosc.h
%{_docdir}/%{name}/bench/plot-speeds.py*
%{_docdir}/%{name}/bench/*.c

It is a bit ugly, but keeps the bench/ directory. But again, the main package inherits from the bench/ folder.

> 
> The -devel package requires:
> 
> blosc = 1.2.3-2.fc20
> libblosc.so.1.2.3()(64bit)

> Those are duplicates, so 
> Requires: %{name} = %{version}-%{release}
> can be removed.

Shouldn't there be an explicit require, according to https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Requiring_Base_Package ?
Otherwise, rpmlint complains.

> %clean section can be removed.

Yes, thanks.

rpmlint output:
rpmlint -v SRPMS/blosc-1.2.3-3.fc19.src.rpm RPMS/x86_64/blosc-1.2.3-3.fc19.x86_64.rpm RPMS/x86_64/blosc-devel-1.2.3-3.fc19.x86_64.rpm RPMS/x86_64/blosc-debuginfo-1.2.3-3.fc19.x86_64.rpm SPECS/blosc.spec 
blosc.src: I: checking
blosc.src: W: spelling-error %description -l en_US datasets -> data sets, data-sets, databases
blosc.src: I: checking-url http://blosc.org (timeout 10 seconds)
blosc.src: I: checking-url http://blosc.org/sources/1.2.3/blosc-1.2.3.tar.gz (timeout 10 seconds)
blosc.x86_64: I: checking
blosc.x86_64: W: spelling-error %description -l en_US datasets -> data sets, data-sets, databases
blosc.x86_64: I: checking-url http://blosc.org (timeout 10 seconds)
blosc-devel.x86_64: I: checking
blosc-devel.x86_64: I: checking-url http://blosc.org (timeout 10 seconds)
blosc-debuginfo.x86_64: I: checking
blosc-debuginfo.x86_64: I: checking-url http://blosc.org (timeout 10 seconds)
/home/tnorth/rpmbuild/SPECS/blosc.spec: I: checking-url http://blosc.org/sources/1.2.3/blosc-1.2.3.tar.gz (timeout 10 seconds)
4 packages and 1 specfiles checked; 0 errors, 2 warnings.

Comment 3 Zbigniew Jędrzejewski-Szmek 2013-10-19 18:44:40 UTC
(In reply to Thibault North from comment #2)
> About that, why are the files duplicated in this case? I don't remember
> having met this issue in the past.
Because %doc includes everything in %{_pkgdocdir} automatically.

BTW, the proper way to refer to %{docdir}/%{name} is through %{_pkgdocdir}.

> I also tried this:
> 
> [...]
> mkdir -p ${RPM_BUILD_ROOT}/%{_docdir}/%{name}/bench
> cp -pr bench/plot-speeds.py* ${RPM_BUILD_ROOT}/%{_docdir}/%{name}/bench
> cp -pr bench/*.c ${RPM_BUILD_ROOT}/%{_docdir}/%{name}/bench
> [...]
> 
> %global _docdir_fmt %{name}
> 
> %files
> %doc README.rst ANNOUNCE.rst RELEASE_NOTES.rst README_HEADER.rst
> README_THREADED.rst RELEASING.rst
> %{_libdir}/libblosc.so.*
> 
> %files devel
> %{_libdir}/libblosc.so
> %{_includedir}/blosc.h
> %{_docdir}/%{name}/bench/plot-speeds.py*
> %{_docdir}/%{name}/bench/*.c
> 
> It is a bit ugly, but keeps the bench/ directory. But again, the main
> package inherits from the bench/ folder.
So, I think that this is not OK.

I found two solutions which seem to work:

a) add %exclude %{_pkgdocdir}/bench in %files
b) remove unwanted files from bench/ in %build, and then simply add '%doc bench'
   in %files devel, without installing anything by hand.

Both of those solutions remove duplicates and preserve the /bench/ in path.

But I think that a different solution is actually better:
c) simply install a compiled version of 'bench'.

I think this is better because as a user, I don't want to have to find out how to compile the .c file to run the benchmarks, I would prefer to be able to invoke it directly. I have now run bench myself, and I think it would be worthwhile to package, because the results are quite interesting, and relevant to how one would use blosc.

There's a problem that making the bench binary and the associated plot-times.py script part of either of the two binary packages is problematic. If it is moved into the main package, it would start requiring python, and x86_64 versions would nod be co-installable. If is is installed as part of the -devel package, again, -devel would require python, and also not be coinstallable. I think that adding a -bench (or -test) package is the best option, with
/usr/bin/blosc-bench and /usr/bin/blosc-plot-times.

> > The -devel package requires:
> > 
> > blosc = 1.2.3-2.fc20
> > libblosc.so.1.2.3()(64bit)
> 
> > Those are duplicates, so 
> > Requires: %{name} = %{version}-%{release}
> > can be removed.
> 
> Shouldn't there be an explicit require, according to
> https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> Guidelines#Requiring_Base_Package ?
> Otherwise, rpmlint complains.
You're right. But then the Requires should be more exact:
Requires:  %{name}%{?_isa} = %{version}-%{release}

So, whatever you decide wrt. the %files problem, please post a new .spec. As a reviewer, I don't think I should impose my view here, and you should pick whatever you think best from the maintainer point of view.

Comment 4 Michael Schwendt 2013-10-19 19:37:23 UTC
> Because %doc includes everything in %{_pkgdocdir} automatically.

That's this one -> https://fedorahosted.org/fpc/ticket/338

Comment 5 Thibault North 2013-10-21 15:35:21 UTC
> BTW, the proper way to refer to %{docdir}/%{name} is through %{_pkgdocdir}.

Yes, but it looks like %{_pkgdocdir} still points to %{_docdir}/%{name}-%{version} even after %global _docdir_fmt %{name}.
Or I am doing something wrong here, I don't know ?
 
> But I think that a different solution is actually better:
> c) simply install a compiled version of 'bench'.
> 
> I think this is better because as a user, I don't want to have to find out
> how to compile the .c file to run the benchmarks, I would prefer to be able
> to invoke it directly. I have now run bench myself, and I think it would be
> worthwhile to package, because the results are quite interesting, and
> relevant to how one would use blosc.

This makes sense. Moreover, the -O2 flag will match the actual blosc library from the package.
On the other hand, the optimization brought by -O3 as well as SSE are lost with this packaged blosc binary, right? (doesn't that somehow defeat the purpose of blosc ?) 

> There's a problem that making the bench binary and the associated
> plot-times.py script part of either of the two binary packages is
> problematic. If it is moved into the main package, it would start requiring
> python, and x86_64 versions would nod be co-installable. If is is installed
> as part of the -devel package, again, -devel would require python, and also
> not be coinstallable. I think that adding a -bench (or -test) package is the
> best option, with
> /usr/bin/blosc-bench and /usr/bin/blosc-plot-times.

Good idea. I tried to implement it in the new spec, which is probably not perfect. The blosc-plot-times is a link pointing to the actual python file in %doc. I guess this one should be in %{_datadir} instead ?

I still had to use %exclude to avoid duplicates. Using %doc bench, for some reason, also adds *.rst.

> So, whatever you decide wrt. the %files problem, please post a new .spec. As
> a reviewer, I don't think I should impose my view here, and you should pick
> whatever you think best from the maintainer point of view.

New spec online:
http://tnorth.fedorapeople.org/rev/blosc.spec
http://tnorth.fedorapeople.org/rev/blosc-1.2.3-4.fc19.src.rpm

(Is it ok to overwrite this SPEC file everytime? Old ones are still available in the SRPMS packages at http://tnorth.fedorapeople.org/rev/blosc-1.2.3-?.fc19.src.rpm)

Comment 6 Christopher Meng 2013-10-21 15:42:09 UTC
Try this:

%{!?_pkgdocdir: %global _pkgdocdir %{_docdir}/%{name}-%{version}}

Comment 7 Zbigniew Jędrzejewski-Szmek 2013-10-21 15:44:37 UTC
(In reply to Thibault North from comment #5)
> > BTW, the proper way to refer to %{docdir}/%{name} is through %{_pkgdocdir}.
> 
> Yes, but it looks like %{_pkgdocdir} still points to
> %{_docdir}/%{name}-%{version} even after %global _docdir_fmt %{name}.
> Or I am doing something wrong here, I don't know ?
Are you building this under F20/F21 or F19? You get an unversioned
%_pkgdocdir only in F >= 20.

> > But I think that a different solution is actually better:
> > c) simply install a compiled version of 'bench'.
> > 
> > I think this is better because as a user, I don't want to have to find out
> > how to compile the .c file to run the benchmarks, I would prefer to be able
> > to invoke it directly. I have now run bench myself, and I think it would be
> > worthwhile to package, because the results are quite interesting, and
> > relevant to how one would use blosc.
> 
> This makes sense. Moreover, the -O2 flag will match the actual blosc library
> from the package.
> On the other hand, the optimization brought by -O3 as well as SSE are lost
> with this packaged blosc binary, right? (doesn't that somehow defeat the
> purpose of blosc ?)
How the binary itself is compiled probably doesn't matter so much, compared to how the library is compiled.

> 
> > There's a problem that making the bench binary and the associated
> > plot-times.py script part of either of the two binary packages is
> > problematic. If it is moved into the main package, it would start requiring
> > python, and x86_64 versions would nod be co-installable. If is is installed
> > as part of the -devel package, again, -devel would require python, and also
> > not be coinstallable. I think that adding a -bench (or -test) package is the
> > best option, with
> > /usr/bin/blosc-bench and /usr/bin/blosc-plot-times.
> 
> Good idea. I tried to implement it in the new spec, which is probably not
> perfect. The blosc-plot-times is a link pointing to the actual python file
> in %doc. I guess this one should be in %{_datadir} instead ?
Maybe just install the script in %{_bindir}, just removing the extension?

> I still had to use %exclude to avoid duplicates. Using %doc bench, for some
> reason, also adds *.rst.
> 
> > So, whatever you decide wrt. the %files problem, please post a new .spec. As
> > a reviewer, I don't think I should impose my view here, and you should pick
> > whatever you think best from the maintainer point of view.
> 
> New spec online:
> http://tnorth.fedorapeople.org/rev/blosc.spec
> http://tnorth.fedorapeople.org/rev/blosc-1.2.3-4.fc19.src.rpm
> 
> (Is it ok to overwrite this SPEC file everytime? Old ones are still
> available in the SRPMS packages at
> http://tnorth.fedorapeople.org/rev/blosc-1.2.3-?.fc19.src.rpm)
Yeah, I think that's common practice.

Comment 8 Thibault North 2013-10-21 16:52:27 UTC
(In reply to  Christopher Meng from comment #6)
> Try this:
>
> %{!?_pkgdocdir: %global _pkgdocdir %{_docdir}/%{name}-%{version}}

s/%{version}// ?
Doesn't work here, but %global _pkgdocdir %{_docdir}/%{name} seems to make it. Thanks.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #7)
> (In reply to Thibault North from comment #5)
> > > BTW, the proper way to refer to %{docdir}/%{name} is through %{_pkgdocdir}.
> > 
> > Yes, but it looks like %{_pkgdocdir} still points to
> > %{_docdir}/%{name}-%{version} even after %global _docdir_fmt %{name}.
> > Or I am doing something wrong here, I don't know ?
> Are you building this under F20/F21 or F19? You get an unversioned
> %_pkgdocdir only in F >= 20.

F19, indeed! Thanks.

> > > But I think that a different solution is actually better:
> > > c) simply install a compiled version of 'bench'.
> > > 
> > > I think this is better because as a user, I don't want to have to find out
> > > how to compile the .c file to run the benchmarks, I would prefer to be able
> > > to invoke it directly. I have now run bench myself, and I think it would be
> > > worthwhile to package, because the results are quite interesting, and
> > > relevant to how one would use blosc.
> > 
> > This makes sense. Moreover, the -O2 flag will match the actual blosc library
> > from the package.
> > On the other hand, the optimization brought by -O3 as well as SSE are lost
> > with this packaged blosc binary, right? (doesn't that somehow defeat the
> > purpose of blosc ?)
> How the binary itself is compiled probably doesn't matter so much, compared
> to how the library is compiled.

My bad, I had in mind the previous Makefile which was compiling directly against the blosc code.
Now, it is too bad that blosc is compiled with %{?_smp_mflags}, because part of the power of blosc is provided by these SSE optimization. (checking how much would be interesting.)

> > Good idea. I tried to implement it in the new spec, which is probably not
> > perfect. The blosc-plot-times is a link pointing to the actual python file
> > in %doc. I guess this one should be in %{_datadir} instead ?
> Maybe just install the script in %{_bindir}, just removing the extension?

Yes, done.

Also, blosc-plot-times requires python-matplotlib%{?_isa}. I thought that requiring it would be ok, but rpmlint complains (explicit-lib-dependency). i guess this can be ignored?

http://tnorth.fedorapeople.org/rev/blosc.spec
http://tnorth.fedorapeople.org/rev/blosc-1.2.3-5.fc19.src.rpm

Comment 9 Zbigniew Jędrzejewski-Szmek 2013-10-21 17:22:10 UTC
(In reply to Thibault North from comment #8)
> (In reply to  Christopher Meng from comment #6)
> > Try this:
> >
> > %{!?_pkgdocdir: %global _pkgdocdir %{_docdir}/%{name}-%{version}}
> 
> s/%{version}// ?
> Doesn't work here, but %global _pkgdocdir %{_docdir}/%{name} seems to make
> it. Thanks.
This is just a fallback for build systems with old rpm. That's why it
is defined with -%{version}, to retain historical behaviour on old systems,
but allowing to use the same %spec. It should be conditional, so that you
get the new behaviour when enabled by the system.

The sed is still wrong, it's backwards...

If plot-times is in %{_bindir}, then %{_pkgdocdir}/bench only contains one
file (bench.c), so maybe it's no longer necessary to have a separate directory,
and bench.c could be installed in %{_pkgdocdir} directly?
    
> (In reply to Zbigniew Jędrzejewski-Szmek from comment #7)
> > (In reply to Thibault North from comment #5)
> > > > BTW, the proper way to refer to %{docdir}/%{name} is through %{_pkgdocdir}.
> > > 
> > > Yes, but it looks like %{_pkgdocdir} still points to
> > > %{_docdir}/%{name}-%{version} even after %global _docdir_fmt %{name}.
> > > Or I am doing something wrong here, I don't know ?
> > Are you building this under F20/F21 or F19? You get an unversioned
> > %_pkgdocdir only in F >= 20.
> 
> F19, indeed! Thanks.
> 
> > > > But I think that a different solution is actually better:
> > > > c) simply install a compiled version of 'bench'.
> > > > 
> > > > I think this is better because as a user, I don't want to have to find out
> > > > how to compile the .c file to run the benchmarks, I would prefer to be able
> > > > to invoke it directly. I have now run bench myself, and I think it would be
> > > > worthwhile to package, because the results are quite interesting, and
> > > > relevant to how one would use blosc.
> > > 
> > > This makes sense. Moreover, the -O2 flag will match the actual blosc library
> > > from the package.
> > > On the other hand, the optimization brought by -O3 as well as SSE are lost
> > > with this packaged blosc binary, right? (doesn't that somehow defeat the
> > > purpose of blosc ?)
> > How the binary itself is compiled probably doesn't matter so much, compared
> > to how the library is compiled.
> 
> My bad, I had in mind the previous Makefile which was compiling directly
> against the blosc code.
> Now, it is too bad that blosc is compiled with %{?_smp_mflags}, because part
> of the power of blosc is provided by these SSE optimization. (checking how
> much would be interesting.)
> 
> > > Good idea. I tried to implement it in the new spec, which is probably not
> > > perfect. The blosc-plot-times is a link pointing to the actual python file
> > > in %doc. I guess this one should be in %{_datadir} instead ?
> > Maybe just install the script in %{_bindir}, just removing the extension?
> 
> Yes, done.
> 
> Also, blosc-plot-times requires python-matplotlib%{?_isa}. I thought that
> requiring it would be ok, but rpmlint complains (explicit-lib-dependency). i
> guess this can be ignored?
Actually, blosc-plot-times is by itself noarch, so I think it's fine with
whatever python-matplotlib, so %{?_isa} can be removed.

> http://tnorth.fedorapeople.org/rev/blosc.spec
> http://tnorth.fedorapeople.org/rev/blosc-1.2.3-5.fc19.src.rpm

> cp -pr %{_pkgdocdir}/bench/plot-speeds.py ${RPM_BUILD_ROOT}/%{_bindir}/%{name}-plot-times 
This looks wrong.

Comment 10 Thibault North 2013-10-21 17:34:33 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #9)
> (In reply to Thibault North from comment #8)
> > (In reply to  Christopher Meng from comment #6)
> > > Try this:
> > >
> > > %{!?_pkgdocdir: %global _pkgdocdir %{_docdir}/%{name}-%{version}}
> > 
> > s/%{version}// ?
> > Doesn't work here, but %global _pkgdocdir %{_docdir}/%{name} seems to make
> > it. Thanks.
> This is just a fallback for build systems with old rpm. That's why it
> is defined with -%{version}, to retain historical behaviour on old systems,
> but allowing to use the same %spec. It should be conditional, so that you
> get the new behaviour when enabled by the system.

Ok, I was trying to have the same behaviour on F19, which wasn't right.

> The sed is still wrong, it's backwards...

Fixed.

> > Also, blosc-plot-times requires python-matplotlib%{?_isa}. I thought that
> > requiring it would be ok, but rpmlint complains (explicit-lib-dependency). i
> > guess this can be ignored?
> Actually, blosc-plot-times is by itself noarch, so I think it's fine with
> whatever python-matplotlib, so %{?_isa} can be removed.

Done.
 
> > cp -pr %{_pkgdocdir}/bench/plot-speeds.py ${RPM_BUILD_ROOT}/%{_bindir}/%{name}-plot-times 
> This looks wrong.

Indeed, remains of the link. Fixed.
http://tnorth.fedorapeople.org/rev/blosc.spec
http://tnorth.fedorapeople.org/rev/blosc-1.2.3-6.fc19.src.rpm

Thanks for your patience.

Comment 11 Zbigniew Jędrzejewski-Szmek 2013-10-21 18:20:30 UTC
> ... the Requires should be more exact (for -devel)
> Requires:  %{name}%{?_isa} = %{version}-%{release}
Still missing, I think.

> blosc-bench.x86_64: E: non-standard-executable-perm /usr/bin/blosc-bench 0775L

Should be 0755. I suppose that this is inherited from the environment, I have umask 0002. Probably 'install' instead of cp would be better, since it sets the permissions to u=rwx,go=rx.

Rpmlint (rest of output)
-------
Checking: blosc-1.2.3-6.fc19.x86_64.rpm
          blosc-devel-1.2.3-6.fc19.x86_64.rpm
          blosc-bench-1.2.3-6.fc19.x86_64.rpm
          blosc-1.2.3-6.fc19.src.rpm
blosc.x86_64: W: spelling-error %description -l en_US datasets -> data sets, data-sets, databases
blosc-devel.x86_64: W: no-documentation
blosc-bench.x86_64: E: explicit-lib-dependency python-matplotlib
The dependency is OK. python-matplotlib only provides
  python-matplotlib = 1.3.0-1.fc20
  python-matplotlib(x86-64) = 1.3.0-1.fc20
and the non-isa one is  more adequate.

blosc-bench.x86_64: W: spelling-error %description -l en_US memcpy -> memory
blosc-bench.x86_64: W: no-manual-page-for-binary blosc-plot-times
blosc-bench.x86_64: W: no-manual-page-for-binary blosc-bench
blosc.src: W: spelling-error %description -l en_US datasets -> data sets, data-sets, databases
4 packages and 0 specfiles checked; 2 errors, 6 warnings.

Rpmlint (installed packages)
----------------------------
# rpmlint blosc-devel blosc blosc-bench
blosc-devel.x86_64: W: no-documentation
blosc.x86_64: W: spelling-error %description -l en_US datasets -> data sets, data-sets, databases
blosc-bench.x86_64: E: explicit-lib-dependency python-matplotlib
blosc-bench.x86_64: W: spelling-error %description -l en_US memcpy -> memory
blosc-bench.x86_64: E: non-standard-executable-perm /usr/bin/blosc-bench 0775L
blosc-bench.x86_64: W: no-manual-page-for-binary blosc-plot-times
blosc-bench.x86_64: W: no-manual-page-for-binary blosc-bench
3 packages and 0 specfiles checked; 2 errors, 5 warnings.

Can be ignored.

I think that except those two issues noted at the top, package is OK.

Comment 12 Thibault North 2013-10-21 18:36:17 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #11)
> > ... the Requires should be more exact (for -devel)
> > Requires:  %{name}%{?_isa} = %{version}-%{release}
> Still missing, I think.

Fixed, thanks.

> > blosc-bench.x86_64: E: non-standard-executable-perm /usr/bin/blosc-bench 0775L
> 
> Should be 0755. I suppose that this is inherited from the environment, I
> have umask 0002. Probably 'install' instead of cp would be better, since it
> sets the permissions to u=rwx,go=rx.

Right!

http://tnorth.fedorapeople.org/rev/blosc.spec
http://tnorth.fedorapeople.org/rev/blosc-1.2.3-7.fc19.src.rpm

New rmplint output:
rpmlint -vv blosc-1.2.3-7.fc19.src.rpm blosc-1.2.3-7.fc19.x86_64.rpm blosc-devel-1.2.3-7.fc19.x86_64.rpm blosc-bench-1.2.3-7.fc19.x86_64.rpm blosc-debuginfo-1.2.3-7.fc19.x86_64.rpm blosc.spec 
blosc.src: I: checking
blosc.src: W: spelling-error %description -l en_US datasets -> data sets, data-sets, databases
blosc.src: I: checking-url http://blosc.org (timeout 10 seconds)
blosc.src: I: checking-url http://blosc.org/sources/1.2.3/blosc-1.2.3.tar.gz (timeout 10 seconds)
blosc.x86_64: I: checking
blosc.x86_64: W: spelling-error %description -l en_US datasets -> data sets, data-sets, databases
blosc.x86_64: I: checking-url http://blosc.org (timeout 10 seconds)
blosc-devel.x86_64: I: checking
blosc-devel.x86_64: I: checking-url http://blosc.org (timeout 10 seconds)
blosc-devel.x86_64: W: no-documentation
blosc-bench.x86_64: I: checking
blosc-bench.x86_64: E: explicit-lib-dependency python-matplotlib
blosc-bench.x86_64: W: spelling-error %description -l en_US memcpy -> memory
blosc-bench.x86_64: I: checking-url http://blosc.org (timeout 10 seconds)
blosc-bench.x86_64: W: no-manual-page-for-binary blosc-plot-times
blosc-bench.x86_64: W: no-manual-page-for-binary blosc-bench
blosc-debuginfo.x86_64: I: checking
blosc-debuginfo.x86_64: I: checking-url http://blosc.org (timeout 10 seconds)
/home/tnorth/rpmbuild/SPECS/blosc.spec: I: checking-url http://blosc.org/sources/1.2.3/blosc-1.2.3.tar.gz (timeout 10 seconds)
5 packages and 1 specfiles checked; 1 errors, 6 warnings.

Thanks everyone. I am a bit rusty, it's been a long time :)

Comment 13 Zbigniew Jędrzejewski-Szmek 2013-10-21 18:49:28 UTC
Package is APPROVED.

Comment 14 Ralf Corsepius 2013-10-22 02:53:58 UTC
Sorry, folks this package is broken:

a) On Fedora < 20, the packages are supposed to install their docs to 
/usr/share/doc/%{name}-%{version}

This package has /usr/share/doc/%{name} hard-coded somewhere:
# rpm -qlp blosc-1.2.3-7.fc19.x86_64.rpm 
...
/usr/share/doc/blosc
...
Please fix this.


b) This section from the *spec hard-codes -msse2:
...
%ifarch x86_64
cmake %{?_cmake_lib_suffix64} \
    -DCMAKE_C_FLAGS:STRING="%{optflags} -msse2" \
...
Please remove this.

Comment 15 Thibault North 2013-10-22 03:41:52 UTC
This seems to fix both issues (tested on f19 and f20):

--- blosc.spec  2013-10-21 14:34:52.000000000 -0400
+++ blosc.spec.new      2013-10-21 23:33:40.395646556 -0400
@@ -39,7 +39,7 @@
 # Use the proper library path and SSE2 instruction on 64bits systems
 %ifarch x86_64
 cmake %{?_cmake_lib_suffix64} \
-    -DCMAKE_C_FLAGS:STRING="%{optflags} -msse2" \
+    -DCMAKE_C_FLAGS:STRING="%{optflags}" \
     -DCMAKE_INSTALL_PREFIX=%{_prefix} \
     -DBUILD_STATIC:BOOL=OFF \
     -DTEST_INCLUDE_BENCH_SUITE:BOOL=OFF .
@@ -69,7 +69,6 @@
 %check
 make test VERBOSE=1
 
-%global _docdir_fmt %{name}
 
 %install
 
http://tnorth.fedorapeople.org/rev/blosc.spec
http://tnorth.fedorapeople.org/rev/blosc-1.2.3-8.fc19.src.rpm

Comment 16 Zbigniew Jędrzejewski-Szmek 2013-10-22 04:45:33 UTC
Yeah, it looks correct now.

Comment 17 Ralf Corsepius 2013-10-22 04:51:50 UTC
(In reply to Thibault North from comment #15)
> This seems to fix both issues (tested on f19 and f20):
Yes, much better now!

However, checking *-8, I just noticed another issue:

* Compilation doesn't honor %optflags:
...
cd /builddir/build/BUILD/blosc-1.2.3/tests && /usr/bin/cmake -E cmake_link_script CMakeFiles/test_basics.dir/link.txt --verbose=1
/usr/lib/ccache/cc  -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m32 -march=i686 -mtune=atom -fasynchronous-unwind-tables -O3 -DNDEBUG    CMakeFiles/test_basics.dir/test_basics.c.o  -o test_basics  -L/builddir/build/BUILD/blosc-1.2.3/blosc -rdynamic ../blosc/libblosc.so.1.2.3 -lpthread -Wl,-rpath,/builddir/build/BUILD/blosc-1.2.3/blosc
...

Note: There is a -O3 overriding the -O2 from %optflags.

I am far from being a cmake specialist, but AFAICT, one fix to this would be using %cmake instead of the cmake-%if-cascade:

%cmake \
     -DBUILD_STATIC:BOOL=OFF \
     -DTEST_INCLUDE_BENCH_SUITE:BOOL=OFF .


Finally, %_pkgdocdir meanwhile is available in all releases of Fedoras. 
I.e. you plan to support EPEL you could also remove %{!?_pkgdocdir:...}.

Comment 18 Ralf Corsepius 2013-10-22 05:17:40 UTC
(In reply to Ralf Corsepius from comment #17)
> I.e. you plan to support EPEL you could also remove %{!?_pkgdocdir:...}.
Sorry, typo. This was meant to be
"I.e. unless you plan to support EPEL, you could also remove %{!?_pkgdocdir:...}."

Comment 19 Michael Schwendt 2013-10-22 09:04:45 UTC
> a) On Fedora < 20, the packages are supposed to install their
> docs to /usr/share/doc/%{name}-%{version}

That's not accurate. If a new package submitter asked where to find that in the packaging guidelines, one could not answer that question.

Documentation files are supposed to be marked as %doc, and when using the %doc macro, that one defaults to %{_docdir}/%{name}-%{version}.

https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation

The guidelines don't explicitly request the files to be stored in that path. They don't comment on the different path per sub-package either. And some source tarballs install into a non-versioned /usr/share/doc/%{name} by default, and packagers only sometimes change that. Sometimes they even turn a versioned dir into a non-versioned one.

Comment 20 Ralf Corsepius 2013-10-22 12:45:30 UTC
(In reply to Michael Schwendt from comment #19)
> > a) On Fedora < 20, the packages are supposed to install their
> > docs to /usr/share/doc/%{name}-%{version}
> 
> That's not accurate. If a new package submitter asked where to find that in
> the packaging guidelines, one could not answer that question.
> 
> Documentation files are supposed to be marked as %doc, and when using the
> %doc macro, that one defaults to %{_docdir}/%{name}-%{version}.
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation
Correct, this needs an update.

> The guidelines don't explicitly request the files to be stored in that path.
> They don't comment on the different path per sub-package either. And some
> source tarballs install into a non-versioned /usr/share/doc/%{name} by
> default, and packagers only sometimes change that. Sometimes they even turn
> a versioned dir into a non-versioned one.

Michael, STOP THAT, please! Common sense and using brains have become rare events on your side? I feel you have become one of those people, who seem to be in need of laws and regulations for everything!

Comment 21 Michael Schwendt 2013-10-22 13:23:00 UTC
Don't shout. Also, your repeated personal attacks and choice of words leave much to be desired. :-(  Please notice http://fedoraproject.org/code-of-conduct

I'm sorry your feeling is misleading you. You read too much into my comment. I've only pointed out that so far the packagers have been free to use a non-versioned docdir. There are examples in Fedora 19 and older. They are not the norm, because %doc defaults to be versioned. But they do exist, and the guidelines don't disallow that. Is this another review you would like to veto?

Comment 22 Ralf Corsepius 2013-10-22 13:34:55 UTC
(In reply to Michael Schwendt from comment #21)
> Don't shout. Also, your repeated personal attacks and choice of words leave
> much to be desired. :-(  Please notice
> http://fedoraproject.org/code-of-conduct
Well, I am seriously pissed off by a series of postings from you, which don't leave me any other choice to use this tone and to consider further consequence.

As this is all OT here, More on this in German and on PM.

> I'm sorry your feeling is misleading you. You read too much into my comment.
> I've only pointed out that so far the packagers have been free to use a
> non-versioned docdir.
No, they have not been - These all were bugs.

> There are examples in Fedora 19 and older.
Correct, there are broken packages.

Comment 23 Michael Schwendt 2013-10-22 14:43:41 UTC
Those packages are not broken. A non-versioned docdir allows for users setting bookmarks to the documentation, for example. That's really very helpful with large HTML trees and even PDFs (where the reader remembers the most recently viewed files and the current page number).

> Well, I am seriously pissed off by a series of postings from you,

Not my fault.

> which don't leave me any other choice to use this tone and

Keep trying. There's absolutely no reason to talk to me that way.

> to consider further consequence.

Perhaps the Fedora CWG may be helpful in this case?

Comment 24 Ralf Corsepius 2013-10-22 15:04:11 UTC
(In reply to Michael Schwendt from comment #23)
> > to consider further consequence.
> 
> Perhaps the Fedora CWG may be helpful in this case?
Great now you are threatening me once more - Don't you realize how hostile and agressive you are?

Comment 25 Ralf Corsepius 2013-10-22 15:04:52 UTC
Get me outa here, I am unable to share a room with Mr. Schwendt.

Comment 26 Michael Schwendt 2013-10-22 15:29:49 UTC
> Great now you are threatening me once more

Not once.

> Don't you realize how hostile and agressive you are?

No, I don't. In this ticket I disagree with you about the docdirs, but that's no reason to attack me personally, and if you draw a connection to disagreements external to this ticket, that's your personal issue. I don't fight. I voice my opinion. It's you who would like to find a way to silence me. You've mentioned "considering further consequence". Consulting the CWG for help and a neutral opinion is an idea.

Comment 27 Thibault North 2013-10-22 15:40:28 UTC
The above kind of reminds me the motivation I had to stay away from review requests/working on packages... (see BZ#852898)

The release type is set to "Debug" instead of "Release".
Plus, the bench/ was compiled twice (once by cmake, once by a call to make).
This should fix the flag issues.

http://tnorth.fedorapeople.org/rev/blosc.spec
http://tnorth.fedorapeople.org/rev/blosc-1.2.3-9.fc19.src.rpm

Comment 29 Thibault North 2013-10-22 17:51:14 UTC
Correct URL :
http://tnorth.fedorapeople.org/rev/blosc-1.2.3-9.fc16.src.rpm

Thank you.

Comment 30 Thibault North 2013-10-22 18:09:57 UTC
New Package SCM Request
=======================
Package Name: blosc
Short Description: A high performance compressor optimized for binary data
Owners: tnorth zbyszek
Branches: f19 f20 el6
InitialCC:

Comment 31 Gwyn Ciesla 2013-10-24 11:59:12 UTC
Git done (by process-git-requests).

Comment 32 Fedora Update System 2013-10-24 13:20:12 UTC
blosc-1.2.3-9.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/blosc-1.2.3-9.fc19

Comment 33 Fedora Update System 2013-10-24 13:20:22 UTC
blosc-1.2.3-9.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/blosc-1.2.3-9.fc20

Comment 34 Zbigniew Jędrzejewski-Szmek 2013-10-24 14:45:44 UTC
Added to https://fedoraproject.org/wiki/Upstream_release_monitoring#B.

Comment 35 Fedora Update System 2013-10-24 17:45:25 UTC
blosc-1.2.3-9.fc20 has been pushed to the Fedora 20 testing repository.

Comment 36 Fedora Update System 2013-11-05 03:01:00 UTC
blosc-1.2.3-9.fc19 has been pushed to the Fedora 19 stable repository.

Comment 37 Fedora Update System 2013-11-10 08:00:33 UTC
blosc-1.2.3-9.fc20 has been pushed to the Fedora 20 stable repository.

Comment 38 Zbigniew Jędrzejewski-Szmek 2015-05-28 14:28:57 UTC
Package Change Request
======================
Package Name: blosc
New Branches: epel7
Owners: zbyszek tnorth
InitialCC:

Comment 39 Gwyn Ciesla 2015-05-28 15:27:44 UTC
Git done (by process-git-requests).


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