Bug 568833

Summary: Review Request: OpenLP - Church projection software
Product: [Fedora] Fedora Reporter: TR Bentley <home>
Component: Package ReviewAssignee: Nils Philippsen <nphilipp>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: christoph.wickert, fedora-package-review, felix, notting, nphilipp, rdieter
Target Milestone: ---Flags: nphilipp: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-07-25 10:55:40 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description TR Bentley 2010-02-26 17:43:03 UTC
Spec URL: <spec info here>
SRPM URL: <srpm info here>

Description: OpenLP is free church presentation software, or lyrics projection software, used to display slides of songs, Bible verses, videos, images, and even presentations (if PowerPoint is installed) for church worship using a computer and a data projector.

The latest version 2.x is being developed in PYQT for cross platform support.  Details of the project can be found on OpenLP.org.

Need hosting access under account trb143 to upload the spec and srpms files

Comment 1 Jason Tibbitts 2010-02-26 18:07:37 UTC
*** Bug 568834 has been marked as a duplicate of this bug. ***

Comment 2 Jason Tibbitts 2010-02-26 18:08:39 UTC
I cleaned up the duplicate submission and gave you the necessary access so that you will have access to fedorapeople to upload the spec and srpm.  It may take up to an hour for that to propagate.

Comment 3 TR Bentley 2010-02-26 18:35:51 UTC
Spec File : OpenLP.spec
Source RPM File : OpenLP-1.9.0_bzr715-1.src.rpm

Uploaded to trb143 @ fedorapeople.org via ssh. 
No sign via the web as yet.

Comment 4 Jason Tibbitts 2010-02-26 18:41:34 UTC
Don't think it works that way.  Did you read the documentation at fedorapeople.org, create your web space, and put the files there?  You'll need to post actual links to the spec and srpm if anyone's to be able to look at them.

Comment 5 TR Bentley 2010-02-26 19:14:02 UTC
Spec File : http://trb143.fedorapeople.org/OpenLP.spec
Source RPM File : http://trb143.fedorapeople.org/OpenLP-1.9.0_bzr715-1.src.rpm

Sorry miss read the instructions.  These should be visible now as links.

Comment 6 Felix Kaechele 2010-02-28 12:52:14 UTC
I just looked at your SPEC file and must say there's still a lot to learn for you.
It would help us a lot if you read https://fedoraproject.org/wiki/PackageMaintainers/Join and followed this guide step by step. There are always sponsors available that are willing to guide you through the process. However it would make it a lot easier if you made yourself familiar with our packaging guidelines beforehand so that your sponsor doesn't need to explain them to you.
I'd suggest you read https://fedoraproject.org/wiki/Packaging:Guidelines which are our guidelines as well as looking into using the rpmlint tool on your packages. This tool will give you lots of hints on what to improve in your package.
If you have any further questions we'll be glad to help you :)

Comment 7 TR Bentley 2010-02-28 19:48:58 UTC
This is my first attempt but the documents are not clear for pre build python programs.

I have read the documents and attempted to build a spec file using Eric and marave as an example.

The version numbering comes from the setup.py which is controlled by the main project.  Do I need a new setup file for fedora as we were hopeing to keep one consistent between Ubunto and Fedora.

What is the best way to move this forward If one pne person were willing to help (Felix?) how can we discuss the individual issues.

Tim

Comment 8 TR Bentley 2010-03-05 11:26:23 UTC
Updated spec file : http://trb143.fedorapeople.org/OpenLP.spec

using rpmlint -i and no errors are now found.

Added source tzr.gz file to folder as well to keep spec file happy.

Attempted to fix other spotted errors.

Comment 9 TR Bentley 2010-03-06 17:01:24 UTC
Updated for new code version

Spec File : http://trb143.fedorapeople.org/OpenLP.spec
Source RPM File : http://trb143.fedorapeople.org/OpenLP-1.9.0_bzr725-1.src.rpm

Comment 10 Christoph Wickert 2010-03-06 18:08:19 UTC
Thanks for your submission. Some more comments from a quick look over your spec:

- Please don't use %define for name version and release. Using defines only makes sense for things that apprear several times in the spec. And if you really need something like this, use %global, see 
https://fedoraproject.org/wiki/Packaging/Guidelines#.25global_preferred_over_.25define

- Version and release tags are wrong, see
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#Snapshot_packages

- Summary should not end with a dot.

- License tag is invalid. "GNU General Public License" is abbreviated as "GPL", but you need to specify what version of the GPL it is.

- The group "Development/Libraries" is wrong, pick a better one from /usr/share/doc/rpm-*/GROUPS

- buildroot should be one of these: https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

- %description should have linebreaks at 80 charakters, otherwise it's hard to read on a terminal.

- Drop the prefix tag, this is set automatically by the %configure macro. Use 'rpm --eval %configure' to see what the macro does.

- Drop the vendor tag, it is added by the buildsystem. BTW: The vendor is Fedora, not the upstream developer.

- "Url" should be "URL".

- Don't use "--record=INSTALLED_FILES". list them manually in the spec please.

