Bug 1122 - Review request: mame - Multiple Arcade Machine Emulator
Summary: Review request: mame - Multiple Arcade Machine Emulator
Status: RESOLVED FIXED
Alias: None
Product: Package Reviews
Classification: Unclassified
Component: Review Request (show other bugs)
Version: Current
Hardware: All GNU/Linux
: P5 normal
Assignee: Chen Lei
URL:
Depends on:
Blocks: RF_ACCEPT
  Show dependency treegraph
 
Reported: 2010-03-17 21:47 CET by Julian Sikorski
Modified: 2010-04-07 23:38 CEST (History)
4 users (show)

See Also:
namespace:


Attachments
ini files and manpages from mame 0.137 for ubuntu (17.27 KB, application/x-gzip-compressed)
2010-03-21 12:51 CET, Chen Lei
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Sikorski 2010-03-17 21:47:35 CET
Spec URL: http://homepage.hispeed.ch/belegdol/mame/mame.spec
SRPM URL: http://homepage.hispeed.ch/belegdol/mame/mame-0137-1.fc13.src.rpm

Description:
MAME stands for Multiple Arcade Machine Emulator.  When used in conjunction with an arcade game's data files (ROMs), MAME will more or less faithfully reproduce that game on a PC.

The ROM images that MAME utilizes are "dumped" from arcade games' original circuit-board ROM chips.  MAME becomes the "hardware" for the games, taking the place of their original CPUs and support chips.  Therefore, these games are NOT simulations, but the actual, original games that appeared in arcades.

MAME's purpose is to preserve these decades of video-game history.  As gaming technology continues to rush forward, MAME prevents these important "vintage" games from being lost and forgotten.  This is achieved by documenting the 
hardware and how it functions, thanks to the talent of programmers from the MAME team and from other contributors.  Being able to play the games is just a nice side-effect, which doesn't happen all the time.  MAME strives for emulating the games faithfully.


This is an initial package based on SDLMAME. The SDL osd layer has now been merged with mainline, so a new package using a slightly different build process is needed.
Comment 1 Chen Lei 2010-03-19 17:53:39 CET
Some comments:
1.
%global baseversion 0137
#global sourceupdate 4

%if 0%{?sourceupdate}
Version:        %{baseversion}u%{sourceupdate}
%else
Version:        %{baseversion}
%endif
Release:        1%{?dist}

#global sourceupdate 4 -> #global sourceupdate u4
Version:        0.%{baseversion}
Release:        1.{?sourceupdate}%{?dist}
Note:{?sourceupdate} should be added in release section as fedora naming guideline, such as 0.137-2.u1.fc13
3.
%ifarch x86_64 ppc64-> %ifarch x86_64 ppc64 sparc64 s390
Also you can not define suffix64 in specfile by using wildcard instead.
install -pm 755 %{name}%{?suffix64}d %{buildroot}%{_bindir}
->install -pm 755 %{name}*d %{buildroot}%{_bindir}
install -pm 755 chdman jedutil ldplayer%{?suffix64} ldresample ldverify \->
install -pm 755 chdman jedutil ldplayer* ldresample ldverify \

4.
Source0->
http://www.aarongiles.com/mirror/releases/%{name}0137s.exe
5.
Patch0 not used in specfile
6.
Provides:       sdlmame = %{version}-%{release}
Obsoletes:      sdlmame < 0136-2
->
Provides:       sdlmame = %{baseversion}-%{release}
Obsoletes:      sdlmame < 0137-1
7.
duplicate file found in subpackage.
%doc docs/license.txt
%doc docs/license.txt
You should not rely all on rpmlint output.
Comment 2 Chen Lei 2010-03-19 18:26:25 CET
Also there is only one file in %{_sysconfdir}/%{name}, why not to put           %{name}.ini directly in %{_sysconfdir}.

