Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=664390 Steve Traylen <steve.traylen@xxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |steve.traylen@xxxxxxx --- Comment #3 from Steve Traylen <steve.traylen@xxxxxxx> 2011-03-03 14:40:50 EST --- Extra items to the informal review. 1) Requiring paths is genrally not good since it requires a whole load of extra data to be downloaded during a yum update. Requires: /usr/bin/wbinfo If the package name changes between distributions then use an OS conditional. 2) Inconsistant use of macros. %{_bindir} is used in some places where as %{_prefix}/bin is used elsewhere. I think %{_bindir} is better but I am not not sure why. 3) When installing files by hand e.g. install -m 755 redhat/init.d %{buildroot}%{_initrddir}/samsd Add a "-p" option to preserve the time. 4) You use chrpath, does the other sed option on libtool not work? It normally does. 5) You called the package sam rather than sam2 as the tar ball is called and the application seems to be called. Your choice though. 6) For the sake of the informal reviewer add a comment to the patch Patch0: sams2.shebang.patch 7) You have Requires: pcre Requires: unixODBC but these are not needed since they are added with libpcre.so.0()(64bit) and libodbc.so.2()(64bit) remove the requires that are superflous. 8) The /usr/lib64/libfsusage.so.1 /usr/lib64/libfsusage.so.1.0.0 /usr/lib64/libloadavg.so.1 /usr/lib64/libloadavg.so.1.0.0 files in the -devel package are run time files, they can't be built against can they? So belong in the main package ? But it's odd nothing in the main package actually uses them? When if ever are these libs used? I'd have to look some more. 9) You will need to do a trick with the Release since this is an rc1 release so see http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Pre-Release_packages for how to handle this. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review