- Each changelog needs version and release. For a list of allowed formats, see
https://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs

- I bet you miss some BuildRequires, use mock to check. See http://fedoraproject.org/wiki/Using_Mock_to_test_package_builds

- Please take a look at https://fedoraproject.org/wiki/Packaging:Python for python specific hints.

Comment 11 TR Bentley 2010-03-07 19:58:53 UTC
Updated for new code version

Spec File : http://trb143.fedorapeople.org/OpenLP.spec
Source RPM File : http://trb143.fedorapeople.org/OpenLP-1.9.0_bzr725-1.src.rpm    

Followed through the comments in #10.
installed and test run with Mock and have a clean build.

Comment 12 TR Bentley 2010-03-12 18:30:03 UTC
Updated for new code version

Spec File : http://trb143.fedorapeople.org/OpenLP.spec
Source RPM File : http://trb143.fedorapeople.org/OpenLP-1.9.0-bzr735.src.rpm  

Followed through the comments in #10.
installed and test run with Mock and have a clean build.

Comment 13 TR Bentley 2010-03-17 20:47:51 UTC
Updated for new code version

Spec File : http://trb143.fedorapeople.org/OpenLP.spec
Source RPM File : http://trb143.fedorapeople.org/OpenLP-1.9.0-bzr741.src.rpm  

Followed through the comments in #10.
installed and test run with Mock and have a clean build.    

I have managed to build and test run this but as this is my first rpm I have some problems I would like some help with.

The release number is different - is this a major issue but it matches upstream

I have problems getting a desktop icon.  It is in the original tar.gz but the python build looses it.  Removing this statement breaks the install so I am confused.  Looked at Eric and marave for guidance.

Can anyone help me sort this out and get it into Fedora .

Thanks

Comment 14 TR Bentley 2010-03-31 17:48:15 UTC
Updated for new code version

Spec File : http://trb143.fedorapeople.org/OpenLP.spec
Source RPM File : http://trb143.fedorapeople.org/OpenLP-1.9.1-1.src.rpm  

Followed through the comments in #10. and a review by deji
installed and test run with Mock and have a clean build.    

I have managed to build and test run this but as this is my first rpm I have
some problems I would like some help with.

Can anyone help me sort this out and get it into Fedora .

Thanks

Comment 15 Nils Philippsen 2010-04-01 17:14:08 UTC
I'll simply work through the points at http://fedoraproject.org/wiki/Packaging/ReviewGuidelines :

Items marked "GOOD" or "PASS" fulfil the guidelines or they don't apply to this
package.
Items marked "CHECK" aren't covered by the guidelines but you should check and
fix them anyway in my opinion.
Items marked "BAD" violate the guidelines in some point and need to be fixed.

- GOOD: rpmlint on source package passes:

nils@gibraltar:~/devel/fedora-review/OpenLP> rpmlint OpenLP-1.9.1-1.src.rpm OpenLP.src: W: invalid-url Source0: http://downloads.sourceforge.net/OpenLP/OpenLP-1.9.1.tar.gz HTTP Error 302: The HTTP server returned a redirect error that would lead to an infinite loop.
The last 30x error message was:
Found
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

-> I've downloaded the tarball earlier and manually verified that the source RPM is the same as upstream.

- BAD: rpmlint on noarch package prints warnings/errors:

nils@gibraltar:~/devel/fedora-review/OpenLP> rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/OpenLP-1.9.1-1.noarch.rpm
OpenLP.noarch: W: file-not-utf8 /usr/share/doc/OpenLP-1.9.1/pyqt-sql-py2exe.txt
OpenLP.noarch: E: non-executable-script /usr/lib/python2.6/site-packages/openlp/plugins/remotes/remoteclient.py 0644 /usr/bin/env
OpenLP.noarch: W: hidden-file-or-dir /usr/lib/python2.6/site-packages/openlp/.version
1 packages and 0 specfiles checked; 1 errors, 2 warnings.

-> convert the doc file to UTF-8 before installing
-> remove the interpreter hashbang line from the non-executable python module
-> The way openlp determines its version seems awfully complicated to me. If I were in your place, rather than putting it in a hidden file in the python module directory, I'd store it as a normal python variable in an openlp.version module which could be generated while installing (there you could check for bazaar versions and whatnot).