BTW, I'm also curious with the origin of Source1(ui.bdf), did it covert from some ttf fonts or download from somewhere?
Comment 3 Julian Sikorski 2010-03-19 20:55:47 CET
1. I believe that the whole dance with splitting version between %version and %release is to ensure proper upgrade paths. I checked that it works, 0137u10 > 0137u1 < 0137. And putting the "u" into the %global will break the algorithm which applies the patches sequentially to the "u" releases. I know it was this way in SDLMAME, but RB released it as full zips. I never liked the versions like 0.137-0.1.0136u1 anyway.

2. I can't see it ;)

3. I could, but I can't do it for the main binary name, which would lead to inconsistent use of macros. I can add other architectures, sure.

4. That's just one of the mirrors, I guess I could hardcode it as we do for sourceforge/nongnu/whatever.

5. Valid point. This is a leftover from testing a bug. I'll fix that.

6. Obsoletes is OK, point 1 for Provides.

7. I can kill the duplicate docs, no problem.

The ini file was put into the subdir so that cab builders could place config files in there without polluting /etc/.
ui.bdc was generated from ui.bdf, which was shipped with sdlmame zip. Aaron said it might return someday, so I left it in.
Comment 4 Chen Lei 2010-03-20 03:19:55 CET
(In reply to comment #3)
> 1. I believe that the whole dance with splitting version between %version and
> %release is to ensure proper upgrade paths. I checked that it works, 0137u10 >
> 0137u1 < 0137. And putting the "u" into the %global will break the algorithm
> which applies the patches sequentially to the "u" releases. I know it was this
> way in SDLMAME, but RB released it as full zips. I never liked the versions
> like 0.137-0.1.0136u1 anyway.
> 
I think 0137 is not a valid a version for mame, upstream and most mame deriatives use 0.137 as their versions. You should not use the numeric embeded in the tarball instead. As a rule of thumb, the version of mame will never reach 1.
I'd like to use 0.137-1 (0.137-2.u1, 0.137-3.u2,...0.138-1) as %{version}-{release}, 0.137-1(0.137u1-1, 0.137u2) may also works fine.
See http://fedoraproject.org/wiki/PackageNamingGuidelines#Post-Release_packages

Moreover, 0.137u1 means the first update for 0.137, it can be considered as a post-Release version. As I see, a lot of mame deriatives(mame32 mameplus) distribute binaries of source updates version, there's no significant difference of statibility between formal release(0.137) and source updates(0.137u1,0.137u2) ones. Also, most people will happy using source updates version(I use mame for more than ten years).
 

> 2. I can't see it ;)
> 
> 3. I could, but I can't do it for the main binary name, which would lead to
> inconsistent use of macros. I can add other architectures, sure.
> 
It's up to you:)

> 4. That's just one of the mirrors, I guess I could hardcode it as we do for
> sourceforge/nongnu/whatever.
> 
fedora's sourceURL guideline only specify the URL usage of sf.net. For other case, using a mirror URL is permittd, also this URL is redirected from http://mamedev.org/downloader.php?&file=mame0137s.exe.

> 5. Valid point. This is a leftover from testing a bug. I'll fix that.
> 
> 6. Obsoletes is OK, point 1 for Provides.
> 
Since you using 0135 and 0136 as version of sdlmame, so you should also provides sdlmame = 0137-1 and sdlmame = 0138-1 to enable seamless upgrade.

> 7. I can kill the duplicate docs, no problem.
> 
> The ini file was put into the subdir so that cab builders could place config
> files in there without polluting /etc/.
Are there files other than %{name}.ini existd in /etc/mame? I'm not familiar with linux version of mame.

> ui.bdc was generated from ui.bdf, which was shipped with sdlmame zip. Aaron
> said it might return someday, so I left it in.
Take care of licensing concern for ui.dbc(ui.dbf), some fonts may be illegal for distribution in linux.


Comment 5 Chen Lei 2010-03-20 03:23:18 CET
Wikipedia use 0.137 and 0.136u4 as version for mame.

See http://en.wikipedia.org/wiki/MAME

BTW, did you already split out the bundled libs included in EMBOSS? 
Comment 6 Julian Sikorski 2010-03-20 09:00:52 CET
Spec URL: http://homepage.hispeed.ch/belegdol/mame/mame.spec
SRPM URL: http://homepage.hispeed.ch/belegdol/mame/mame-0.137-2.fc13.src.rpm

New release:
- Changed the versioning scheme to include the dot
- Changed the source URL to point to aarongiles.com mirror directly
- Added missing application of the fortify patch
- Added sparc64 and s390 to architectures getting suffix64
- Removed duplicate license.txt

If we consider u versions as a post-release, the versioning scheme is even more valid:

Properly ordered simple versions. These are usually due to quick bugfix releases, such as openssl-0.9.6b or gkrellm-2.1.7a. As new versions come out, the non-numeric tag is properly incremented (e.g. openssl-0.9.6c) or the numeric version is increased and the non-numeric tag is dropped (openssl-0.9.7). In this case, the non-numeric characters are permitted in the Version: field.

Other files do not exist in /etc/mame at the moment, but if my memory serves me well, we added that subdir with XulChris in case somebody wants to put them there.

Please note that ui.bdc is commented out at this point. I'll check the licensing when/if it ever returns.

And no, I haven't touched EMBOSS, unfortunately. I have no idea how to do that, my coding skills are next to none. So, unless a co-maintainer steps up, I'm afraid I might need to orphan the package. It turned out to require much more work than I expected.
Comment 7 Chen Lei 2010-03-21 04:23:00 CET
(In reply to comment #6)
> Spec URL: http://homepage.hispeed.ch/belegdol/mame/mame.spec
> SRPM URL: http://homepage.hispeed.ch/belegdol/mame/mame-0.137-2.fc13.src.rpm
> 
> New release:
> - Changed the versioning scheme to include the dot
> - Changed the source URL to point to aarongiles.com mirror directly
> - Added missing application of the fortify patch
> - Added sparc64 and s390 to architectures getting suffix64
> - Removed duplicate license.txt
> 
> If we consider u versions as a post-release, the versioning scheme is even more
> valid:
> 
> Properly ordered simple versions. These are usually due to quick bugfix
> releases, such as openssl-0.9.6b or gkrellm-2.1.7a. As new versions come out,
> the non-numeric tag is properly incremented (e.g. openssl-0.9.6c) or the
> numeric version is increased and the non-numeric tag is dropped
> (openssl-0.9.7). In this case, the non-numeric characters are permitted in the
> Version: field.
> 
> Other files do not exist in /etc/mame at the moment, but if my memory serves me
> well, we added that subdir with XulChris in case somebody wants to put them
> there.
> 
> Please note that ui.bdc is commented out at this point. I'll check the
> licensing when/if it ever returns.
> 
> And no, I haven't touched EMBOSS, unfortunately. I have no idea how to do that,
> my coding skills are next to none. So, unless a co-maintainer steps up, I'm
> afraid I might need to orphan the package. It turned out to require much more
> work than I expected.
> 
The spec seems perfect except some minor problems.
1.OPT_FLAGS='%{optflags}' is useless.
You should export optflags in the %build section.
export CCOMFLAGS = "%{optflags}"
export CONLYFLAGS = "%{optflags}"
export CPPONLYFLAGS = "%{optflags}"
2.
Since Mame build with GCONF2, maybe you need some rpm scriptlets(I'm not sure about this).
See 
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#GConf

Comment 8 Chen Lei 2010-03-21 07:20:21 CET
> The spec seems perfect except some minor problems.
> 1.OPT_FLAGS='%{optflags}' is useless.
> You should export optflags in the %build section.
> export CCOMFLAGS = "%{optflags}"
> export CONLYFLAGS = "%{optflags}"
> export CPPONLYFLAGS = "%{optflags}"
I fails to find the define of OPT_FLAGS in sdl.mak,sorry.

But mame fails to build on both ppc(Internal error, please submit a full bug report for gcc) and ppc64(should set PTR64=1 manually for 64bit arch).
Comment 9 Chen Lei 2010-03-21 07:21:07 CET
(In reply to comment #8)
> > The spec seems perfect except some minor problems.
> > 1.OPT_FLAGS='%{optflags}' is useless.
> > You should export optflags in the %build section.
> > export CCOMFLAGS = "%{optflags}"
> > export CONLYFLAGS = "%{optflags}"
> > export CPPONLYFLAGS = "%{optflags}"
> I fails to find the define of OPT_FLAGS in sdl.mak,sorry.
> 
> But mame fails to build on both ppc(Internal error, please submit a full bug
> report for gcc) and ppc64(should set PTR64=1 manually for 64bit arch).
> 
See http://koji.fedoraproject.org/koji/taskinfo?taskID=2065568
Comment 10 Orcan Ogetbil 2010-03-21 07:36:52 CET
(In reply to comment #9)
> See http://koji.fedoraproject.org/koji/taskinfo?taskID=2065568
> 

Please DO NOT build non-free or patent encumbered software in Fedora's koji.
Comment 11 Chen Lei 2010-03-21 08:52:21 CET
(In reply to comment #9)
> (In reply to comment #8)
> > > The spec seems perfect except some minor problems.
> > > 1.OPT_FLAGS='%{optflags}' is useless.
> > > You should export optflags in the %build section.
> > > export CCOMFLAGS = "%{optflags}"
> > > export CONLYFLAGS = "%{optflags}"
> > > export CPPONLYFLAGS = "%{optflags}"
> > I fails to find the define of OPT_FLAGS in sdl.mak,sorry.
> > 
> > But mame fails to build on both ppc(Internal error, please submit a full bug
> > report for gcc) and ppc64(should set PTR64=1 manually for 64bit arch).
> > 
> See http://koji.fedoraproject.org/koji/taskinfo?taskID=2065568
> 

I also recommand you to add VERBOSE = 1 in the build line to generate verbose build information, so that we can find useful information from rpmbuild output.
Comment 12 Chen Lei 2010-03-21 12:51:39 CET
Created attachment 395 [details]
ini files and manpages from mame 0.137 for ubuntu

contrib/vector.ini
contrib/mame.ini
contrib/jedutil.1
contrib/testkeys.1
contrib/ldverify.1
contrib/romcmp.1
contrib/chdman.1
contrib/mame.1
contrib/ldplayer.ini
contrib/ldplayer.1
Comment 13 Chen Lei 2010-03-21 13:38:52 CET
(In reply to comment #12)
> Created an attachment (id=395) [details]
> ini files and manpages from mame 0.137 for ubuntu
> contrib/vector.ini
> contrib/mame.ini
> contrib/jedutil.1
> contrib/testkeys.1
> contrib/ldverify.1
> contrib/romcmp.1
> contrib/chdman.1
> contrib/mame.1
> contrib/ldplayer.ini
> contrib/ldplayer.1
Whole diff file for ubuntu here:
http://sdlmame4ubuntu.org/cur/major/pool/unofficial/m/mame/mame_0.137-0ubuntu1~ww1~karmic.diff.gz
Notes: 
1.It may be better to export SUFFIX64="" at build line to make the consistency of name between 64 bit and 32bit fedora. Defining arch_flagsas you do in sdlmame is also recommended, because for some arches we have to define PTR64, BIGENDIAN, NOASM, FORCE_DRC_C_BACKEND manually(e.g. PRT64=1 for ppc64).


2.mame 0.137 for ubuntu move ctrlr from %{_datadir}/%{name}/ctrlr to /etc/%{name}/ctrlr several months ago (I'm not sure if it's the right place for ctrlr), See mame.ini.

Comment 14 Julian Sikorski 2010-03-21 14:34:40 CET
I asked upstream to include ppc64 detection a while ago, I guess they forgot. I think I can make a patch, I think. The problem is the lack of ppc64 machine to test, since as oget said, it's not allowed to use koji for that.
Same goes for internal error, I can't test it until the package is imported.

The main difference for inis is that they enable mouse by default, which I don't think is a particularly good idea - IIRC it gets enabled for gun games anyway.
When it comes to man pages, I'm against it, unless we find a way to generate them dynamically at build time. Otherwise, they are pretty much bound to get out of sync at some point, spelling trouble.

VERBOSE=1 could be added, sure, but this makes build logs _huge_. In the past I only enabled it when something went wrong. And I usually only changed @gcc to gcc.

Dropping suffix64 is a good idea, I don't think that anybody would want to install 32 and 64-bit versions of MAME in parallel.

Other arches are not supported anyway, and there is likely to be more trouble than just the wrong flags. In case someone wants to build mame on them, I'm happy to push this through RB/mamedev.

We can move the ctrlr files, but this has to be coordinated with frontend maintainers, so I'd rather wait after the package is imported.

Having said that:
Spec URL: http://homepage.hispeed.ch/belegdol/mame/mame.spec
SRPM URL: http://homepage.hispeed.ch/belegdol/mame/mame-0.137-3.fc13.src.rpm

Changes:
- Dropped suffix64
- Added ppc64 autodetection support
- Re-diffed the fortify patch
Comment 15 Chen Lei 2010-03-21 15:06:04 CET
formal review here:

 +:ok, =:needs attention, -:needs fixing

MUST Items:
[+] MUST: rpmlint must be run on every package.
<<output if not already posted>>
[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}
[+] MUST: The package must meet the Packaging Guidelines. [FIXME?: covers this
list and more]
[+] MUST: The package must be licensed with a rpmfusion approved license and meet
the Licensing Guidelines.
[+] MUST: The License field in the package spec file must match the actual
license.
[+] MUST: 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 must be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source,
as provided in the spec URL.
<<md5sum checksum>>1b70adfbe6aa35959881b82e48f93d82
[+] MUST: The package must successfully compile and build into binary rpms on
at least one supported architecture.
[+] MUST: If the package does not successfully compile, build or work on an
architecture, then those architectures should be listed in the spec in
ExcludeArch.
[+] MUST: All build dependencies must be listed in BuildRequires
[N/A] MUST: The spec file MUST handle locales properly. This is done by using the
%find_lang macro.
[N/A] MUST: Every binary RPM package which stores shared library files (not just
symlinks) in any of the dynamic linker's default paths, must call ldconfig in
%post and %postun.
[N/A] MUST: If the package is designed to be relocatable, the packager must state
this fact in the request for review
[+] MUST: A package must own all directories that it creates. If it does not
create a directory that it uses, then it should require a package which does
create that directory.
[+] MUST: A package must not contain any duplicate files in the %files listing.
[+] MUST: Permissions on files must be set properly. Executables should be set
with executable permissions, for example. Every %files section must include a
%defattr(...) line.
[+] MUST: Each package must have a %clean section, which contains rm -rf
%{buildroot} (or $RPM_BUILD_ROOT).
[+] MUST: Each package must consistently use macros, as described in the macros
section of Packaging Guidelines.
[+] MUST: The package must contain code, or permissible content. This is
described in detail in the code vs. content section of Packaging Guidelines.
[N/A] MUST: Large documentation files should go in a doc subpackage.
[+] MUST: If a package includes something as %doc, it must not affect the
runtime of the application.
[N/A] MUST: Header files must be in a -devel package.
[N/A] MUST: Static libraries must be in a -static package.
[N/A] MUST: If a package contains library files with a suffix (e.g.
libfoo.so.1.1), then library files that end in .so (without suffix) must go in
a -devel package.
[N/A] MUST: In the vast majority of cases, devel packages must require the base
package using a fully versioned dependency: Requires: %{name} =
%{version}-%{release} 
[+] MUST: Packages must NOT contain any .la libtool archives, these should be
removed in the spec.
[N/A] MUST: Packages containing GUI applications must include a %{name}.desktop
file, and that file must be properly installed with desktop-file-install in the
%install section.
[+] MUST: Packages must not own files or directories already owned by other
packages.
[+] MUST: At the beginning of %install, each package MUST run rm -rf
%{buildroot} (or $RPM_BUILD_ROOT).
[+] MUST: All filenames in rpm packages must be valid UTF-8.

SHOULD Items:
[N/A] SHOULD: If the source package does not include license text(s) as a
separate file from upstream, the packager SHOULD query upstream to include it.
[N/A] SHOULD: The description and summary sections in the package spec file
should contain translations for supported Non-English languages, if available.
[+] SHOULD: The reviewer should test that the package builds in mock.
[+] SHOULD: The package should compile and build into binary rpms on all
supported architectures.
[+] SHOULD: The reviewer should test that the package functions as described.
[=] SHOULD: If scriptlets are used, those scriptlets must be sane.
[+] SHOULD: Usually, subpackages other than devel should require the base
package using a fully versioned dependency.
[N/A] SHOULD: The placement of pkgconfig(.pc) files depends on their usecase,
and this is usually for development purposes, so should be placed in a -devel
pkg. A reasonable exception is that the main pkg itself is a devel tool not
installed in a user runtime, e.g. gcc or gdb.
[+] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin,
/usr/bin, or /usr/sbin consider requiring the package which provides the file
instead of the file itself.
[+] SHOULD: Packages should try to preserve timestamps of original installed
files.    
Comment 16 Chen Lei 2010-03-21 15:11:13 CET
------------------------------
  This package is APPROVED 
------------------------------ 

Should we also create a ldplayer.ini for -ldplayer subpackage as ubuntu's package?

Comment 17 Julian Sikorski 2010-03-21 15:20:19 CET
No need, it uses mame.ini already.
Comment 18 Julian Sikorski 2010-03-21 15:22:27 CET
Package CVS request
======================
Package Name: mame
Short Description: Multiple Arcade Machine Emulator
Owners: belegdol
Branches: F-12 F-11
InitialCC:
----------------------
License tag: nonfree
Comment 19 Chen Lei 2010-03-21 15:29:19 CET
(In reply to comment #17)
> No need, it uses mame.ini already.

From http://mamedev.org/devwiki/index.php/FAQ:Tools, I found "while ldplayer uses the same default inipath as MAME, it looks for ldplayer.ini instead of mame.ini. To preserve your video and other options, make a copy of your mame.ini file named ldplayer.ini". 


mess 0.137 also released, can it be compiled on fedora and replace sdlmess?
See http://mess.redump.net/compiling_mess
Comment 20 Julian Sikorski 2010-03-21 15:31:31 CET
It seems to use mame.ini, I just checked. And yes, I plan to package sdlmess.
Comment 21 rc040203 2010-03-21 15:32:36 CET
(In reply to comment #14)

> VERBOSE=1 could be added, sure, but this makes build logs _huge_.
Wrong ... VERBOSE=1 is a *MUST*

> In the past I
> only enabled it when something went wrong.
Such as in your case: 

Building on ppc64 goes wrong, but your logs don't provide sufficient 
infomation allow investigation.


(In reply to comment #16)
> ------------------------------
>   This package is APPROVED 
> ------------------------------ 

I hereby revoke the package approval, because 
non-verbose makes are a violation of the FPG.
Comment 22 Julian Sikorski 2010-03-21 15:46:17 CET
VERBOSE=1 adds -v to gcc, I don't think that's a must. How about stripping the @:
Spec URL: http://homepage.hispeed.ch/belegdol/mame/mame.spec
SRPM URL: http://homepage.hispeed.ch/belegdol/mame/mame-0.137-4.fc13.src.rpm

Changes:
- Stripped @ from the commands to make the build more verbose
Comment 23 Julian Sikorski 2010-03-23 20:50:44 CET
Ralf, is the package good to go?
Comment 24 Chen Lei 2010-04-02 07:06:40 CEST
(In reply to comment #23)
> Ralf, is the package good to go?

Maybe you need to contact cvs admin directly. The package is OK for replacing sdlmame in rpmfusion.