Bug 516906

Summary: Review Request: frePPLe - Free Production Planning Library
Product: [Fedora] Fedora Reporter: Johan De Taeye <jdetaeye>
Component: Package ReviewAssignee: Michel Alexandre Salim <michel>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: fedora-package-review, martin.gieseking, michel, notting
Target Milestone: ---Flags: michel: fedora‑review+
kevin: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: 0.7.1-1.fc11 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-09-29 10:30:48 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Attachments:
Description Flags
test.out files from failing tests none

Description Johan De Taeye 2009-08-11 16:48:19 EDT
Spec URL: http://downloads.sourceforge.net/project/frepple/frepple/0.7.1/fedora_SRPM/frepple.spec
SRPM URL: http://downloads.sourceforge.net/project/frepple/frepple/0.7.1/fedora_SRPM/frepple-0.7.1-1.fc12.src.rpm
Description: 

Hello,

I am leading the open source frePPLe project and would like to include it as a fedora package. Can you please review the package?

FrePPLe stands for "Free Production Planning Library". It is a toolkit/
framework for modeling and solving production planning problems, targeted
primarily at discrete manufacturing industries.

BTW, a very similar review process has also been started for including frePPle in the Ubuntu distribution: see http://revu.ubuntuwire.com/p/frepple.
Since I would like to focus on the upstream development, a volunteer maintainer of the Fedora package would be very welcome.
Comment 1 Martin Gieseking 2009-08-12 03:23:47 EDT
Hello Johan,

since this seems to be your first review request and I can't find you in the Fedora account system, you need a sponsor reviewing this package. To find one, add the FE-NEEDSPONSOR tag in the Blocks field above. For further information see 
http://fedoraproject.org/wiki/PackageMaintainers/Join#Create_Your_Review_Request

I just had a quick look at your spec file and noticed a couple of things to fix:  
- please remove the leading header comment
- the License tag should probably be LGPLv2+ (see the license abbreviation page)
- fix the Source tags according to http://fedoraproject.org/wiki/Packaging:SourceURL
- remove "python" from Requires, as it's determined automatically
- you probably don't need to add the autotools to BuildRequires
- add a %changelog section at the bottom of the spec file showing the revision history

$ rpm -qlp frepple-0.7.1-1.fc12.src.rpm
frepple-0.7.1.spec
frepple-0.7.1.tar.gz

The spec filename must be frepple.spec (remove the version number).

$ rpmlint frepple-0.7.1-1.fc12.src.rpm
frepple.src: E: no-changelogname-tag
frepple.src: W: invalid-license LGPL, v2.1
frepple.src: W: invalid-license greater
frepple.src: E: invalid-spec-name
1 packages and 0 specfiles checked; 2 errors, 2 warnings.

Again, the complete review must be done by a sponsor. :)

Martin
Comment 2 Johan De Taeye 2009-08-12 09:38:08 EDT
Danke Martin!

All your remarks are just and correct. 
I just uploaded a new SRPM and spec-file that fixes these issues.

Johan


$ mock -r fedora-devel-i386 rebuild frepple-0.7.1-1.fc11.src.rpm
INFO: mock.py version 0.9.16 starting...
State Changed: init plugins
State Changed: start
INFO: Start(frepple-0.7.1-1.fc11.src.rpm)  Config(fedora-rawhide-i386)
State Changed: lock buildroot
State Changed: clean
State Changed: init
State Changed: lock buildroot
Mock Version: 0.9.16
INFO: Mock Version: 0.9.16
INFO: enabled root cache
State Changed: unpacking root cache
INFO: enabled yum cache
State Changed: cleaning yum metadata
INFO: enabled ccache
State Changed: running yum
State Changed: setup
State Changed: build
INFO: Done(frepple-0.7.1-1.fc11.src.rpm) Config(fedora-devel-i386) 4 minutes 11 seconds
INFO: Results and/or logs in: /var/lib/mock/fedora-rawhide-i386/result

$ rpmlint /var/lib/mock/fedora-rawhide-i386/result/frepple-0.7.1-1.fc12.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpm -qlp /var/lib/mock/fedora-rawhide-i386/result/frepple-0.7.1-1.fc12.src.rpm
frepple-0.7.1.tar.gz
frepple.spec
Comment 3 Michel Alexandre Salim 2009-09-15 01:53:39 EDT
@ Martin (hi again!)

