Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 233139 (SDLmm) - Review Request: SDLmm - C++ interface for the popular SDL library
Summary: Review Request: SDLmm - C++ interface for the popular SDL library
Keywords:
Status: CLOSED NEXTRELEASE
Alias: SDLmm
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: 233142
TreeView+ depends on / blocked
 
Reported: 2007-03-20 16:45 UTC by Hans de Goede
Modified: 2007-11-30 22:11 UTC (History)
0 users

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-03-21 20:48:34 UTC
chris.stone: fedora-review+
wtogami: fedora-cvs+


Attachments (Terms of Use)

Description Hans de Goede 2007-03-20 16:45:46 UTC
Spec URL: http://people.atrpms.net/~hdegoede/SDLmm.spec
SRPM URL: http://people.atrpms.net/~hdegoede/SDLmm-0.1.8-1.fc7.src.rpm
Description:
SDLmm is a C++ glue for SDL, or the Simple DirectMedia Layer, which is a
generic API that provides low level access to audio, keyboard, mouse,
joystick, 3D hardware via OpenGL, and 2D framebuffer across multiple
platforms.

SDLmm aims to stay as close as possible to the C API while taking
advantage of native C++ features like object orientation.

Comment 1 Christopher Stone 2007-03-20 17:56:33 UTC
Please investigate these rpmlint warnings:

W: SDLmm undefined-non-weak-symbol /usr/lib64/libSDLmm-0.1.so.8.0.0
__cxa_pure_virtual
W: SDLmm undefined-non-weak-symbol /usr/lib64/libSDLmm-0.1.so.8.0.0
_ZNSs6assignEPKcm
W: SDLmm undefined-non-weak-symbol /usr/lib64/libSDLmm-0.1.so.8.0.0
__cxa_guard_release
W: SDLmm undefined-non-weak-symbol /usr/lib64/libSDLmm-0.1.so.8.0.0 _ZdlPv
W: SDLmm undefined-non-weak-symbol /usr/lib64/libSDLmm-0.1.so.8.0.0
_ZNSt8ios_base4InitD1Ev
W: SDLmm undefined-non-weak-symbol /usr/lib64/libSDLmm-0.1.so.8.0.0
__cxa_guard_acquire
W: SDLmm undefined-non-weak-symbol /usr/lib64/libSDLmm-0.1.so.8.0.0
_ZNSt8ios_base4InitC1Ev
W: SDLmm unused-direct-shlib-dependency /usr/lib64/libSDLmm-0.1.so.8.0.0
/lib64/libm.so.6


Comment 2 Hans de Goede 2007-03-20 20:11:08 UTC
Good catch, I really should teach myself to not only run rpmlint on the build
packages, but also on the installed packages.

Here is a new version fixing this:
Spec URL: http://people.atrpms.net/~hdegoede/SDLmm.spec
SRPM URL: http://people.atrpms.net/~hdegoede/SDLmm-0.1.8-2.fc7.src.rpm

rpmlint warnings still left on the installed package:
W: SDLmm unused-direct-shlib-dependency /usr/lib64/libSDLmm-0.1.so.8.0.0
/lib64/libpthread.so.0

This is caused by the output of "sdl-config --libs", and harmless since SDL
itself does indeed require pthreads.


Comment 3 Christopher Stone 2007-03-21 02:23:44 UTC
==== REVIEW CHECKLIST ====
- rpmlint output clean
- package named according to package naming guidelines
- spec file name matches %{name}
- package meets packaging guidelines
- licensed with open source compatible license
- license matches actual license
- license included in %doc
- spec written in American English
- spec file legible
- sources match upstream
0a05d27d1aed72af3c7a37b6378f50e5  SDLmm-0.1.8.tar.bz2
- package successfully compiles and builds on FC-6 x86_64
- all build dependencies listed in BR
- no locales
- proper use of ldconfig in %post/un sections
- package is not relocatable
- package owns all directories it creates
- directories it does not create owned by filesystem
- no duplicates in %files
- package contains proper %clean
- macro usage is consistent
- contains code
- no large documentation
- %doc does not affect runtime
- header files located in devel subpackage
- no static libraries
- no pkgconfig files
- libraries w/o suffix in devel
- devel has fully versioned requires
- no libtool archives
- not a GUI application
- package does not own files or directories owned by other packages


==== MUST FIX ====
- Source0 URL is incorrect, the directory is sdlmm, not SDLmm


==== SHOULD FIX ====
- use of -n %{name} is redundant in the %post/un sections



Comment 4 Hans de Goede 2007-03-21 10:23:15 UTC
Both fixed, see:
Spec URL: http://people.atrpms.net/~hdegoede/SDLmm.spec
SRPM URL: http://people.atrpms.net/~hdegoede/SDLmm-0.1.8-3.fc7.src.rpm


Comment 5 Christopher Stone 2007-03-21 16:35:14 UTC
I believe the new patch adds these warnings:
sdlmm_audio.cpp
sdlmm_srect.cpp: In function 'SDLmm::SRect SDLmm::Intersect(const SDLmm::SRect&,
const SDLmm::SRect&)':
sdlmm_srect.cpp:77: warning: comparison is always false due to limited range of
data type
sdlmm_srect.cpp:78: warning: comparison is always false due to limited range of
data type
sdlmm_srect.cpp: In function 'SDLmm::SRect SDLmm::Union(const SDLmm::SRect&,
const SDLmm::SRect&)':
sdlmm_srect.cpp:88: warning: comparison is always true due to limited range of
data type
sdlmm_srect.cpp:89: warning: comparison is always true due to limited range of
data type

Should investigate, but not a blocker.

All other must fix items have been fixed.

*** APPROVED ***


Comment 6 Hans de Goede 2007-03-21 18:52:11 UTC
New Package CVS Request
=======================
Package Name:      SDLmm
Short Description: C++ interface for the popular SDL library
Owners:            j.w.r.degoede@hhs.nl
Branches:          FC-6 devel
InitialCC:         <empty>


Comment 7 Hans de Goede 2007-03-21 20:48:34 UTC
Thanks for the review!

Imported and build, closing.

p.s.

I've updated (fixed) the patch to no longer cause those valid compiler warnings.


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