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 208678 - Review Request: SimGear - Simulation library components
Summary: Review Request: SimGear - Simulation library components
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Luya Tshimbalanga
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT 208679
TreeView+ depends on / blocked
 
Reported: 2006-09-30 04:51 UTC by Tom "spot" Callaway
Modified: 2007-11-30 22:11 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-04-02 20:17:08 UTC
Type: ---
Embargoed:
dennis: fedora-cvs+


Attachments (Terms of Use)

Description Tom "spot" Callaway 2006-09-30 04:51:35 UTC
Spec URL: http://www.auroralinux.org/people/spot/review/SimGear.spec
SRPM URL: http://www.auroralinux.org/people/spot/review/SimGear-0.3.10-1.fc6.src.rpm
Description: 
SimGear is a set of open-source libraries designed to be used as building
blocks for quickly assembling 3d simulations, games, and visualization
applications.

This library is used in several open source simulation games, most notably, vamos and flightgear. Accordingly, this is a pre-requisite for those games to be packaged in Fedora.

Comment 1 Luya Tshimbalanga 2006-10-01 00:57:09 UTC
Here is a short review:
- Spec file should be lowercase by default if the upstream maintainer has not a
preference.
- Concerning smp_flags, does any reviewers accept that problem can be ignored?

Once these problem is ironed out, I will give a detailed review


Comment 2 Tom "spot" Callaway 2006-10-01 05:05:03 UTC
Upstream uses "SimGear" as the naming, since this comes from the "FlightGear"
project, and the tarball has the name as "SimGear", I opted to honor it.

The smp_mflags is due to a lousy makefile, not a case of bad code.

Comment 3 Luya Tshimbalanga 2006-10-01 07:29:10 UTC
Full reviews:
+ spec file follows the packaging guideline.
+ spec name machtes the package name.
+ Source rpm is succesfully rebuild via mock and rpmbuild command.
+ package from SRPM matches the upstream version via md5sum and sha1sum.
+ rpmlint did not report any error.
+ No error when installing and removing the built package

Overall, the SRPM package conforms to the packaging guideline and ready to be
included in Extras. 

Comment 4 Ralf Corsepius 2006-10-02 05:30:58 UTC
Some remarks:

1. This doesn't seem right to me:

...
%package devel
Group: Development/Libraries
Summary: Simulation library components
Requires: openal, plib, libXt, libXext, libXi, libXmu, freeglut, freealut
Requires: libjpeg, zlib
..

Devel packages should never have to "Require" any lib* packages. 
They should "Require: lib*-devel" packages - Please check.

2. rpmlint SimGear-debuginfo-0.3.10-1.i386.rpm
E: SimGear-debuginfo empty-debuginfo-package

WTH? The package seems to acknowledge RPM_OPT_FLAGS, also I can't spot any
"stripping", so ... defect in rpm??


