Bug 242566 - Review Request: cegui (0.5.x branch) - Free library providing windowing and widgets for graphics APIs / engines
Review Request: cegui (0.5.x branch) - Free library providing windowing and w...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-06-04 16:22 EDT by Ian Chapman
Modified: 2007-11-30 17:12 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-06-21 20:20:31 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Ian Chapman 2007-06-04 16:22:39 EDT
Spec URL: http://dribble.org.uk/reviews/cegui-0.5.0b-1.src.rpm
SRPM URL: http://dribble.org.uk/reviews/cegui.spec
Description: 

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!

NOTE:

cegui 0.4.x is already in Fedora and is maintained by me, however I feel that the 0.5.x branch (and subsequently the SPEC) has changed significantly enough that another review would be useful.
Comment 1 Jason Tibbitts 2007-06-09 17:31:15 EDT
I'll take a quick look.  I won't do a full review because I trust things like
the upstream source and licensing and such are OK.

The versioning seems OK; the "b" is a post-release update, not "beta", or at
least it seems that way from the upstream web pages.

The spec looks OK; if the dependencies of the -devel packages you need are OK,
you shouldn't need to actually list a build dependency on pkgconfig but it
doesn't hurt.

rpmlint spews a massive boatload of warnings.  Well over 1300 of them.  Most are
undefined-non-weak-symbol warnings; some are unused-direct-shlib-dependency.  I
haven't a clue about how you'd go about fixing them.

I'd give my ack to this if I understood the rpmlint warnings or was convinced
they're unfixable.  They're the only thing I see of issue with this package.
Comment 2 Ian Chapman 2007-06-09 19:24:12 EDT
(In reply to comment #1)
> I'll take a quick look.  I won't do a full review because I trust things like
> the upstream source and licensing and such are OK.

No probs :-)
 
> The versioning seems OK; the "b" is a post-release update, not "beta", or at
> least it seems that way from the upstream web pages.

Yep, that's pretty much the case. the 'b' release was a quick bug fixed version
of 0.5.0.

> rpmlint spews a massive boatload of warnings.  Well over 1300 of them.  Most are
> undefined-non-weak-symbol warnings; some are unused-direct-shlib-dependency.  I
> haven't a clue about how you'd go about fixing them.

Hmm, I was confused by this because for me rpmlint only reports two messages,
the first one is the usual no documentation found in the devel package, and the
second is that the devel-doc package requires the -devel package. Both of wish
are not real errors and are ingorable. Can I ask which release/arch you tested
it on? I was using FC6/i386. 

> I'd give my ack to this if I understood the rpmlint warnings or was convinced
> they're unfixable.  They're the only thing I see of issue with this package.

No problem Jason. Thanks for the headsup, I'll look into those message and see
if I can figure out the cause and if anything needs doing.
Comment 3 Ian Chapman 2007-06-09 19:27:31 EDT
Ahhh, just realised I do indeed get those errors if I check in the installed
package rather than the RPMs themselves.
Comment 4 Jason Tibbitts 2007-06-09 20:03:06 EDT
Yes, you have to run rpmlint against the installed packages.  I install all of
the packages I test (into the mock chroot, a convenient hack) and run rpmlint on
them but I don't think many other reviewers do.

BTW, I build on x86_64 and almost always using rawhide.
Comment 5 Ian Chapman 2007-06-10 18:38:30 EDT
(In reply to comment #4)
> Yes, you have to run rpmlint against the installed packages.  I install all of
> the packages I test (into the mock chroot, a convenient hack) and run rpmlint on

Yeah I knew you could use rpmlint against installed packages, but it never
occurred to me that certain issues can only be picked up this way, I'd just
assumed it was for convenience. =)

Ok, well I've done a bit of research and this is how things stand. Neither
undefined-non-weak-symbol or unused-direct-shlib-dependency are considered blockers.

The first warning is caused by a library/exe not being explicitly linked with a
library that contains those symbols. Sometimes that's actually desirable eg in
the case where you have two libraries which provide a compatible ABI but their
functionality may be different you can use either one, but generally it isn't.
The peformance penalty for this tends to be a slightly longer startup time as
the linker has to locate and satisfy those symbols.

