Bug 328771 - Review Request: babl - A dynamic, any to any, pixel format conversion library
Review Request: babl - A dynamic, any to any, pixel format conversion library
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Patrice Dumas
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 328781
  Show dependency treegraph
 
Reported: 2007-10-11 20:51 EDT by Deji Akingunola
Modified: 2007-11-30 17:12 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-10-22 22:31:43 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
pertusus: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Deji Akingunola 2007-10-11 20:51:52 EDT
Spec URL: ftp://czar.eas.yorku.ca/pub/babl/bal.spec
SRPM URL: ftp://czar.eas.yorku.ca/pub/babl/babl-0.0.15-0.1.20071011svn.fc8.src.rpm
Description: Babl is a dynamic, any to any, pixel format conversion library. It
provides conversions between the myriad of buffer types images can be
stored in. Babl doesn't only help with existing pixel formats, but also
facilitates creation of new and uncommon ones.

NOTE:
This software, babl and gegl (that I'll also be submitting for review) are both currently alpha quality software under development. However, they are both needed to build/run the latest version of another package I maintain, gnomes-scan. The last version of gnome-scan without these dependencies doesn't build successfully on current rawhide.
Comment 1 Patrice Dumas 2007-10-12 03:37:55 EDT
For the url, you should use, such that it is easier to check 
your source:
http://fedoraproject.org/wiki/Packaging/SourceURL#head-615f6271efb394ab340a93a6cf030f2d08cf0d49

and not use a inexistent source url.

Do you intend to add this package to another branch 
than devel?

I propose the following to ship the html docs:

rm -rf __package_docs
mkdir -p __package_docs
cp -pr docs __package_docs
mv __package_docs/docs __package_docs/html
rm -rf __package_docs/html/tools __package_docs/html/.svn
__package_docs/html/graphics/.svn
rm __package_docs/html/Makefile* __package_docs/html/graphics/Makefile*

and add 
%doc __package_docs/html

You could use
make DESTDIR=%{buildroot} install INSTALL='install -p'
to keep timestamps

Suggestion: use
rm %{buildroot}%{_libdir}/*.la
to be noticed when .la files aren't shipped anymore.
Comment 2 Deji Akingunola 2007-10-12 10:12:17 EDT
(In reply to comment #1)
> For the url, you should use, such that it is easier to check 
> your source:
>
http://fedoraproject.org/wiki/Packaging/SourceURL#head-615f6271efb394ab340a93a6cf030f2d08cf0d49
> 
> and not use a inexistent source url.
>
Fixed 
> Do you intend to add this package to another branch 
> than devel?
>
Maybe F-7. I already got a request to update gnome-scan there to the latest.
 
> I propose the following to ship the html docs:
> 
I did this in a slightly different way.
Thanks.
Spec URL: ftp://czar.eas.yorku.ca/pub/babl/bal.spec
SRPM URL: ftp://czar.eas.yorku.ca/pub/babl/babl-0.0.15-0.2.20071011svn.fc8.src.rpm

Comment 3 Patrice Dumas 2007-10-12 10:34:21 EDT
There is a dot missing at the end of the devel %description.

Also the timestamps are still not kept. As above, I propose:
make DESTDIR=%{buildroot} install INSTALL='install -p'

The changelog entry is not very clear in my opinion:
- Remove the use of inexistent source (from package reviews)
I would suggest something along:
- clarify how the source may be generated
and don't mention the fact that the sources were inexistent, since
the source were not really inexistent the url was. It is only
a suggestion and not a big deal.

As for shipping babl svn snapshot in F-7 I think that it 
should be avoided, unless there are severe regression/bugs or 
there are wonderful new functionalities.
Comment 4 Deji Akingunola 2007-10-12 10:48:31 EDT
(In reply to comment #3)
> There is a dot missing at the end of the devel %description.
>
Will add it.
 
> Also the timestamps are still not kept. As above, I propose:
> make DESTDIR=%{buildroot} install INSTALL='install -p'
>
I don't see why its necessary, the packaging guildeline say to do it, when the
copy command is used. There's no mention of doing it for make install.
  
> The changelog entry is not very clear in my opinion:
> - Remove the use of inexistent source (from package reviews)
A typo, should have been 'inexistent source url'.  Will fix.

> 
> As for shipping babl svn snapshot in F-7 I think that it 
> should be avoided, unless there are severe regression/bugs or 
> there are wonderful new functionalities.
Actually, there are 'wonderful new functionalities' in latest gnome-scan (at
least according to the author), and it depends the svn version of babl and gegl.
Comment 5 Patrice Dumas 2007-10-12 10:54:51 EDT
(In reply to comment #4)

> > Also the timestamps are still not kept. As above, I propose:
> > make DESTDIR=%{buildroot} install INSTALL='install -p'
> >
> I don't see why its necessary, the packaging guildeline say to do it, when the
> copy command is used. There's no mention of doing it for make install.

It is an obvious deficiency of the guidelines. It is 
obviously better to keep timestamps for all the files 
that are not generated (and sometimes it is also better
to have relevant timestamps on generated files, in case
of multiarch packages sharing those files).

> > The changelog entry is not very clear in my opinion:
> > - Remove the use of inexistent source (from package reviews)
> A typo, should have been 'inexistent source url'.  Will fix.

Ok.
 
> > As for shipping babl svn snapshot in F-7 I think that it 
> > should be avoided, unless there are severe regression/bugs or 
> > there are wonderful new functionalities.
> Actually, there are 'wonderful new functionalities' in latest gnome-scan (at
> least according to the author), and it depends the svn version of babl and gegl.

Ok.
Comment 7 Patrice Dumas 2007-10-12 16:35:11 EDT
There is a typo in the %changelog, as rpmlint shows:
babl.i386: W: incoherent-version-in-changelog 0.0.15-0.3.svn20071011
0.0.15-0.3.20071011svn.fc8

What is exactly the use of 
/usr/lib/babl-0.0/CIE-Lab.so
/usr/lib/babl-0.0/gegl-fixups.so
/usr/lib/babl-0.0/gggl-lies.so
/usr/lib/babl-0.0/gggl.so
/usr/lib/babl-0.0/naive-CMYK.so

They don't look like library to link against, but rather like
dlopened extensions. Is it the case? In that case they should be
in the main package, unless they are examples that shouldn't be 
shipped. And since gggl stuff seems to be GPLv2+, the version 
should be updated.

What about adding:

%check
make check
Comment 8 Deji Akingunola 2007-10-13 08:49:35 EDT
(In reply to comment #7)

> What is exactly the use of 
> /usr/lib/babl-0.0/CIE-Lab.so
> /usr/lib/babl-0.0/gegl-fixups.so
> /usr/lib/babl-0.0/gggl-lies.so
> /usr/lib/babl-0.0/gggl.so
> /usr/lib/babl-0.0/naive-CMYK.so
> 
You're correct, they are extension. I've put them in the main package.
 
> They don't look like library to link against, but rather like
> dlopened extensions. Is it the case? In that case they should be
> in the main package, unless they are examples that shouldn't be 
> shipped. And since gggl stuff seems to be GPLv2+, the version 
> should be updated.

I dont understand what you mean by the last sentence.

Spec URL: ftp://czar.eas.yorku.ca/pub/babl/bal.spec
SRPM URL: ftp://czar.eas.yorku.ca/pub/babl/babl-0.0.15-0.4.20071011svn.fc8.src.rpm


Comment 9 Patrice Dumas 2007-10-13 11:01:01 EDT
(In reply to comment #8)

> > shipped. And since gggl stuff seems to be GPLv2+, the version 
> > should be updated.
> 
> I dont understand what you mean by the last sentence.

The gggl code, that makes /usr/lib/babl-0.0/gggl.so,
for example, is covered by the GPLv2+. If the babl library dlopens 
gggl.so, then the whole is covered by the GPLv2+. So license
should be GPLv2+. If the extension is not used in the default
case, maybe the best would be to have
License: LGPLv2+ and GPLv2+
I haven't seen in the doc something clear about enabling
or disabling the extensions, though.
Comment 10 Patrice Dumas 2007-10-13 11:01:39 EDT
In general %check is after %install. Just a suggestion.
Comment 11 Patrice Dumas 2007-10-22 05:50:15 EDT
(In reply to comment #8)
> >  And since gggl stuff seems to be GPLv2+, the version 
> > should be updated.
> 
> I dont understand what you mean by the last sentence.

Rereading myself, I just used version instead of license!
Should have been:

And since gggl stuff seems to be GPLv2+, the License
should be updated.
Comment 12 Deji Akingunola 2007-10-22 09:09:07 EDT
Thanks, I've updated the spec file.

Spec URL: ftp://czar.eas.yorku.ca/pub/babl/bal.spec
SRPM URL: ftp://czar.eas.yorku.ca/pub/babl/babl-0.0.15-0.5.20071011svn.fc8.src.rpm
Comment 13 Patrice Dumas 2007-10-22 16:56:28 EDT
* rpmlint is silent
* follow packaging guidelines
* free software, one of the license included. 
* source match upstream except for some dates in .svn/entries 
* %files section right
* library packaged rightly

Please add a comment explaining what files are covered by the 
GPL.

APPROVED

Comment 14 Deji Akingunola 2007-10-22 18:12:23 EDT
New Package CVS Request
=======================
Package Name: babl
Short Description: A dynamic, any to any, pixel format conversion library
Owners: deji
Branches: F-7 F-8
InitialCC: 
Cvsextras Commits: yes
Comment 15 Jason Tibbitts 2007-10-22 20:30:11 EDT
CVS done.
Comment 16 Deji Akingunola 2007-10-22 22:31:43 EDT
Thanks to Patrice and Jason. I'll add the comment about while file are under the
GPL later in CVS.

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