Finally: This package ships static libs, only :(

Comment 5 Luya Tshimbalanga 2006-10-02 08:40:00 UTC
Weird, rpmlint did not spot any error on my system. I will check further when I
will reach my Fedora system. I am reverting the block to FE-REVIEW until these
problems are ironed out.

Comment 6 Mamoru TASAKA 2006-10-03 17:19:44 UTC
(In reply to comment #4)
> Some remarks:
> 
> 1. This doesn't seem right to me:
> 
> ...
> %package devel
> Group: Development/Libraries
> Summary: Simulation library components
> Requires: openal, plib, libXt, libXext, libXi, libXmu, freeglut, freealut
> Requires: libjpeg, zlib
> ..
> 
> Devel packages should never have to "Require" any lib* packages. 
> They should "Require: lib*-devel" packages - Please check.
I have the same opinion.

> 
> 2. rpmlint SimGear-debuginfo-0.3.10-1.i386.rpm
> E: SimGear-debuginfo empty-debuginfo-package
> 
> WTH? The package seems to acknowledge RPM_OPT_FLAGS, also I can't spot any
> "stripping", so ... defect in rpm??
> 

Static archives are not stripped by /usr/lib/rpm/find-debuginfo.sh,
so this package creates empty debuginfo rpm.



Comment 7 Ralf Corsepius 2006-10-04 06:05:59 UTC
(In reply to comment #6)

> > WTH? The package seems to acknowledge RPM_OPT_FLAGS, also I can't spot any
> > "stripping", so ... defect in rpm??
> > 
> 
> Static archives are not stripped by /usr/lib/rpm/find-debuginfo.sh,
> so this package creates empty debuginfo rpm.

i.e. static libs can't be debugged because find-debuginfo doesn't provide the
necessary info ...

So, bug or feature? I am inclined to think it qualifies as bug in rpm.

Comment 8 Luya Tshimbalanga 2006-10-04 06:19:49 UTC
I confirm the empty debuginfo package. The source version has debug folder so it
is possibly a rpm bug.

Comment 9 Tom "spot" Callaway 2006-10-04 15:36:49 UTC
OK. New package builds shared libs instead of static ones, has a proper SimGear
and SimGear-devel.

Also, the Requires: are trimmed down, and only on the -devel as -devel.

New SRPM: http://www.auroralinux.org/people/spot/review/SimGear-0.3.10-2.fc6.src.rpm
New SPEC:
http://www.auroralinux.org/people/spot/review/SimGear.spec

Comment 10 Ralf Corsepius 2006-10-05 08:14:27 UTC
Package doesn't ship any static libs anymore.
Remove blocking PR
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=208678

Further remarks:
* Please let the *-devel package depend upon libGL-devel instead of
mesa-libGL-devel (mesa-libGL-devel is just one implementation of libGL.

* The headers being shipped as part of devel contain defines which are reserved
to autoconf (HAVE_*) and are likely to conflict with autoheaders when using
these library with packages using autotools.

Most of them seem mostly harmless, but there is at least one is used in a way,
which surely breaks:

Some header contain:

#ifdef HAVE_CONFIG_H
#  include <simgear_config.h>
#endif

HAVE_CONFIG_H is defined by many autotool'ed applications, so this will trigger
 pulling-in simgear_config.h, which is not part of SimGear-devel.


Comment 11 Luya Tshimbalanga 2006-10-05 09:43:12 UTC
Result from mock build:

$ rpmlint /var/lib/mock/fedora-development-x86_64-core/result/*.rpm
E: SimGear-debuginfo script-without-shebang
/usr/src/debug/SimGear-0.3.10/simgear/screen/GLBitmaps.cxx
W: SimGear-devel no-documentation


Comment 12 Ralf Corsepius 2006-10-05 09:58:16 UTC
(In reply to comment #11)
> Result from mock build:
> 
> $ rpmlint /var/lib/mock/fedora-development-x86_64-core/result/*.rpm
> E: SimGear-debuginfo script-without-shebang
> /usr/src/debug/SimGear-0.3.10/simgear/screen/GLBitmaps.cxx
> W: SimGear-devel no-documentation
It's safe to ignore both.

The first is caused by source file being executable, the latter is a rpmlint
being overly critical.
 



Comment 13 Luya Tshimbalanga 2006-10-16 00:44:36 UTC
Sorry for the delay of reviewing. Once the issue mentioned on post #10 is
solved, the package will pass the reviewing as it is safe to ignore post #11.  

Comment 14 Tom "spot" Callaway 2006-10-18 20:44:54 UTC
Changed the Requires to libGL-devel, patched out the internal header defines.

New SRPM: http://www.auroralinux.org/people/spot/review/SimGear-0.3.10-3.fc6.src.rpm
New SPEC:
http://www.auroralinux.org/people/spot/review/SimGear.spec

Comment 15 Luya Tshimbalanga 2006-10-19 08:47:28 UTC
Checked the specs. It conforms to Package Guideline.
Original package extracted from SRPM matches both md5sum and sha1sum.
Source RPM is succesfully built by mock without noticeable warning or errors.
Built package is succesfully installed and removed without possible issues.
debuginfo package still has issue related on comment #11 except the
documentation with is fixed. However, comment #12 assured that related issues
can be safely ignored.

Therefore, SimGear SRPM and SPEC files have passed the reviewing and can be
build on Extras repository.

Comment 16 Kevin Fenzi 2006-12-06 04:47:57 UTC
Hey Tom. 

I was going to take a look at reviewing vamos (bug #208679), but it requires
this package and I don't see it in yet. 

Is there anything holding back this package from being imported and built? 
It looks accepted to me... 

Comment 17 Luya Tshimbalanga 2007-02-08 21:29:09 UTC
Hmm, Spot. Have you brought SimGear into the repository?

Comment 18 Tom "spot" Callaway 2007-02-22 21:12:59 UTC
New Package CVS Request
=======================
Package Name: SimGear
Short Description: Simulation library components
Owners: tcallawa
Branches: FC-5 FC-6 devel


Comment 19 Dennis Gilmore 2007-02-23 12:49:27 UTC
cvs created

Comment 20 Hans de Goede 2007-03-28 13:20:53 UTC
Spot, I'm currently working together with Fabrice Bellet to get FlightGear into
Fedora (and to make him become a Fedora contributer).

I'm doing it this way because Fabrice has quite a bit of packaging experience
and important also quite a bit of experience with FlightGear. 

I know you're always very busy, but it would make things easier for us, if you
could find a few spare cycles to import and build this. Would it be ok for you
if I co-maintain this with you? Then I can fix things quickly (if needed) to
keep the FlightGear inclusion process going smooth. 

For more on Fabrice see:
http://bellet.info/
http://fr2.rpmfind.net/linux/local/fedora/


Comment 21 Hans de Goede 2007-03-28 14:20:40 UTC
Running rpmlint on the installed SimGear package results in lots of undefined
non weak symbols warnings. It looks like the .so's need to be linked against
other simgear libs.

Also I think its not wise to use .1.0.0 as soname versioning. Since upstream
only supports static libs, and this is a c++ library, ABI breakage with a new
upstream release is _extremely_ likely. Thus I would like to advice to use
"-release %{version}" flag to libtool when linking the .so files instead of
"-version xxx"

Using -release will result in sonames like this:
libSimGear-%{version}.so

Thus changing the soname with every upstream release, which (unfortunately) most
likely is a good thing todo in this case. If you're ok with me becoming a
co-maintainer and you can add me to the ACL, then I can fix both the
non-weak-symbols and the soname issue and do an import then. I think its
important to get the soname right from the start.


Comment 22 Tom "spot" Callaway 2007-03-28 14:54:09 UTC
This is imported now, but not built, because the plague builders were down when
I tried. I'm more than willing to share maintainer duties with you for this one.
The changes you've suggested above seem reasonable to me.

Comment 23 Hans de Goede 2007-03-28 18:20:52 UTC
(In reply to comment #22)
> This is imported now, but not built, because the plague builders were down when
> I tried. I'm more than willing to share maintainer duties with you for this one.
> The changes you've suggested above seem reasonable to me.

Ok,

Can you open up the ACL for me then (for now), and add a request here to make me
co-maintainer? Then I'll start working on writing and applying the suggested
fixes and request a build of the fixed version. As said before I think its not
wise to build the current version, only to change the soname with the next release.


Comment 24 Tom "spot" Callaway 2007-03-28 18:43:41 UTC
I thought I did the acls correctly... let me know if you can't make changes.

Comment 25 Hans de Goede 2007-03-30 10:06:33 UTC
ARRRGGGGGHHHHHH circular dependency hell

I've managed to untangle things, so that I now am left with this list of
unresolved non weak symbols:
W: SimGear undefined-non-weak-symbol /usr/lib/libsgstructure-0.3.10.so
_ZNK14SGPropertyNode14getDoubleValueEv
W: SimGear undefined-non-weak-symbol /usr/lib/libsgprops-0.3.10.so _ZNK6SGPath3dirEv
W: SimGear undefined-non-weak-symbol /usr/lib/libsgprops-0.3.10.so _ZN6SGPathD1Ev
W: SimGear undefined-non-weak-symbol /usr/lib/libsgprops-0.3.10.so
_ZN6SGPath6appendERKSs
W: SimGear undefined-non-weak-symbol /usr/lib/libsgprops-0.3.10.so _ZN6SGPathC1ERKSs
W: SimGear undefined-non-weak-symbol /usr/lib/libsgsky-0.3.10.so thesky
W: SimGear undefined-non-weak-symbol /usr/lib/libsgsky-0.3.10.so sgEnviro
W: SimGear undefined-non-weak-symbol /usr/lib/libsgsky-0.3.10.so
_ZN8SGEnviro6getFOVERfS0_
W: SimGear undefined-non-weak-symbol /usr/lib/libsgsky-0.3.10.so
_ZN8SGEnviro17set_view_in_cloudEb
W: SimGear undefined-non-weak-symbol /usr/lib/libsgsky-0.3.10.so
_ZN8SGEnviro14callback_cloudEfffifi
W: SimGear undefined-non-weak-symbol /usr/lib/libsgmisc-0.3.10.so
_ZNK14SGPropertyNode14getDoubleValueEv
W: SimGear undefined-non-weak-symbol /usr/lib/libsgmisc-0.3.10.so
_ZNK14SGPropertyNode14getDoubleValueEv
W: SimGear undefined-non-weak-symbol /usr/lib/libsgmisc-0.3.10.so
_ZN14SGPropertyNode14setDoubleValueEd
W: SimGear undefined-non-weak-symbol /usr/lib/libsgxml-0.3.10.so
_ZNK14SGPropertyNode14getDoubleValueEv
W: SimGear undefined-non-weak-symbol /usr/lib/libsgmodel-0.3.10.so thesky
W: SimGear undefined-non-weak-symbol /usr/lib/libsgenvironment-0.3.10.so thesky

Further untangling is impossible:
W: SimGear undefined-non-weak-symbol /usr/lib/libsgstructure-0.3.10.so
_ZNK14SGPropertyNode14getDoubleValueEv
Requires libsgprops-0.3.10.so, but since libsgstructure-0.3.10.so defines the
basic exception types used by all other classes, libsgprops-0.3.10.so also
requires libsgstructure-0.3.10.so -> circular. So other libs (already done) and
apps using -lsgstructure must also always specify -lsgprops (and thus also
-lsgmisc, see below)

W: SimGear undefined-non-weak-symbol /usr/lib/libsgxml-0.3.10.so
_ZNK14SGPropertyNode14getDoubleValueEv
This comes from the uncomplete libsgstructure, cannot link to sgprops, as
sgprops is based on sgxml


W: SimGear undefined-non-weak-symbol /usr/lib/libsgprops-0.3.10.so _ZNK6SGPath3dirEv
W: SimGear undefined-non-weak-symbol /usr/lib/libsgprops-0.3.10.so _ZN6SGPathD1Ev
W: SimGear undefined-non-weak-symbol /usr/lib/libsgprops-0.3.10.so
_ZN6SGPath6appendERKSs
W: SimGear undefined-non-weak-symbol /usr/lib/libsgprops-0.3.10.so _ZN6SGPathC1ERKSs
W: SimGear undefined-non-weak-symbol /usr/lib/libsgmisc-0.3.10.so
_ZNK14SGPropertyNode14getDoubleValueEv
W: SimGear undefined-non-weak-symbol /usr/lib/libsgmisc-0.3.10.so
_ZNK14SGPropertyNode14getDoubleValueEv
W: SimGear undefined-non-weak-symbol /usr/lib/libsgmisc-0.3.10.so
_ZN14SGPropertyNode14setDoubleValueEd
sgprops requires a few symbols from sgmisc, which also requires a few from
sgprops, circular. So always use -lsgmisc and -lsgprops simultanious

W: SimGear undefined-non-weak-symbol /usr/lib/libsgsky-0.3.10.so sgEnviro
W: SimGear undefined-non-weak-symbol /usr/lib/libsgsky-0.3.10.so
_ZN8SGEnviro6getFOVERfS0_
W: SimGear undefined-non-weak-symbol /usr/lib/libsgsky-0.3.10.so
_ZN8SGEnviro17set_view_in_cloudEb
W: SimGear undefined-non-weak-symbol /usr/lib/libsgsky-0.3.10.so
_ZN8SGEnviro14callback_cloudEfffifi
lib

sgsky needs sgenvironment and vica versa, since normally (I think) a user will
always link with sgenviroment sgenviroment is linked against sgsky. If you want
to use sgsky directly, you must still add -lsgenvironment.

W: SimGear undefined-non-weak-symbol /usr/lib/libsgsky-0.3.10.so thesky
W: SimGear undefined-non-weak-symbol /usr/lib/libsgmodel-0.3.10.so thesky
W: SimGear undefined-non-weak-symbol /usr/lib/libsgenvironment-0.3.10.so thesky

This symbol isn't defined anywhere, at first I thought this must be defined by
the user, but thats not true, as flightgear needs to be patched to define it in
order for it to link. Appereantly the involved piece of code doesn't get used,
so when using .a files there is no problem as the object file doesn't become
involved in the linking, so thesky is never missed.

For now I'll "fix" this by adding a patch to sky.cxx defining thesky,
initializing  it to NULL and make it point to the first Sky created (one may
assume there is only one sky).

Some further untangling may be possible with code changes, but I don't know if
we want that.

So I'm going to fix the thesky issue and then build this.

Comment 26 Hans de Goede 2007-03-30 12:05:11 UTC
(In reply to comment #24)
> I thought I did the acls correctly... let me know if you can't make changes.

I can't:
**** Access denied: jwrdegoede is not in ACL for rpms/SimGear/devel

I guess you should put CVS / accountsys names in there not email addresses. My
account name is jwrdegoede

For you it doesn't matter as you are in owners.list as owner. Could you file a
fedora-cvs request here to make me co-owner (and in the mean time fix the acl),
then I will get BZ mail for this too.


Comment 27 Tom "spot" Callaway 2007-03-30 13:25:52 UTC
(In reply to comment #25)

> Some further untangling may be possible with code changes, but I don't know if
> we want that.
> 
> So I'm going to fix the thesky issue and then build this.

I think we may want this. This has merit for FlightGear (and other dependent
apps), so lets try to rework the code. If upstream barfs on it, we can pull it back.

The ACLs should be fixed now.

Comment 28 Hans de Goede 2007-03-30 15:11:25 UTC
(In reply to comment #27)
> (In reply to comment #25)
> 
> > Some further untangling may be possible with code changes, but I don't know if
> > we want that.
> > 
> > So I'm going to fix the thesky issue and then build this.
> 
> I think we may want this. This has merit for FlightGear (and other dependent
> apps), so lets try to rework the code. If upstream barfs on it, we can pull it
back.
> 

Okay, I've "fixed" most of this I use "" because I fixed it by inlining some
functions into the classes. In the sgmisc case this is harmless, in the sgprops
case its less pretty. With this untangling in place the only remaining problems are:
W: SimGear undefined-non-weak-symbol /usr/lib/libsgsky-0.3.10.so sgEnviro
W: SimGear undefined-non-weak-symbol /usr/lib/libsgsky-0.3.10.so
_ZN8SGEnviro6getFOVERfS0_
W: SimGear undefined-non-weak-symbol /usr/lib/libsgsky-0.3.10.so
_ZN8SGEnviro17set_view_in_cloudEb
W: SimGear undefined-non-weak-symbol /usr/lib/libsgsky-0.3.10.so
_ZN8SGEnviro14callback_cloudEfffifi


These are much harder to fix as sky/cloud and environment are really entangled
pretty much.

Anyways I've committed the current state of afairs to CVS and requested a build,
since I believe this is as good as its going to get for now.

As I myself am not very familiar with SimGear / Flightgear, I would like to ask
you to take these patches upstream and see how they respond, good luck!


Comment 29 Hans de Goede 2007-04-02 20:17:08 UTC
SimGear is in the repo now (and has been successfully tested with FlightGear),
closing.

For those interested FlightGear's review is here:
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=234826
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=234831
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=234835 



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