Only just tried to build, but after some long time it failed
only on ppc??

http://koji.fedoraproject.org/koji/taskinfo?taskID=403509

Hmm, it fails with an assembler syntax error while assembling code generated by
g++ -> toolchain bug. I've filed bug 432185 for this.

If possible I would like to continue with the review of this, if bug 432185
doesn't have a fix in sight when the review is completed I'll just add
ExcludeArch ppc for now.

Well, I want to review this, however if you want me to review this
package please wait until I write comments on other (about) 5-6
review requests... (and perhaps I will go to bed in one or two hours)

BTW I am reviewing bug 426492 , however this fails to rebuild
with gcc43 on many points and I would appreciate it if you would
help me fixing the source codes.

(In reply to comment #4)
> BTW I am reviewing bug 426492 , however this fails to rebuild
> with gcc43 on many points and I would appreciate it if you would
> help me fixing the source codes.

Okay, I've downloaded the srpm (link in the review is broken btw) and I'm
working on fixing this with gcc-4.3 now. Feel free to drop requests like this to
me more often, I think you're doing an _awesome_ job with all those reviews and
I'm always happy to help out where I can.

Hans, I noticed today that:
(note: I always enable "koji static-repos" repo)

[root@localhost ~]# yum check-update
.... skip ........
xerces-c.i386                            2.8.0-1.fc9            koji-rawhide    
xerces-c-devel.i386                      2.8.0-1.fc9            koji-rawhide    
....................
[root@localhost ~]#  Resolving Dependencies
--> Running transaction check
---> Package xerces-c-devel.i386 0:2.8.0-1.fc9 set to be updated
---> Package xerces-c.i386 0:2.8.0-1.fc9 set to be updated
--> Processing Dependency: libxerces-c.so.27 for package: cegui
--> Finished Dependency Resolution
cegui-0.5.0b-6.fc8.i386 from installed has depsolving problems
  --> Missing Dependency: libxerces-c.so.27 is needed by package
cegui-0.5.0b-6.fc8.i386 (installed)
Error: Missing Dependency: libxerces-c.so.27 is needed by package
cegui-0.5.0b-6.fc8.i386 (installed)

We have to update cegui first (as this package has cegui-devel
as BR)

Currently cegui build on rawhide fails as:
http://koji.fedoraproject.org/koji/taskinfo?taskID=411388

For 1.2-2:

A. spec file issue
* scriptlet output
  - I guess that (although I could not find where it is written)
    Fedora requests that scriptlet output must be quiet.

    For now -utils %post scriptlets shows a lot of output messages.
    If you want to keep these messages IMO these should be redirected
    to some log file.

* Directory ownership issue
  - My directory check shows (I have not installed -doc subpackage)
----------------------------------------------------------------
[tasaka1@localhost crystalspace]$ grep package DIR-check.log | grep any
/usr/share/doc/crystalspace-1.2/LICENSE         crystalspace-1.2-2.1.fc9       
        file /usr/share/doc/crystalspace-1.2 is not owned by any package
/usr/share/doc/crystalspace-1.2/README          crystalspace-1.2-2.1.fc9       
        file /usr/share/doc/crystalspace-1.2 is not owned by any package
/usr/share/doc/crystalspace-1.2/history.old             crystalspace-1.2-2.1.fc9
               file /usr/share/doc/crystalspace-1.2 is not owned by any package
/usr/share/doc/crystalspace-1.2/history.txt             crystalspace-1.2-2.1.fc9
               file /usr/share/doc/crystalspace-1.2 is not owned by any package
/usr/share/doc/crystalspace-1.2/history_1.2.txt         crystalspace-1.2-2.1.fc9
               file /usr/share/doc/crystalspace-1.2 is not owned by any package
----------------------------------------------------------------

* Multilib issue
  - I don't know how we should deal with multilib issue (no, I REALLY
    don't know!!), however
    at least %_bindir/cs-config-1.2 causes multilib conflict.
----------------------------------------------------------------
[tasaka1@localhost bin]$ diff -u /usr/bin/cs-config-1.2 ./cs-config-1.2 
--- /usr/bin/cs-config-1.2      2008-02-14 13:47:02.000000000 +0900
+++ ./cs-config-1.2     2008-02-14 13:41:13.000000000 +0900
@@ -26,7 +26,7 @@
 do
   prefix="${p}"
   exec_prefix="${prefix}"
-  makeout="./out/linuxx86/debug"
+  makeout="./out/linux/debug"
   version="1.2"
   longversion="crystalspace 1.2"
   newincdir=""
@@ -92,7 +92,7 @@
     case $1 in
         -lcrystalspace-1.2) DEPS=" -lpthread -lpthread -lz" ;;
         -lcrystalspace_opengl-1.2) DEPS=" -lcrystalspace-1.2 -lGL -lXext -lX11
