Bug 474603 - Review Request: irrlicht - A high performance realtime 3D engine
Review Request: irrlicht - A high performance realtime 3D engine
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Lubomir Rintel
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 474606
  Show dependency treegraph
 
Reported: 2008-12-04 11:47 EST by Tom "spot" Callaway
Modified: 2015-03-27 08:29 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-01-12 13:00:06 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
lkundrak: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Tom "spot" Callaway 2008-12-04 11:47:25 EST
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 12:36:39 EST
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 12:44:39 EST
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 12:56:06 EST
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 14:47:31 EST
(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 15:04:32 EST
Nope, not as root. ldconfig seems to succeed with -n as a normal user.
Comment 6 Lubomir Rintel 2009-01-07 19:48:04 EST
For some reason I thought a complete review was already being done on this.
Taking it.
Comment 7 Lubomir Rintel 2009-01-08 12:15:24 EST
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 14:29:45 EST
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 15:05:08 EST
Thanks!
Looks well now; good to go

APPROVED
Comment 10 Tom "spot" Callaway 2009-01-12 11:18:31 EST
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 13:00:06 EST
Built for F-9, F-10, devel.
Comment 12 Orion Poplawski 2012-07-03 14:12:12 EDT
Package Change Request
======================
Package Name: irrlicht
New Branches: el6
Owners: orion
InitialCC:
Comment 13 Gwyn Ciesla 2012-07-03 14:17:22 EDT
Git done (by process-git-requests).

Added spot as comaintainer.
Comment 14 Orion Poplawski 2015-03-26 17:42:55 EDT
Package Change Request
======================
Package Name: irrlicht
New Branches: epel7
Owners: orion
InitialCC: spot
Comment 15 Gwyn Ciesla 2015-03-27 08:29:40 EDT
Git done (by process-git-requests).

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