Bug 234826 - Review Request: FlightGear - Flight Simulator
Review Request: FlightGear - Flight Simulator
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Bernard Johnson
Fedora Package Reviews List
:
Depends On: 234831
Blocks:
  Show dependency treegraph
 
Reported: 2007-04-02 09:33 EDT by Fabrice Bellet
Modified: 2007-11-30 17:12 EST (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-05-10 10:15:08 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
bjohnson: fedora‑review+
wtogami: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Fabrice Bellet 2007-04-02 09:33:13 EDT
Spec URL: http://fr2.rpmfind.net/pub/bellet/review/FlightGear.spec
SRPM URL: http://fr2.rpmfind.net/pub/bellet/review/FlightGear-0.9.10-2.fc6.src.rpm

Description: 
The Flight Gear project is working to create a sophisticated flight
simulator framework for the development and pursuit of interesting
flight simulator ideas.

Now that SimGear is in Fedora, games using this library can be submitted.
Comment 1 Bernard Johnson 2007-04-07 12:34:05 EDT
What is:
Requires:       fgfs-base = %{version}


Sed is preferred:
dos2unix docs-mini/AptNavFAQ.FlightGear.html

From build.log:
Automake version: ./configure: line 13604: automake: command not found

Comment 2 Bernard Johnson 2007-04-07 12:36:10 EDT
(In reply to comment #1)
> What is:
> Requires:       fgfs-base = %{version}

Ignore that comment, I found it and set a dependency bug.
Comment 3 Fabrice Bellet 2007-04-07 16:33:56 EDT
(In reply to comment #1)

> Sed is preferred:
> dos2unix docs-mini/AptNavFAQ.FlightGear.html

ok. fixed (and also in the fgfs-base package)

> From build.log:
> Automake version: ./configure: line 13604: automake: command not found

This one is an upstream problem. automake is called at the end of the configure
script, and is just used to display summary information (automake version).
configure should not use automake. I added a patch to fix this, and will report
upstream if you agree. Should I also patch configure.ac, even if this is not
required for a clean build ? If yes, some care must be taken with files
modification date, as the Makefile expects configure to be newer than
configure.ac to start compiling.

New SPEC : http://fr2.rpmfind.net/pub/bellet/review/FlightGear.spec
New SRPM : http://fr2.rpmfind.net/pub/bellet/review/FlightGear-0.9.10-3.fc6.src.rpm
Comment 4 Aurelien Bompard 2007-04-08 08:08:20 EDT
By the way, there's a typo in the %build section : it's not "%{?smp_mflags}",
it's "%{?_smp_mflags}".
Comment 5 Fabrice Bellet 2007-04-08 09:46:26 EDT
Thanks for the typo Aurelien. I tried a parallel build, but the compilation
failed. So until parallel build is fixed upstream, it is probably better IMO to
disable it in the spec file.

New SPEC: http://fr2.rpmfind.net/pub/bellet/review/FlightGear.spec
New SRPM: http://fr2.rpmfind.net/pub/bellet/review/FlightGear-0.9.10-4.fc6.src.rpm

I also fixed some duplicate BuildRequires in this new build.
Comment 6 Hans de Goede 2007-04-08 13:33:58 EDT
Removing needsponsor blocker as Fabrice has been sponsored now. Bernard /
Aurelien, feel free to review this (hint hint).
Comment 7 Bernard Johnson 2007-04-16 06:33:04 EDT
Package Review
==============

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Buildroot is correct
(%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
 [x] Rpmlint output: None
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format %{name}.spec.
 [x] Package meets the  Packaging Guidelines.
 [x] Package is licensed with an open-source compatible license and meet other
legal requirements as defined in the legal section of Packaging Guidelines.
 [x] License field in the package spec file matches the actual license.
     License type:
 [x] If (and only if) the source package includes the text of the license(s) in
its own file, then that file, containing the text of the license(s) for the
package is included in %doc.
 [x] Spec file is written in American English.
 [x] Spec file for the package is legible.
 [x] Sources used to build the package matches the upstream source, as provided
in the spec URL.
     MD5SUM this package    : f4b89c9cafc18d56beab77a04f1ebdce
     MD5SUM upstream package: f4b89c9cafc18d56beab77a04f1ebdce
 [x] Package successfully compiles and builds into binary rpms on at least one
supported architecture.
     Tested on: FC-6 / i386
 [x] Package is not known to require ExcludeArch, OR:
     Arches excluded:
     Why:
 [?] All build dependencies are listed in BuildRequires, except for any that are
listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [-] ldconfig called in %post and %postun if required.
 [x] Package is not relocatable.
 [x] Package must own all directories that it creates.
 [-] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf %{buildroot} (or
$RPM_BUILD_ROOT).
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [x] Package uses nothing in %doc for runtime.
 [-] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [-] Package requires pkgconfig, if .pc files are present.
 [-] Development .so files in -devel subpackage, if present.
 [-] Fully versioned dependency in subpackages, if present.
 [-] Package does not contain any libtool archives (.la).
 [!] Package contains a properly installed %{name}.desktop file if it is a GUI
application.
 [x] Package does not own files or directories owned by other packages.

=== SUGGESTED ITEMS ===
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains
translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on: FC-6 / i386
 [-] Package should compile and build into binary rpms on all supported
architectures.
     Tested on:
 [x] Package functions as described.
 [-] Scriptlets must be sane, if used.
 [-] The placement of pkgconfig(.pc) files are correct.
 [-] File based requires are sane.
 [x] Latest version is packaged.

=== Issues ===
1. Possible missing BR, from build.log:
   "checking for library containing hid_init... no"
   "checking for X... no"
2. Host listed in Source0 URL appears to be down.  Please verify if this is
still a valid download link.
3. Please clean up %doc entries that are related to building, installation, or
other operating systems.
4. Please change "%patch0 -p1" to "%patch0 -p1 -b .no-automake-in-configure"
5. What is the reason to add -fPIC to $RPM_OPT_FLAGS?  If this is truly needed,
it must be documented.
6. Please change "%defattr(-, root, root, 0755)" to "%defattr(-, root, root, -)
and if needed, fix install section to set correct permissions.
7. Must include a .desktop file (please see
http://fedoraproject.org/wiki/Packaging/Guidelines#desktop)


=== Final Notes ===
1.

Minor cleanups, then we'll call it good.
Comment 8 Fabrice Bellet 2007-04-16 11:39:35 EDT
(In reply to comment #7)

Thanks for the review Bernard,

> === Issues ===
> 1. Possible missing BR, from build.log:
>    "checking for library containing hid_init... no"

hid_init() from libusbhid is specific to freebsd (from configure.ac):
dnl add joystick support libraries
dnl search for FreeBSD library
AC_SEARCH_LIBS(hid_init, usbhid)
joystick_LIBS="$LIBS"
LIBS=""

so IMO, it's safe to ignore this unsatisfied check (joystick interface should be
handled by plib IIRC)

>    "checking for X... no"

Checking for X (in fact looking for XIntrisic.h from libXt-devel) comes from the
AC_PATH_XTRA macro, that defines X_DISPLAY_MISSING in src/Include/config.h. This
macro is used nowhere in the source code and in the included headers. My
conclusion is that adding libXt-devel to the buildroot has no consequence on the
build result.

> 2. Host listed in Source0 URL appears to be down.  Please verify if this is
> still a valid download link.

yes, I confirm that ftp.flightgear.org is currently unreachable. This is
probably a transcient problem, as it appears on
http://www.flightgear.org/mirrors.html#ftp at the first row and its dns name
indicates that its probably the primary/master mirror.

> 3. Please clean up %doc entries that are related to building, installation, or
> other operating systems.

done.

> 4. Please change "%patch0 -p1" to "%patch0 -p1 -b .no-automake-in-configure"

done.

> 5. What is the reason to add -fPIC to $RPM_OPT_FLAGS?  If this is truly needed,
> it must be documented.

This option is not needed, it's a mistake. I removed it from the spec file.
 
> 6. Please change "%defattr(-, root, root, 0755)" to "%defattr(-, root, root, -)
> and if needed, fix install section to set correct permissions.

done

> 7. Must include a .desktop file (please see
> http://fedoraproject.org/wiki/Packaging/Guidelines#desktop)

Done. I added a .desktop file, but I didn't find a suitable icon, neither in the
 source tarball, nor in the fgfs-base package. Is it acceptable to ship a
desktop file without icon ? 

New SPEC: http://fr2.rpmfind.net/pub/bellet/review/FlightGear.spec
New SRPM: http://fr2.rpmfind.net/pub/bellet/review/FlightGear-0.9.10-5.fc6.src.rpm
Comment 9 Fabrice Bellet 2007-04-16 16:20:34 EDT
(In reply to comment #7)

> 2. Host listed in Source0 URL appears to be down.  Please verify if this is
> still a valid download link.

ftp.flightgear.org is back online, and the link is okay.
Comment 10 Bernard Johnson 2007-04-16 23:58:10 EDT
(In reply to comment #8)
> > 7. Must include a .desktop file (please see
> > http://fedoraproject.org/wiki/Packaging/Guidelines#desktop)
> 
> Done. I added a .desktop file, but I didn't find a suitable icon, neither in the
>  source tarball, nor in the fgfs-base package. Is it acceptable to ship a
> desktop file without icon ? 

It's VERY preferable to have the icon in place.  Please take a look at this:
http://mail.flightgear.org/pipermail/flightgear-devel/2005-September/039106.html
http://jrbabcock.home.comcast.net/flightgear/icons/index.html

Contact the author of this artwork and see if he will release it under a license
that allows you to use it (and include that license as well).

You also need to add this:
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?highlight=%28scriptlets%29#head-de6770dd9867fcd085a73a4700f6bcd0d10294ef
and
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?highlight=%28scriptlets%29#head-7103f6c38d1b5735e8477bdd569ad73ea2c49bda
Comment 11 Fabrice Bellet 2007-04-17 19:06:32 EDT
(In reply to comment #10)
> (In reply to comment #8)
> > > 7. Must include a .desktop file (please see
> > > http://fedoraproject.org/wiki/Packaging/Guidelines#desktop)
> > 
> > Done. I added a .desktop file, but I didn't find a suitable icon, neither in the
> >  source tarball, nor in the fgfs-base package. Is it acceptable to ship a
> > desktop file without icon ? 
> 
> It's VERY preferable to have the icon in place.  Please take a look at this:
> http://mail.flightgear.org/pipermail/flightgear-devel/2005-September/039106.html
> http://jrbabcock.home.comcast.net/flightgear/icons/index.html
> 
> Contact the author of this artwork and see if he will release it under a license
> that allows you to use it (and include that license as well).

Nice find. I contacted him, and he kindly provides the icons under the GPL
license, and updated his web page accordingly. I added the icon to the desktop
file, and included all available sizes.

> 
> You also need to add this:
>
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?highlight=%28scriptlets%29#head-de6770dd9867fcd085a73a4700f6bcd0d10294ef

done.

> and
>
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?highlight=%28scriptlets%29#head-7103f6c38d1b5735e8477bdd569ad73ea2c49bda

done.

New SPEC: http://fr2.rpmfind.net/pub/bellet/review/FlightGear.spec
New SRPM: http://fr2.rpmfind.net/pub/bellet/review/FlightGear-0.9.10-6.fc6.src.rpm
Comment 12 Bernard Johnson 2007-04-17 20:56:54 EDT
================
*** APPROVED ***
================
Comment 13 Fabrice Bellet 2007-04-18 04:33:15 EDT
New Package CVS Request
=======================
Package Name: FlightGear
Short Description: The FlightGear Flight Simulator
Owners: fabrice@bellet.info
Branches: FC-6
InitialCC:
Comment 14 Bernard Johnson 2007-04-26 18:09:16 EDT
Fabrice,

Please finish up the steps listed at
http://fedoraproject.org/wiki/PackageReviewProcess#head-df921556b35438a4c78b4b6a790151ea568e8f9e

Namely, import, build, and close the review ticket.  Thanks.
Comment 15 Hans de Goede 2007-04-27 02:31:19 EDT
AFAIK Fabrice is on vacation, I'm sure he will handle this when he is back.
Comment 16 Fabrice Bellet 2007-05-10 10:15:08 EDT
Yes. Back from vacation. FlightGear has been successfully build for FC-6 and
devel. Thanks for the review.

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