-lX11 -lXext -lpthread -lm" ;;
-        -lcrystalspace_python-1.2) DEPS=" -lcrystalspace-1.2
-L/usr/lib/python2.5 -lpython2.5 -lpthread -ldl -lutil -lm -lpthread" ;;
+        -lcrystalspace_python-1.2) DEPS=" -lcrystalspace-1.2
-L/usr/lib64/python2.5 -lpython2.5 -lpthread -ldl -lutil -lm -lpthread" ;;
         -lcrystalspace_staticplugins-1.2) DEPS="" ;;
 
        *)
-----------------------------------------------------------------
    Someone says that on -devel subpackage multilib confilct must be
    avoided.

* Dependency for -devel subpackage
  - Please check the dependency for -devel subpackage.
    Example:
    - From /usr/include/crystalspace-1.2/csplugincommon/opengl/glcommon2d.h :
-----------------------------------------------------------------
    26  #if defined(CS_OPENGL_PATH)
    27  #include CS_HEADER_GLOBAL(CS_OPENGL_PATH,gl.h)
    28  #else
    29  #include <GL/gl.h>
    30  #endif
-----------------------------------------------------------------
    - /usr/include/crystalspace-1.2/ivideo/wxwin.h has #include <wx/wx.h>
    - And some others.

? Timestamps
  - This rpm installs many "non-built" files and keeping timestamps on
    them are generally desirable. Would you try to keep timestamps on
    installed files as much as possible?
    (usually adding INSTALL="install -p" works, at least on recent
     autotool-based Makefiles)

!!
  For license issues, I will check it later (as the tarball contains more than
  10000 files, which is the largest number I have ever reviewed)

