Bug 196740

Summary: Review Request: ogre - Object-Oriented Graphics Rendering Engine
Product: [Fedora] Fedora Reporter: Hans de Goede <hdegoede>
Component: Package ReviewAssignee: Wart <wart>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: che666, chris.stone
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2006-07-07 19:30:47 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:    
Bug Blocks: 163779, 196744    

Description Hans de Goede 2006-06-26 20:16:30 UTC
Spec URL: http://people.atrpms.net/~hdegoede/ogre.spec
SRPM URL: http://people.atrpms.net/~hdegoede/ogre-1.2.1-1.src.rpm
Description:
OGRE (Object-Oriented Graphics Rendering Engine) is a scene-oriented,
flexible 3D engine written in C++ designed to make it easier and more
intuitive for developers to produce applications utilising
hardware-accelerated 3D graphics. The class library abstracts all the
details of using the underlying system libraries like Direct3D and
OpenGL and provides an interface based on world objects and other
intuitive classes.

Comment 1 Dan Horák 2006-06-26 20:34:36 UTC
I have packaged the current versions of Ogre for my internal needs using the
same basis (Xavier Decoret) ;-)

Comment 2 Hans de Goede 2006-06-26 20:36:54 UTC
May I advise you to swithc to my version then, it fixes a bug with fonts with
relative sizes not showing, which was causing fonts for me to disappear in
chess, caused me ages to find.


Comment 3 Dan Horák 2006-06-26 20:48:31 UTC
Sure, I will use it. I have now some troubles when building Ogre with cegui from
Extras in FC4.

Comment 4 Ralf Corsepius 2006-06-27 09:35:08 UTC
Just a proposal and a matter of personal preference:
Did you consider to put the docs currently in ogre-devel-doc into
/usr/share/doc/ogre-devel* instead of /usr/share/doc/ogre-devel-doc?


