Bug 438588 (zfstream) - Review Request: zfstream - C++ iostream like access to compressed files
Summary: Review Request: zfstream - C++ iostream like access to compressed files
Keywords:
Status: CLOSED NEXTRELEASE
Alias: zfstream
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Orcan Ogetbil
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 491616 vfrnav
TreeView+ depends on / blocked
 
Reported: 2008-03-22 14:29 UTC by Thomas Sailer
Modified: 2009-05-09 04:17 UTC (History)
4 users (show)

Fixed In Version: 20041202-5.fc11
Clone Of:
Environment:
Last Closed: 2009-05-02 16:33:22 UTC
Type: ---
Embargoed:
oget.fedora: fedora-review+
dennis: fedora-cvs+


Attachments (Terms of Use)

Description Thomas Sailer 2008-03-22 14:29:25 UTC
Spec URL: http://sailer.fedorapeople.org/zfstream.spec
SRPM URL: http://sailer.fedorapeople.org/zfstream-20041202-1.fc8.src.rpm
Description: zfstream provides a library to access compressed files (gzip, bzip2, single file contained in a ZIP archive) using a C++ iostream compatible interface.

Comment 1 Jason Tibbitts 2008-05-11 04:23:10 UTC
I noticed the following troubling rpmlint complaints; it looks like this library
isn't linked against libbz2 but is linked against libm even though it doesn't
need to be.  The latter is easily fixed with the trick in the
unused-direct-shlib-dependency section of
http://fedoraproject.org/wiki/PackageMaintainers/CommonRpmlintIssues.

zfstream.x86_64: W: undefined-non-weak-symbol /usr/lib64/libzfstream.so.0.0.0
inflateEnd
zfstream.x86_64: W: undefined-non-weak-symbol /usr/lib64/libzfstream.so.0.0.0 gzopen
zfstream.x86_64: W: undefined-non-weak-symbol /usr/lib64/libzfstream.so.0.0.0
gzclose
zfstream.x86_64: W: undefined-non-weak-symbol /usr/lib64/libzfstream.so.0.0.0
deflate
zfstream.x86_64: W: undefined-non-weak-symbol /usr/lib64/libzfstream.so.0.0.0
BZ2_bzwrite
zfstream.x86_64: W: undefined-non-weak-symbol /usr/lib64/libzfstream.so.0.0.0
inflateInit2_
zfstream.x86_64: W: undefined-non-weak-symbol /usr/lib64/libzfstream.so.0.0.0
inflate
zfstream.x86_64: W: undefined-non-weak-symbol /usr/lib64/libzfstream.so.0.0.0
BZ2_bzopen
zfstream.x86_64: W: undefined-non-weak-symbol /usr/lib64/libzfstream.so.0.0.0
gzwrite
zfstream.x86_64: W: undefined-non-weak-symbol /usr/lib64/libzfstream.so.0.0.0 crc32
zfstream.x86_64: W: undefined-non-weak-symbol /usr/lib64/libzfstream.so.0.0.0
deflateEnd
zfstream.x86_64: W: undefined-non-weak-symbol /usr/lib64/libzfstream.so.0.0.0
BZ2_bzclose
zfstream.x86_64: W: undefined-non-weak-symbol /usr/lib64/libzfstream.so.0.0.0
deflateInit2_
zfstream.x86_64: W: undefined-non-weak-symbol /usr/lib64/libzfstream.so.0.0.0 gzread
zfstream.x86_64: W: undefined-non-weak-symbol /usr/lib64/libzfstream.so.0.0.0
BZ2_bzread
zfstream.x86_64: W: undefined-non-weak-symbol /usr/lib64/libzfstream.so.0.0.0
get_crc_table
zfstream.x86_64: W: undefined-non-weak-symbol /usr/lib64/libzfstream.so.0.0.0
BZ2_bzerror
zfstream.x86_64: W: unused-direct-shlib-dependency
/usr/lib64/libzfstream.so.0.0.0 /lib64/libm.so.6


Comment 2 Thomas Sailer 2008-05-16 22:39:10 UTC
What tool versions did you use?

I just tried to compile it on a freshly installed F9 system with all released
updates applied, and it works.

The following packages were used:
libtool-1.5.24-6.fc9.i386
automake-1.10.1-2.noarch
autoconf-2.61-10.fc9.noarch
binutils-2.18.50.0.6-2.i386
gcc-4.3.0-8.i386


Comment 3 Jason Tibbitts 2008-07-03 02:53:27 UTC
FYI, this is all still present in current rawhide.

Please note that in order to see these complaints, you must install the package
and then run "rpmlint zfstream".  I don't think any of this is x86_64-dependent.

