[Bug 15] Review Request: bsnes - SNES emulator focused on accuracy

[RPM Fusion Bugzilla]

http://bugzilla.rpmfusion.org/show_bug.cgi?id=15





--- Comment #42 from David Timms <dtimms at iinet.net.au>  2008-11-30 15:19:42 ---
(In reply to comment #41)
> Spec URL: http://belegdol.republika.pl/rpmstuff/bsnes.spec
> SRPM URL: http://belegdol.republika.pl/rpmstuff/bsnes-0.037a-4.fc10.src.rpm
> 
> Changes:
> - Fixed README.Fedora permissions
> - Added information concerning pulseaudio issues
Done as requested.

Full review:

[ OK] rpmlint is clean:
$ rpmlint  /home/davidt/rpmbuild/SRPMS/bsnes-0.037a-4.fc9.src.rpm
/home/davidt/rpmbuild/RPMS/i386/bsnes-0.037a-4.fc9.i386.rpm
/home/davidt/rpmbuild/RPMS/i386/bsnes-debuginfo-0.037a-4.fc9.i386.rpm
3 packages and 0 specfiles checked; 0 errors, 0 warnings.

[ OK] name meets guidelines
[ OK] spec filename is correct
[ OK] meets package quidelines
[ OK] license is non-free => rpmfusion not fedora
[ OK] license field matches upstream redistributable, no mods.
[ OK] license text file is included
[ OK] spec file is legible, easy to understand

[ x ] source URL is incorrect:
  think it should be: ...%{name}_v0.%{vernumber}...
  upstream calls it 037a but compresses source to v0.037a

[ OK] upstream md5sum matches .src.rpm
$ md5sum bsnes_v037a.tar.bz2 
9fa2fbae8a09a747f9b4a44123bde0bb  bsnes_v037a.tar.bz2
$ md5sum bsnes-0.037a-4.fc10.src/bsnes_v037a.tar.bz2 
9fa2fbae8a09a747f9b4a44123bde0bb  bsnes-0.037a-4.fc10.src/bsnes_v037a.tar.bz2

[ OK] rpmbuild binary rpms built.

[ x ] ExcludeArch not used in favour of ExclusiveArch:  i386 x86_64
  is there no possibility of this working on ppc etc. If so, I believe a
comment
  above this field to briefly indicate why it's x86 only, could be "application
uses x86 CPU machine code, so needs an x86 CPU" {if that is the reason}.
  [Not sure if we want to make a bugzilla/block tracker bug - depends on the
reason why no ppc/other architectures]

[ OK] BuiildRequires are specified.

[ ? ] No localization is provided in the main source. I am not sure what work
would be required to use the available individual locale.cfg files
automatically. It seems you would simply copy a single one of them into the bin
dir as locale.cfg, and bsnes would only use that translation ?

[ NA] no shared libraries are compiled or installed
[ OK] No relocatable (Prefix: is not used)
[ OK] owns directories it creates, disowns other dirs.
[ OK] no duplicates in the %files stanza
[ OK] permissions defattr included, other files have their perms fixed in %prep
[ OK] %clean matches required content
[ OK] code versus content: this is a hardware emulator, no roms included.
[ OK] doc is small -> no separate -doc package
[ OK] no %doc files are not required at runtime
[ NA] no header files, no static libraries, no pkgconfig, no libaries, 
      no -devel, no libtool archives.
[ OK] gui app and has legit .desktop file, installed by desktop-file-install,
    and BRs: desktop-file-utils
[ OK] source filenames appear to be valid utf-8
[ OK] package functions as described.
[ ? ] an icon provided at /usr/share/icons/, yet the GTK+ icon cache update
scriptlet isn't used - why not ? why was it removed when it was previously
included ?

[ ? ] (In reply to comment #8)
> It seems that bsnes is using its own versions of system libraries. This should
> be avoided. 
    my opinion on use of included rather than system library for snes_ntsc is 
    it would be a lot of effort, and the included version calculates the values
    differently, so we would need to decide if those changes could get pushed
    upstream. Also given byuu's opinion on the zlib that you worked on already,
    it would be unlikely to be included upstream.

Julian: I think we are really close here, just a few items above {x and ?} to
either fix or provide reasoning for. Reading back over the review, it seems you
have completed pretty well all requests reviewers have made, beside the "make
it use all system libaries". If there are other reviewers who believe further
work should be done on the "use system libraries" item, can you please speak up
now ?


-- 
Configure bugmail: http://bugzilla.rpmfusion.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.