Bug 484798 - Review Request: xiphos - Bible study and research tool
Review Request: xiphos - Bible study and research tool
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nils Philippsen
Fedora Extras Quality Assurance
:
: 497497 (view as bug list)
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-09 18:13 EST by Deji Akingunola
Modified: 2009-10-26 16:20 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-07-16 09:57:57 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
nphilipp: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Deji Akingunola 2009-02-09 18:13:15 EST
Spec URL: http://deji.fedorapeople.org/xiphos.spec
SRPM URL: http://deji.fedorapeople.org/xiphos-3.0.0-1.fc10.src.rpm
Description: Xiphos (formerly known as GnomeSword) is a Bible study tool written for Linux,
UNIX, and Windows under the GNOME toolkit, offering a rich and featureful
environment for reading, study, and research using modules from The SWORD
Project and elsewhere.


NOTE: This is just a rename of the existing gnomesword package.
Comment 1 Deji Akingunola 2009-02-20 19:40:08 EST
A new bugfix update has been released.

Spec URL: http://deji.fedorapeople.org/xiphos.spec
SRPM URL: http://deji.fedorapeople.org/xiphos-3.0.1-1.fc10.src.rpm
Comment 2 Nils Philippsen 2009-03-10 08:18:56 EDT
Sorry it took me so long to get started on this.

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.

Here we go:

- BAD/CHECK: rpmlint on xiphos-3.0.1-1.fc10.src.rpm:

  xiphos.src:26: W: unversioned-explicit-provides gnomesword
  --> either add version/release or rather (if no package directly depends on
      gnomesword), get rid of it (preferred if possible)