Comment 4 Debarshi Ray 2008-08-24 14:24:47 UTC
Ping?

Comment 5 Debarshi Ray 2008-09-26 05:33:52 UTC
The actual description in the Spec does not match with the one in the review. In any case, I don't think it is relevant to include the names of upstream developers in the description. Use a separate file in %doc if need be.

Comment 6 Debarshi Ray 2008-10-15 20:15:15 UTC
Ping?

Comment 7 Thomas Sailer 2008-11-04 10:18:53 UTC
http://sailer.fedorapeople.org/zfstream.spec
http://sailer.fedorapeople.org/zfstream-20041202-2.fc10.src.rpm

New %description:
zfstream is a small C++ library which provides an abstraction API for
reading and writing compressed and non-compressed files using the same
API. It supports libz and libbz2 compression schemes. The library is
trivial to use and provides client applications with a unified
interface for reading and writing files without having to know whether
they are compressed or not.

On my current x86_64 rawhide system:
rpmlint zfstream-20041202-2.fc10.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
pmlint zfstream.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
rpmlint zfstream-20041202-2.fc10.x86_64.rpm. zfstream-devel-20041202-2.fc10.x86_64.rpm zfstream-debuginfo-20041202-2.fc10.x86_64.rpm
zfstream.x86_64: W: no-documentation
zfstream-devel.x86_64: W: no-documentation
zfstream-debuginfo.x86_64: E: non-standard-dir-perm /usr/lib/debug 0775
3 packages and 0 specfiles checked; 1 errors, 2 warnings.
rpmlint zfstream
zfstream.x86_64: W: no-documentation

I don't know what to do about the debuginfo error, as the debuginfo package is generated automatically and I don't do anything evil in the spec file, afaik.

Comment 8 Jason Tibbitts 2008-11-05 16:06:07 UTC
I wouldn't worry about the non-standard-dir-perm thing; I believe it simply has to do with your umask.  I don't see it here.

I have to ask, though, where does zfstream-autotools.tar.gz come from?  It seems to contain several additional source files and such.  What is the upstream status of all that code?  We strongly discourage significantly forking upstream source within a package; if you want to fork, do it properly and set up some kind of project hosting.

Comment 9 Thomas Sailer 2008-11-05 16:27:14 UTC
zfstream-autotools.tar.gz has been written by me. Yes, it has been sent upstream, but the author won't take it, as he is also the author of toc, the original build system.

I tried half a day to get toc working, without success. Even if I got it to
work, it would unlikely have worked and picked the correct compiler flags etc.
for any architecture I don't own (i.e. everything but x86). So it seemed far
less work to just drop in autotools scripts. And autotool is working and
maintained for pretty much all architectures fedora supports, so it seems to me
that with autotools build scripts, I will have a better chance of getting the
packages to compile and run on other architectures as well.

See the similar discussion on:
https://bugzilla.redhat.com/show_bug.cgi?id=438587

BTW: I do not intend to actually fork the project source code, just replace the build system, as it proved unworkable for me. I will send source code changes upstream, and the author will apply them. He's just not keen on replacing the build system.

Comment 10 Jason Tibbitts 2008-11-05 16:53:08 UTC
So you're saying that all of the .c, .cpp and .h files in zfstream-autotools.tar.gz are part of the build system?  That seems difficult to believe.

Obviously if toc doesn't work we have to do something.  I'm surprised that someone thinks autotools is an improvement over anything, but so be it.  Still, adding new source goes beyond a simple buildsystem change.

Comment 11 Thomas Sailer 2008-11-05 17:14:44 UTC
autotools may be crap, but at least it's known crap 8-)

If you have an example of a Makefile, that builds a shared library and works on all supported/soon to be supported architectures (including mingw), then I'd be interested!

The archive also contains the source code for supporting zip files (in addition to gz and bz2). This code has been sent upstream, and the author agreed (on Jan 2008) to include it, but apparently hasn't gotten around it. I will ping again...

Comment 12 Orcan Ogetbil 2009-04-30 05:35:26 UTC
Here are my notes for this package:

- Package builds fine in koji rawhide:
   http://koji.fedoraproject.org/koji/taskinfo?taskID=1329697

* rpmlints:
  zfstream-devel.x86_64: W: no-documentation
  zfstream.x86_64: W: no-documentation

The file ChangeLog can go to %doc. I don't think it is worth packaging the LICENSE file as it doesn't say much about the license.

* Patches should be explained and be submitted to upstream. Since the project seems dead, it won't hurt to bypass the submission part. But please explain in the SPEC file what the patch does.

* Similarly, please give the reasoning of Source1.

