Created attachment 130112 [details]
Patch making cegui use the system pcre

cegui comes with its own copy of pcre. Which is a library which is also
available seperatly in FE. Using private copies of libraries is concidered a
bad practice (it removes / reverts most advantages of using shared libs if
there are many copies of each lib in different packages).

Please do a rm -fr src/pcre in %prep, add pcre-devel to the BR and use the
attached patch to get cegui to compile with the system pcre.

Thanks very much for the patch. I had to make a tiny change to it, because it
introduced the following trivial error:

src/Makefile.am:151: noinst_HEADERS must be set with `=' before using `+='

Does this also apply to the private copy of lua include with the source package?
I suspect it probably does, although there's no shared libs in the FE lua.  

(In reply to comment #2)
> Thanks very much for the patch. I had to make a tiny change to it, because it
> introduced the following trivial error:
> 
> src/Makefile.am:151: noinst_HEADERS must be set with `=' before using `+='
> 

Ah, I didn't get that error because I removed ./bootstrap from %build. You
should not call automake, aclocal, autoconf, etc (which ./bootstrap does) unless
you've got a really good reason. If you do call them you should do so in %prep
not %build.


> Does this also apply to the private copy of lua include with the source
> package? I suspect it probably does, although there's no shared libs in the FE
> lua.  

Yes it does apply to lua and tinyxml too. I did notice lua but I forgot again.
Even though lua as packaged only provides a .a file and not a .so file you
should still use the system version during the build, it might contain Fedora
specific fixes (for example for the always bleeding edge Fedora gcc version) and
feel free to file a bug against lua asking for a .so instead of the .a .

Since tinyxml isn't currently packaged we can leave that in, if you want todo
something with regard to the xml code in cegui, it would prabably be best to
convert to libxml or expat, we alreayd have more then enough xml libs as is. But
as said, just leave it as is for now.

While I'm at here are some things which need fixing (for starters, not a full
review yet):
-duplicate files in %doc between base -devel -devel-docs
-remove / fix non modular X (Build)Requires
And as a reminder (see above)
-remove call to ./bootstrap.sh
-use system pcre

About the system lua, it seems that cegui contains a special version of lua
called tolua++, I'm still investigating this, so don't waste any time on it in
the mean time.

Okay,

tolua++ which is used by cegui is an extension to lua, you can find tolua++ here:
http://www.codenix.com/~tolua/

cegui actually includes both lua and tolua++. Since they have been dropped into
one dir by the cegui author and since the tolua++ included in cegui is so old
that it probably doesn't suppor the lua shipped with Fe. Its probably best to:
-package the real tolua++ from above
-completly remove the one included in cegui 
-use the system installed tolua++ when building cegui (just as with pcre)

Since this is going to be a somewhat longer path the originally planned, first
get DevIL into shape and then I'll sponsor you so you can import DevIL and
AGReader while you (we) are working on this.

Also please be so kind as to file a bug against lua about it only containing a
.a and put me in the CC of this bug, it might be a good idea to put a pointer to
this bug into this new bug. (just type "bug 193342" then BZ will automaticly
create a link).

Ok, here's the current state of play for cegui.

http://dribble.org.uk/cegui.spec
http://dribble.org.uk/cegui-0.4.1-3.src.rpm

I've filed a bug against lua and begin looking at tolua++. Thanks Hans

Glad to hear you're looking into tolua++, I think the best way to proceed is to
first package tolua++ and then fix cegui to use it.

Once you're done packaging tolua++ feel free to add me to the cc-list for the
review then I'll take a look (as time permits).

Latest lua released, now with a shared lib. No reply as yet from the maintainers
regarding the license of tolua++ although I suspect it's probably MIT, but that
should be confirmed. I'll proceed with at least getting an RPM constructed in
the meantime.

The license is hidden in the README, also see tolua++-1.0.92/debian/copyright in
the tarbal.

I mus admit the license is a bit vague in details though, I would drop freeware
as description in the License: field.

I'm in the process of trying to patch cegui, not to use the bundled version of
tolua++. So far I can get it to build successfully to the point where it ignores
the bundled tolua++. It also has to regenerate the c++ bindings using the
'system' tolua++ otherwise it won't compile. However the following library
'libCEGUIluatoluapp.so.0', does not get built. On further investigation it
appears that this library is probably just a wrapper library, containing the
equivalent of 'liblua.so' and/or 'libtolua++.so' although it may be useful to
confirm this. I'm not sure how best to proceed. 

1. Do not supply 'libCEGUIluatoluapp.so.0' and hope the packages compiled
against cegui will link with 'liblua.so' and/or 'libtolua++.so' when necessary
and patch them if they dont.

2. Construct libCEGUIluatoluapp.so.0 based upon the system 'liblua.so' and
'libtolua++.so', but this seems a little messy to me and largely circumvents the
entire reason for not using the bundled version in the first place.

3. Perhaps some sort of alias method using symlinks / sonames? Not even sure if
this is possible.

Anyway if you'd like to see the current state of things with the SRPM I'll
upload it. I'd appreciate your thoughts on this, if you have the time :) Thanks
Hans.

I say go with 1, but you do need to add -llua -ltolua++ to the link command
building the cegui.so which actually calls the (to)lua functions. If it only
calls tolua++ functions then -ltolua++ should be enough, tolua++ itself is
linked against lua itself and this will bring in lua for itself as needed.

When the .so containing the functions which call (to)lua is linked against
to(lua) then the app doesn't need the to link with 'liblua.so' and/or
'libtolua++.so'. We may still need the app to not link to CEGUIluatoluapp though.