- GOOD: package is named according to the Package Naming Guidelines
- GOOD: spec file name matches package name
- CHECK: The summary is really long, probably get rid of "Open Source" (kinda implied in Fedora).
- BAD/CHECK: The description contains information not necessary for someone installing the Fedora package (we didn't have the old openlp.org software, neither is there Powerpoint on Fedora). You could clarify and condense this  a good bit (and avoid any trademark issues) if it were phrased like this (wrapped at 80 chars/line of course):

OpenLP is a church presentation and lyrics projection program, used to display song texts, Bible verses, 
videos and images using a computer and a beamer.

I don't know if it can use openoffice.org for displaying presentations, if yes, just re-add that part (preferably without mentioning Powerpoint).

- GOOD: package licensed properly
- GOOD: license field matches actual license (GPLv2)
- GOOD: license text included in package
- GOOD: spec file written in American English
- GOOD: spec file is legible
- GOOD: source RPM tarball matches upstream
- GOOD: package builds in mock
- GOOD: all build requirements listed
- PASS: No locales included (see below, though).
- PASS: no libraries
- GOOD: no bundled system libraries
- GOOD: no relocation
- BAD: Doesn't own all directories it creates: /usr/share/icons/hicolor/... are contained in the hicolor-icon-theme package -> please add hicolor-icon-theme as a dependency.
- GOOD: doesn't list files more than once
- GOOD: permissions set properly
- GOOD: has %clean
- GOOD: consistently uses macros
- GOOD: package contains code
- PASS/CHECK: no large documentation. You might want to include only files that have actual content, though -- PluginDevelopersGuide.txt is pretty empty. Feel free to leave this in if you're confident that this will get filled in time for 2.0 ;-).
- GOOD: %doc files don't affect runtime
- PASS: no header files
- PASS: no static libraries
- PASS: no devel libraries
- PASS: no -devel subpackage
- GOOD: no libtool archives
- GOOD: contains desktop file, gets installed properly
- GOOD: cleans buildroot at %install
- GOOD: all filenames are valid UTF-8

Following some other things I noticed that you might want to check even though they're not covered by guidelines:

- CHECK: It would be good if the package was localized, i.e. contained translations so people speaking other languages could use it.
- CHECK: I think you should add a disttag to your release, e.g. "1%{?dist}", so that you can build the same version/release for different Fedora versions. Mind that you could have different python directories/major versions or need different other packages in different Fedora versions.
- CHECK: The python executables use "#!/usr/bin/env python" as the interpreter. While there exists no guideline against using this rather than the regular "#!/usr/bin/python" it makes behavior of the program unpredictable if people e.g. install python 3 manually to /usr/local/bin.
- CHECK: The executable is called "openlp.pyw". Why that extension? IMO, it would be totally sufficient if the file was called "openlp" without extension. That the package uses python is just an implementation detail.

Comment 16 TR Bentley 2010-04-05 17:20:11 UTC
Updated for new code version

Spec File : http://trb143.fedorapeople.org/OpenLP.spec
Source RPM File : http://trb143.fedorapeople.org/OpenLP-1.9.1.1-1.src.rpm  

Comments from above review.

Run rpmlint to remove errors apart from spelling of openoffice.org and a grumble about version numbers.

Fixed UTF-8 error to make UTF-8
Moved executable file to correct directory.
Add Icons dependency. 

Notes.
Translations will come with alpha2
dist tag added as suggested.
openlp.pyw is needed for windows tried to rename in spec file and got access violations.  Any suggestions for a fix?

Comment 17 TR Bentley 2010-04-06 17:35:37 UTC
Updated for new code version

Spec File : http://trb143.fedorapeople.org/OpenLP.spec
Source RPM File : http://trb143.fedorapeople.org/OpenLP-1.9.1.1-1.fc12.src.rpm  

Provide correct package location

Sourceforge now is correct.
Scripts now do not have .py suffixes.

Comment 18 Nils Philippsen 2010-04-08 15:55:59 UTC
- GOOD/CHECK: rpmlint doesn't find serious issues:

nils@gibraltar:~/devel/fedora-review/OpenLP> rpmlint /var/lib/mock/fedora-rawhide-x86_64/result/OpenLP-1.9.1.1-1.fc14.noarch.rpm
OpenLP.noarch: W: spelling-error %description -l en_US openoffice -> open office, open-office, interoffice

-> see below

OpenLP.noarch: W: incoherent-version-in-changelog 1.9.1.1 ['1.9.1.1-1.fc14', '1.9.1.1-1']

-> bogus

OpenLP.noarch: W: hidden-file-or-dir /usr/lib/python2.6/site-packages/openlp/.version

-> I'd do that differently, see my last comment. Ping me if you have questions about that.

1 packages and 0 specfiles checked; 0 errors, 3 warnings.

- BAD/CHECK: The description contains grammar errors: "OpenLP is a church presentation software, for lyrics projection software, ...", rpmlint complains about "openoffice", should be "OpenOffice.org", e.g.:

OpenLP is a church presentation software and lyrics projection software,
used to display slides of songs,  Bible verses, videos, images, and 
presentations (if OpenOffice.org is installed) using a computer and a data
projector.

- GOOD: Owns all directories it creates.
- GOOD: disttag added
- GOOD: /usr/bin/python used
- GOOD: .pyw extension dropped

This package is APPROVED. Please proceed with requesting CVS branches, you should be sponsored for packager shortly.

Comment 19 TR Bentley 2010-04-08 16:30:05 UTC
New Package CVS Request
=======================
Package Name: OpenLP
Short Description: OpenLP Church Song projection software 
Owners: trb143
Branches: F-12 F-13

Comment 20 Kevin Fenzi 2010-04-09 04:49:49 UTC
CVS done (by process-cvs-requests.py).