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

[RPM Fusion Bugzilla]

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





--- Comment #43 from Julian Sikorski <belegdol at gmail.com>  2008-11-30 15:47:11 ---
(In reply to comment #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

I'm not sure what you mean here, that's how the upstream tarball is called (I'm
aware it uncompresses to a folder of slightly different name. Besides, if the
Source0 URL would be incorrect, the package won't build, would it? 

> [ 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, I'll add an explanation then. The reason is that libco only supports these
two 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 ?

AFAIK, bsnes can use only one locale.cfg at a time, so packaging several at
once seems impossible.

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

It was removed because the icon was installed to datadir/pixmaps at the time
upstream was not installing it at all. I pointed out to byuu that datadir/icons
is a place for themed icons and bsnes one should go to datadir/pixmaps, but
that was left without response. Also, packaging guidelines are only saying
about subdirs of datadir/icons.

> [ ? ] (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.