[Bug 46] Review request: rt2870-kmod - Kernel module for wireless devices with rt2870 chipsets

[RPM Fusion Bugzilla]

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


Thorsten Leemhuis <fedora at leemhuis.info> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fedora at leemhuis.info
         AssignedTo|rpmfusion-package-          |fedora at leemhuis.info
                   |review at rpmfusion.org        |




--- Comment #2 from Thorsten Leemhuis <fedora at leemhuis.info>  2008-09-26 18:11:11 ---
First: thx for packaging this driver; this work is highly appreciated. Some
initial review comments.

= common =

 * That yelling of product names in the description looks odd and annoying.
Further: such lists are hard to keep up2date and are often misleading,
incomplete or partly wrong. Thus better remove the whole product names section
completely 

= rt2870 =

 * if you use things only once then it doesn't make much sense to define a
macro for them:
 >%define SourceDir [...]
 >%define ReleaseNote [...]

 * something like "Common files for RaLink 802.11 rt2860 kernel modules" would
IMHO make more sense as summary

 * using %{SourceDir} is bad; instead of
 > cp %{SOURCE1} %{SourceDir}
 simply use something like
 > cp -p %{SOURCE1} .
 all the other usage of %{SourceDir} is afaics unneeded as well... 

 Hint to avoid things like this: download some spec files from Fedora (maybe
even all of them using the daily checkout; see
http://fedoraproject.org/wiki/Using_Fedora_CVS ); skimm through those and look
how others did it or solved problems that you run into. 

 * I'd say that everything that is in the %build section should be in the %prep
section

 * the command
 > mkdir -p $RPM_BUILD_ROOT
 in the install section likely is unneeded

 * why
 > %defattr(0644,root,root,0755)
 instead of the usual
 > %defattr(-,root,root,-)

= rt2870-kmod =

Didn't look at it yet; please recheck if any of the comments I made above might
be relevant for this package as well 


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