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
Let this block FE-Legal as a precaution to check if this OK trademark-wise.
Is --with-ccache something relevant for the koji builders? (Should you require ccache then?)
I'm going to say this falls on the acceptable side of the trademark line. Lifting FE-Legal.
(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!
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.
(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.
(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
(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.
(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 ?
(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.
(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.
(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.
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.
(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.
After huge flamewars, flags policy was suspended so we can live without hiding-flags patch :)
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
(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
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
Your icon-cache scriptlets are not up-to-date. Check the latest version at https://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache
(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.
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:
CVS done.
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
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
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
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
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
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
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
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
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.
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.