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=693425 Matthias Saou <matthias@xxxxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |matthias@xxxxxxxxxxxx --- Comment #62 from Matthias Saou <matthias@xxxxxxxxxxxx> 2011-08-13 09:09:40 EDT --- Here are a few minor nitpicks about the 6.0.2-6 spec files : * Big spaces vs. tabs indentation mess, which denotes a general lack of attention to detail. * Included Patch1000 isn't applied. No indication as of why. * No %changelog entry for 6.0.2-6. * Maybe the first line should read something like "simplified" and not "crippled". * Some non-relevant leftovers which should be taken care of, such as "perhaps the matplotlib could yield for pytz, in Mdv >=2009.0", "Suggests: postgresql-server >= 8.2", "I don't understand why this is needed at this stage" and "Hope that the upstream one will do". * Lack of consistency in %description URLs (trailing "/"). * Lack of consistency in section separation (why 2 empty lines between %prep and %patch?). * My, oh my! I hadn't seen that "urban legend"-ish line for a long time : "[ -n "%{buildroot}" -a "%{buildroot}" != / ] && rm -rf %{buildroot}". Remove the check, just rm : The buildroot can't ever be "/"... and it's in %clean already anyway. * The few explicit "/" after %{buildroot} aren't needed (again, consistency issue), as in %{buildroot}/%{_bindir} which should be %{buildroot}%{_bindir}. * Lots of consistency problems in file modes. The worst being server-check.sh installed with mode 744 in %install but being included with mode 755 in %files : Very confusing. * Consistency problems in scriplets. For instance once "|| :" is used, but then "exit 0" is also used. Once $1 is quoted, once it's not... * Consistency problems in %files : openerp-server.* (no number) vs. openerp_serverrc.5* (number). Overall, the spec file could be much cleaner, making it much easier to read, understand and review. Blocker : The /var/run/ content needs to be handled differently. See http://fedoraproject.org/wiki/Packaging:Tmpfiles.d to learn how. Doubt that I have : Do the main configuration directory and the main configuration file really need to belong and be writeable by the openerp user? If the configuration can be saved at run time, then yes, but otherwise if it's only meant to be modified by an administrator it would be best avoided : If some vulnerability in the daemon process allows to dump content to any file, it would be trivial to overwrite the configuration, then possibly do something very nasty upon the next restart. -- 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