Bug 491518 - Review Request: openttd - Transport system simulation game
Review Request: openttd - Transport system simulation game
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Hans de Goede
Fedora Extras Quality Assurance
:
Depends On: 491519
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-22 10:50 EDT by Felix Kaechele
Modified: 2009-07-16 03:14 EDT (History)
11 users (show)

See Also:
Fixed In Version: 0.7.1-1.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-07-16 02:52:15 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
hdegoede: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Felix Kaechele 2009-03-22 10:50:18 EDT
Spec URL: http://heffer.fedorapeople.org/review/openttd.spec
SRPM URL: http://heffer.fedorapeople.org/review/openttd-0.7.0-0.2.rc1.fc11.src.rpm
Description: OpenTTD is modeled after a popular transportation business simulation game
by Chris Sawyer and enhances the game experience dramatically. Many features
were inspired by TTDPatch while others are original.

openttd.spec:45: W: rpm-buildroot-usage %build --install-dir=$RPM_BUILD_ROOT \
openttd.spec:34: W: configure-without-libdir-spec
is due to the non-standard buildsystem OpenTTD uses
Comment 1 Felix Kaechele 2009-03-22 11:08:31 EDT
Let this block FE-Legal as a precaution to check if this OK trademark-wise.
Comment 2 Conrad Meyer 2009-03-22 18:42:35 EDT
Is --with-ccache something relevant for the koji builders? (Should you require ccache then?)
Comment 3 Tom "spot" Callaway 2009-03-23 09:49:11 EDT
I'm going to say this falls on the acceptable side of the trademark line. Lifting FE-Legal.
Comment 4 Alexey Torkhov 2009-03-24 05:07:58 EDT
(In reply to comment #2)
> Is --with-ccache something relevant for the koji builders? (Should you require
> ccache then?)  

It is relevant with default mock settings where ccache is installed, not sure about koji. It should check whether ccache present and silently continue if it is not.

(In reply to comment #3)
> I'm going to say this falls on the acceptable side of the trademark line.
> Lifting FE-Legal.  

Thanks for checking this!
Comment 5 Alexey Torkhov 2009-03-29 15:10:08 EDT
Few notes about the package:

- We must patch it to hide country flags from network server list
I've asked them to replace them in graphics, but there is no guarantee that they will do it:
https://secure.openttd.org/bugs/task/2771

- Please update it to 0.7.0-RC2.

- Would be good to generate AI docs from src/ai/api/Doxyfile and put them to subpackage.

- Macros issue in opengfx package is not addressed.
Comment 6 Hans de Goede 2009-05-17 06:07:03 EDT
(In reply to comment #5)
> Few notes about the package:
> 
> - We must patch it to hide country flags from network server list
> I've asked them to replace them in graphics, but there is no guarantee that
> they will do it:
> https://secure.openttd.org/bugs/task/2771
> 

Hmm, you write there:
"The reason behind this report is Fedora's policy of handling flags. They must be split (or removed completely) to a subpackage that is not installed by default. This was caused by some political case."

Is this actual policy (as in written down somewhere and approved by Fesco) ?
or just something which was decided adhoc somewhere ?

> - Please update it to 0.7.0-RC2.
> 

Ack.

> - Would be good to generate AI docs from src/ai/api/Doxyfile and put them to
> subpackage.
> 

This IMHO makes no sense, this is an internal API, which is not exported
so no need to buid or include the docs.

> - Macros issue in opengfx package is not addressed.  

??


As promised I will review this, but it looks like we first need to address
the flags issue <sigh>, assigning to me.
Comment 7 Alexey Torkhov 2009-05-17 06:16:32 EDT
(In reply to comment #6)
> Hmm, you write there:
> "The reason behind this report is Fedora's policy of handling flags. They must
> be split (or removed completely) to a subpackage that is not installed by
> default. This was caused by some political case."
> 
> Is this actual policy (as in written down somewhere and approved by Fesco) ?
> or just something which was decided adhoc somewhere ?

The policy https://fedoraproject.org/wiki/TomCallaway/Flags was approved on one of FESCo meetings as per https://fedorahosted.org/fesco/ticket/110
Comment 8 Alexey Torkhov 2009-05-17 06:18:54 EDT
(In reply to comment #6)
> As promised I will review this, but it looks like we first need to address
> the flags issue <sigh>, assigning to me.  

I'm thinking, that we can just hide language column with flags. If such solution would be acceptable, I can come with a patch for it.
Comment 9 Hans de Goede 2009-05-17 06:24:06 EDT
(In reply to comment #7)
> (In reply to comment #6)
> > Hmm, you write there:
> > "The reason behind this report is Fedora's policy of handling flags. They must
> > be split (or removed completely) to a subpackage that is not installed by
> > default. This was caused by some political case."
> > 
> > Is this actual policy (as in written down somewhere and approved by Fesco) ?
> > or just something which was decided adhoc somewhere ?
> 
> The policy https://fedoraproject.org/wiki/TomCallaway/Flags was approved on one
> of FESCo meetings as per https://fedorahosted.org/fesco/ticket/110  

Ah I wasn't aware of this policy, glad I mist the flame fest one that one :)

(In reply to comment #8)
> (In reply to comment #6)
> > As promised I will review this, but it looks like we first need to address
> > the flags issue <sigh>, assigning to me.  
> 
> I'm thinking, that we can just hide language column with flags. If such
> solution would be acceptable, I can come with a patch for it.  

Would there then still be a way for the user to find out the server language
from the pick a server UI ?
Comment 10 Alexey Torkhov 2009-05-17 06:34:25 EDT
(In reply to comment #6)
> > - Would be good to generate AI docs from src/ai/api/Doxyfile and put them to
> > subpackage.
> > 
> 
> This IMHO makes no sense, this is an internal API, which is not exported
> so no need to buid or include the docs.

This is only script "external" API that is used by squirrel AI scripts. And it
could be used by AI writers as scripts are easily added as plugins.

> > - Macros issue in opengfx package is not addressed.  

Macro usage is not consistent there - it is mixing "rm" and %{__rm} there for instance. I think, all macros like %{__rm} should be replaced in favour of simple commands.

(In reply to comment #9)
> Ah I wasn't aware of this policy, glad I mist the flame fest one that one :)

Hehe.

> (In reply to comment #8)
> > (In reply to comment #6)
> > > As promised I will review this, but it looks like we first need to address
> > > the flags issue <sigh>, assigning to me.  
> > 
> > I'm thinking, that we can just hide language column with flags. If such
> > solution would be acceptable, I can come with a patch for it.  
> 
> Would there then still be a way for the user to find out the server language
> from the pick a server UI ?  

Yes, when user clicks on individual server line, server language will be shown on server details panel. But in such, there won't be simple way to fast search for particular language - user will have to click on each server if he wants to find server where is that particular language spoken.
Comment 11 Alexey Torkhov 2009-05-17 07:02:19 EDT
(In reply to comment #10)
> > > - Macros issue in opengfx package is not addressed.  
> 
> Macro usage is not consistent there - it is mixing "rm" and %{__rm} there for
> instance. I think, all macros like %{__rm} should be replaced in favour of
> simple commands.

Oh, that was fixed in new srpms. I was looking at old spec link.
Comment 12 Hans de Goede 2009-05-17 07:47:19 EDT
(In reply to comment #10)
> (In reply to comment #6)
> > > - Would be good to generate AI docs from src/ai/api/Doxyfile and put them to
> > > subpackage.
> > > 
> > 
> > This IMHO makes no sense, this is an internal API, which is not exported
> > so no need to buid or include the docs.
> 
> This is only script "external" API that is used by squirrel AI scripts. And it
> could be used by AI writers as scripts are easily added as plugins.
> 

Ah, ok then indeed it would be good to package this in a sub-package.

> > (In reply to comment #8)
> > > (In reply to comment #6)
> > > > As promised I will review this, but it looks like we first need to address
> > > > the flags issue <sigh>, assigning to me.  
> > > 
> > > I'm thinking, that we can just hide language column with flags. If such
> > > solution would be acceptable, I can come with a patch for it.  
> > 
> > Would there then still be a way for the user to find out the server language
> > from the pick a server UI ?  
> 
> Yes, when user clicks on individual server line, server language will be shown
> on server details panel. But in such, there won't be simple way to fast search
> for particular language - user will have to click on each server if he wants to
> find server where is that particular language spoken.  

Hmm, not ideal but I guess just hiding the flags column in the UI is an ok
solution then.
Comment 13 Felix Kaechele 2009-05-17 07:56:04 EDT
Okay. Here's something to play with: 0.7.1-RC1

I added the docs tarball from upstream and put it in the package as a docs subpackage. It should contain everything that is needed to develop OTTD addons such as AI etc.

SPEC: http://heffer.fedorapeople.org/review/openttd.spec
SRPM: http://heffer.fedorapeople.org/review/openttd-0.7.1-0.1.rc1.fc11.src.rpm

Alexey, if you had a patch for the flag issue that would be awesome.

BTW: I updated the opengfx spec to match the one that is in the srpm.
Comment 14 Alexey Torkhov 2009-05-17 09:17:38 EDT
(In reply to comment #13)
> I added the docs tarball from upstream and put it in the package as a docs
> subpackage. It should contain everything that is needed to develop OTTD addons
> such as AI etc.

Seems, docs tarball they distribute is documentation for whole internal API. And we want only docs for AI API. It is simple to generate, just call doxygen from src/ai/api dir (doxygen should be in BR).

> Alexey, if you had a patch for the flag issue that would be awesome.

The simplest patch I used is just hiding flag:
http://atorkhov.fedorapeople.org/openttd-0.7.0-hide-flag.patch


The idea of representing languages by two-letter ISO codes does not work well is this case - they have Brazilian and Portuguese languages/flags but it will be represented by "pt" code. It should be either long code pt_BR/pt_PT which is not very good for users, or some shortening of full language name.
Comment 15 Alexey Torkhov 2009-05-22 15:28:18 EDT
After huge flamewars, flags policy was suspended so we can live without hiding-flags patch :)
Comment 16 Hans de Goede 2009-05-27 13:30:37 EDT
Reviewing as the flags policy has been suspended for now.

Full review done:

MUST FIX
--------
* The docs are currently duplicated between the main package
  and the -docs package. put the README, COPYING etc only in the main package
  and the rest in the -docs package
* Make the -docs package Require the main package (with full evr)
* Upstream source files are AWOL (newer RC?) so I cannot verify the origin
  of the files included in the srpm

SHOULD FIX
----------
* Why aren't you using %configure, if there is a specific reason not to
  please add a comment explaining this to the .spec file
* Don't untar %{SOURCE1} in %build instead add " -a 1" as extra arguments to
  %setup, or even better take a look at generating the docs buildtime as
  suggested in comment #14
Comment 17 Remko Bijker 2009-05-27 17:11:44 EDT
(In reply to comment #16)
> * Upstream source files are AWOL (newer RC?) so I cannot verify the origin
>   of the files included in the srpm
All official releases OpenTTD release (ever) can be found at either http://binaries.openttd.org/releases/ or at http://sourceforge.net/project/showfiles.php?group_id=103924&package_id=111717
Comment 18 Felix Kaechele 2009-05-28 16:48:40 EDT
I hope this fixes all the problems you pointed out:

http://heffer.fedorapeople.org/review/openttd.spec
http://heffer.fedorapeople.org/review/openttd-0.7.1-0.2.rc2.fc11.src.rpm
Comment 19 Christoph Wickert 2009-05-28 18:19:51 EDT
Your icon-cache scriptlets are not up-to-date. Check the latest version at https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache
Comment 20 Hans de Goede 2009-05-29 04:02:46 EDT
(In reply to comment #18)
> I hope this fixes all the problems you pointed out:
> 
> http://heffer.fedorapeople.org/review/openttd.spec
> http://heffer.fedorapeople.org/review/openttd-0.7.1-0.2.rc2.fc11.src.rpm  

Looks good, but as said you should update the icon-cache scriptlets, thats not a blocker though, so: APPROVED.
Comment 21 Felix Kaechele 2009-05-29 07:46:14 EDT
Okay. I'll change the scriptlets before adding it to the CVS.
Thank you for your review. It is much appreciated.

New Package CVS Request
=======================
Package Name: openttd
Short Description: Transport system simulation game
Owners: heffer
Branches: F-10 F-11 devel
InitialCC:
Comment 22 Jason Tibbitts 2009-05-29 11:31:57 EDT
CVS done.
Comment 23 Fedora Update System 2009-05-29 12:24:56 EDT
openttd-0.7.1-0.3.rc2.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/openttd-0.7.1-0.3.rc2.fc11
Comment 24 Fedora Update System 2009-05-29 12:24:57 EDT
openttd-0.7.1-0.3.rc2.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/openttd-0.7.1-0.3.rc2.fc10
Comment 25 Fedora Update System 2009-06-02 10:19:35 EDT
openttd-0.7.1-0.3.rc2.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update openttd'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-5730
Comment 26 Fedora Update System 2009-06-02 10:36:30 EDT
openttd-0.7.1-0.3.rc2.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update openttd'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-5810
Comment 27 Fedora Update System 2009-06-12 14:36:55 EDT
openttd-0.7.1-1.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/openttd-0.7.1-1.fc11
Comment 28 Fedora Update System 2009-06-12 14:53:51 EDT
openttd-0.7.1-1.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/openttd-0.7.1-1.fc10
Comment 29 Fedora Update System 2009-06-15 22:09:03 EDT
openttd-0.7.1-1.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update openttd'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-6308
Comment 30 Fedora Update System 2009-06-15 22:12:54 EDT
openttd-0.7.1-1.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update openttd'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-6329
Comment 31 Fedora Update System 2009-07-16 02:52:08 EDT
openttd-0.7.1-1.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 32 Fedora Update System 2009-07-16 03:13:55 EDT
openttd-0.7.1-1.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

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