Spec URL: http://dribble.org.uk/cegui.spec SRPM URL: http://dribble.org.uk/cegui-0.4.1-2.src.rpm Description: Package uploaded for review by Hans de Goede Crazy Eddie's GUI System is a free library providing windowing and widgets for graphics APIs / engines where such functionality is not natively available, or severely lacking. The library is object orientated, written in C++, and targeted at games developers who should be spending their time creating great games, not building GUI sub-systems!
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.