Bug 441965 - Review Request: eet - Library for speedy data storage, retrieval, and compression
Summary: Review Request: eet - Library for speedy data storage, retrieval, and compres...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Patrice Dumas
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 442276
TreeView+ depends on / blocked
 
Reported: 2008-04-10 21:29 UTC by Pavel Shevchuk
Modified: 2008-04-24 23:47 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-04-24 23:47:28 UTC
Type: ---
Embargoed:
pertusus: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Pavel Shevchuk 2008-04-10 21:29:44 UTC
Spec URL: http://rpm.scwlab.com/eet-goes-rawhide/eet.spec
SRPM URL: http://rpm.scwlab.com/eet-goes-rawhide/eet-0.9.99900-1.fc9.src.rpm
i386 and x86_64 RPMs built in mock: http://rpm.scwlab.com/eet-goes-rawhide/

Description:
Eet is a tiny library designed to write an arbitary set of chunks of
data to a file and optionally compress each chunk (very much like a
zip file) and allow fast random-access reading of the file later
on. It does not do zip as a zip itself has more complexity than is
needed, and it was much simpler to implement this once here.

It also can encode and decode data structures in memory, as well as
image data for saving to eet files or sending across the network to
other machines, or just writing to arbitary files on the system. All
data is encoded in a platform independent way and can be written and
read by any architecture.


RPMLint is silent on all RPMs i built.
License is a bit customized, but Tom "spot" Callawayin from fedora-legal-list said it's MIT ( https://www.redhat.com/archives/fedora-legal-list/2008-April/msg00020.html )
I've been able to successfully use rpms built in mock on different machine to built software against this library.

I spent a week studying RPM documentation and packaging guidelines, i appreciate any feedback

Comment 1 Rex Dieter 2008-04-11 15:29:34 UTC
I'll take a look in a bit.

Comment 2 Terje Røsten 2008-04-13 20:52:23 UTC
Some small comments: 
 o add -p to install to preserve timestamps 
 o find has a nice option -delete:
    find ..... -exec rm -f {} ';' -> find ..... -delete





Comment 3 Pavel Shevchuk 2008-04-13 21:43:16 UTC
Thank you very much for your notes. Fixed these two bugs in this package, and 
also in embryo and evas (also submitted for review)

Comment 4 Patrice Dumas 2008-04-18 17:37:16 UTC
Each time you modify your package, you should increment the release number, and
redo a .src.rpm and repost the link to the modified .src.rpm.

There are some warnings about missing declarations corresponding with missing
includes that should be fixed, if possible:
eet_image.c: In function 'eet_data_image_lossless_convert':
eet_image.c:384: warning: implicit declaration of function 'htonl'
eet_lib.c: In function 'eet_open':
eet_lib.c:1173: warning: implicit declaration of function 'unlink'

htonl: #include <arpa/in.h>
unlink: #include <unistd.h>

I suggest using
make install DESTDIR=$RPM_BUILD_ROOT INSTALL='install -p'
to keep timestamps of installed files.

The -devel subpackage should depend on pkgconfig.

rpmlint is silent.

The source timestamp is not kept:
$ ls -l eet-0.9.99900.tar.bz2 ../SOURCES/eet-0.9.99900.tar.bz2 
-rw-rw-r-- 1 pat pat 2788829 mar 31 22:08 eet-0.9.99900.tar.bz2
-rw-rw-r-- 1 pat pat 2788829 avr  4 14:21 ../SOURCES/eet-0.9.99900.tar.bz2

but match upstream:
c9f75595152f571139df450314bdb5fc  eet-0.9.99900.tar.bz2

Some documentation should be shipped, either in a -doc subpackage, or in the
devel package. I suggest shipping doc/html/.

You can use wget -N, or spectool -g.

Comment 5 Pavel Shevchuk 2008-04-19 00:17:39 UTC
* Sat Apr 19 2008 Pavel "Stalwart" Shevchuk <stlwrt> - 0.9.99900-2
- Fixed timestamp of source tarball
- Preserve timestamps of installed files
- Added pkgconfig to -devel dependencies
- Added html docs

Missing includes are fixed in upstream trunk, new release candidate tarball 
will be released soon. I don't think patch is needed for current tarball 
because this bug doesn't impact anything - i'm building and runnings apps using 
this lib without any problems.

New spec: http://rpm.scwlab.com/eet-goes-rawhide/eet.spec
New SRPM: http://rpm.scwlab.com/eet-goes-rawhide/eet-0.9.99900-2.fc9.src.rpm

I will fix timestamps for other packages too, next week

Comment 6 Terje Røsten 2008-04-19 08:19:26 UTC
> - Added html docs

