Bug 693425 - Review Request: openerp - OpenERP business application
Review Request: openerp - OpenERP business application
Status: CLOSED WONTFIX
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
: 641261 641271 677639 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-04 11:55 EDT by Panos Christeas
Modified: 2012-04-20 16:24 EDT (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-04-20 16:24:28 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
Informal review , first step (4.96 KB, text/plain)
2011-04-08 04:06 EDT, Alec Leamas
no flags Details
openerp-server.spec, another variant for reference. (4.39 KB, text/plain)
2011-04-08 07:50 EDT, Alec Leamas
no flags Details
openerp-client.spec, another variant for reference (2.45 KB, text/plain)
2011-04-08 07:51 EDT, Alec Leamas
no flags Details
openerp-web.spec, another variant for reference (2.71 KB, text/plain)
2011-04-08 07:52 EDT, Alec Leamas
no flags Details
rpmlint check output (1.58 KB, text/plain)
2011-04-21 17:24 EDT, Panos Christeas
no flags Details
Client spec, as in 6.0.2-5 (7.70 KB, text/plain)
2011-04-21 17:27 EDT, Panos Christeas
no flags Details
Server spec, as in 6.0.2-5 (10.10 KB, text/plain)
2011-04-21 17:29 EDT, Panos Christeas
no flags Details

  None (edit)
Description Panos Christeas 2011-04-04 11:55:52 EDT
Spec URL: http://members.hellug.gr/xrg/Redhat/openerp-official/SPECS/openerp-redhat.spec
SRPM URL: http://members.hellug.gr/xrg/Redhat/openerp-official/SRPMS/openerp-6.0.2-2.src.rpm
Upstream URL: http://www.openerp.com
Gitweb URL: http://git.hellug.gr/?p=xrg/openerp
Related to Bugs: 641271, 641261, 677639

Description: OpenERP is a free Enterprise Resource Planning and Customer Relationship Management software. Developed fully under the Affero GPLv3 and appreciated by a large community (open source).

At this attempt, we (OpenERP SA.) request that packages for version 6.0 are reviewed for Fedora and RedHat. Once mentored, we plan to officially maintain our software for inclusion in Fedora/RedHat systems.

First (personal) packaging request for Fedora, need a sponsor, please!
Comment 1 Jason Tibbitts 2011-04-04 12:09:59 EDT
How does this differ from the several other openerp-related reviews which already exist?

https://bugzilla.redhat.com/buglist.cgi?short_desc=openerp&short_desc_type=allwordssubstr&component=Package+Review&query_format=advanced
Comment 2 Panos Christeas 2011-04-04 13:13:51 EDT
This time we are the publisher of the software (OpenERP SA., I work there) and thus it's an implicitly "upstream" packaging.

The spec file originates from the Mandriva one, which has been working in production since 2008. I've cleaned most of it (it used git-rpm build methodology, converted to static now) and try to apply the packaging guidelines to the official branch.
Comment 3 Jason Tibbitts 2011-04-05 15:06:01 EDT
The point is that if there's an existing package review, you should work with the people who submitted it.  You shouldn't just submit another review ticket for the same thing.  It doesn't matter that you're the publisher; you shouldn't just ignore the work that others have already done.

I should close this ticket as a duplicate, but I'll wait to see what the other submitters would prefer to do.  I will leave it up to you to communicate with them, however.
Comment 4 Alec Leamas 2011-04-07 04:59:03 EDT
OK, so far three of us have open requests for this one: Andrea V has bug #641271 and bug #641261), Alec (that's me) has bug #677639 and Panos Christeas has bug #693425. That is, a mess.

As for Andrea's bugs, he has declared that he has no time to work with this. Also, his work is now outdated w r t current Fedora and OpenERP versions. I thus suggest that his two bugs are closed.

This would leave my and Panos's request. One way to handle this would be to close my request and that I instead make an informal review of Panos's one. As long as all agree, I'm willing to do my part of this to the best of my limited ability. 

Other ideas?
Comment 5 Panos Christeas 2011-04-08 03:31:07 EDT
Alec Leamas: that would be ideal for me.

Please note that I /do/ expect corrections to be suggested on my proposal. Let's make some packaging that will install this beast and let it run out of the box!
Comment 6 Alec Leamas 2011-04-08 04:06:35 EDT
Created attachment 490726 [details]
Informal review , first step

Somewhat long review. This is still *very* informal, hopefully it can enter a more structured (and smaller) state at next iteration.
Comment 7 Panos Christeas 2011-04-08 04:29:32 EDT
Review acceptable. 
Expect next ver. of SPEC file(s) pretty soon.

Draft Notes (answers):
The wrong permissions should be fixed upstream, inside the tarballs. Trying to push that change there.

There has been an alternative approach, of per-addon rpms, which would solve the complicated dependencies problem. See the 'modulize.py' script at my git repos.

Hard dependency on Postgres shall only occur with the "serverinit" subpackage. Is there an objection? Will fix init script, however.


Brb..
Comment 8 Alec Leamas 2011-04-08 05:38:16 EDT
*** Bug 677639 has been marked as a duplicate of this bug. ***
Comment 9 Panos Christeas 2011-04-08 07:25:46 EDT
> Basically, what I miss is a INSTALL (INSTALL.fedora?) file. I envision a 
> simple doc, something like

> INSTALL  ON A SINGLE HOST
>...
> Create a openerp database user:
>     # /etc/init.d/openerp-server  db-add-user
> Make a basic test
>     # /etc/init.d/openerp-server  db-test
> Install certificate. If you already have a server certificate:
>     # /etc/init.d/openerp-server certificate-install /path/to/certificate
> If you don't have a certificate, openerp can create a self-signed one 
> for you:
>     # /etc/init.d/openerp-server certificate-create
> Start openerp-server:
>      # service openerp-server start
>... (snip)

Certainly interesting.

The purpose of the -serverinit subpackage was this (it is an old concept):
   Suppose we want to provision a system (or image ;) ) with a default, ready 
