Bug 432034 (cel)

Summary: Review Request: cel - Crystal Entity Layer
Product: [Fedora] Fedora Reporter: Hans de Goede <hdegoede>
Component: Package ReviewAssignee: Christopher Stone <chris.stone>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, me, mtasaka, notting
Target Milestone: ---Flags: chris.stone: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2008-03-14 17:53:18 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On: 432033    
Bug Blocks:    

Description Hans de Goede 2008-02-08 15:00:01 UTC
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 15:00:52 UTC
Note this package requires crystalspace who's review is bug 432033.

Comment 2 Christopher Stone 2008-02-25 00:59:51 UTC
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-25 01:13:33 UTC
Be sure to include the license in %doc.

Comment 4 Hans de Goede 2008-02-25 12:21:47 UTC
(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-03 00:32:07 UTC
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 08:34:27 UTC
(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 21:46:21 UTC
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 23:59:48 UTC
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 07:03:08 UTC
(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 19:25:25 UTC
==== 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 21:52:46 UTC
(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 04:17:12 UTC
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 07:43:00 UTC
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 07:43:54 UTC
Erm, oops that should be fedora-cvs -> ?


Comment 15 Kevin Fenzi 2008-03-13 23:30:09 UTC
cvs done.

Comment 16 Hans de Goede 2008-03-14 17:53:18 UTC
Imported and build, closing.