[Bug 693425] Review Request: openerp - OpenERP business application

[bugzilla at redhat.com]

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

--- Comment #44 from Alec Leamas <leamas.alec at gmail.com> 2011-04-26 09:54:41 EDT ---
As for server, the source rpm is missing. Reviewing based on old source +
updated spec file.

MUST

- rpmlint must be run on the source rpm and all binary rpms...
  + Should be run on 4 files, not 2; see server review. Also, please separate
output from server and client.
  + Fix changelog version; see server.
  + obsolete-not-provided: Presuming that openerp is incompatible w tinyerp,
this could be ignored.

- The spec file name must match the base package %{name}.... NOK
  + The name should be openerp-client.spec; see server review.

- The package must meet the Packaging Guidelines. TBD
  + Some deps listed in setup.py are missing in spec file. Is this as intended?

- The package must be licensed with a Fedora approved license...: OK

- The License field in the package spec file must match the actual license: NOK
  + No overall copyright clause in README or separate copyright file,
acceptable but "odd".
  + License: field is AGPLv3, README.txt says all files are GPL.
  + mydistutils.py is GPLv2+
  + GUI (openerp.glade)  talks about GPLv3
  + msgfmt.py has a Tiny SPRL copyright, but is *very* similar to msgfmt.py as
of python-tools, which has another author reference but no copyright info.
Since the copyright is questionable, I suggest that the file is removed and
build depends on python-tools instead. 
See http://fedoraproject.org/wiki/Packaging:LicensingGuidelines

- Separate License file must be in %doc...: OK

- The spec file must be written in American English: TBD
  + Being a Swede, I really don't know. Looks fine to me, though :)

- The spec file for the package MUST be legible: OK

- The sources used to build the package must match the upstream source...TBD
- The package MUST successfully compile and build into binary rpms...: TBD
- All build dependencies must be listed in BuildRequires...TBD
   + No source submitted
   + gettext build req missing; see
http://fedoraproject.org/wiki/PackagingGuidelines#Handling_Locale_Files

- The spec file MUST handle locales properly...: OK

- Packages must NOT bundle copies of system libraries: NOK
   + contains bundled SpiffGtkWidgets

- A package must own all directories that it creates. OK
- A Fedora package must not list a file more than once in %file lists...: OK
- Permissions on files must be set properly...: OK

- Each package must consistently use macros : NOK
  + Defining macros prefixed w _, like _iconsdir,  is bad practise, reserved
for internal use. 

- The package must contain code, or permissable content: OK
- %doc must not affect the runtime of the application..: OK
- Header files must be in a -devel package
- Packages containing GUI applications must include a .desktop file...: OK
- Packages must not own files or directories owned by other packages...OK
- All filenames in rpm packages must be valid UTF-8.: OK

SHOULD:

- Package built on koji/mock. TBD
- Testing...
  No source submitted

- Scriptlets should be sane...: NOK
  + Icons are not handled properly, use of _iconsdir, no icon cache mgmt,
possibly odd icon location. See
https://bugzilla.redhat.com/show_bug.cgi?id=495412 and
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#Icon_Cache
  + Since postun requires desktop-file-utils, the test for
.../update-desktop-database is not required. OTOH, it's wise to add a '|| : '
just in case these tools return an error code.

OTHER REMARKS:
- The for BIN in... loop could be be written in one line using 'sed -i'.
- The copyright info in debian/copyright is outdated, SpiffGtkWidtgets is now
AGPL, other updates.
- The files debian/ and setup.* should be removed; they make no sense in a
Fedora package.

-- 
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.