The second is caused by a library/exe being linked with a library for which it
has no need to be. In other words, it's been linked with a library it doesn't
use. I couldn't really find any useful pointers on this one which was quite
surprising seeing as how many libraries in Fedora also have this issue. The
penalty for this tends to be slightly bigger files and perhaps slightly more
memory usage but we're only talking in the region of a few K at most. In order
to try and fix this I tried to make use of the linker option "--as-needed" which
I couldn't get to work. Additionally there's a good gentoo article on this issue
(http://www.gentoo.org/proj/en/qa/asneeded.xml#doc_chap2) which explains you can
often do more harm than good. Lastly it's not always possible for a build system
to determine exactly which libraries it shouldn't link, particularly when
querying .pc files etc. 

So in summary... the weak symbols issue has been fixed. The
unused-direct-shlib-dependency hasn't because I don't feel it can fixed in any
reasonable manner. Of course I completely happy to be proved wrong. Anyway,
here's the latest version:


Spec URL: http://dribble.org.uk/reviews/cegui-0.5.0b-2.src.rpm
SRPM URL: http://dribble.org.uk/reviews/cegui.spec
Comment 6 Jason Tibbitts 2007-06-16 02:17:02 EDT
OK, now there are far fewer rpmlint complaints.  This exposes some things which
I perhaps simply didn't notice last time.  One trivial thing:
  W: cegui mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 17)
which is no big deal, and then a bunch of the dreaded rpath errors:
   E: cegui binary-or-shlib-defines-rpath 
    /usr/lib64/libCEGUITGAImageCodec.so.0.0.0 ['/usr/lib64']
   E: cegui binary-or-shlib-defines-rpath 
    /usr/lib64/libCEGUILibxmlParser.so.0.0.0 ['/usr/lib64']
   E: cegui binary-or-shlib-defines-rpath 
    /usr/lib64/libCEGUIOpenGLRenderer.so.0.0.1 ['/usr/lib64']
   E: cegui binary-or-shlib-defines-rpath 
    /usr/lib64/libCEGUIExpatParser.so.0.0.0 ['/usr/lib64']
   E: cegui binary-or-shlib-defines-rpath 
    /usr/lib64/libCEGUITinyXMLParser.so.0.0.0 ['/usr/lib64']
   E: cegui binary-or-shlib-defines-rpath 
    /usr/lib64/libCEGUISILLYImageCodec.so.0.0.0 ['/usr/lib64']
   E: cegui binary-or-shlib-defines-rpath 
    /usr/lib64/libCEGUIDevILImageCodec.so.0.0.0 ['/usr/lib64']
   E: cegui binary-or-shlib-defines-rpath 
    /usr/lib64/libCEGUIFalagardWRBase.so.1.0.0 ['/usr/lib64']
   E: cegui binary-or-shlib-defines-rpath 
    /usr/lib64/libCEGUIXercesParser.so.0.0.0 ['/usr/lib64']
   E: cegui binary-or-shlib-defines-rpath 
    /usr/lib64/libCEGUILuaScriptModule.so.1.0.0 ['/usr/lib64']

According to the package changelog, some work was done to get rid of rpath in
the past but I guess it hasn't survived into this rewrite.  I tried a few of the
usual things to deal with it (--disable-rpath, export LIBTOOL=libtool,
libtoolize) but none seemed to make any difference for whatever reason. 
However, doing
   make LIBTOOL=/usr/bin/libtool
and deleting the resulting .a files got things working OK.
Comment 7 Ian Chapman 2007-06-16 19:53:36 EDT
(In reply to comment #6)

> I perhaps simply didn't notice last time.  One trivial thing:
>   W: cegui mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 17)

I normally have my editor convert tabs to spaces but this is a silly side effect
of me temporarily using a different editor. I'll fix that.

> which is no big deal, and then a bunch of the dreaded rpath errors:
>    E: cegui binary-or-shlib-defines-rpath 
>     /usr/lib64/libCEGUITGAImageCodec.so.0.0.0 ['/usr/lib64']

Yeah I remember this now from the 0.4 branch. It only effects 64bit and as I
don't readily have access to a 64bit machine it can go unnoticed.

>   > However, doing
>    make LIBTOOL=/usr/bin/libtool
> and deleting the resulting .a files got things working OK.

Thanks Jason, these issues are trivial to fix so  I'll post an updated version
shortly.
Comment 8 Ian Chapman 2007-06-20 21:57:28 EDT
Ok hopefully this fixes all 'fixable' rpmlint errors. Bizarrely FC6/X86_64 also
needed LIBTOOL="/usr/bin/libtool" on the install line which took a bit to work out.

http://dribble.org.uk/reviews/cegui-0.5.0b-3.src.rpm
http://dribble.org.uk/reviews/cegui.spec
Comment 9 Jason Tibbitts 2007-06-20 22:34:30 EDT
Looks great now.  The only things left from rpmlint are:

W: cegui-devel no-documentation
E: cegui-devel-doc devel-dependency cegui-devel
(which is kind of funny, actually) and all of the unused-direct-shlib-dependency
warnings.

So I think we're good.

APPROVED
Comment 10 Ian Chapman 2007-06-21 20:20:31 EDT
Thanks for the review. Built in devel, I'll wait about a week before pushing to
FC6 & 7

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