to work, installation of OpenERP. This cannot involve any manual configuration 
steps by root/postgres/openerp user. It must just start, boot, and end up with 
an openerp client, from which the "admin" user will be able to create his first 
database.
   I appreciate that assumptions are not welcome in RPM packaging. That's why 
I have isolated these steps into the 'serverinit' package. 

So, at the end, you will have either a set of "post-install" steps (that will 
let you chose the db server, certificate etc), or an automated meta-package.

In the meanwhile, I agree to move as much as possible inside the initscript (I 
didn't know that extra steps apart from start/stop/status were welcome in 
Fedora).
Comment 10 Alec Leamas 2011-04-08 07:50:45 EDT
Created attachment 490768 [details]
openerp-server.spec, another variant for reference.
Comment 11 Alec Leamas 2011-04-08 07:51:36 EDT
Created attachment 490769 [details]
openerp-client.spec, another variant for reference
Comment 12 Alec Leamas 2011-04-08 07:52:32 EDT
Created attachment 490770 [details]
openerp-web.spec, another variant for reference
Comment 13 Alec Leamas 2011-04-08 08:29:44 EDT
(In reply to comment #9)

OK, as I get it, there are some variants

1) User already knows what to do, and installs openerp-server-init.

2) User doesn't know what to do, installs openerp-server. She reads INSTALL, and there are two options:
   - Iff "a lot of conditions", just install openerp-server-init to complete installation.
   - Else,  use the following steps to complete install "Manual steps".

One problem is that guidelines recommends against setting start level (chkconfig) in spec file, and strongly advices against actually starting the service(s). So some manual steps seems to be required anyway.

Personally I'm happy as long as there is an INSTALL, and the package doesn't have a verb as name :)

> 
> So, at the end, you will have either a set of "post-install" steps (that will 
> let you chose the db server, certificate etc), or an automated meta-package.
> 
> In the meanwhile, I agree to move as much as possible inside the initscript (I 
> didn't know that extra steps apart from start/stop/status were welcome in 
> Fedora).
Comment 14 Panos Christeas 2011-04-08 10:32:12 EDT
About the %{NoDisplay} macro:
Some months/years ago, we did have the case that the shebang was missing from
python scripts. Usually, the first command to appear on a .py file is:
  rpm -q -f $(which import) # ;) guess what it does..

So, I had added 'define %{NoDisplay} DISPLAY= ' to block that case and make
sure we have an error in such a case.

