Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 196740 - Review Request: ogre - Object-Oriented Graphics Rendering Engine
Summary: Review Request: ogre - Object-Oriented Graphics Rendering Engine
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Wart
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT 196744
TreeView+ depends on / blocked
 
Reported: 2006-06-26 20:16 UTC by Hans de Goede
Modified: 2007-11-30 22:11 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-07-07 19:30:47 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

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.



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