just noticed you are actually a sponsor -- ignore my question on the other bug entry. Are you doing both reviews? I ended up here because the review flag is not set yet. We can split the reviews between us if you want.
Comment 4 Martin Gieseking 2009-09-15 02:16:17 EDT
(In reply to comment #3)
> @ Martin (hi again!)
> just noticed you are actually a sponsor 

Michel, no I'm not a sponsor. I hope I didn't convey this impression anywhere. So you can do the pending formal review of frepple too. :)
Comment 5 Michel Alexandre Salim 2009-09-16 00:50:31 EDT
Ah, no, you did not imply that. It's just that between two preliminary reviews on two FE-NEEDSPONSOR tickets, and someone with a very similar name to you being a sponsor in the account system, I added up 2 + 2 = 5.

Thanks for clarifying; I'm assigning myself the review; will sponsor at the end of the process.
Comment 6 Michel Alexandre Salim 2009-09-16 17:29:02 EDT
Here's a preliminary review; there are some things to be fixed, packaging-wise, and a build failure. The failures occur on x86_64, whereas you've only tested ix86, but whether ix86 is fine on the server or x86_64 just happened to fail first, you'd have to verify. I'll try building locally on my x86_64 box and see what's going on.

MUST

• rpmlint
$ rpmlint frepple-0.7.1-1.fc12.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

binary not checked yet; builds fail

OK package name - matches source tarball
OK spec file name - matches package name
OK package guideline-compliant
OK license complies with guidelines
OK license field accurate
  note: do you have to disable mod_lpsolver? Just make it a subpackage, and
  license *that* subpackage under the GPL. You'd then want to bundle the GPLv2
  COPYING file in the tarball, and rename the LGPL license file to COPYING.LIB
OK license file not deleted
OK spec in US English
FIX spec legible
- source tarball listed twice, and Source1 is never used. It's fine to specify
  a remote URL, as rpmbuild will just strip the remote part of the URL anyway.
  Using a remote URL is so that, given the spec, someone can use spectool -gf
  to retrieve the tarball from upstream.

- in %build, pass %{?_smp_mflags} to make if possible, for build parallelism
- don't hardcode .gz extension for manpage, use * instead

OK source matches upstream
$ md5sum frepple-0.7.1.tar.gz ../SOURCES/frepple-0.7.1.tar.gz 
52c03862345f6247bfda5f7e848457f3  frepple-0.7.1.tar.gz
52c03862345f6247bfda5f7e848457f3  ../SOURCES/frepple-0.7.1.tar.gz

FIX builds under >= 1 archs, others excluded
  xml_remote test fails:
  F-11 http://koji.fedoraproject.org/koji/taskinfo?taskID=1684341
  F-12 http://koji.fedoraproject.org/koji/taskinfo?taskID=1684245
OK build dependencies complete
OK library -> ldconfig

FIX own all directories
  going overboard by owning %{python_sitelib} ! Don't own files already owned
  by Python or Django

OK no dupes in %files
FIX permission
  use %defattr(-,root,root,-)
OK %clean RPM_BUILD_ROOT
OK macros used consistently
OK Package contains code

FIX large docs => -doc
• doc not runtime dependent

Rule of thumb is, if it has PDFs and/or more than a couple of HTML files, put them in a separate subpackage

OK headers in -devel
OK if libfiles are suffixed, the non-suffixed goes to devel
OK devel requires versioned base package
OK clean buildroot before install
? filenames UTF-8

SHOULD
FIX if license text missing, ask upstream to include it
  include GPLv2 license text when packaging the currently disabled module
FIX package build in mock on all architectures
? package functioned as described
• other subpackages should require versioned base
  do this for -doc
OK require package not files
Comment 7 Michel Alexandre Salim 2009-09-16 17:47:50 EDT
On my machine, I get *more* errors, but xml_remote actually passed (so that problem might be a missing Build Requirement).

As long as it passes unit tests on the build servers, it does not matter so much if it fails on individual machines, though that's still odd and we should try fixing it.

http://torrents.thepiratebay.org/4893556/The_Big_Bang_Theory_Season_2_Complete.4893556.TPB.torrent
Comment 8 Johan De Taeye 2009-09-17 02:23:28 EDT
I'll fix the package errors over the weekend. Here are some quick comments:

* The unit test xml_remote uses the Python standard library to pick up a data file from the frepple website. On machines without internet connection this test will fail. It is probably safer to skip the test - it is not a critical unit test for the package anyway.

