Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 227198 (jpgalleg) - Review Request: jpgalleg - JPEG library for the Allegro game library
Summary: Review Request: jpgalleg - JPEG library for the Allegro game library
Keywords:
Status: CLOSED NEXTRELEASE
Alias: jpgalleg
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Christopher Stone
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT scorchwentbonkers
TreeView+ depends on / blocked
 
Reported: 2007-02-03 12:04 UTC by Hans de Goede
Modified: 2007-11-30 22:11 UTC (History)
1 user (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-03-14 14:57:18 UTC
Type: ---
Embargoed:
chris.stone: fedora-review+
jwboyer: fedora-cvs+


Attachments (Terms of Use)

Description Hans de Goede 2007-02-03 12:04:41 UTC
Spec URL: http://people.atrpms.net/~hdegoede/jpgalleg.spec
SRPM URL: http://people.atrpms.net/~hdegoede/jpgalleg-2.5-1.fc7.src.rpm
Description:
jpgalleg is a jpeg library for use with the Allegro game library. It allows
using jpeg's as Allegro bitmaps.

Comment 1 John Guthrie 2007-02-11 04:35:24 UTC
This is my first review, so this is kind of unofficial.  So here goes:

- rpmlint -i is silent on the src.rpm.
- Name of spec file matches name of package which matches base part of source
code name
- License in License tag seems to match the license in the source code, but
would it be possible to be more specific that zlib/libpng?  (zlib has a BSD
license, libpng has an "OSI certified license" that looks kind of BSD-ish.)
- source file matches that given in the URL.
- spec file successfuly builds jpgalleg, jpgalleg-devel, and jpgalleg-debuginfo
RPMS on i386.  (I don't have access to any other architectures.)
- BuildRequires seems good.
- calls ldconfig in %post and %postun as it should.
- premissions look good.
- Requires: for the -devel subpackage look good.

I haven't verified that it builds in mock yet, but everything else except for
the license issue above looks quite good.  Even the license thing is minor.

Comment 2 Hans de Goede 2007-02-14 08:51:22 UTC
(In reply to comment #1)
> This is my first review, so this is kind of unofficial.  So here goes:
> 
> - License in License tag seems to match the license in the source code, but
> would it be possible to be more specific that zlib/libpng?  (zlib has a BSD
> license, libpng has an "OSI certified license" that looks kind of BSD-ish.)

zlib/libpng License is an often used term, its even in the list of licenses
rpmlint recognises as valid, also this is how upstream refers to the license for
this package, thus I believe this best describes the license.



Comment 3 John Guthrie 2007-02-14 16:21:53 UTC
My apologies.  I was not aware of that.  Thanks for the info.  (As you can tell,
I'm *really* new at this.  Hence, why I didn't make this an official review.)

Comment 4 Christopher Stone 2007-03-12 21:09:02 UTC
==== REVIEW GUIDELINES ====
- rpmlint output:
W: jpgalleg-devel no-documentation
W: jpgalleg undefined-non-weak-symbol /usr/lib64/libjpgal.so.2.5 _i_cx_r
W: jpgalleg undefined-non-weak-symbol /usr/lib64/libjpgal.so.2.5 _i_cx_w
W: jpgalleg undefined-non-weak-symbol /usr/lib64/libjpgal.so.2.5 _i_get_cpuid_info
W: jpgalleg undefined-non-weak-symbol /usr/lib64/libjpgal.so.2.5 _i_is_486
W: jpgalleg undefined-non-weak-symbol /usr/lib64/libjpgal.so.2.5
_i_is_cpuid_supported
W: jpgalleg undefined-non-weak-symbol /usr/lib64/libjpgal.so.2.5 _i_is_cyrix
W: jpgalleg undefined-non-weak-symbol /usr/lib64/libjpgal.so.2.5 _i_is_fpu

Hans: can you please investigate these rpmlint warnings before I continue with
the review?

Comment 5 Hans de Goede 2007-03-12 21:17:54 UTC
Those are "normal" for an allegro extension library. These symbols are defined
in /usr/lib/liballeg_unsharable.a, against which each allegro using app should
be linked (allegro-config --libs).

As you can see in the .spec file for libraries build against allegro like
libjpeg, liballeg_unsharable.a is stripped from the allegro-config --libs
output, as including this would make the library non pic, which is why these
routines are in the .a file and are linked into the binary, not into any libs.


Comment 6 Christopher Stone 2007-03-13 20:48:01 UTC
==== REVIEW GUIDELINES ====
- rpmlint output:
W: jpgalleg-devel no-documentation
W: jpgalleg undefined-non-weak-symbol /usr/lib64/libjpgal.so.2.5 _i_cx_r
W: jpgalleg undefined-non-weak-symbol /usr/lib64/libjpgal.so.2.5 _i_cx_w
W: jpgalleg undefined-non-weak-symbol /usr/lib64/libjpgal.so.2.5 _i_get_cpuid_info
W: jpgalleg undefined-non-weak-symbol /usr/lib64/libjpgal.so.2.5 _i_is_486
W: jpgalleg undefined-non-weak-symbol /usr/lib64/libjpgal.so.2.5
_i_is_cpuid_supported
W: jpgalleg undefined-non-weak-symbol /usr/lib64/libjpgal.so.2.5 _i_is_cyrix
W: jpgalleg undefined-non-weak-symbol /usr/lib64/libjpgal.so.2.5 _i_is_fpu

Okay, see comment #5

- package named according to package naming guidelines
- spec file name matches %{name}
- package meets packaging guidelines
- licensed with open source compatible license
- license field matches actual license
- license text included in %doc
- spec written in American english
- spec legible
- sources match upstream 4bdd12fc3c1afb06f4ec23b4cb200559
- successfully compiles and builds on FC-6 x86_64
- all build dependencies listed in BR
- no locales
- ldconfig called in %post/un sections
- package is not relocatable
- package owns all directories it creates
- directories it does not owned belong to filesystem
- no duplicates in %files
- file permissions set properly
- contains proper %clean
- macro usage is consistent
- contains code
- no large documentation
- files in %doc do not affect runtime
- header files located in devel subpackage
- no static libraries
- no pkgconfig files
- .so file in devel subpackage
- devel have fully versioned dependency
- no libtool archive files
- not a GUI application
- package does not own files or directories owned by other packages

*** APPROVED ***


Comment 7 Hans de Goede 2007-03-14 11:07:42 UTC
New Package CVS Request
=======================
Package Name:       jpgalleg
Short Description:  JPEG library for the Allegro game library
Owners:             j.w.r.degoede
Branches:           FC-6 devel
InitialCC:          <empty>


Comment 8 Hans de Goede 2007-03-14 14:57:18 UTC
Thanks for the review!

Imported and build, closing.



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