[Bug 440] Review request: vbam - Nintendo Gameboy, Gameboy Color, and Gameboy Advance Emulator

[RPM Fusion Bugzilla]

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





--- Comment #5 from Dave Straub <dave at is1337.net>  2009-03-16 20:08:23 ---
(In reply to comment #4)
> Some comments:
> 
> * You should specify a version to checkout from upstream VCS. Otherwise the
> reviewer cannot be sure to compare/recreate the same exact version you used.
> Place this details above the Source tag.
> https://fedoraproject.org/wiki/Packaging/SourceURL#Using_Revision_Control

The revision number was specified as outlined here:
https://fedoraproject.org/wiki/Packaging/NamingGuidelines#SnapshotPackages
I added the svn comment above the Source tag rather than a wget at the top.

> * Some BR are not required:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2

They were specified more to remind myself I need them when using a live CD to
test a clean build. I removed them if they are troublesome to include.

> * Do not strip binaries! Otherwise the debuginfo package is useless:
> https://fedoraproject.org/wiki/Packaging/Debuginfo

I forgot to install redhat-rpm-config at first, so rpmlint was angry at the
unstripped files. This is fixed.

> * Avoid using dos2unix. Use sed instead:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Rpmlint_Errors

Done.

> * I see that you use the %{__xxx} macro style. Even though this is not a bug
> and it is not required that you change it, I suggest you not to use this style
> because it is not very readable. I cannot think of a good reason to use it
> instead of just using "xxx".

I removed the macros.

> * The icon cache scriptlets are not the same used by the Fedora guidelines:
> http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache

I didn't know they were required to be the same; I'll use their version now.

> I haven't checked every requirements but the spec file is quite good as a first
> release from a rookie :)
> 
> Unluckily I'm not a sponsor and therefore I cannot sponsor you. I suggest you
> to do "informal" reviews of other packages to show your understanding of Fedora
> guidelines to potential sponsors.
> 

Thanks for the corrections. Let me know if you notice anything else. I will
have a look around other review requests.

Updated Spec: http://dave.is1337.net/sources/vbam.spec
Updated SRPM:
http://dave.is1337.net/sources/vbam-1.8.0-0.2.20090316svn856.fc10.src.rpm


-- 
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.
You are the assignee for the bug.