WDYT? Do we still need to enforce such a rule? Or clean up the %{NoDisplay} references?
Comment 15 Jason Tibbitts 2011-04-08 10:34:43 EDT
Please don't attach reviews, thanks.  Just include them inline so we can actually see them.
Comment 16 Jason Tibbitts 2011-04-08 10:35:56 EDT
*** Bug 641261 has been marked as a duplicate of this bug. ***
Comment 17 Jason Tibbitts 2011-04-08 10:36:14 EDT
*** Bug 641271 has been marked as a duplicate of this bug. ***
Comment 18 Alec Leamas 2011-04-08 10:50:55 EDT
(In reply to comment #14)

> WDYT? Do we still need to enforce such a rule? Or clean up the %{NoDisplay}
> references?

Thx for explanation. I think the %{NoDisplay} should be cleaned up. Legibility, don't hide other bugs...
Comment 19 Alec Leamas 2011-04-08 10:54:01 EDT
(In reply to comment #15)
> Please don't attach reviews, thanks.  Just include them inline so we can
> actually see them.

I will, as soon as it's not a multi-page document, hopefully at next iteration :)
Comment 20 David Nalley 2011-04-09 23:30:21 EDT
Just a fair warning - you have at least two (I really quickly ran over the source, there may be many more that I didn't find) bundled libraries in source. These will need to be broken out and packaged separately (actually they may already exist, which will make your job much easier) 

http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries

You've also got a VERY complex spec file, especially with it being the first package you've submitted for review, and I question the need for a lot of that complexity. 

Also, keep in mind that the purpose of the spec file is to install the software, not to install and configure, using chkconfig to manipulate other packages and other things like that are frowned upon. 

Sorry for seeming all negative, I really am excited about openERP dedicating the resources to do this work, and I'll be happy to help you get it in Fedora/EPEL.
Comment 21 Panos Christeas 2011-04-11 03:08:04 EDT
ACK.

The remarks are being considered, as can be seen at:
http://git.hellug.gr/?p=xrg/openerp
Comment 22 Panos Christeas 2011-04-12 05:01:40 EDT
A little update of the ongoing progress:
Most of the remarks of this "bug" thread have been honoured in the spec file, as can be seen in the git repo.
I still have 3 points, please advise:
  1. IMHO it's better to have 1x spec file for both client and server (and any meta-packages, if needed). Is that such a bad practice? Versioning, according to company policy _is_ the same for both sources. We don't do individual releases.
  2. I'd prefer to abandon the old statically allocated user id = 13 for tinyerp and instead follow modern guidelines for an 'openerp' user.
  3. Unbundling of SpiffGtkWidgets is not immediately feasible. We do have enough custom code[2] in there to say[1] that we can't use upstream. OTOH, fortunately, I had already formed a Git branch with our work, and published to GitHub. This means, I can push for merging[3], although we may not have the changes and repackaging of the upstream project in time. That said, I definitely see that as a _temporary_ request, pending unbundling of the library as soon as possible.

The rpmlint errors (about file permissions etc.) are not all cleared yet, because I wait our main repo (of code) to publish a new tarball with the fixes. I'm trying to put as many of the necessary improvements in the code, rather than the .spec .

Thanks for your attention.

[1] switching to "upstream" will need some time just because of testing, in any case. 
[2] yes, you can call it "forking on laziness", it's our fault.
[3] this would also involve a series of iterations so that other projects won't be affected by the changes we may introduce to SpiffGtkWidgets.
Comment 23 Alec Leamas 2011-04-12 13:01:41 EDT
(
> I still have 3 points, please advise:
>   1. IMHO it's better to have 1x spec file for both client and server (and any
> meta-packages, if needed).

I don't think the combined server/client approach will pass the formal review. My own personal opinion is the same. The arguments:
- Using the combined approach, the it's not possible to meet the Naming Guidelines.
- The client and the server package does not depend on each other in any way from installation point of view.
- Using sub-packages this way is not as intended and implies more or less two sub-packages and an empty base package. Current main package is just one file server/README, which sort of says it all.
- The code becomes cluttered with if-defs in dependencies and pushd/popd in a lot of code, which makes it less legible.
- The combined approach does not scale to the web client or other stuff.

Bottom line: Separate spec file for each source is so much easier that it justifies the extra lines to maintain. My opinion is also that it's a requirement to pass the formal review.
Comment 24 Alec Leamas 2011-04-12 13:05:17 EDT
>   2. I'd prefer to abandon the old statically allocated user id = 13 for
> tinyerp and instead follow modern guidelines for an 'openerp' user.
That should not be a problem, the review remark was just a question.
Comment 25 David Nalley 2011-04-12 13:25:43 EDT
>   3. Unbundling of SpiffGtkWidgets is not immediately feasible. We do have
> enough custom code[2] in there to say[1] that we can't use upstream. OTOH,
> fortunately, I had already formed a Git branch with our work, and published to
> GitHub. This means, I can push for merging[3], although we may not have the
> changes and repackaging of the upstream project in time. That said, I
> definitely see that as a _temporary_ request, pending unbundling of the library
> as soon as possible.

You'll need to get an exception from FPC for this:
http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries#Exceptions
Comment 26 Alec Leamas 2011-04-12 13:31:34 EDT
> You'll need to get an exception from FPC for this:
> http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries#Exceptions

Basically, I doubt you will get an exception for this (?)

I suggest to handle it this way:

1) Try to go back to the upstream package. I understand this will take time,
possibly pushing some patches upstream and run tests. However, in the long run
this is the Right Thing for all of us.
2) In the meantime, let's focus on the server spec file. See, yet another
argument to split the files :)
Comment 27 Jason Tibbitts 2011-04-12 14:38:58 EDT
FPC is generally reasonable when it comes to granting exceptions.  If you provide answers to all the standard questions, did not fork capriciously and have a reasonable plan for dealing with (and preferably eliminating) the fork in the relatively near future (or a really good reason why that is not possible) then things will probably move forward.
Comment 28 Alec Leamas 2011-04-14 07:19:22 EDT
A fast scan for licenses reveals some more libs:
ftpserver, possibly replaced by existing pyftpdlib.
rml2pdf, possibly replaced by existing python-trml2pdf.
Comment 29 Panos Christeas 2011-04-19 07:49:04 EDT
@Alec:
I remember the pyftpdlib case, I did hack our bundled version myself. In short, we had to alter several places of that code to make an ftp server of our virtual documents (pyftpdlib was mostly about serving real files). I think we could either admit that our case is a hacked fork or drop the FTP suport (the "document_ftp" module). I have a WiP branch of porting to latest pyftpdlib and cleanup, but know that I cannot promise an unbundling of that part soon enough.