* The module mod_lpsolver is currently at an experimental stage only. I'ld prefer to keep it disabled for now. Once it is ready, I agree that it should be packaged as a subpackage.
Comment 9 Johan De Taeye 2009-09-17 15:19:37 EDT
A corrected version, addressing all the remarks, has just been uploaded.
Comment 10 Michel Alexandre Salim 2009-09-17 16:00:00 EDT
(meant to copy-and-paste the Koji build URL, sorry)
Koji F-12: http://koji.fedoraproject.org/koji/taskinfo?taskID=1687259

rpmlint warnings on the binaries:
frepple.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libfrepple.so.0.0.0 /lib64/libm.so.6
frepple.x86_64: E: non-executable-script /usr/lib/python2.6/site-packages/freppledb/manage.py 0644 /usr/bin/env
frepple.x86_64: W: file-not-in-%lang /usr/lib/python2.6/site-packages/freppledb/locale/nl/LC_MESSAGES/django.mo
frepple.x86_64: W: file-not-in-%lang /usr/lib/python2.6/site-packages/freppledb/locale/nl/LC_MESSAGES/djangojs.mo
frepple.x86_64: W: file-not-in-%lang /usr/lib/python2.6/site-packages/freppledb/locale/zh_TW/LC_MESSAGES/django.mo
frepple.x86_64: W: file-not-in-%lang /usr/lib/python2.6/site-packages/freppledb/locale/zh_TW/LC_MESSAGES/djangojs.mo
1 packages and 0 specfiles checked; 1 errors, 5 warnings.

frepple-devel.x86_64: W: summary-ended-with-dot The libraries and header files needed for frePPLe development.

frepple-doc.x86_64: W: summary-ended-with-dot Documentation subpackage for frePPLe.



Changes look good so far. But.. the URL field should not be removed, really; it points to the program's homepage and is visible to the user (try rpm -qi on the produced RPM). Only Source1 needs to be removed as it's redundant and never referred to.

Some tests still fail locally:

======================================================================
FAIL: callback
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./runtest.py", line 245, in runExecutable
    self.assertFalse("Difference in output")
AssertionError

======================================================================
FAIL: name
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./runtest.py", line 245, in runExecutable
    self.assertFalse("Difference in output")
AssertionError

======================================================================
FAIL: problems
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./runtest.py", line 245, in runExecutable
    self.assertFalse("Difference in output")
AssertionError

----------------------------------------------------------------------
Ran 37 tests in 50.983s

FAILED (failures=3)
make[1]: *** [check] Error 1

Any idea why they pass on the build server? Might it be that they terminate with no error if there is no Internet connection, but fail if there actually is?

If you're planning to support only Fedora (and not EPEL, Extra Packages for Enterprise Linux) then you can make the documentation subpackage noarch:

%package doc
...
BuildArch: noarch

to be safe, though, in case some EPEL maintainers want to bundle this at some point in the future, you can %if-guard it as such:

# noarch subpackages require RPM >= 4.6.0, available in F-10 or later
# (and the upcoming RHEL 6). All Fedora versions are fine since the lowest
# supported version is 10

%if 0%{?fedora} || 0%{?rhel} > 5
BuildArch: noarch
%endif
Comment 11 Johan De Taeye 2009-09-18 06:09:18 EDT
An updated version has been uploaded, addressing the rpmlint warnings and other remarks.

>>Changes look good so far. But.. the URL field should not be removed, 
>>really; it points to the program's homepage and is visible to the user 
>>(try rpm -qi on the produced RPM). Only Source1 needs to be removed as 
>>it's redundant and never referred to.

URL and Source fields are present in the spec file. Source1 is removed.

>>Any idea why they pass on the build server? Might it be that they 
>>terminate with no error if there is no Internet connection, but fail 
>>if there actually is?

