Bug 432034 - (cel) Review Request: cel - Crystal Entity Layer
Review Request: cel - Crystal Entity Layer
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Christopher Stone
Fedora Extras Quality Assurance
:
Depends On: 432033
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-08 10:00 EST by Hans de Goede
Modified: 2008-03-14 13:53 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-03-14 13:53:18 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
chris.stone: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Hans de Goede 2008-02-08 10:00:01 EST
Spec URL: http://people.atrpms.net/~hdegoede/cel.spec
SRPM URL: http://people.atrpms.net/~hdegoede/cel-1.2-1.fc9.src.rpm
Description:
Crystal Entity Layer (CEL) is a game entity layer based on Crystal Space.
It makes it easier for game developers to create games based on Crystal Space.
CEL can optionally be used together with Python or other scripting languages.
Comment 1 Hans de Goede 2008-02-08 10:00:52 EST
Note this package requires crystalspace who's review is bug 432033.
Comment 2 Christopher Stone 2008-02-24 19:59:51 EST
I get the following rpmlint warnings:
cel.x86_64: W: undefined-non-weak-symbol /usr/lib64/libceltool-1.2.so
csStaticVarCleanup
cel.x86_64: W: undefined-non-weak-symbol /usr/lib64/libcel_python-1.2.so
csStaticVarCleanup
cel.x86_64: W: undefined-non-weak-symbol /usr/lib64/libcel_python-1.2.so
csStaticVarCleanup

Please investigate.
Comment 3 Christopher Stone 2008-02-24 20:13:33 EST
Be sure to include the license in %doc.
Comment 4 Hans de Goede 2008-02-25 07:21:47 EST
(In reply to comment #2)
> I get the following rpmlint warnings:
> cel.x86_64: W: undefined-non-weak-symbol /usr/lib64/libceltool-1.2.so
> csStaticVarCleanup
> cel.x86_64: W: undefined-non-weak-symbol /usr/lib64/libcel_python-1.2.so
> csStaticVarCleanup
> cel.x86_64: W: undefined-non-weak-symbol /usr/lib64/libcel_python-1.2.so
> csStaticVarCleanup
> 
> Please investigate.

I've investigated and it seems that this is intentional, because the files under
/usr/bin, do report no errors with ldd -r, so the files under /usr/bin are
providing this symbol, strange, but that seems to be how it is.

(In reply to comment #3)
> Be sure to include the license in %doc.

I will do with the next revision (once a full review is done).
Comment 5 Christopher Stone 2008-03-02 19:32:07 EST
I've asked Brad to do the review on this package as part of his sponsorship process.
Comment 6 Hans de Goede 2008-03-03 03:34:27 EST
(In reply to comment #5)
> I've asked Brad to do the review on this package as part of his sponsorship
process.

OK.
Comment 7 Christopher Stone 2008-03-08 16:46:21 EST
Brad let me know if you cannot do the review this weekend.  I will do it if you
don't have the time or are unable to.
Comment 8 Christopher Stone 2008-03-08 18:59:48 EST
Okay, Brad said he can't do the review. :(

Hans, is there any reason why you are not calling ldconfig in %post/%postun?
Comment 9 Hans de Goede 2008-03-09 03:03:08 EDT
(In reply to comment #8)
> Hans, is there any reason why you are not calling ldconfig in %post/%postun?

Other then me being stupid / caffeine deprived at the time I wrote that? No.
Comment 10 Christopher Stone 2008-03-09 15:25:25 EDT
==== REVIEW CHECKLIST ====
- rpmlint output
See comment #4
- package named according to package naming guidelines
- spec file name matches %{name}
- package meets packaging guidelines
- package licensed with a fedora approved license
- license matches actual license
X license file not included in %doc
- spec file written in American English
- spec file legible
- source match upstream
fafcd7c049d23d43efd8f7de465c6496  cel-src-1.2.tar.bz2
- successfully compiles on x86_64 F-8
- all build dependencies listed in BR
- no locales
X does not call ldconfig in %post/%postun (see comment #9)
- package is not relocatable
- package owns all directories it creates
- no duplicates in %files
- contains proper %clean
- macro usage is consistent
- contains code
- large documentation in doc subpackage
- files in %doc do not affect runtime
- header files located in devel subpackage
- no static libs
- no pkgconfig files
- no library files with suffix
- devel subpackage requires base package
- no libtool archives
- not a GUI application
- package does not own files or directories owned by other packages
- buildroot removed on %install
- filenames UTF-8

==== MUST FIX ====
- Include license in %doc
- Add %post/%postun sections which run ldconfig

==== SHOULD FIX ====
- Investigate if other files should be placed in %doc
- Test for crashes and success in demo programs
Comment 11 Hans de Goede 2008-03-09 17:52:46 EDT
(In reply to comment #10)

Thanks!

> ==== MUST FIX ====
> - Include license in %doc
> - Add %post/%postun sections which run ldconfig
> 

Both fixed.

> ==== SHOULD FIX ====
> - Investigate if other files should be placed in %doc
Checked, nothing else interesting found (note there is 800KB worth of docs in
the -docs package.

> - Test for crashes and success in demo programs
I don't understand what you mwan by this.

New version:
Spec URL: http://people.atrpms.net/~hdegoede/cel.spec
SRPM URL: http://people.atrpms.net/~hdegoede/cel-1.2-2.fc9.src.rpm
Comment 12 Christopher Stone 2008-03-13 00:17:12 EDT
Hi, this looks good.  I'm going to be gone until next week.  Can you try running
the demos, IIRC one of them crashed on exit and I'm not sure if they all worked
as intended.

All must items fixed, APPROVED.
Comment 13 Hans de Goede 2008-03-13 03:43:00 EDT
Thanks for the review!

New Package CVS Request
=======================
Package Name:      cel
Short Description: Crystal Entity Layer
Owners:            jwrdegoede
Branches:          F-8
InitialCC: 
Cvsextras Commits: Yes
Comment 14 Hans de Goede 2008-03-13 03:43:54 EDT
Erm, oops that should be fedora-cvs -> ?
Comment 15 Kevin Fenzi 2008-03-13 19:30:09 EDT
cvs done.
Comment 16 Hans de Goede 2008-03-14 13:53:18 EDT
Imported and build, closing.

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