- GOOD: package name according to guidelines
- GOOD: spec file named properly
- BAD: licensing is unclear: source files mention "GPLv2+" (TBH, I haven't checked all of them), documentation mentions "GPLv2". Both are ok for Fedora, but please clarify this with upstream.
- BAD: The documentation is licensed under GFDL, please mention this.
- GOOD: license files are shipped as documentation
- GOOD: the spec file is written in American English
- GOOD: the spec file is legible
- BAD: download URL doesn't work, the upstream URL is http://downloads.sourceforge.net/gnomesword/xiphos-3.0.1.tar.gz (i.e. not in the "xiphos" directory)
- GOOD: source tarball is the same as from http://downloads.sourceforge.net/gnomesword/xiphos-3.0.1.tar.gz
- GOOD: builds in mock for x86_64/Rawhide
- CHECK: I found some warnings which you should check and eventually fix (and submit upstream):
  - "suggest parentheses around assignment used as truth value": these can be
    either legit (then add parentheses) or typos ("=" instead of "=="), please
    fix
  - "no return statement in function returning non-void", "control reaches end
    of non-void function": surely an omission, please fix
  - "implicit declaration of function '...'": probably headers that weren't
    included. I found that these can cause nasty, hard to debug crashes (see
    #486122 "Unexpected exit from gimp"), so please get these fixed as well.
  - "'...' may be used uninitialized in this function": ditto, please fix
  - "format '...' expects type '...', but argument ... has type '...'":
    may not show symptoms right now, but new compiler versions may break that
    please fix
  - There are other warnings where I'm not sure if the cause problems when
    running the program, perhaps consult with upstream about them (try to build
    with "-Werror" to catch all of them).
- GOOD: all build dependencies listed
- GOOD: uses %find_lang macro for locale files
- PASS: no libraries shipped
- PASS: not relocatable
- GOOD: all shipped directories owned by package, direct dependency or filesystem
- CHECK: please do or do not append a slash to directories in %files consistently
- GOOD: no duplicates in %files
- GOOD: permissions on files are set properly
- GOOD: package has a %clean section
- GOOD: package uses macros consistently
- GOOD: the package contains code, not content
- PASS: no large documentation files
- GOOD: %doc doesn't affect runtime
- PASS: no header files
- PASS: no static libraries
- PASS: no pkgconfig files
- PASS: no libraries included
- PASS: no devel package
- GOOD: no *.la libtool archives
- GOOD: desktop file is installed properly in %install
- BAD: don't apply a vendor tag to the desktop file as this is a new package
- GOOD: doesn't own files or directories owned by other packages
- GOOD: build root is cleaned at the beginning of %install
- GOOD: all file names are valid UTF-8
Comment 3 Deji Akingunola 2009-03-16 23:20:03 EDT
Thank so much you for the detailed review. On the various issues raised, upstream Developers have worked to clean up the code to now compile without any warning at all (they've also followed your advice and added -Werror to the build flags). A new release is scheduled to come out shortly with this fixes and others.

On the licensing issue, I or other folks upstream can't find where documentation differs from what's stated in the src files, they all seems to say GPLv2+. Can you please point me to the documentation that states only "GPLv2".

Concerning the documentation license, I've been informed that the documentation is licensed under GFDL *without invariant* sections. Hence it's almost like gpl / CC by-at sh-al no-co. And that this should not be a problem since the worst issue with GFDL are invariant sections.

All the other editorial spec file issues have been taken care of too. Will upload the new spec and srpm as soon as the new release becomes available.
Comment 4 Nils Philippsen 2009-03-17 05:24:44 EDT
(In reply to comment #3)
> Thank so much you for the detailed review. On the various issues raised,
> upstream Developers have worked to clean up the code to now compile without any
> warning at all (they've also followed your advice and added -Werror to the
> build flags). A new release is scheduled to come out shortly with this fixes
> and others.

Great!

> On the licensing issue, I or other folks upstream can't find where
> documentation differs from what's stated in the src files, they all seems to
> say GPLv2+. Can you please point me to the documentation that states only
> "GPLv2".

Here: http://xiphos.org/manual/license and in help/xiphos.pdf in the source tarball (both mention version GPL v2, but not the option to use "any later version").

> Concerning the documentation license, I've been informed that the documentation
> is licensed under GFDL *without invariant* sections. Hence it's almost like gpl
> / CC by-at sh-al no-co. And that this should not be a problem since the worst
> issue with GFDL are invariant sections.

I don't have a problem with the license of the documentation, I'm not sure whether it should be mentioned in the spec file, though.

> All the other editorial spec file issues have been taken care of too. Will
> upload the new spec and srpm as soon as the new release becomes available.
Comment 5 Deji Akingunola 2009-04-24 06:50:04 EDT
*** Bug 497497 has been marked as a duplicate of this bug. ***
Comment 6 Nils Philippsen 2009-04-24 10:43:35 EDT
Deji, mind that you could simply list the license as "GPLv2" for the time being, until this is clarified by upstream. Do you have a version of the package where the other issue is fixed? I'm asking because I'll be away on vacation for a few weeks with low or even no internet access from next Wednesday on.
Comment 7 Nils Philippsen 2009-04-24 10:54:25 EDT
"... issues are fixed?" of course.
Comment 8 Deji Akingunola 2009-04-24 11:22:28 EDT
Thanks Nils,

I've uploaded new spec and srpm files (see below) with fixes for packaging bugs in comment #2 and your suggestion in comment #6. As I pointed out in comment #3, the compile-time warnings have all being fixed in upstream SVN, but the its hard to pull it together in one patch without getting it mixed-up with the new stuff that had been introduced. Upstream is planning a new release, but I'm afraid not in good time before you leave for vacation. Hope this is sufficient for now. 

Spec URL: http://deji.fedorapeople.org/xiphos.spec
SRPM URL: http://deji.fedorapeople.org/xiphos-3.0.1-2.fc11.src.rpm
Comment 9 Deji Akingunola 2009-05-21 09:17:45 EDT
Xiphos-3.1, which includes fixes for the compile-time warnings in comment #3 have now being released.

Spec URL: http://deji.fedorapeople.org/xiphos.spec
SRPM URL: http://deji.fedorapeople.org/xiphos-3.1-1.fc11.src.rpm
Comment 10 Nils Philippsen 2009-06-08 07:45:51 EDT
Approved.

One remaining minor issue:

- WARNING/CHECK: rpmlint on xiphos-3.1-1.fc11.src.rpm:

xiphos.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 1)
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

--> probably trailing spaces on the first line. Please fix that when/after importing into CVS.
Comment 11 Deji Akingunola 2009-06-08 08:33:53 EDT
(In reply to comment #10)
> Approved.
Thanks Nils. I've fixed the trailing spaces issues.

New Package CVS Request
=======================
Package Name: xiphos
Short Description: Bible study and research tool
Owners: deji
Branches: F-9 F-10 F-11
InitialCC:
Comment 12 Jason Tibbitts 2009-06-08 12:42:22 EDT
CVS done.
Comment 13 Nils Philippsen 2009-07-16 08:04:41 EDT
Deji, I've seen packages of this already, would you please close the bug? See:

http://fedoraproject.org/wiki/Package_Review_Process#Contributor

Thanks.
Comment 14 Deji Akingunola 2009-07-16 09:57:57 EDT
(In reply to comment #13)
> Deji, I've seen packages of this already, would you please close the bug? See:
> 
I am very sorry for forgetting to do that. Thanks for the review.
Comment 15 Deji Akingunola 2009-10-24 00:15:38 EDT
Package Change Request
======================
Package Name: xiphos
New Branches: EL-5
Owners: deji
Comment 16 Kevin Fenzi 2009-10-26 16:20:50 EDT
cvs done.

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