From spec
install -d -m 755 $RPM_BUILD_ROOT%{_docdir}/%{name}-%{version}/html
install -p -m 644 doc/html/* $RPM_BUILD_ROOT%{_docdir}/%{name}-%{version}/html

There is more elegant and correct way, just add html to the %doc line:

%doc AUTHORS COPYING COPYING-PLAIN README html


Comment 7 Patrice Dumas 2008-04-19 09:36:11 UTC
(In reply to comment #5)

> Missing includes are fixed in upstream trunk, new release candidate tarball 
> will be released soon. I don't think patch is needed for current tarball 
> because this bug doesn't impact anything - i'm building and runnings apps using 
> this lib without any problems.

Alright. In my opinion it is indeed sufficient to have it fixed upstream. 
But the fact that it builds and run is not an argument, since what not having
prototypes shows is that the compiler cannot replace those functions by macros
that do, for example, bound checking or whatever. I mean, if the prototypes
werre incorrect it would be much more problematic anyway.

Comment 8 Patrice Dumas 2008-04-19 09:56:37 UTC
The pkgconfig file is wrong, since it has
Libs.private: -lz -ljpeg  @winsock_libs@ -lm

I am not sure that the zlib-devel requires is needed for the devel
sub package, since the zlib and jpeg are linked through eet only, and
there is no static eet lib (and there is certainly no need for a 
static eet library). And there is no reference to zlib, jpeg headers
in th eet headers either.

Comment 9 Patrice Dumas 2008-04-19 10:03:19 UTC
(In reply to comment #6)
> > - Added html docs
> 
> From spec
> install -d -m 755 $RPM_BUILD_ROOT%{_docdir}/%{name}-%{version}/html
> install -p -m 644 doc/html/* $RPM_BUILD_ROOT%{_docdir}/%{name}-%{version}/html
>
> There is more elegant and correct way, just add html to the %doc line:
> 
> %doc AUTHORS COPYING COPYING-PLAIN README html

In fact it is already done in -devel.

What happens is that the files installed by the above 2 commands are 
erased by the %doc command of the main package:

Exécution_de(%doc): /bin/sh -e /var/tmp/rpm-tmp.33007
+ umask 022
+ cd /home/dumas/RPM-fc/BUILD
+ cd eet-0.9.99900
+ DOCDIR=/var/tmp/eet-0.9.99900-2.fc9-root-dumas/usr/share/doc/eet-0.9.99900
+ export DOCDIR
+ rm -rf /var/tmp/eet-0.9.99900-2.fc9-root-dumas/usr/share/doc/eet-0.9.99900
+ /bin/mkdir -p /var/tmp/eet-0.9.99900-2.fc9-root-dumas/usr/share/doc/eet-0.9.99900

So you just need to remove those commands and leave the %doc in -devel
as you have done.


Comment 10 Pavel Shevchuk 2008-04-19 12:11:31 UTC
* Sat Apr 19 2008 Pavel "Stalwart" Shevchuk <stlwrt> - 0.9.99900-3
- Cleaned up documentation installation
- Removed unneded dependency on zlib-devel from eet-devel

Removed manual installation of html docs

zlib-devel is required by almost all packages that use eet so i put it as 
dependency for eet-devel, so i thought it's good idea to put it here.

What's wrong with pkgconfig?

Comment 11 Patrice Dumas 2008-04-19 12:31:52 UTC
(In reply to comment #10)
> * Sat Apr 19 2008 Pavel "Stalwart" Shevchuk <stlwrt> - 0.9.99900-3

> zlib-devel is required by almost all packages that use eet so i put it as 
> dependency for eet-devel, so i thought it's good idea to put it here.

I don't think so, if a package directly requires zlib-devel it should do it,
but if it requires it only through eet, it should not have be in the buildroot
such that the build fails if zlib-devel is not build required.
 
> What's wrong with pkgconfig?

The @winsock_libs@ autoconf variable that is not substituted.

$ pkg-config --static --libs eet
@winsock_libs@ -leet -lz -ljpeg -lm  


In the fedora case it is not that problematic since there is no static 
libs, but it should be fixed upstream.

Comment 12 Pavel Shevchuk 2008-04-19 13:08:55 UTC
* Sat Apr 19 2008 Pavel "Stalwart" Shevchuk <stlwrt> - 0.9.99900-4
- Added workaround for bug in eet.pc. Proper fix is commited upstream

I contacted developer, bug is now fixed upstream: http://enlightenment.org/
viewvc/e17/libs/eet/eet.pc.in?r1=1.6&r2=1.7

New spec: http://rpm.scwlab.com/eet-goes-rawhide/eet.spec
New SRPM: http://rpm.scwlab.com/eet-goes-rawhide/eet-0.9.99900-4.fc9.src.rpm

Comment 13 Patrice Dumas 2008-04-19 13:38:41 UTC
* rpmlint is silent
* free software, license included
* follow guidelines
* libraries packaged correctly
* match upstream
c9f75595152f571139df450314bdb5fc  ../SOURCES/eet-0.9.99900.tar.bz2
* %files section right

APPROVED

I can sponsor you. I theory you should prove more, but since you have
a whole stack of interdependent packages, I think it is better to 
sponsor you now, such that reviewers can install simply dependent
packages. 

Any objection, Rex, Terje?

Comment 14 Pavel Shevchuk 2008-04-19 13:47:05 UTC
Thank you for your comments and suggestions =)

Rex planned to stay away from internet for this weekend, according to his 
Twitter: http://twitter.com/rdieter

Comment 15 Rex Dieter 2008-04-19 14:04:50 UTC
:)  I had good intentions.

Patrice, no objections, thanks for jumping in here.

Comment 16 Terje Røsten 2008-04-19 14:37:25 UTC
> Any objection, Rex, Terje?

Package seems to be in good shape now.

Could maybe ping upstream about the rpath issue, which are a problem
in more efl packages.



Comment 17 Pavel Shevchuk 2008-04-19 15:27:24 UTC
Many E-packages also bundle Vera font (which i'm going to strip out when 
building). People are strange, especially enlighted =)

Comment 18 Pavel Shevchuk 2008-04-23 22:58:51 UTC
New Package CVS Request
=======================
Package Name: eet
Short Description: Library for speedy data storage, retrieval, and compression
Owners: stalwart
Branches: F-8 F-9
InitialCC:
Cvsextras Commits: yes

Comment 19 Kevin Fenzi 2008-04-24 16:24:00 UTC
cvs done. 

Note that this was a package that was orphaned in the FC6 timeframe.
You might want to look at the last spec for any improvements before importing. 


Comment 20 Pavel Shevchuk 2008-04-24 23:46:31 UTC
Thanks to everyone, koji just finished building packages ;)


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