Bug 674180 - Review Request: knights - A chess board for KDE
Summary: Review Request: knights - A chess board for KDE
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Golo Fuchert
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-01-31 21:37 UTC by Julian Aloofi
Modified: 2011-03-06 03:41 UTC (History)
5 users (show)

Fixed In Version: knights-2.2.0-4.fc15
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-02-15 19:41:51 UTC
Type: ---
packages: fedora-review+
tibbs: fedora-cvs+


Attachments (Terms of Use)

Description Julian Aloofi 2011-01-31 21:37:06 UTC
Spec URL: http://julian.fedorapeople.org/knights/knights.spec
SRPM URL: http://julian.fedorapeople.org/knights/knights-2.2.0-1.fc14.src.rpm
Description: Knights is a chess board for KDE that supports playing against
computer engines that support the XBoard protocol like GNUChess and also
multiplayer games over the internet on FICS. It features automatic rule
checking, themes, and nice animations

Comment 1 Golo Fuchert 2011-02-05 23:09:02 UTC
Knights should really be included in Fedora! I will do the review.

Comment 2 Golo Fuchert 2011-02-06 23:09:35 UTC
Here are some initial comments/questions before the review.

- I think the homepage in your spec file is wrong. It seems that there was a 
  rewrite of Knights and your sources are provided by http://opendesktop.org/content/show.php?content=122046
- You might want to consider to use macros like %{name} in the Source0 tag, 
  too. This will save you some work later on!
- The headers of the source files state:
  "GNU General Public License [...]; either version 2 of
    the License or (at your option) version 3 or any later version
    accepted by the membership of KDE e.V."
  so GPLv3+ is somehow an oversimplification, isn't it?
- Concerning Licensing: The source file contains no file containing the license 
  text (e.g. COPYING), it is best to ask upstream to change this.
  http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text
- Why are you not using make %{?_smp_mflags} ? Did I miss something?

Beside of that I think the package is ready for the review.