The only test connecting to the internet is xml_remote. The others run completely local.
What distinguish this subset of tests from the others is that they compile a seperate executable, linking with the frepple shared library. The other tests are using the frepple executable.
The tests are expected to pass. The tests produce an output file test.out in their subdirectory. Its contents should explain the cause of the problem - my guess would be that it's a SH_LIBPATH issue...
Comment 12 Michel Alexandre Salim 2009-09-18 14:59:43 EDT
(In reply to comment #11)
> An updated version has been uploaded, addressing the rpmlint warnings and other
> remarks.
> 
> 
> URL and Source fields are present in the spec file. Source1 is removed.
Ah, yes. Sorry about that: I'm used to looking at rpmdev-newspec created templates, which have slightly better formatting.

> >>Any idea why they pass on the build server? Might it be that they 
> >>terminate with no error if there is no Internet connection, but fail 
> >>if there actually is?
> 
> The only test connecting to the internet is xml_remote. The others run
> completely local.
> What distinguish this subset of tests from the others is that they compile a
> seperate executable, linking with the frepple shared library. The other tests
> are using the frepple executable.
> The tests are expected to pass. The tests produce an output file test.out in
> their subdirectory. Its contents should explain the cause of the problem - my
> guess would be that it's a SH_LIBPATH issue...  

Will attach the failing test results.
Comment 13 Michel Alexandre Salim 2009-09-18 15:01:03 EDT
Created attachment 361708 [details]
test.out files from failing tests
Comment 14 Johan De Taeye 2009-09-19 02:44:57 EDT
The difference between you local machine and the build machine is that you have the CherryPy package installed.

Comparing the test.out files with the .expect files, one can verify the difference is coming from the following deprecation warnings generated by CherryPy. The deprecation warnings don't affect the functioning of frePPLe at all.
>>/usr/lib64/python2.6/site-packages/cherrypy/lib/cptools.py:4: >>DeprecationWarning: the md5 module is deprecated; use hashlib instead
>>  import md5
>>/usr/lib64/python2.6/site-packages/cherrypy/lib/sessions.py:16: >>DeprecationWarning: the sha module is deprecated; use the hashlib module instead
>>  import sha
>>/usr/lib64/python2.6/site-packages/cherrypy/_cperror.py:190: >>DeprecationWarning: BaseException.message has been deprecated as of Python 2.6
>>  self.message = message

frePPLe uses CherryPy to implement an optional REST web service in Python code. I didn't list CherryPy as a dependency in the spec file. Should it be added?
Since the web service will be used only by a limited subset of frePPLe users, I give an error message when a user tries to use the web service module for the first time. See the code in the file init.py:
>>try:
>>  import cherrypy
>>except:
>>  # Alternative definitions when cherrypy is not available.
>>  # We only want to report the missing module when the REST web service is 
>>  # really used.
>>  def RESTwebservice(address=None, port=8080):
>>    raise ImportError, "no module named cherrypy"    
>>else:  
>>  def RESTwebservice(address=None, port=8080):
>>  ... implementation of the web service ...  

Conclusion: The test failures can be ignored.
Comment 15 Michel Alexandre Salim 2009-09-21 16:12:35 EDT
Koji F-12: http://koji.fedoraproject.org/koji/taskinfo?taskID=1695723

Clean rpmlint:
$ rpmlint /home/michel/Download/frepple*frepple-devel.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 1 warnings.

This package is approved -- what's your FAS username, so I can sponsor the application?

And welcome to Fedora :) ping me if you have any questions.
Comment 16 Johan De Taeye 2009-09-22 02:36:27 EDT
>>And welcome to Fedora :) ping me if you have any questions.

Thanks! I'm glad to join the club.

I have just created a FAS account as user "jdetaeye".
Comment 17 Michel Alexandre Salim 2009-09-23 17:38:01 EDT
OK, I just sponsored your application. You can now proceed with the CVS admin request:

http://fedoraproject.org/wiki/CVS_admin_requests


Please let me know if you have any question about using our update system, if you need another package reviewed, or any other question at all, at salimma@fedoraproject.org.
Comment 18 Johan De Taeye 2009-09-24 02:59:47 EDT
New Package CVS Request
=======================
Package Name: frepple
Short Description: Free Production Planning Library
Owners: jdetaeye
Branches: F-10 F-11 F-12
InitialCC: salimma
Comment 19 Kevin Fenzi 2009-09-24 12:28:07 EDT
cvs done.
Comment 20 Fedora Update System 2009-09-25 12:13:58 EDT
frepple-0.7.1-1.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/frepple-0.7.1-1.fc11
Comment 21 Fedora Update System 2009-09-25 12:14:06 EDT
frepple-0.7.1-1.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/frepple-0.7.1-1.fc10
Comment 22 Fedora Update System 2009-09-29 10:30:41 EDT
frepple-0.7.1-1.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 23 Fedora Update System 2009-09-29 10:35:24 EDT
frepple-0.7.1-1.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.