Comment 5 Hans de Goede 2006-06-27 10:34:21 UTC
(In reply to comment #4)
> Just a proposal and a matter of personal preference:
> Did you consider to put the docs currently in ogre-devel-doc into
> /usr/share/doc/ogre-devel* instead of /usr/share/doc/ogre-devel-doc?
> 

Nope I didn't consider this because that is not the way how rpm works.


Comment 6 Dan Horák 2006-06-29 09:45:09 UTC
Could you also package the samples into ogre-samples subpackage? They can serve
as some kind of tests.

Comment 7 Hans de Goede 2006-06-29 11:05:48 UTC
(In reply to comment #6)
> Could you also package the samples into ogre-samples subpackage? They can serve
> as some kind of tests.

I'm not planning on doing this, many of the samples won't work withpout a
propietary nvidea lib, also the samples don't even compile out of the box. Last
but not least the samples are expecting to be run form a directory with certain
subdirs present (all ogre programs suffer from this, see the workaround I've put
into chess). So all in all the pain is not worth the gain IMHO.

If someone however would submit a clean patch to add a working -samples
subpackage I would happy to add that.

Comment 8 Rudolf Kastl 2006-06-29 11:56:55 UTC
#6 and #7 all the cg stuff would have to be replaced with glsl. afaik ogre
supports glsl (i think theres even a trivial way to convert the shaders).

Comment 9 Wart 2006-07-06 22:36:19 UTC
rpmlint warnings:

W: ogre-devel no-documentation
   - Can be safely ignored since docs are in a -doc subpackage.

E: ogre-devel invalid-soname /usr/lib64/libOgrePlatform.so libOgrePlatform.so
   - Not sure where this is coming from.

MUST
====
* Package/spec named appropriately
* GPL license ok, license file included
* spec file legible and in Am. English
* Builds on FC6-i386, FC6-x86_64, FC5-i386, FC5-x86_64
* Sources match upstream:
  6ff98b1f14ca679ceaeec00daff2ff87  ogre-linux_osx-v1-2-1.tar.bz2
* No locales
* ldconfig called correctly from %post/%postun
* Not relocatable
* RPM_BUILD_ROOT cleaned as needed
* headers, unversioned .so, and pkgconfig files in -devel subpackage
* No libtool archives
* Does not own any directories that it should not.
* No .desktop file needed

MUSTFIX
=======
* The 'tr' trick in Source0: is cute, but my preference is to limit
  macro substitutions in Source0 to simple %{name} and %{version} only.
  Anything more complicated (like spawning subshells) becomes
  harder to read.  In this case, just hard code the version string.
* -devel subpackage should use full version in base package dependency (it
  is missing -%{release}), or add a comment why it's not needed:
  Requires: %{name} = %{version}-%{release}
* BR: flex, bison seem to be unnecessary.

COMMENTS
========
* One duplicate file found:  Docs/styles.css.  This is ok, however, as it
  is needed for the docs in both the base and the -devel-doc subpackage

Comment 10 Hans de Goede 2006-07-07 11:30:02 UTC
(In reply to comment #9)
> rpmlint warnings:
> 
> W: ogre-devel no-documentation
>    - Can be safely ignored since docs are in a -doc subpackage.
> 
> E: ogre-devel invalid-soname /usr/lib64/libOgrePlatform.so libOgrePlatform.so
>    - Not sure where this is coming from.

Oops, I meant to fix that one but never got around to fixing it, rpmlint is
unhappy because that is an unversioned .so directly under libdir. Since this lib
actually gets dlopened by libOgreMain I've moved it to libdir/OGRE

> MUSTFIX
> =======
> * The 'tr' trick in Source0: is cute, but my preference is to limit
>   macro substitutions in Source0 to simple %{name} and %{version} only.
>   Anything more complicated (like spawning subshells) becomes
>   harder to read.  In this case, just hard code the version string.

Erm, shouldn't that be a should fix. Nowhere in the guidelines it says use of
macros etc is forbbiden in Source0. I know some people want to copy and paste
the Source0 URL to wget, some people even want it to contain 0 macro's. I say
those people should learn to use "spectool -g name.spec" which will do the macro
expansion and cut and pasting for them, directly calling wget. -> EWONTFIX

> * -devel subpackage should use full version in base package dependency (it
>   is missing -%{release}), or add a comment why it's not needed:
>   Requires: %{name} = %{version}-%{release}
Fixed

> * BR: flex, bison seem to be unnecessary.
./configure was checking for them but indeed it builds fine without, removed.

New version is here:
* Fri Jul  7 2006 Hans de Goede <j.w.r.degoede> 1.2.1-2
- Make -devel package Requires on the main package fully versioned.
- Move libOgrePlatform.so out of %%{_libdir} and into the OGRE plugins dirs as
  its not versioned and only used through dlopen, so its effectivly a plugin.

Spec URL: http://people.atrpms.net/~hdegoede/ogre.spec
SRPM URL: http://people.atrpms.net/~hdegoede/ogre-1.2.1-2.src.rpm


Comment 11 Wart 2006-07-07 17:35:54 UTC
(In reply to comment #10)
> (In reply to comment #9)

> > MUSTFIX
> > =======
> > * The 'tr' trick in Source0: is cute, but my preference is to limit
> >   macro substitutions in Source0 to simple %{name} and %{version} only.
> >   Anything more complicated (like spawning subshells) becomes
> >   harder to read.  In this case, just hard code the version string.
> 
> Erm, shouldn't that be a should fix. Nowhere in the guidelines it says use of
> macros etc is forbbiden in Source0. I know some people want to copy and paste
> the Source0 URL to wget, some people even want it to contain 0 macro's. I say
> those people should learn to use "spectool -g name.spec" which will do the macro
> expansion and cut and pasting for them, directly calling wget. -> EWONTFIX

While there are no explicit guidelines against this, I feel that it falls into
the "spec file must be legible" category.  I don't think we should have to rely
on spectool to decipher source urls.  I'm willing to live with macro
substitutions, but I think shell escapes add too much indirection.

Comment 12 Paul Howarth 2006-07-07 17:52:21 UTC
There is precedent in Extras for using shell escapes for this. For instance, my
own perl-Math-Pari package has a far less obvious one:

http://cvs.fedora.redhat.com/viewcvs/devel/perl-Math-Pari/perl-Math-Pari.spec?root=extras&view=markup

I think that if anyone looking at ths spec file doesn't "get" the "tr"
subsitution almost instantly, they're going to have great difficulty following
just about any type of shell script, including the rest of the spec file. The
"tr" program is something you'd come across in "Scripting 101" after all.

I really do think this comes down to the packager's preference myself.

Comment 13 Hans de Goede 2006-07-07 17:57:05 UTC
Thanks for the support Paul!

Wart,

There are plenty of examples in FE with similar and much much harder to read
shell scriptlets, please if this is the only blokcer approve.

What gain is there in my removing the shellscript only to readd it with the
first upstream release.

I want a new upstream release to be as easy as:
-bump %version
-reset %release to 1
-add changelog
-spectool -g
-rebuild

Now I know things aren't always going to be as easy as this, but having to
manually edit Source0 for each upstream update is a pain, especially since one
then has to remember the prefered filename-formating for all upstreams, or goto
the website of upstream find the download page and find out the correct URL that
way.


Comment 14 Wart 2006-07-07 18:00:34 UTC
I didn't realize that there was already precedent for this type of thing.  I
hadn't seen it until now.  While I don't necessarily like it, I won't have it be
the only thing blocking the review.

APPROVED

Comment 15 Hans de Goede 2006-07-07 19:30:47 UTC
(In reply to comment #14)
> I didn't realize that there was already precedent for this type of thing.  I
> hadn't seen it until now.  While I don't necessarily like it, I won't have it be
> the only thing blocking the review.
> 
> APPROVED

Thanks, actually the precedents for this is where I learned todo this, thats how
one develops bad habbits :)

Imported & build, closing.