Bug 491519

Should this really be a separate package? Seems like a core part of OpenTTD to me (if Fedora is going to distribute the game).

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.

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

It was just a thought. I would love to see this included in Fedora.

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

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

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.

Contacted upstream about sources

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

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

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

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

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

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

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?

I'll bite the bullet - grfcodec review submitted as bug #498744 - you want to co-maintain?

It builds from source now:

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

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

I'm suggesting to add %check that does checking that md5 sums in *.obg are correspondent to ones that were built by grfcodec.

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

Okay. I'll try implementing that and then put a new spec online before requesting CVS.

New Package CVS Request
=======================
Package Name: openttd-opengfx
Short Description: OpenGFX replacement graphics for OpenTTD
Owners: heffer
Branches: F-10 F-11 devel
InitialCC:

CVS done.

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

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

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

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

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.

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.