Bug 474603

Summary: Review Request: irrlicht - A high performance realtime 3D engine
Product: [Fedora] Fedora Reporter: Tom "spot" Callaway <tcallawa>
Component: Package ReviewAssignee: Lubomir Rintel <lkundrak>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, lkundrak, mmahut, notting, orion
Target Milestone: ---Flags: lkundrak: fedora-review+
gwync: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-01-12 18:00:06 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 474606    

Description Tom "spot" Callaway 2008-12-04 16:47:25 UTC
Spec URL: http://www.auroralinux.org/people/spot/review/new/irrlicht.spec
SRPM URL: http://www.auroralinux.org/people/spot/review/new/irrlicht-1.5-0.1.beta.fc11.src.rpm
Description: 
The Irrlicht Engine is an open source high performance realtime 3D engine
written and usable in C++ and also available for .NET languages. It is
completely cross-platform, using D3D, OpenGL and its own software renderer,
and has all of the state-of-the-art features which can be found in
commercial 3d engines.

Comment 1 Marek Mahut 2008-12-04 17:36:39 UTC
Hey Tom, its trying to run ldconfig during build.

+ unset DISPLAY
+ rm -rf /var/tmp/irrlicht-1.5-0.1.beta.fc8-root-mmahut
+ cd source/Irrlicht
+ mkdir -p /var/tmp/irrlicht-1.5-0.1.beta.fc8-root-mmahut/usr/lib
+ mkdir -p /var/tmp/irrlicht-1.5-0.1.beta.fc8-root-mmahut/usr/include/irrlicht
+ make INSTALL_DIR=/var/tmp/irrlicht-1.5-0.1.beta.fc8-root-mmahut/usr/lib install
cp ../../lib/Linux/libIrrlicht.so.1.5.beta /var/tmp/irrlicht-1.5-0.1.beta.fc8-root-mmahut/usr/lib
cd /var/tmp/irrlicht-1.5-0.1.beta.fc8-root-mmahut/usr/lib && ln -s libIrrlicht.so.1.5.beta libIrrlicht.so
ldconfig -n /var/tmp/irrlicht-1.5-0.1.beta.fc8-root-mmahut/usr/lib
make: ldconfig: Command not found
make: *** [install] Error 127

Comment 2 Tom "spot" Callaway 2008-12-04 17:44:39 UTC
Weird, dunno how I missed that, I ran it through koji to completion. Let me fix that. :)

Comment 3 Tom "spot" Callaway 2008-12-04 17:56:06 UTC
New SRPM: http://www.auroralinux.org/people/spot/review/new/irrlicht-1.5-0.2.beta.fc11.src.rpm
New SPEC: http://www.auroralinux.org/people/spot/review/new/irrlicht.spec

I'm not sure how you got a build going without glibc, but I digress. :)

Comment 4 Lubomir Rintel 2008-12-04 19:47:31 UTC
(In reply to comment #3)
> I'm not sure how you got a build going without glibc, but I digress. :)

I guess he uses Fedora 8, and therefore does not have sbin in his path.

I'm wondering how could that ldconfig succeed for you -- you don't build packages as root, do you? :)

Comment 5 Tom "spot" Callaway 2008-12-04 20:04:32 UTC
Nope, not as root. ldconfig seems to succeed with -n as a normal user.

Comment 6 Lubomir Rintel 2009-01-08 00:48:04 UTC
For some reason I thought a complete review was already being done on this.
Taking it.

Comment 7 Lubomir Rintel 2009-01-08 17:15:24 UTC
Just a few notes, most of them not serious:

1.) Is this needed?

# We don't use any of this. Deleting it so the debuginfo doesn't pick it up.
rm -rf source/Irrlicht/jpeglib source/Irrlicht/zlib

I think the debuginfo is generated only from files that have an actual reference in dwarf debugging information in binaries

2.) Preserve timestamps

for i in include/*.h doc/upgrade-guide.txt source/Irrlicht/*.cpp source/Irrlicht/*.h source/Irrlicht/libpng/*.c source/Irrlicht/libpng/*.h; do
        sed -i 's/\r//' $i
        chmod -x $i
done

I think sed -i modifies a timestamp. I think you should care about keeping the timestamp at least in doc file, to prevent a multilib conflict between the file in packages of different architectures, but, well, I am not quite sure...

3.) You could improve the legibility of your SPEC file

cd source/Irrlicht
...
make INSTALL_DIR=%{buildroot}%{_libdir} install
cd ../..

This could be just

make -C source/Irrlicht  INSTALL_DIR=%{buildroot}%{_libdir} install

4.) Please use system libpng.

While image handling libraries are frequently prone to security vulnerabilities [1], it's probably not a problem for our irrlicht's embedded one since it only reads trusted images. Still, code duplication is bad, please get libpng fixed at least in rawhide.

[1] http://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=libpng

5.) The license does not seem to be GPLv2+

Short look at the top of ./include/irrlicht.h gives me an impression this is not exactly GPLv2+. Seems like zlib license.

The last paragraph (after 3.) seems to suggest something like an advertising clause for IJG code, but I think it is not applicable for this package since we link against external libjpeg, right? You'll definitely know better.

* builds fine in mock
* optflags used correctly
- license (see above)
* rpmlint silent
* spec file clean and legible

Comment 8 Tom "spot" Callaway 2009-01-10 19:29:45 UTC
1) Ehh, debuginfo looked like it was pulling in files from those dirs, I did this to be safe.
2) I've fixed it so that we have constant timestamps.
3) Good point, done.
4) Done, Tom Lane helped me find the proper fix.
5) You're right. It's zlib. My bad. :)

New SPEC: http://www.auroralinux.org/people/spot/review/new/irrlicht.spec
New SRPM: http://www.auroralinux.org/people/spot/review/new/irrlicht-1.5-2.fc11.src.rpm

Comment 9 Lubomir Rintel 2009-01-10 20:05:08 UTC
Thanks!
Looks well now; good to go

APPROVED

Comment 10 Tom "spot" Callaway 2009-01-12 16:18:31 UTC
New Package CVS Request
=======================
Package Name: irrlicht
Short Description: A high performance real-time 3D engine
Owners: spot
Branches: F-9 F-10 devel
InitialCC: 

... and it's done.

Comment 11 Tom "spot" Callaway 2009-01-12 18:00:06 UTC
Built for F-9, F-10, devel.

Comment 12 Orion Poplawski 2012-07-03 18:12:12 UTC
Package Change Request
======================
Package Name: irrlicht
New Branches: el6
Owners: orion
InitialCC:

Comment 13 Gwyn Ciesla 2012-07-03 18:17:22 UTC
Git done (by process-git-requests).

Added spot as comaintainer.

Comment 14 Orion Poplawski 2015-03-26 21:42:55 UTC
Package Change Request
======================
Package Name: irrlicht
New Branches: epel7
Owners: orion
InitialCC: spot

Comment 15 Gwyn Ciesla 2015-03-27 12:29:40 UTC
Git done (by process-git-requests).