Comment 3 Julian Aloofi 2011-02-07 12:50:22 UTC
(In reply to comment #2)
> Here are some initial comments/questions before the review.

Thanks for reviewing it!

> - I think the homepage in your spec file is wrong. It seems that there was a 
>   rewrite of Knights and your sources are provided by
> http://opendesktop.org/content/show.php?content=122046

kde-apps.org is a service run by opendesktop.org, so this is basically the same page as far as I understand.
> - You might want to consider to use macros like %{name} in the Source0 tag, 
>   too. This will save you some work later on!

Good suggestion.
> - The headers of the source files state:
>   "GNU General Public License [...]; either version 2 of
>     the License or (at your option) version 3 or any later version
>     accepted by the membership of KDE e.V."
>   so GPLv3+ is somehow an oversimplification, isn't it?

Probably yes. I'll try and find other applications with these licensing terms in Fedora and see what they did, if I can't find any I'll send a mail to Fedora legal.
> - Concerning Licensing: The source file contains no file containing the license 
>   text (e.g. COPYING), it is best to ask upstream to change this.
>   http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text

I'll tell upstream about it, missed that :)
> - Why are you not using make %{?_smp_mflags} ? Did I miss something?

Oh, they should be there of course. I'll add them.

Comment 4 Julian Aloofi 2011-02-07 14:09:27 UTC
> > - The headers of the source files state:
> >   "GNU General Public License [...]; either version 2 of
> >     the License or (at your option) version 3 or any later version
> >     accepted by the membership of KDE e.V."
> >   so GPLv3+ is somehow an oversimplification, isn't it?
> 
> Probably yes. I'll try and find other applications with these licensing terms
> in Fedora and see what they did, if I can't find any I'll send a mail to Fedora
> legal.

I've spotted the license in Yakuake as well, and it doesn't have any special stuff in the "License:" field, so I guess this is OK.

Here are the latest packages so far:

Spec URL: http://julian.fedorapeople.org/knights/knights.spec
SRPM URL: http://julian.fedorapeople.org/knights/knights-2.2.0-2.fc14.src.rpm

Comment 5 Martin Gieseking 2011-02-07 18:11:46 UTC
I'd say the license is "GPLv2 or GPLv3" (plus a short comment about the mentioned restriction regarding later versions). GPLv3+ doesn't fit here.

Comment 6 Julian Aloofi 2011-02-07 19:10:06 UTC
(In reply to comment #5)
> I'd say the license is "GPLv2 or GPLv3" (plus a short comment about the
> mentioned restriction regarding later versions). GPLv3+ doesn't fit here.

I would just change it to GPLv2+ now. As pointed out, Yakuake does that and has the same licensing terms.
Or is there so much uncertainty that a mail to legal@lists.fpo would be preferred?

Comment 7 Martin Gieseking 2011-02-07 19:42:38 UTC
To my limited legal knowledge, GPLv2+ is not quite correct here because of the given restriction. According to the copyright notice, only GPLv2 and GPLv3 have been approved while potential future GPL versions are still to be confirmed by the KDE team. GPLv2+ doesn't reflect this.
But again, I'm not a legal expert. Maybe a question on the legal mailing list can shed some light on this.

Comment 8 Julian Aloofi 2011-02-07 19:54:24 UTC
I'll send them a mail now.

Comment 9 Golo Fuchert 2011-02-07 20:16:33 UTC
I agree with Martin on this. I think this should be clarified, especially since it seems to affect other packages (like Yakuake) as well. Maybe after clarification this is an issue for the Fedora-packaging mailing list?

Concerning the homepage, you are absolutely right. I somehow ended up on the old homepage (http://kde-apps.org/content/show.php?content=20534) and was confused. Sorry for that.

Comment 10 Julian Aloofi 2011-02-07 23:00:10 UTC
I got a reply from Fedora legal, and the appropriate License would be "GPLv2 and GPLv3" with a comment pointing out that the KDE e.V. might change this in the future.
So here are the new packages:

Spec URL: http://julian.fedorapeople.org/knights/knights.spec
SRPM URL: http://julian.fedorapeople.org/knights/knights-2.2.0-3.fc14.src.rpm

Comment 11 Martin Gieseking 2011-02-08 07:06:09 UTC
(In reply to comment #10)
> "GPLv2 and GPLv3" with a comment pointing out that the KDE e.V. might change 
> this in the future.

I think you meant "GPLv2 or GPLv3". "and" would be rather odd here.

Comment 12 Golo Fuchert 2011-02-09 22:22:36 UTC
Julian,

the package is almost ready, only the permissions for one file needs to be corrected.

$ rpmlint SPECS/knights.spec SRPMS/knights-2.2.0-3.fc14.src.rpm RPMS/i686/knights-2.2.0-3.fc14.i686.rpm 
knights.src: W: spelling-error %description -l en_US multiplayer -> multilayer, multiplexer, multiplier
knights.i686: W: spelling-error %description -l en_US multiplayer -> multilayer, multiplexer, multiplier
knights.i686: E: script-without-shebang /usr/share/applications/kde4/knights.desktop
knights.i686: W: no-manual-page-for-binary knights
2 packages and 1 specfiles checked; 1 errors, 3 warnings.

Spelling errors are false positives, the .desktop file seems to have wrong permissions (755).

---------------------------------
key:

[+] OK
[.] OK, not applicable
[X] needs work
---------------------------------

[+] MUST: The package must be named according to the Package Naming Guidelines.
[+] MUST: The spec file name must match the base package %{name}.
[+] MUST: The package must meet the Packaging Guidelines.
[+] MUST: The package must be licensed with a Fedora approved license.
    - GPLv2 or GPLv3 (with the possibility for an extension by KDE e.V.)
[+] MUST: The License field in the package spec file must match the actual
license.
[.] MUST: The file containing the text of the license(s) for the package must
be included in %doc.
[+] MUST: The spec file must be written in American English.
[+] MUST: The spec file for the package MUST be legible.
[+] MUST: The sources used to build the package must match the upstream source.
    $ md5sum knights-2.2.0.tar.bz2{,.packaged}
    6196e8ef8d2e7c16e38307469708f7cf  knights-2.2.0.tar.bz2
    6196e8ef8d2e7c16e38307469708f7cf  knights-2.2.0.tar.bz2.packaged

[+] MUST: The package MUST successfully compile and build into binary rpms on
at least one primary architecture.
[.] MUST: If the package does not successfully compile, build or work on an
architecture, ...
[+] MUST: All build dependencies must be listed in BuildRequires.
[+] MUST: When compiling C, C++, or Fortran files, %{optflags} must be applied.
    - implicitly by %cmake_kde4
[+] MUST: The spec file MUST handle locales properly.
[+] MUST: If a package installs files below %{_datadir}/icons, the icon cache
must be updated.
[.] MUST: Packages storing shared library files (not just symlinks) must call
ldconfig in %post and %postun.
[+] MUST: Packages must NOT bundle copies of system libraries.
[.] MUST: If the package is designed to be relocatable, ...
[+] MUST: A package must own all directories that it creates. 
[+] MUST: A Fedora package must not list a file more than once in %files.
[X] MUST: Permissions on files must be set properly.
    - see the rpmlint output above
[+] MUST: Packages must not provide RPM dependency information when that
information is not global in nature, or are otherwise handled.
[.] MUST: When filtering automatically generated RPM dependency information,
the filtering system implemented by Fedora must be used.
[+] MUST: Each package must consistently use macros.
[+] MUST: The package must contain code, or permissable content.
[.] MUST: Large documentation files must go in a -doc subpackage.
[+] MUST: Files in %doc must not affect the runtime of the application.
[.] MUST: Header files must be in a -devel package.
[.] MUST: Static libraries must be in a -static package.
[.] MUST: If a package contains library files with a suffix (e.g.
libfoo.so.1.1), ...
[.] MUST: devel packages must require the base package using a fully versioned
dependency.
[+] MUST: Packages must NOT contain any .la libtool archives.
[+] MUST: Packages containing GUI applications must include a %{name}.desktop
file
[+] MUST: .desktop files must be properly installed with desktop-file-install
in the %install section.
[+] MUST: Packages must not own files or directories already owned by other
packages.
[+] MUST: All filenames in rpm packages must be valid UTF-8.


[+] SHOULD: If the source package does not include license text(s) as a
separate file from upstream, the packager SHOULD query upstream...
    - has been/will be done by the packager.
[+] SHOULD: Timestamps of files should be preserved.
[+] SHOULD: The reviewer should test that the package builds in mock.
[+] SHOULD: The reviewer should test that the package functions as described.
    - Almost, the description didn't warn the reviewer to lose against
      gnuchess. ;-)
[+] SHOULD: If scriptlets are used, those scriptlets must be sane.
[.] SHOULD: Usually, subpackages other than devel should require the base
package using a fully versioned dependency.
[.] SHOULD: pkgconfig(.pc) files should be placed in a -devel pkg.
[.] SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin,
/usr/bin, or /usr/sbin ...
[X] SHOULD: Your package should contain man pages for binaries/scripts.
    - Maybe a plan for the future?

Comment 13 Julian Aloofi 2011-02-13 01:00:36 UTC
Here are the new packages with fixed .desktop file permissions:

Spec URL: http://julian.fedorapeople.org/knights/knights.spec
SRPM URL: http://julian.fedorapeople.org/knights/knights-2.2.0-4.fc14.src.rpm

(In reply to comment #12)
> [+] SHOULD: The reviewer should test that the package functions as described.
>     - Almost, the description didn't warn the reviewer to lose against
>       gnuchess. ;-)

I'm glad I'm not the only one who can reproduce that problem ;)

Comment 14 Golo Fuchert 2011-02-13 10:44:08 UTC
All right, no further objections.

----------------
Package APPROVED
----------------

Comment 15 Julian Aloofi 2011-02-13 19:50:50 UTC
New Package SCM Request
=======================
Package Name: knights
Short Description: A chess board for KDE
Owners: julian
Branches: f13 f14
InitialCC:

Comment 16 Julian Aloofi 2011-02-13 19:51:47 UTC
(In reply to comment #14)
> All right, no further objections.
> 
> ----------------
> Package APPROVED
> ----------------

Thanks for the review! :)
If I can return the favour some day, just send me a mail ;)