The case about "trml2pdf" is quite reverse: that library, is actually ours (Fabien Pinkaers wrote it in the first place, 't' stands for Tiny). So, we now have to go back to the community that have forked it and try to put everything back together. I may need help from you, guys, to locate the forks of trml2pdf and try to blend them in.
Comment 30 Alec Leamas 2011-04-19 12:53:28 EDT
trml2pdf is at  http://www.satchmoproject.com/snapshots/  Getting the spec file: yum install yumdownloader; yumdownloader --source python-trml2pdf; rpm2cpio python-trml2pdf-*.src.rpm | cpio -i.
Comment 31 Alec Leamas 2011-04-19 13:28:29 EDT
As far as I can see, there are four things which are beyond just fixing the spec file:

- pyftpdlib: Suggestion: apply for an exception, based on the changes you have made which are required but unusable for others.
- trml2pdf: Suggestion: apply for an exception, based on that you are upstream for the Fedora package (a strange situation, indeed) and will try to merge w current Fedora maintainer
- SpiffGTkWidgets: Suggestion: apply for an exception, based on a plan for unbundling.
- The OpenERP Public License, which has to either be replaced w other license(s) or approved, see http://fedoraproject.org/wiki/Licensing:Main#Unknown_Licenses

One option might be to first focus on the license and trm2pdf; with these issues solved it should be possible to package a working server (w/o ftp lib).
Comment 32 Panos Christeas 2011-04-20 02:54:26 EDT
pyftpdlib: agreed, although theoretically our changes could be applied upstream w/o breaking the others (I tried to code them carefully). However, they are against previous ver., so we'd need to work merging them.

trml2pdf: an exception should be granted. Saw the same link as you did, cannot locate where these forks keep repositories (tried some combinations of their main project in the url). Ideally, we should be able to publish a revised version of /our/ lib[1] that will cover the satchmo or whatever other project.

the OEP License: I think we're not really using it. We are at AGPLv3 and thus I could ask to remove that text file from upstream to clear the situation.


