Bug 491519 - Review Request: openttd-opengfx - OpenGFX replacement graphics for OpenTTD
Review Request: openttd-opengfx - OpenGFX replacement graphics for OpenTTD
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: 498744
Blocks: 491518
  Show dependency treegraph
 
Reported: 2009-03-22 10:51 EDT by Felix Kaechele
Modified: 2009-07-16 03:31 EDT (History)
7 users (show)

See Also:
Fixed In Version: 0-0.4.alpha4.2.fc10
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-07-16 03:22:13 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:51:37 EDT
Spec URL: http://heffer.fedorapeople.org/review/openttd-opengfx.spec
SRPM URL: http://heffer.fedorapeople.org/review/openttd-opengfx-0-0.1.alpha4.2.fc11.src.rpm
Description: The ultimate aim of this project is to have a full replacement set of graphics,
so that OpenTTD can be distributed freely without the need of the copyrighted
graphics from the original game.
Comment 1 Conrad Meyer 2009-03-22 17:58:39 EDT
Should this really be a separate package? Seems like a core part of OpenTTD to me (if Fedora is going to distribute the game).
Comment 2 Felix Kaechele 2009-03-22 18:08:27 EDT
I actually never thought about this from that perspective. I started it as a separate package because the opengfx project is separate from openttd development.
Comment 3 Alexey Torkhov 2009-03-22 18:15:28 EDT
(In reply to comment #1)
> Should this really be a separate package? Seems like a core part of OpenTTD to
> me (if Fedora is going to distribute the game).  

It is separate project with separate releases and separate developers. Yes, it is essential for OpenTTD, but why they should be bundled together?
Comment 4 Conrad Meyer 2009-03-22 18:21:21 EDT
It was just a thought. I would love to see this included in Fedora.
Comment 5 Conrad Meyer 2009-03-22 19:35:30 EDT
Requires: openttd
seems wrong (because openttd requires this for graphics). I think maybe a solution is for this to go in %{_datadir}/%{name}/ and when openttd is installed and requires this, it can symlink %{_datadir}/openttd/data to this package (if it's too hard to change where openttd looks).

This is one argument for including opengfx in openttd (opengfx can be installed into the correct place without a hack, and the two-way requires can be avoided).


The use of __<command> macros is unnecessary (%{__rm}, %{__mv}, etc). Fedora is not Mandriva.


This bit concerns me (slightly):
# These are already in %doc
%{__rm} $RPM_BUILD_ROOT%{_datadir}/openttd/data/COPYING
%{__rm} $RPM_BUILD_ROOT%{_datadir}/openttd/data/readme.txt
Are these really never used by OpenTTD? If so it's fine. 


Otherwise this looks pretty clean (fairly uncomplicated data-only package).
Comment 6 Alexey Torkhov 2009-03-24 04:59:10 EDT
(In reply to comment #5)
> Requires: openttd
> seems wrong (because openttd requires this for graphics). I think maybe a
> solution is for this to go in %{_datadir}/%{name}/ and when openttd is
> installed and requires this, it can symlink %{_datadir}/openttd/data to this
> package (if it's too hard to change where openttd looks).

This Requires is needed for game data uninstalled simultaneously with the game itself. It is pretty standard among game data packages.

> This is one argument for including opengfx in openttd (opengfx can be
> installed into the correct place without a hack, and the two-way requires
> can be avoided).

Separating game from game data allows to decrease size of updates as there is no need to update data when binary updated and vice versa.
And, after all, bundling multiple projects in not recommended by guidelines.

> The use of __<command> macros is unnecessary (%{__rm}, %{__mv}, etc). Fedora
> is not Mandriva.

Agree with this. It makes macros usage non-consistent. Please fix it.

> This bit concerns me (slightly):
> # These are already in %doc
> %{__rm} $RPM_BUILD_ROOT%{_datadir}/openttd/data/COPYING
> %{__rm} $RPM_BUILD_ROOT%{_datadir}/openttd/data/readme.txt
> Are these really never used by OpenTTD? If so it's fine. 

OpenTTD uses metadata from *.obg. Those files are only docs.
Comment 7 Hans de Goede 2009-04-05 13:55:34 EDT
Felix,

Thanks for the offer to swap reviews, you're lucky as one of the packages
I wanted to have reviewed turns out to already be a part of Fedora, so
you only need to review 2 packages.

Taking review, no full review done yet. My first concern with this
package is (was) the legal status of it. But it looks like it
is a properly licensed original creation so no issues there.

However it comes licenses as GPL, but does not come with sources
that is a show stopper. Can you please request the sources (and
build instructions) from FooBar (as described in the README). Since
this is GPL, we really need to be shipping with sources to meet
the legal requirements. And the build from source to meet
Fedora's own guidelines.
Comment 8 Felix Kaechele 2009-04-11 18:14:58 EDT
Contacted upstream about sources
Comment 9 Alexey Torkhov 2009-04-12 02:55:29 EDT
(In reply to comment #7)
> Can you please request the sources (and build instructions)

To build .grf from sources it will need grfcodec program (http://www.ttdpatch.net/grfcodec/) which is has to be packaged.
Comment 10 Hans de Goede 2009-04-12 10:15:12 EDT
(In reply to comment #9)
> (In reply to comment #7)
> > Can you please request the sources (and build instructions)
> 
> To build .grf from sources it will need grfcodec program
> (http://www.ttdpatch.net/grfcodec/) which is has to be packaged.  

Well luckily that seems to be open source too, so it looks like that
needs to be packaged too and then this package can BuildRequires it.

I'll review it too when packaged.
Comment 11 Felix Kaechele 2009-04-19 12:31:05 EDT
I tried to package grfcodec but it's a total mess. They mix C and ASM stuff and totally ignore that there are other arches than ix86. I hacked at the SPEC to at least build on x86_64 but due to my not so leet skills I'm probably unable to bring it to build on ppc{64}.

Here's the task:
https://koji.fedoraproject.org/koji/taskinfo?taskID=1308035
Comment 12 Alexey Torkhov 2009-04-19 13:11:16 EDT
(In reply to comment #11)
> I tried to package grfcodec but it's a total mess. They mix C and ASM stuff and
> totally ignore that there are other arches than ix86. I hacked at the SPEC to
> at least build on x86_64 but due to my not so leet skills I'm probably unable
> to bring it to build on ppc{64}.

They use ASM for stuff like self-decompressing-files. This part could be omitted either for all arches or only for non-x86.
Comment 13 Hans de Goede 2009-04-20 03:54:29 EDT
(In reply to comment #11)
> I tried to package grfcodec but it's a total mess. They mix C and ASM stuff and
> totally ignore that there are other arches than ix86. I hacked at the SPEC to
> at least build on x86_64 but due to my not so leet skills I'm probably unable
> to bring it to build on ppc{64}.
> 
> Here's the task:
> https://koji.fedoraproject.org/koji/taskinfo?taskID=1308035  

I agree with Alexey, try to get it to build without the self decompressing support. If that turns out to be a problem, we can probably come up with some
hack where we make grfcodec and then build noarch files using it on x86, which
we then can use everywhere.
Comment 14 Iain Arnell 2009-05-02 06:02:05 EDT
Fortunately, there's no real assembler involved - just including object code as data inside another binary. Trivial to rewrite using "as" rather than nasm and have it work (or at least build - I've no idea how to test it) everywhere.

Example:  http://koji.fedoraproject.org/koji/taskinfo?taskID=1332503
Comment 15 Felix Kaechele 2009-05-02 08:48:25 EDT
Wow! That's some great news. Thanks for your work!

So how would you suggest to continue? Do you want to import the package? Or should I import it and set you as co-maintainer? Or do you wish to not be involved with the package at all?
Comment 16 Iain Arnell 2009-05-02 13:49:32 EDT
I'll bite the bullet - grfcodec review submitted as bug #498744 - you want to co-maintain?
Comment 18 Hans de Goede 2009-05-17 05:58:16 EDT
(In reply to comment #17)
> It builds from source now:
> 

Great!

> http://koji.fedoraproject.org/koji/taskinfo?taskID=1358203
> 
> New SPEC: http://heffer.fedorapeople.org/review/openttd-opengfx.spec
> New SRPM:
> http://heffer.fedorapeople.org/review/openttd-opengfx-0-0.3.alpha4.2.fc11.src.rpm  

Full review done: Approved !
Comment 19 Alexey Torkhov 2009-05-17 06:13:18 EDT
I'm suggesting to add %check that does checking that md5 sums in *.obg are correspondent to ones that were built by grfcodec.
Comment 20 Hans de Goede 2009-05-17 06:16:24 EDT
(In reply to comment #19)
> I'm suggesting to add %check that does checking that md5 sums in *.obg are
> correspondent to ones that were built by grfcodec.  

That sounds like a good plan, yes, esp. wrt network play, Felix ?
Comment 21 Felix Kaechele 2009-05-17 06:18:59 EDT
Okay. I'll try implementing that and then put a new spec online before requesting CVS.
Comment 22 Felix Kaechele 2009-05-28 16:42:30 EDT
New Package CVS Request
=======================
Package Name: openttd-opengfx
Short Description: OpenGFX replacement graphics for OpenTTD
Owners: heffer
Branches: F-10 F-11 devel
InitialCC:
Comment 23 Jason Tibbitts 2009-05-29 00:49:54 EDT
CVS done.
Comment 24 Fedora Update System 2009-05-29 08:57:16 EDT
openttd-opengfx-0-0.4.alpha4.2.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/openttd-opengfx-0-0.4.alpha4.2.fc10
Comment 25 Fedora Update System 2009-05-29 09:11:21 EDT
openttd-opengfx-0-0.4.alpha4.2.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/openttd-opengfx-0-0.4.alpha4.2.fc11
Comment 26 Fedora Update System 2009-05-29 22:28:26 EDT
openttd-opengfx-0-0.4.alpha4.2.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-opengfx'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-5657
Comment 27 Fedora Update System 2009-05-29 22:35:55 EDT
openttd-opengfx-0-0.4.alpha4.2.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-opengfx'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-5696
Comment 28 Fedora Update System 2009-07-16 03:22:06 EDT
openttd-opengfx-0-0.4.alpha4.2.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 29 Fedora Update System 2009-07-16 03:31:32 EDT
openttd-opengfx-0-0.4.alpha4.2.fc10 has been pushed to the Fedora 10 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.