Comment 17 Jason Tibbitts 2011-02-15 19:24:12 UTC
I also added an f15 branch.

Git done (by process-git-requests).

Comment 18 Julian Aloofi 2011-02-15 19:41:51 UTC
(In reply to comment #17)
> I also added an f15 branch.

Thanks, totally forgot that it's branched already! :)

Comment 19 Fedora Update System 2011-02-15 20:39:18 UTC
knights-2.2.0-4.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/knights-2.2.0-4.fc15

Comment 20 Fedora Update System 2011-02-15 20:41:34 UTC
knights-2.2.0-4.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/knights-2.2.0-4.fc14

Comment 21 Fedora Update System 2011-02-15 20:43:28 UTC
knights-2.2.0-4.fc13 has been submitted as an update for Fedora 13.
https://admin.fedoraproject.org/updates/knights-2.2.0-4.fc13

Comment 22 Fedora Update System 2011-03-05 22:56:58 UTC
knights-2.2.0-4.fc14 has been pushed to the Fedora 14 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 23 Fedora Update System 2011-03-05 23:02:47 UTC
knights-2.2.0-4.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 24 Fedora Update System 2011-03-06 03:40:45 UTC
knights-2.2.0-4.fc15 has been pushed to the Fedora 15 stable repository.  If problems still persist, please make note of it in this bug report.


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