[1] one thing I know for sure, is that we have a strict replacement of python's 'eval()' for security reasons. This binds the trml2pdf to our code so far, non-trivial to break.
Comment 33 Alec Leamas 2011-04-20 03:57:42 EDT
OEP License: It is used, 'grep -r  "OpenERP Public License" .' reveals some files which needs to be edited/relicensed(?)
Comment 34 Panos Christeas 2011-04-20 05:01:56 EDT
My grep reveals these 2 places:
addons/thunderbird/plugin/...
addons/wiki/web/...

For the second, I have not yet asked you to package the web-interface, which means we can surely leave it out. For the first one, we can discuss.

You see, the point is that both these parts have originally be developed by partners of OpenERP and have been relicensed to our open-source terms. I will need to check with them if GPL can be applied.
Comment 35 Alec Leamas 2011-04-20 06:17:08 EDT
So lets skip addons/wiki/web/..., my bad.

Relicensing the thunderbird plugin is the way to go, it's hopefully easier than trying to get the obsolete OEP license approved (if it's at all possible).
Comment 36 Alec Leamas 2011-04-20 06:26:49 EDT
With all issues in comment #31 resolved, we are approaching another other big issue: how to handle the server addons: their deps and in some cases also licenses.

To decouple the thunderbird addon license, one obvious option is to package it in in a sub-package. This seems reasonable from many aspects for me.

However, we also have all other addons and their deps. I know from experience that using certain addons will break due to missing deps. So, we need to somehow fix this. One could of course try to add all deps for all addons, but this would pull in just to much for simple installations(?)

OTOH, creating a sub-package for each addon (150+) seems just not feasible. Or is it? Seems that tryton (an OpenERP fork) is packaged this way in rawhide, 60+ packages.

Would it be possible to somehow group the addons based on functionality, deps etc., into a more manageable set of sub-packages?

Basically, I have no idea of the best way to handle this...
Comment 37 Panos Christeas 2011-04-20 07:03:47 EDT
http://members.hellug.gr/xrg/Mandriva/2010.1/RPMS/noarch/

If you see there, I had been packaging one rpm per addon, for those non-trivial addons.
The way to do that is to have a /generated/ spec file for the addons (one, big spec for all of them), but I guess that's way off the practices of Fedora. I can also tell you that my colleagues at OpenERP want all of the addons packaged into the server rpm.
.. which means we need to discuss more.
Comment 38 Alec Leamas 2011-04-20 07:50:07 EDT
...which means we need a second opinion, at least. Anyone, out there?

My thoughts:

One spec file w any number of subpackages is fine w Fedora from a formal point of view.

Since both tryton/Fedora and OpenERP/Mandriva is walking the one addon/subpackage path, why not stick to this approach (even if it means 150+ packages)?

Did you have any support for finding out subpackage/addons deps in the script generating the spec file?
Comment 39 Panos Christeas 2011-04-21 17:21:44 EDT
Some update, again:
I've built the rpms against a set of draft tarballs, based on the latest official commits (taken from bzr->git). Pending the decisions on bundled libs, we can examine any remaining issues. 

I attach the spec files and rpmlint output. All sources are traced in my git repository.

One question I really have is why does rpmlint complain about using the 'openerp' group in files.
Also, I don't like the fact that rpmlint forced me to set /etc/openerp-server.conf as o+rx , whereas I prefer not to let others read info (and super-admin password) off that file.
Comment 40 Panos Christeas 2011-04-21 17:24:47 EDT
Created attachment 494004 [details]
rpmlint check output
Comment 41 Panos Christeas 2011-04-21 17:27:19 EDT
Created attachment 494006 [details]
Client spec, as in 6.0.2-5
Comment 42 Panos Christeas 2011-04-21 17:29:07 EDT
Created attachment 494007 [details]
Server spec, as in 6.0.2-5
Comment 43 Alec Leamas 2011-04-26 08:13:26 EDT
Easter holiday is over, here we go! First, a server review:

The submission is not complete: there should be a source RPM which is missing. The source URL in the spec file seems to be invalid. This review is based on the previous submitted sources + the updated spec file.  Please submit source rpm + spec file urls next time.

MUST

- rpmlint must be run on the source rpm and all binary rpms... NOK
 + Only 2 packages are checked, should be 4 (2 srpms + 2 noarch rpms)
 + The "Non standard uid/gid" warnings can be ignored.
 + The warning on URL http://www.openerp.com/ seems to be a temporary server problem.
 + The error on zero-length file (.../office.dtd): Is this file required?
 + Fedora init scripts does normally not enable services by default. See http://fedoraproject.org/wiki/Packaging/SysVInitScript#.23_chkconfig:_line
 + The changelog version should be fixed. See http://fedoraproject.org/wiki/PackagingGuidelines#Changelogs