!!
  Again currently crystalspace does not build due to missing dep for hal :(
  http://koji.fedoraproject.org/koji/taskinfo?taskID=425708
  Simply rebuilding hal against new libsmbios seems good
  http://koji.fedoraproject.org/koji/taskinfo?taskID=425729

(In reply to comment #7)
> For 1.2-2:
> 
> A. spec file issue
> * scriptlet output
>   - I guess that (although I could not find where it is written)
>     Fedora requests that scriptlet output must be quiet.
> 
>     For now -utils %post scriptlets shows a lot of output messages.
>     If you want to keep these messages IMO these should be redirected
>     to some log file.
> 

I didn't silence this to check it went ok during development, silenced now.

> * Directory ownership issue
>   - My directory check shows (I have not installed -doc subpackage)

Fixed


> * Multilib issue
>   - I don't know how we should deal with multilib issue (no, I REALLY
>     don't know!!), however
>     at least %_bindir/cs-config-1.2 causes multilib conflict.

Fixed


> * Dependency for -devel subpackage
>   - Please check the dependency for -devel subpackage.
>     Example:
>     - From /usr/include/crystalspace-1.2/csplugincommon/opengl/glcommon2d.h :
> -----------------------------------------------------------------
>     26  #if defined(CS_OPENGL_PATH)
>     27  #include CS_HEADER_GLOBAL(CS_OPENGL_PATH,gl.h)
>     28  #else
>     29  #include <GL/gl.h>
>     30  #endif
> -----------------------------------------------------------------
>     - /usr/include/crystalspace-1.2/ivideo/wxwin.h has #include <wx/wx.h>

Hmm, thats optional better to not frag in wx for people who use crystalspace
without wx.

Otherwise Fixed.

> ? Timestamps
>   - This rpm installs many "non-built" files and keeping timestamps on
>     them are generally desirable. Would you try to keep timestamps on
>     installed files as much as possible?
>     (usually adding INSTALL="install -p" works, at least on recent
>      autotool-based Makefiles)
> 

This does not use regular makefiles but jam, which I've been fighting all the
way to stop it from using custom CFLAGS, so I see no sane way to fix this.

New version:
Spec URL: http://people.atrpms.net/~hdegoede/crystalspace.spec
SRPM URL: http://people.atrpms.net/~hdegoede/crystalspace-1.2-3.fc9.src.rpm

For 1.2-3:

* As I wrote some missing dependency for -devel subpackage as "Example",
  it seems that there are some more missing dependencies for -devel package
  - cegui-devel
  - zlib-devel
  (I guess adding more 2 BR listed above should be enough)

* It seems that adding INSTALL="install -p" to make install actually works
  http://koji.fedoraproject.org/koji/taskinfo?taskID=426911

! This weekend I will try to check license issues (if any) for this
  package (more than 10000 files needs checking)

Created attachment 295066 [details]
License check list for crystalspace

(For non-legal issue, please see my previous comments)
Note:
Unless you specify license tag for subpackages, subpackages
inherits license tag from main package.
For example, currently -doc subpackage has
"License: LGPLv2+ and GPLv2+ and GPLv2".

Legal issue:

GPLv2:
GPLv2/apps/import/maya2spr/binarytree.cpp
  -> maya2spr (in -utils) is GPLv2
GPLv2/scripts/max/exportcsp.mcr
GPLv2/scripts/max/exportlights.mcr
GPLv2/scripts/max/exportsprite.mcr
GPLv2/scripts/max/fixmaterials.mcr
GPLv2/scripts/max/sanitycheck.ms
GPLv2/scripts/max/showMap.mcr
  -> all these files (in -utils) are GPLv2

GPLv2+:
GPLv2+/apps/tests/ceguitest/ceguitest.cpp
GPLv2+/apps/tests/ceguitest/ceguitest.h
  -> ceguitest (in -demos) is GPLv2+
GPLv2+/libs/csplugincommon/sndsys/
  -> libcrystalspace-1.2.so (in main) is GPLv2+ (not LGPLv2+)
-----------------------------------------------------------------------------
  1810	  echo './out/linuxx86/debug/libs/cstool/memory_pen.o@'
'./out/linuxx86/debug/libs/cstool/csfxscr.o@'
'./out/linuxx86/debug/libs/csplugincommon/canvas/scrshot.o@'
'./out/linuxx86/debug/libs/csplugincommon/canvas/x11-keys.o@'
'./out/linuxx86/debug/libs/csplugincommon/canvas/cursorconvert.o@'
'./out/linuxx86/debug/libs/csplugincommon/canvas/softfontcache.o@'
'./out/linuxx86/debug/libs/csplugincommon/canvas/fontcache.o@'
'./out/linuxx86/debug/libs/csplugincommon/canvas/graph2d.o@'
'./out/linuxx86/debug/libs/csplugincommon/imageloader/commonimagefile.o@'
'./out/linuxx86/debug/libs/csplugincommon/imageloader/optionsparser.o@'
'./out/linuxx86/debug/libs/csplugincommon/particlesys/partgen.o@'
'./out/linuxx86/debug/libs/csplugincommon/render3d/txtmgr.o@'
'./out/linuxx86/debug/libs/csplugincommon/render3d/normalizationcube.o@'
'./out/linuxx86/debug/libs/csplugincommon/renderstep/parserenderstep.o@'
'./out/linuxx86/debug/libs/csplugincommon/renderstep/basesteptype.o@'
'./out/linuxx86/debug/libs/csplugincommon/renderstep/basesteploader.o@'
'./out/linuxx86/debug/libs/csplugincommon/script/scriptcommon.o@'
'./out/linuxx86/debug/libs/csplugincommon/shader/shaderprogram.o@'
'./out/linuxx86/debug/libs/csplugincommon/sndsys/convert.o@'
'./out/linuxx86/debug/libs/csplugincommon/sndsys/cyclicbuf.o@'
'./out/linuxx86/debug/libs/csplugincommon/sndsys/sndstream.o@'
'./out/linuxx86/debug/libs/csplugincommon/sndsys/snddata.o@' | sed 's/@ /@/g' |
tr '@' '
  1811	' >> ./out/linuxx86/debug/libs/libcrystalspace-1.2.so.resp
  1812	
  1813	
  1814	  g++ -Wl,--as-needed -shared -o
./out/linuxx86/debug/libs/libcrystalspace-1.2.so
-Wl,@./out/linuxx86/debug/libs/libcrystalspace-1.2.so.resp  -lm -ldl -lnsl
-Wl,-z,defs -Wl,--warn-unresolved-symbols -Wl,-E -g3 -lpthread -lpthread -lz
-lm -ldl -lnsl -Wl,-z,defs -Wl,--warn-unresolved-symbols -Wl,-E -g3 \
  1815	    -Wl,-soname,libcrystalspace-1.2.so
-----------------------------------------------------------------------------
GPLv2+/plugins/sndsys/element/
  -> sndsysXXX.so (in main) is GPLv2+
GPLv2+/plugins/utilities/movierecorder/
  -> movierecorder.so (in main) is GPLv2+ (not GPLv2)
GPLv2+/scripts/jamtemplate/createproject.sh
  -> createproject.sh (in -devel) is GPLv2+

Check for GPLv2+ pollution
root:
GPLv2+/include/csplugincommon/sndsys/snddata.h ->
LGPLv2+/include/csplugincommon.h:#include "csplugincommon/sndsys/snddata.h" ->
LGPLv2+/include/crystalspace.h:#include "csplugincommon.h" ->
File_C_or_C++/plugins/cscript/pycscegui/cs_cegui.cpp:#include "crystalspace.h"
File_C_or_C++/plugins/cscript/csperl5/cswigpl5.cpp:#include "crystalspace.h"
LGPLv2+/apps/tools/genmeshify/converter.cpp:#include "crystalspace.h"
LGPLv2+/apps/tools/genmeshify/genmeshify.cpp:#include "crystalspace.h"
LGPLv2+/apps/tools/genmeshify/processor.cpp:#include "crystalspace.h"
LGPLv2+/apps/tools/genmeshify/stdloadercontext.cpp:#include "crystalspace.h"
LGPLv2+/apps/tools/startme/startme.h:#include <crystalspace.h>
LGPLv2+/apps/tools/lighter2/common.h:#include "crystalspace.h"
LGPLv2+/apps/tools/basemapgen/basemapgen.h:#include "crystalspace.h"
LGPLv2+/apps/tools/heightmapgen/heightmapgen.h:#include "crystalspace.h"
LGPLv2+/apps/tools/partconv/partconv.cpp:#include "crystalspace.h"
( -----------------
LGPLv2+/apps/walktest/walktest.cpp:#include "crystalspace.h"
LGPLv2+/apps/pysimp/pysimp.h:#include <crystalspace.h>
LGPLv2+/apps/tests/imptest/imptest.h:#include <crystalspace.h>
LGPLv2+/apps/tests/eventtest/eventtest.h:#include <crystalspace.h>
LGPLv2+/apps/tests/sndtest/sndtest.h:#include <crystalspace.h>
LGPLv2+/apps/tutorial/pathtut/pathtut.h:#include <crystalspace.h>
LGPLv2+/apps/tutorial/simpcd/simpcd.h:#include <crystalspace.h>
LGPLv2+/apps/tutorial/simple2/simple2.h:#include <crystalspace.h>
LGPLv2+/apps/tutorial/mazing/gamestuff.cpp:#include <crystalspace.h>
LGPLv2+/apps/tutorial/mazing/appmazing.h:#include <crystalspace.h>
LGPLv2+/apps/tutorial/simplept/simplept.h:#include <crystalspace.h>
LGPLv2+/apps/tutorial/phystut/phystut.h:#include <crystalspace.h>
LGPLv2+/apps/tutorial/simpmap/simpmap.h:#include <crystalspace.h>
LGPLv2+/apps/tutorial/simpvs/simpvs.h:#include <crystalspace.h>
LGPLv2+/apps/tutorial/simple1/simple1.h:#include <crystalspace.h>
------------- can be killed )
-> _pycscegui.so (in crystalspace-devel) is polluted
-> Some files in -utils are also polluted

GPLv2+/include/csplugincommon/sndsys/sndstream.h ->
LGPLv2+/include/csplugincommon.h:#include "csplugincommon/sndsys/sndstream.h"
(-> same above)

GPLv2+/include/ivaria/movierecorder.h ->
LGPLv2+/include/ivaria.h:#include "ivaria/movierecorder.h" ->
(-> not pollute any more)

Summary
main  : LGPLv2+ and GPLv2+
-utils: LGPLv2+ and GPLv2+ and GPLv2
-demos: LGPLv2+ and GPLv2+
-devel: LGPLv2+ and GPLv2+
-doc:	????

Ah... as libcrystalspace-1.2.so is polluted by GPLv2+,
the tag "LGPLv2+" seems almost nonsense.

(In reply to comment #9)
> For 1.2-3:
> 
> * As I wrote some missing dependency for -devel subpackage as "Example",
>   it seems that there are some more missing dependencies for -devel package
>   - cegui-devel
>   - zlib-devel
>   (I guess adding more 2 BR listed above should be enough)
> 

Sorry, I missunderstood. I did a full check and couldn't find any other headers
besides the 2 above which I've added

> * It seems that adding INSTALL="install -p" to make install actually works
>   http://koji.fedoraproject.org/koji/taskinfo?taskID=426911
> 

Strange as the makefile is justb a skeleton calling jam, but if it works thats
good! Added.

> ! This weekend I will try to check license issues (if any) for this
>   package (more than 10000 files needs checking)

Okay, about your analysis, I agree, except that the movierecorder plugin really
is GPLv2 and not GPLv2+, as it includes (and uses) the nuppelvideo.h file which
says:
/* This file is from the NuppelVideo project:
 *
 * (c) Roman Hochleitner roman.ac.at
 * NuppelVideo is distributed under the GNU GENERAL PUBLIC LICENSE version 2
 */

This makes the list:
main  : GPLv2+ and GPLv2
-utils: GPLv2+ and GPLv2
-demos: GPLv2+
-devel: GPLv2+
-doc:	????

So to make things easier, esp the ????, I've just added a License tag of:
"GPLv2+ and GPLv2" to the main package, and let all the subpackages inherent
this. I've added a large comment above the main License tag explaining why it is
what it is. Also I will contact upstream about this, as I believe they intend
the core of crystalspace to be LGPL not GPL.

Here is a new (hopefully the last) version:
Spec URL: http://people.atrpms.net/~hdegoede/crystalspace.spec
SRPM URL: http://people.atrpms.net/~hdegoede/crystalspace-1.2-4.fc9.src.rpm

Thanks I will check this after rebuilding this on koji again.

By the way I will appreciate it if you would review my review request
bug 433182 (IMO very simple)

(In reply to comment #12)
> Also I will contact upstream about this, as I believe they intend
> the core of crystalspace to be LGPL not GPL.
I guess so.

Okay.

------------------------------------------------------------------------
   This package (crystalspace) is APPROVED by me
------------------------------------------------------------------------

Thanks for the review!

New Package CVS Request
=======================
Package Name:      crystalspace
Short Description: Crystal Space a free 3D engine
Owners:            jwrdegoede
Branches:          F-8
InitialCC: 
Cvsextras Commits: Yes

cvs done.

Imported, build and Fedora 8 update requested in bodhi, closing.