* Please remove the duplicate copy of minizip, which we already have in Fedora, from Source1 and adjust the BR's. Note that minizip-devel already requires zlib-devel.

* The package must be named according to the Package Naming Guidelines. The upstream tarball has a different name. Why don't we use it?

* No need to explicitly BR automake. libtool does and always will pull that for you.

! Please make use of the %{name} macro.

* The devel package must require "bzip2-devel", "zlib-devel or minizip-devel (?)", and "pkgconfig" 

? About "touch NEWS README AUTHORS": Wasn't there an --add-missing flag to automake?

! No need for blank %doc's.

! Please make the descriptions span 80 columns.

Comment 13 Thomas Sailer 2009-04-30 13:45:09 UTC
Update
Spec URL: http://sailer.fedorapeople.org/zfstream.spec
SRPM URL: http://sailer.fedorapeople.org/zfstream-20041202-3.fc11.src.rpm

(In reply to comment #12)
> The file ChangeLog can go to %doc. I don't think it is worth packaging the
> LICENSE file as it doesn't say much about the license.

Done and agreed.

> * Patches should be explained and be submitted to upstream. Since the project
> seems dead, it won't hurt to bypass the submission part. But please explain in
> the SPEC file what the patch does.

I have sent the patch upstream by PM (the project lacks a bug tracker). While the original author said he integrated the patch into his tree, he apparently hasn't gotten around to release a new version.

> * Similarly, please give the reasoning of Source1.

See comment #9. Basically because I couldn't get toc working, and furthermore allows autotools to cross-compile the lib (I have also submitted mingw32-zfstream for review).

> * Please remove the duplicate copy of minizip, which we already have in Fedora,
> from Source1 and adjust the BR's. Note that minizip-devel already requires
> zlib-devel.

Done.

> * The package must be named according to the Package Naming Guidelines. The
> upstream tarball has a different name. Why don't we use it?

The project is named just "zfstream" on the author's homepage. It's just that the tarball has libs11n_ prefixed. s11n.net is another project by the same author, so I guess that's where the prefix comes from. zfstream however has nothing to do with s11n.net, so I found it more natural to just name the package zfstream.

> * No need to explicitly BR automake. libtool does and always will pull that for
> you.

Done

> ! Please make use of the %{name} macro.

Done.

> * The devel package must require "bzip2-devel", "zlib-devel or minizip-devel
> (?)", and "pkgconfig" 

Done. I think zlib-devel is enough, as zipstream.hpp does not include anything from minizip-devel, minizip is only needed for building

> ? About "touch NEWS README AUTHORS": Wasn't there an --add-missing flag to
> automake?

Actually, -a (which is already present) or --add-missing (the full name for -a) only adds missing files _other_ than NEWS, README and AUTHORS. So build will fail if you remove the touch.

> ! No need for blank %doc's.

Done.

> ! Please make the descriptions span 80 columns.  

Shortened.

Comment 14 Orcan Ogetbil 2009-04-30 15:39:49 UTC
Thanks for adding the source file explanations to the specfile.

Everything looks good, except

> > ! Please make the descriptions span 80 columns.  
> 
> Shortened.  

I don't see any change. In fact, you shouldn't shorten them. Some lines in the description are even smaller than 70 columns. They need stretched out a little.

But this is not a blocker. You can do it before you commit

-------------------------------------------
This package (zfstream) is APPROVED by oget
-------------------------------------------

Comment 15 Thomas Sailer 2009-04-30 15:53:22 UTC
(In reply to comment #14)

> I don't see any change. In fact, you shouldn't shorten them. Some lines in the
> description are even smaller than 70 columns. They need stretched out a little.

Ah sorry, I misunderstood what you meant. Will fix before checkin, thanks!

Comment 16 Thomas Sailer 2009-04-30 15:54:48 UTC
Thank you for the review.


New Package CVS Request
=======================
Package Name: zfstream
Short Description: C++ iostream like access to compressed files
Owners: sailer
Branches: F-10 F-11
InitialCC:

Comment 17 Dennis Gilmore 2009-05-01 20:54:44 UTC
CVS Done

Comment 18 Fedora Update System 2009-05-01 21:21:12 UTC
zfstream-20041202-5.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/zfstream-20041202-5.fc11

Comment 19 Fedora Update System 2009-05-01 21:22:15 UTC
zfstream-20041202-5.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/zfstream-20041202-5.fc10

Comment 20 Fedora Update System 2009-05-02 16:33:16 UTC
zfstream-20041202-5.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 21 Fedora Update System 2009-05-09 04:17:09 UTC
zfstream-20041202-5.fc11 has been pushed to the Fedora 11 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.