Bug 208678

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

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.

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. 

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 :(

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.

(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.

(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.

I confirm the empty debuginfo package. The source version has debug folder so it
is possibly a rpm bug.

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

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.

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

(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.
 

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.  

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

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.

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... 

Hmm, Spot. Have you brought SimGear into the repository?

New Package CVS Request
=======================
Package Name: SimGear
Short Description: Simulation library components
Owners: tcallawa
Branches: FC-5 FC-6 devel

cvs created

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/

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.

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.

(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.

I thought I did the acls correctly... let me know if you can't make changes.

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.

(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.

(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.

(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!

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