Anyways please do post a link to the SRPM then I can see what you've got sofar
and offer some feedback.

p.s.

Great work!

Thanks! Ok here's the current situation. The patch links the library
'libCEGUILuaScriptModule.so' against libtolua++.so and liblua.so. I think this
is the only library where this is needed, but not being a C/C++ programmer I'm
digging in the dark sometimes. :-) So of course, let me know if this wrong.

http://dribble.org.uk/cegui.spec
http://dribble.org.uk/cegui-0.4.1-4.src.rpm

I also think some of the includes probably need fixing in the devel package, and
I have my doubts as to whether CEGUI/renderers/IrrlichtRenderer/* should appear
in the devel package, seeing as we're not supporting Irrlicht. 

(In reply to comment #12)
> I also think some of the includes probably need fixing in the devel package, and
> I have my doubts as to whether CEGUI/renderers/IrrlichtRenderer/* should appear
> in the devel package, seeing as we're not supporting Irrlicht. 

Since we don't support it I say either nuke that directory in %install or
%exclude it on %files


MUST:
=====
* rpmlint output is: 
W: cegui-devel no-documentation
No problem there 
* Package and spec file named appropriately
* Packaged according to packaging guidelines
* License (LGPL) ok, license file included
* spec file is legible and in Am. English.
* Source matches upstream
* Compiles and builds on devel-i386
* BR: ok
* No locales
* ldconfig properly called for shared libraries
* Not relocatable
* Package owns / or requires all dirs
* No duplicate files & Permissions ok
* %clean & macro usage OK
* Contains code only
* %doc does not affect runtime
* seperate -devel-docs sub package for large development docs
* proper -devel package as needed
* .la files not packaged
* no gui -> no .desktop file required


MUST fix:
=========
* in ScriptingModules/CEGUILua/LuaScriptModule/src/Makefile.in you add
   -llua -ltolua++ to a link command and in the belonging Makefile.am
  you don't you should also add this in the .am file so that the .in file gets
  properly regenerated if automake ever gets run.
* I notice that -rpath is used in the same link command, -rpath is evil and 
  should (normally) not be used. Please remove the use of "-rpath <path>" from
  this and other Makefiles.
* The build bombed out on me the first time because I didn't have lua-devel 
  installed. tolua++-devel should have a "Requires: lua-devel" in the
  specfile. I wanted to file a seperate bug for this but I can't file bugs 
  against tolua++ as Bugzilla hasn't picked up the addition to owners.list
  (yet). (My fault I should have seen that in the review).

Should fix:
===========
* in %setup you write:
cd ScriptingModules/CEGUILua/LuaScriptModule/package/
tolua++ -o ../src/lua_CEGUI.cpp CEGUI.pkg

Why not just pass the full path  as the second arg to tolua++?

Couple of things, I'm confused about what you're saying with regards to the
tolua++-devel specfile missing lua-devel? The specfile for tolua++ I have here
shows it is included, also if I do a requirements on the devel RPM downloaded
from the repo it tells me..

libc.so.6
libc.so.6(GLIBC_2.0)
libc.so.6(GLIBC_2.3.4)
libdl.so.2
liblua-5.1.so
libm.so.6
libtolua++-5.1.so
lua-devel >= 5.1
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
tolua++ = 1.0.92-3

Regarding the binding regeneration - the reason why I don't specify the full
path is because CEGUI.pkg requires the other pkg files also, but tolua++ looks
in the current directory for them (hence why I do the cd first) only, so it
fails with something like can't find Basic.pkg. I'm sure CEGUI.pkg and the other
.pkg files could be patched, but this seems overkill when a two lines of shell
script is enough. If I've missed the obvious, let me know of course :-) If not,
I'll add a comment to the spec file explaining the rationale.

(In reply to comment #14)
> Couple of things, I'm confused about what you're saying with regards to the
> tolua++-devel specfile missing lua-devel? The specfile for tolua++ I have here
> shows it is included, also if I do a requirements on the devel RPM downloaded
> from the repo it tells me..
> 
> libc.so.6
> libc.so.6(GLIBC_2.0)
> libc.so.6(GLIBC_2.3.4)
> libdl.so.2
> liblua-5.1.so
> libm.so.6
> libtolua++-5.1.so
> lua-devel >= 5.1
> rpmlib(CompressedFileNames) <= 3.0.4-1
> rpmlib(PayloadFilesHavePrefix) <= 4.0-1
> tolua++ = 1.0.92-3
> 

You're right, tolua++ is fine. Its a bug in lua, the base lua package still
provides lua-devel even though the package has been split. I'll file a bug
against lua.


> Regarding the binding regeneration - the reason why I don't specify the full
> path is because CEGUI.pkg requires the other pkg files also, but tolua++ looks
> in the current directory for them (hence why I do the cd first) only, so it
> fails with something like can't find Basic.pkg. I'm sure CEGUI.pkg and the other
> .pkg files could be patched, but this seems overkill when a two lines of shell
> script is enough. 

Indeed that would be overkill, leave it as is. Do beware though that if you now
add additional commands to the end of %prep that they get executed in a
different dir then one might expect. Also I really feel that these commands
belong in %build as they are building not prepping, once they are in %install
you need to restore the dir, best to use this for that:
pushd xxxxx
tolua++ xxxxx
popd

Ok Hans, here's the latest version:

http://dribble.org.uk/cegui.spec
http://dribble.org.uk/cegui-0.4.1-5.src.rpm

Recovering from bugzilla crash, this one was approved by me and imported and
build by Ian, closing.