- The spec file name must match the base package %{name}.... NOK
 + The spec file should be named openerp-server.spec; see http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Spec_file_name

- The package must meet the Packaging Guidelines. TBD
 + The dependency list looks incomplete. Samples like user_ldap indicates that some module dependencies are missing. Also, there are references to pytz and matplotlib, but these are not required. Datetutil is used (imported), no dependency on python-dateutil. Some deps listed in setup.py seems not to be included. An overall review of the dependencies is needed.

- 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
 + ./bin/addons/document_ftp/ftpserver is MIT, part of bundled pyftpdlib.
 + ./build/lib/openerp-server/addons/wiki/web/widgets/rss/feedparser.py is MIT.
 + ./bin/addons/resource/faces/* are GPLv2+.
 + ./build/lib/openerp-server/addons/document/dict_tools.py is LGPL 2.0, (C) by you :)
 + The thunderbird plugin is "OpenERP Public License"
 + 'grep -ir  license . | grep -v Affero | grep -i "version 2"' lists files which are not AGPLv3, mostly LGPL 2.1+. 
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
   No source submitted.

- The package MUST successfully compile and build into binary rpms...: TBD
   No source submitted.

- All build dependencies must be listed in BuildRequires...TBD
   No source submitted.

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

- Packages must NOT bundle copies of system libraries: NOK
  + contains bundled ftpserver (in Fedora: pyftpdlib)
  + contains bundled rml2pdf  (in Fedora: python-trml2pdf)

- 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
 + The _iconsdir macro is not used in the server spec file, remove definition.
 + _initrddir is deprecated, replaced by _initddir; see http://fedoraproject.org/wiki/Packaging/SysVInitScript

- 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 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
  No source submitted

- Testing...TBD

- Scriptlets should be sane...: OK

OTHER REMARKS:
- The for BIN in... loop could be written using 'sed -i' in just one line
- Remove setup.cfg together with setup.inf. These files are just not relevant in a Fedora package.
- The copyright info in debian/copyright is outdated, SpiffGtkWidtgets is now AGPL, missing licenses.
- As for your question: Fedora init scripts just should be 755, see http://fedoraproject.org/wiki/Packaging/SysVInitScript.
- Since you want to retain %clean, you can remove the ifdef's around it. %clean is not required, but certainly allowed.
- The %description should expand on the summary. It's now really short, a few sentences about this being part of an ERP application and perhaps an URL would be nice :)
Comment 44 Alec Leamas 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.
Comment 45 Panos Christeas 2011-05-16 04:19:57 EDT
I think we're ready to apply for exceptions. In short:
 - bad copyrights have been restored (upstream)
 - threadinglocal.py is removed, msgfmt.py is taken from python-tools. 
(unbundling)
 - SpiffGtkWidgets have merged our work into their tree. Will apply for 
exception until we test a little more and Fedora ships the new upstream ver.
  https://github.com/knipknap/SpiffGtkWidgets
 - faces-project has been resurrected (I talked with original author and 
collected the forks, into the original project page)
  http://sourceforge.net/projects/faces-project/
  http://faces-project.git.sourceforge.net/git/gitweb.cgi?p=faces-
project/faces-project;a=summary

- pyftpdlib is a copylib+hack case. A not-so-nice one. We either need an 
exception until we find resources to merge their 0.5.x API and push the 
required changes upstream, or let us not have the FTP functionality in 
Fedora.
Comment 46 Alec Leamas 2011-05-30 07:20:56 EDT
Ping... Is the exception process under way?
Comment 47 Panos Christeas 2011-05-30 08:56:47 EDT
Pong: I was waiting for your help on that: can't find some way through to the board. I'm not a member of the lists or bugzilla (closed registration?) or know a valid mail to post to. :(
Comment 48 Alec Leamas 2011-05-30 13:51:08 EDT
A cute deadlock, indeed. Nice job you have done w upstream!

Seems that FPC nowadays are using a ticket system at https://fedorahosted.org/fpc/. Besides that, there is http://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries#Exceptions.
Comment 49 Panos Christeas 2011-05-30 18:18:50 EDT
A little more help, please? (see personal email)
Comment 50 Alec Leamas 2011-05-31 10:39:23 EDT
Beats me, need more help! According to the links in #48, an exception request should be entered as a ticket in the trac system. However, I find no info on how to get permissions to submit such a ticket. 

Are we missing something, or is this a mistake in the new "Getting an exception"-process? How should Panos proceed to submit ticket(s) at https://fedorahosted.org/fpc/ ?
Comment 51 David Nalley 2011-05-31 10:45:44 EDT
He'll need to login with his FAS account to create a ticket there.
Comment 52 Alec Leamas 2011-05-31 12:07:37 EDT
I. e., https://admin.fedoraproject.org/accounts/.

BTW, this needs to be added to the trac main page. When coming from nowhere, this is far from obvious.
Comment 53 Panos Christeas 2011-06-01 03:45:20 EDT
btw, can you locate the SpiffGtkWidgets package in pkgdb? In a quick look [limited to 3min], I couldn't find it. If so, I'd be glad to start packaging that, too.
Comment 54 Panos Christeas 2011-06-01 04:41:33 EDT
Tickets #88, #89 and #90 have been registered in Trac. I hope I'm not torturing the committee :S

FYI, I'm personally /against/ using FTP at all (even though being in charge of its development here). But #censored# users still consider FTP as a good method of transferring files (private data, we are talking about), I know they ask us for it all the time.
Comment 55 Alec Leamas 2011-06-06 11:52:17 EDT
Comment #53: SpiffGtkWidgets doesn't seem to be packaged.
Comment 56 Panos Christeas 2011-06-06 12:06:02 EDT
Yes, Alec.
I guess we have to package that, too. Not bad, it's a win-win deal.

The only thing is, I had some trouble today in my distro (Mageia) testing the calendar[1] so far. I want to make sure, first, that upstream SpiffGtkWidgets don't have any regression over the included ones. 

[1] one method is to put the changes in and run the software for a few cycles (of working with it) just in case Murphy has kicked in.
Comment 58 Alec Leamas 2011-06-17 06:51:33 EDT
Server review:


MUST

rpmlint must be run on the source rpm and all binary rpms...NOK
Seems that all warnings could be removed: 
- The spelling errors could be handled by a hyphen 
  (addons -> add-ons, the US folks are just strange). 
- Expand all tabs in specfile to blanks. 
- Remove comment line w macros (29). 
- Fix patches (see below)
- Don't use macros in changelog, expand them "by hand".

The spec file name must match the base package %{name}....OK
 
The package must meet the Packaging Guidelines. TBD
Dependencies: Havn't really time for this ATM, but I notice 
that much is improved. Let's presume it's ok
 
The package must be licensed with a Fedora approved license...: NOK
Once again, without checking the details, the License Field 
in the spec does not reflect the breakdown in debian/copyright

See http://fedoraproject.org/wiki/Packaging:LicensingGuidelines 

Separate License file must be in %doc...: NOK
- The only reasonable breakdown, debian/copyright is not present  

The spec file must be written in American English: OK
  To my understanding :)

The spec file for the package MUST be legible: OK

The build sources must match the upstream source...NOK
   
  You have used patched source, not OK. Use the orignal source
  and apply the patches in the spec file instead. It's easy
  to remove once they are accepted. This will also fix the last
  rpmlint warnings.

The package MUST successfully compile and build into binary rpms...: OK
  Builds OK on f14 and f15.
  f14:  http://koji.fedoraproject.org/koji/taskinfo?taskID=3136833
  f15: http://koji.fedoraproject.org/koji/taskinfo?taskID=3136871

All build dependencies must be listed in BuildRequires...OK
The spec file MUST handle locales properly...: OK

Packages must NOT bundle copies of system libraries: NOK
  - contains bundled ftpserver (in Fedora: pyftpdlib)
  - contains bundled rml2pdf  (in Fedora: python-trml2pdf)

A package must own all directories that it creates. OK
Package must not list a file more than once in %file ..: OK
Permissions on files must be set properly...: OK
Each package must consistently use macros: OK
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 N/a
Packages must not own files or dir's owned by other ones...OK
All filenames in rpm packages must be valid UTF-8.: OK

SHOULD:

Package built on koji/mock: OK
Testing...TBD
Scriptlets should be sane: OK
  (The test that update-desktop-database exists is not required:)

SUMMARY:
Besides minor editing in spec file the open issues are the bundled
libs, the use of patched source and that the different licenses
are not handled properly.
Comment 59 Alec Leamas 2011-06-17 06:55:17 EDT
Client: I wait until a new variant is submitted, presuming that the the main server remarks are valid also for the client
Comment 60 Panos Christeas 2011-06-17 08:00:38 EDT
> Packages must NOT bundle copies of system libraries: NOK
>   - contains bundled rml2pdf  (in Fedora: python-trml2pdf)

That's a bit unfair to OpenERP, as I have told you: the original lib is what we bundle, while "python-trml2pdf" one of Fedora is an old ver+hacks.

> The build sources must match the upstream source...NOK

The tar.gz's used are taken from:
   http://nightly.openerp.com/6.0/
so, they are what I expect to be tagged '6.0.3' soon.
Comment 61 Alec Leamas 2011-06-17 11:52:02 EDT
As for rml2pdf, I think we agree on the overall situation, you are certainly right (and my phrasing is not really ideal) OTOH, from the distribution perspective, the simple fact is that openerp contains a modified version of something which already exists in Fedora, and this needs to be resolved. Let's hope the exception is granted!

I'm just making a review, and one duty is to check the source vs the source URL. And they don't match. I'm sure they will, next time :) However, the requirement for the source url to match the actual source is not negotiable to my understanding.
Comment 62 Matthias Saou 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.
Comment 63 Matthias Saou 2011-08-15 09:51:54 EDT
I've now started testing the openerp-server package. First blocker encountered :

--
%pre
    getent group openerp >/dev/null || groupadd -r openerp
    getent passwd openerp >/dev/null || \
        useradd -r -d /var/spool/openerp -s /sbin/nologin \
        -c "OpenERP Server" openerp
--

This results in the following :
useradd: group openerp exists - if you want to add this user to that group, use -g.

The error message is quite explicit : Either don't manage the group separately, or pass "-g openerp" to the useradd command.

I would suggest this simple and effective approach (see the "httpd" rpm) :

/usr/sbin/useradd -c "OpenERP Server" \
	-s /sbin/nologin -r -d /var/spool/openerp openerp 2>/dev/null || :
Comment 64 Matthias Saou 2011-08-15 11:37:10 EDT
Dependency problems :
 * The openerp-server requires pytz otherwise it won't start.
 * At least one of the client or server packages requires python-matplotlib for the "Graphs" button to work in the client. Since I'm running both on the same computer, I'm unsure which, but not having seen anything in the server's log, I'm assuming it's required by the client (and that would make most sense).
Comment 65 Alec Leamas 2011-08-17 18:45:19 EDT
Are the bundled libraries issues stalled? My understanding:
- We need to package SpiffGtkWidgets separately (https://fedorahosted.org/fpc/ticket/88)
- We need to provide the answers to the standard questions for https://fedorahosted.org/fpc/ticket/90 and https://fedorahosted.org/fpc/ticket/89. This has been open since june, 1. 

We really need to sort this out! What's happening?!
Comment 66 Matthias Saou 2011-08-18 06:35:52 EDT
* Regarding SpiffGtkWidgets, I've created a quick package :
http://thias.fedorapeople.org/review/python-spiffgtkwidgets/
If anyone wants to maintain it, just pick it up, adapt if wanted, submit for review and I'll review it. Otherwise, I don't really mind maintaining it, but I think it makes more sense to have it done by the OpenERP maintainer, or at least someone close to upstream.
The changes required to openerp-client should then be trivial (untested) :
  * "Requires: python-spiffgtkwidgets"
  * "rm -rf bin/SpiffGtkWidgets/" in %prep
  * Patch to setup.py to remove the two lines related to SpiffGtkWidgets

* Regarding the two other points. It's mostly political... For faces-project my opinion is that we should try to package in Fedora a version which suits OpenERP for now, making sure that the next future-merged-cleaned-up version up on sourceforge is something that upstream OpenERP is willing to switch to in the near future. As for pyftpdlib... dropping FTP support shouldn't be a critical issue, but if this is a very small and heavily modified library, then maybe the exception makes sense. Ideally the latest upstream should be patched in a backwards-compatible way, yet suitable for OpenERP.
Comment 67 Alec Leamas 2012-04-20 09:52:27 EDT
Bump... Panos, are you there?

Note You need to log in before you can comment on or make changes to this bug.