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

[RPM Fusion Bugzilla]

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





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

What an awful grammar. Should be: “Besides, if the Source0 URL were
incorrect, the package wouldn'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.